Re: [PATCH] ASoC: omap-mcbsp: Add PM QoS support for McBSP to prevent glitches

[Peter Ujfalusi <peter.ujfalusi@ti.com>]

On 08/31/16 22:41, Tony Lindgren wrote:
> * Peter Ujfalusi <peter.ujfalusi@ti.com> [160831 11:34]:
>> On 08/31/16 19:59, Tony Lindgren wrote:
>>> * Tony Lindgren <tony@atomide.com> [160831 07:14]:
>>>> * Peter Ujfalusi <peter.ujfalusi@ti.com> [160831 04:38]:
>>>>> If we have McBSP w/o FIFO there is no need for the qos AFAIK.
>>>>> I'm fine with the 30ms, if you have done the testing on OMAP3.McBSP2, probably
>>>>> setting 30ms for McBSP with 1280 FIFO, 3ms for 128 FIFO and no qos for McBSP
>>>>> w/o FIFO might be better. Or:
>>>>>
>>>>> latency = mcbsp->pdata->buffer_size * 23; /* 29.44ms on omap3's mcbsp2 */
>>>>>
>>>>> if (latency)
>>>>> 	pm_qos_add_request(&mcbsp->pm_qos_req, PM_QOS_CPU_DMA_LATENCY,
>>>>> 			   latency);
>>>>
>>>> OK let me run some tests with that and mcbsp2 fifo set to 128. My guess
>>>> is that things already work for that case with no patches with the existing
>>>> fifo based  snd_pcm_hw_constraint_step() setting limits low enough so
>>>> we never enter deeper idle states, but let's see :)
>>>
>>> Well not quite so :) The fixed limit of 30 ms also works with fifo
>>> set to 128, but we still have audio fail without any patches.
>>
>> does it fail at start time or later?
>> At start time we will have ~10 DMA bursts of 128 words to fill the FIFO on McBSP2.

OK, so it is not happening during DMA bursts.

> The problems start only later on, about 10 seconds into playing of
> course depending on things like console activity etc.
> 
>>> @@ -1064,6 +1072,16 @@ int omap_mcbsp_init(struct platform_device *pdev)
>>>  		mcbsp->max_tx_thres = max_thres(mcbsp) - 0x10;
>>>  		mcbsp->max_rx_thres = max_thres(mcbsp) - 0x10;
>>>  
>>> +		/*
>>> +		 * Prevent device from hitting deeper idle states between
>>> +		 * DMA fifo loads. On omap3 mcbsp2 we have a larger FIFO of
>>> +		 * 1280 bytes instead of the usual 128 bytes, and the measured
>>
>> not bytes, but words. The word size does not matter, it can be 8, 16 or
>> 32bits, but we have 1280 or 128 of these words.
> 
> Oh OK, will fix.
> 
>>> +		 * max latency we can tolerate is somewhere below 40 ms. To be
>>> +		 * safe, use a fixed value of 30 ms to block off idle on omap3
>>> +		 * while still allowing retention idle.
>>> +		 */
>>> +		mcbsp->latency = 30 * 1000;
>>
>> were there issue with calculating the latency like
>> 		mcbsp->latency = max_thres(mcbsp) * 23;
> 
> Yes, we're not hitting retention idle at all during play with FIFO set
> to 128 words.

What is the audio format you are using? sampling rate and how many channels?
cat /proc/asound/card0/pcm0p/sub0/hw_params

What happens if you set the FIFO to 256? Do you still need the 30ms or it can
be higher, like 60ms?

When the FIFO is set to 128, it means that after the initial FIFO fill we will
have DMA request coming from McBSP to sDMA with a rate of:

(1000/sampling_rate) * (FIFO-threshold / channels) = DMA_req_distance_in_ms

So in case of 44.1KHz, stereo with 128 FIFO threshold DMA request will come at
every 1.45ms. If I'm not mistaken. The whole FIFO (1280) holds 14.51ms of
audio in this case.

I don't see this correlate with the 30ms at all.

Do you see anything in the kernel log? If McBSP is not powered and DMA tries
to write data we should see l3 error at least. If the reason is that the
system is not able to wake up fast enough to react to the DMA request from
McBSP, then we should see FIFO underrun from McBSP (this patch will show them:
http://mailman.alsa-project.org/pipermail/alsa-devel/2016-August/111671.html)

Have you tried with different sampling rates? That would have similar effect
as changing the FIFO threshold for the DMA request periods.

-- 
Péter