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

[Mukunda,Vijendar]

On 31/05/23 19:23, Pierre-Louis Bossart wrote:
>
> 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.
Understood your point. We will use irq thread instead of workqueue
for SoundWire DMA interrupts handling.
We will push V3 version.
>
>>>>>>> +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.