Message-ID: <b9538103-4c72-722d-0abd-015f9432f17f@linux.intel.com>
Date:   Wed, 15 Jul 2020 09:11:25 -0500
From:   Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To:     Vinod Koul <vkoul@...nel.org>
Cc:     "Liao, Bard" <bard.liao@...el.com>,
        "alsa-devel@...a-project.org" <alsa-devel@...a-project.org>,
        "tiwai@...e.de" <tiwai@...e.de>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "ranjani.sridharan@...ux.intel.com" 
        <ranjani.sridharan@...ux.intel.com>,
        "hui.wang@...onical.com" <hui.wang@...onical.com>,
        "broonie@...nel.org" <broonie@...nel.org>,
        "srinivas.kandagatla@...aro.org" <srinivas.kandagatla@...aro.org>,
        "jank@...ence.com" <jank@...ence.com>,
        "Lin, Mengdong" <mengdong.lin@...el.com>,
        "Blauciak, Slawomir" <slawomir.blauciak@...el.com>,
        "Kale, Sanyog R" <sanyog.r.kale@...el.com>,
        Bard Liao <yung-chuan.liao@...ux.intel.com>,
        "rander.wang@...ux.intel.com" <rander.wang@...ux.intel.com>
Subject: Re: [PATCH 7/9] soundwire: intel/cadence: merge Soundwire interrupt
 handlers/threads



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.

Powered by blists - more mailing lists