Re: [PATCH v2 1/2] ASoC: dmaengine-pcm: Add support for querying DMA capabilities

[Lars-Peter Clausen <lars@metafoo.de>]

On 10/08/2013 05:30 PM, Vinod Koul wrote:
> On Tue, Oct 08, 2013 at 06:13:56PM +0200, Lars-Peter Clausen wrote:
>> On 10/08/2013 05:06 PM, Vinod Koul wrote:
>>> On Tue, Oct 08, 2013 at 03:07:59PM +0200, Lars-Peter Clausen wrote:
>>>> Currently each platform making use the the generic dmaengine PCM driver still
>>>> needs to provide a custom snd_pcm_hardware struct which specifies the
>>>> capabilities of the DMA controller, e.g. the maximum period size that can be
>>>> supported. This patch adds code which uses the newly introduced
>>>> dma_get_slave_caps() API to query this information from the dmaengine driver.
>>>> The new code path will only be taken if the 'pcm_hardware' field of the
>>>> snd_dmaengine_pcm_config struct is NULL.
>>>>
>>>> The patch also introduces a new 'fifo_size' field to the
>>>> snd_dmaengine_dai_dma_data struct which is used to initialize the
>>>> snd_pcm_hardware 'fifo_size' field and needs to be set by the DAI driver.
>>>>
>>>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>>>>
>>>> ---
>>>> Changes since v1:
>>>> 	* Use dma_get_max_seg_size() to get the maximum size of one period
>>>> 	* Assume that a DMA engine driver is able to support an infinite number of
>>>> 	  periods
>>>> 	* Increase min_period_bytes from 16 to 256.
>>>> ---
>>>>   include/sound/dmaengine_pcm.h         |  2 ++
>>>>   sound/soc/soc-generic-dmaengine-pcm.c | 55 ++++++++++++++++++++++++++++-------
>>>>   2 files changed, 47 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/include/sound/dmaengine_pcm.h b/include/sound/dmaengine_pcm.h
>>>> index f11c35c..83b2c3e 100644
>>>> --- a/include/sound/dmaengine_pcm.h
>>>> +++ b/include/sound/dmaengine_pcm.h
>>>> @@ -61,6 +61,7 @@ struct dma_chan *snd_dmaengine_pcm_get_chan(struct snd_pcm_substream *substream)
>>>>    * @slave_id: Slave requester id for the DMA channel.
>>>>    * @filter_data: Custom DMA channel filter data, this will usually be used when
>>>>    * requesting the DMA channel.
>>>> + * @fifo_size: FIFO size of the DAI controller in bytes
>>> is this supposed to be DMA FIFO or I2S FIFO?
>>
>> This field is the internal FIFO of the audio core.
>>
>>>
>>> And second question for Takakshi or Mark. I see that this value ends up in
>>> usermode, what is the intented usage of this?
>>
>> I think it is supposed to help to calculate the audio delay, but I
>> don't think it is actually that widely used at the moment. And yes,
>> in order to make this more accurate we should probably add support
>> for incorporating also the fifo size of the DMA channel.
> But then why dont add to delay field, why report to usermode

I don't now :)

>
>> [...]
>>>> +	hw.periods_min = 2;
>>>> +	hw.periods_max = UINT_MAX;
>>>> +	hw.period_bytes_min = 256;
>>>> +	hw.period_bytes_max = dma_get_max_seg_size(dma_dev);
>>> I think you should check if the driver has set this, if not goto some default
>>>
>>
>> I don't think we are supposed to directly peak at the dma_parms
>> field. If the DMA driver does not work with the default, and it did
>> not set up a proper value, it is a bug in the DMA driver.
> Well i didnt mean to peek but to check the value returned by
> dma_get_max_seg_size(). Also, while buggy driver get fixed we need this :)

The default value, if the DMA driver did not setup something explicitly, is 
65535, which is a legitimate value. So we won't be able to tell if this is a 
wrong default or a correct default. But the DAI driver has to explicitly enable 
this new functionality (by not setting any static hw_params). So we hopefully 
won't hit this path for DMA drivers which do not initialize their segment size 
correctly.

- Lars