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.
