Re: [PATCH V2 5/9] ASoC: amd: ps: add support for SoundWire DMA interrupts

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

On 5/31/23 02:28, Mukunda,Vijendar wrote:
> On 24/05/23 13:15, Mukunda,Vijendar wrote:
>> On 23/05/23 20:30, Pierre-Louis Bossart wrote:
>>> On 5/23/23 02:36, Mukunda,Vijendar wrote:
>>>> On 22/05/23 23:42, Pierre-Louis Bossart wrote:
>>>>> On 5/22/23 08:31, Vijendar Mukunda wrote:
>>>>>> Initialize workqueue for SoundWire DMA interrupts handling.
>>>>>> Whenever audio data equal to the SoundWire FIFO watermark level
>>>>>> are produced/consumed, interrupt is generated.
>>>>>> Acknowledge the interrupt and schedule the workqueue.
>>>>> It would help to explain why a work queue is needed is the first place,
>>>>> as opposed to handling periods in the interrupt thread.
>>>> For SoundWire DAI link, we are setting nonatomic flag to true.
>>>> If we return period elapsed from hard irq handler instead of workqueue,
>>>> soft lock up is observed during stream closure.
>>>>
>>>> We can use interrupt thread as well. To have a symmetry with
>>>> SoundWire manager work queues, we have used workqueue for
>>>> DMA interrupts.
>>> Oh, I completely missed the model here.
>>>
>>> If you are using the bottom half/hard irq handler to read status
>>> information, the natural thing to do would be to have an irq thread, no?
>>>
>>> Not sure I see the benefit of aligning with the manager work queues -
>>> unless it makes your life simpler to avoid race conditions with
>>> cancel_work_sync()?
>> We can implement request_threaded_irq() and move the handling of
>> DMA interrupts to thread function whereas we need to handle SoundWire
>> manager interrupts in top half only. Reason as follows.
>>
>> As per our design, we are not masking the interrupts in top half and
>> restoring mask after thread execution like Intel and
>> our IP supports line based interrupts. If we move SoundWire manager
>> interrupt handling to thread function, we have observed interrupts are
>> reported but not handled properly due to thread execution is in progress
>> sometimes.
>> we will add comments for this design constraint in the code if we have to
>> go with threaded_irq implementation.
>>
>> @Bossart: we are waiting for your reply.

I am not sure I get the point about using workqueues v. threads for the
manager, which in turn makes it difficult to understand why the DMA
interrupt handling should be aligned with that of the manager interrupt
handling.

Using the combination of hard irq handler + workqueue feels odd. I may
very well 'work' but others should chime in since I am far from the most
knowledgeable reviewer in this area.

>>>>>> +static void acp63_sdw_dma_workthread(struct work_struct *work)
>>>>>> +{
>>>>>> +	struct acp63_dev_data *adata = container_of(work, struct acp63_dev_data,
>>>>>> +						    acp_sdw_dma_work);
>>>>>> +	struct sdw_dma_dev_data *sdw_dma_data;
>>>>>> +	u32 stream_index;
>>>>>> +	u16 pdev_index;
>>>>>> +
>>>>>> +	pdev_index = adata->sdw_dma_dev_index;
>>>>>> +	sdw_dma_data = dev_get_drvdata(&adata->pdev[pdev_index]->dev);
>>>>>> +
>>>>>> +	for (stream_index = 0; stream_index < ACP63_SDW0_DMA_MAX_STREAMS; stream_index++) {
>>>>>> +		if (adata->sdw0_dma_intr_stat[stream_index]) {
>>>>>> +			if (sdw_dma_data->sdw0_dma_stream[stream_index])
>>>>>> +				snd_pcm_period_elapsed(sdw_dma_data->sdw0_dma_stream[stream_index]);
>>>>>> +			adata->sdw0_dma_intr_stat[stream_index] = 0;
>>>>>> +		}
>>>>>> +	}
>>>>>> +	for (stream_index = 0; stream_index < ACP63_SDW1_DMA_MAX_STREAMS; stream_index++) {
>>>>>> +		if (adata->sdw1_dma_intr_stat[stream_index]) {
>>>>>> +			if (sdw_dma_data->sdw1_dma_stream[stream_index])
>>>>>> +				snd_pcm_period_elapsed(sdw_dma_data->sdw1_dma_stream[stream_index]);
>>>>>> +			adata->sdw1_dma_intr_stat[stream_index] = 0;
>>>>>> +		}
>>>>>> +	}
>>>>> I am not clear on the benefits of the workqueue which only tests a flag
>>>>> that's set ...
>>>> In top half, we are checking all stream irq mask and setting
>>>> corresponding stream id index in interrupt status array when dma
>>>> irq is raised.
>>>>
>>>> Our intention is to handle snd_pcm_period_elapsed in process context.
>>>> if the flag is set, call the period elapsed for the substream based on stream
>>>> id in work queue.
>