Re: [PATCH 7/9] soundwire: intel/cadence: merge Soundwire interrupt handlers/threads

[Pierre-Louis Bossart]

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.