Messages in this thread | | | Subject | Re: [PATCH 7/9] soundwire: intel/cadence: merge Soundwire interrupt handlers/threads | From | Pierre-Louis Bossart <> | Date | Wed, 15 Jul 2020 09:11:25 -0500 |
| |
On 7/14/20 11:54 PM, Vinod Koul wrote: > On 02-07-20, 10:01, Pierre-Louis Bossart wrote: > >>>> Sounds good. Now that you are already in irq thread, does it make sense >>>> to spawn a worker thread for this and handle it there? Why not do in the >>>> irq thread itself. Using a thread kind of defeats the whole point behind >>>> concept of irq threads >>> >>> Not sure If you are talking about cdns_update_slave_status_work(). >>> The reason we need to spawn a worker thread in sdw_cdns_irq() is >>> that we will do sdw transfer which will generate an interrupt when >>> a slave interrupt is triggered. And the handler will not be invoked if the >>> previous handler is not return yet. >>> Please see the scenario below for better explanation. >>> 1. Slave interrupt arrives >>> 2.1 Try to read Slave register and waiting for the transfer response >>> 2.2 Get the transfer response interrupt and finish the sdw transfer. >>> 3. Finish the Slave interrupt handling. >>> >>> Interrupts are triggered in step 1 and 2.2, but step 2.2's handler will not be >>> invoked if step 1's handler is not return yet. >>> What we do is to spawn a worker thread to do step 2 and return from step 1. >>> So the handler can be invoked when the transfer response interrupt arrives. >> >> To build on Bard's correct answer, the irq thread only takes care of >> 'immediate' actions, such as command completion, parity or bus clash errors. >> The rest of the work can be split in >> a) changes to device state, usually for attachment and enumeration. This is >> rather slow and will entail regmap syncs. >> b) device interrupts - typically only for jack detection which is also >> rather slow. >> >> Since this irq thread function is actually part of the entire HDaudio >> controller interrupt handling, we have to defer the work for cases a) and b) >> and re-enable the HDaudio interrupts at the end of the irq thread function - >> see the code I shared earlier. >> >> In addition, both a) and b) will result in transactions over the bus, which >> will trigger interrupts to signal the command completions. In other words, >> because of the asynchronous nature of the transactions, we need a two-level >> implementation. If you look at the previous solution it was the same, the >> commands were issued in the irq thread and the command completion was >> handled in the handler, since we had to make the handler minimal with a >> global GIE interrupt disable we kept the same hierarchy to deal with >> commands but move it up one level. >> >> You could argue that maybe a worker thread is not optimal and could be >> replaced by something better/faster. Since the jack detection is typically >> handled with a worker thread in all ASoC codec drivers, we didn't feel the >> need to optimize further. We did not see any performance impact with this >> change. >> >> Does this answer to your concern? > > The point is that we are already in irq_thread which is designed to > handle any bottom half processing and can be given priority, spawning of > worker threads for another bottom half seems unnecessary to me and would > increase the latency for you. > > I would have handled everything in irq_thread and returned, after all we > are in bottom half :) > > Is there a reason for worker thread here, if so it is not clear to me > atm.
I think we explained it at length: the irq thread deals with command completion so the command initiation required for enumeration and imp-def interrupt needs to be issued in *another* thread.
You cannot have in the same thread a wait_for_completion() and complete(), it'd be a by-design deadlock.
Maybe a comparison would help.
previous design for N masters N+2 Handlers + threads (one IPC, one stream, N SoundWire) each SoundWire handler takes care of command completion and wakes a thread for enumeration and imp-def interrupt.
New design Single handler for ALL interrupt sources The handler masks the global interrupt and wakes a thread that deals with all interrupt sources, one after the other. The SoundWire thread function for each Master will take case of command completion and schedules a workqueue for enumeration and imp-def interrupt. The irq thread then unmask the global interrupt and returns IRQ_HANDLED.
| |