Re: [RFC PATCH 0/6] soc-pcm: Add separate snd_pcm_runtime for BEs

[<Codrin.Ciubotariu@microchip.com>]

On 19.05.2021 18:41, Takashi Iwai wrote:
> On Wed, 19 May 2021 17:08:10 +0200,
> <Codrin.Ciubotariu@microchip.com> wrote:
>>
>> On 19.05.2021 17:15, Takashi Iwai wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On Wed, 19 May 2021 12:48:36 +0200,
>>> Codrin Ciubotariu wrote:
>>>>
>>>> This patchset adds a different snd_pcm_runtime in the BE's substream,
>>>> replacing the FE's snd_pcm_runtime. With a different structure, the BE
>>>> HW capabilities and constraints will no longer merge with the FE ones.
>>>> This allows for error detection if the be_hw_params_fixup() applies HW
>>>> parameters not supported by the BE DAIs. Also, it calculates values
>>>> needed for mem-to-dev/dev-to-mem DMA transfers, such as buffer size and
>>>> period size, if needed.
>>>>
>>>> The first 4 patches are preparatory patches, that just group and export
>>>> functions used to allocate and initialize the snd_pcm_runtime. Also, the
>>>> functions that set and apply the HW constraints are exported.
>>>> The 5th patch does (almost) everything need to create the new snd_pcm_runtime
>>>> for BEs, which includes allocation, initializing the HW capabilities,
>>>> HW constraints and HW parameters. The BE HW parameters are no longer
>>>> copied from the FE. They are recalculated, based on HW capabilities,
>>>> constraints and the be_hw_params_fixup() callback.
>>>> The 6th and last patch basically adds support for the PCM generic
>>>> dmaengine to be used as a platform driver for BE DAI links. It allocates
>>>> a buffer, needed by the DMA transfers that do not support dev-to-dev
>>>> transfers between FE and BE DAIs.
>>>>
>>>> This is a superset of
>>>> https://mailman.alsa-project.org/pipermail/alsa-devel/2021-March/182630.html
>>>> which only handles the BE HW constraints. This patchset aims to be more
>>>> complete, defining a a snd_pcm_runtime between each FE and BE and can
>>>> be used between any DAI link connection. I am sure I am not handling all
>>>> the needed members of snd_pcm_runtime (such as handling
>>>> struct snd_pcm_mmap_status *status), but I would like to have your
>>>> feedback regarding this idea.
>>>
>>> I'm also concerned about the handling of other fields in runtime
>>> object, maybe allocating a complete runtime object for each BE is an
>>> overkill and fragile.  Could it be rather only hw_constraints to be
>>> unique for each BE, instead?
>>
>> I tried with only the hw constraints in the previous patchset and it's
>> difficult to handle the snd_pcm_hw_rule_add() calls, without changing
>> the function's declaration. This solution requires no changes to
>> constraints API, nor to their 'clients'. I agree that handling all the
>> runtime fields might be over-complicated. From what I see, the scary
>> ones are used to describe the buffer and the status of the transfers. I
>> do not think there are BEs that use these values at this moment (the FE
>> ones). I think that the HW params, private section, hardware description
>> and maybe DMA members (at least in my case) are mostly needed by BEs.
> 
> OK, I'll check your previous series again, but my gut feeling is for
> pursuit to the hw_constraints hacks.  e.g. we may split
> snd_pcm_hw_constraints and snd_pcm_hw_rule, too, if that matters.

Something like adding snd_pcm_hw_rule directly under
snd_pcm_runtime, to store the BE constraints? It could work, but I think 
we should also be able to remove rules, if one BE gets disconnected. 
This means that we will need a way to identify or separate them, for 
each BE, right?

> 
>>> Also, the last patch allows only IRAM type, which sounds already
>>> doubtful.  The dmaengine code should be generic.
>>
>> dmaengine, when used with normal PCM, preallocates only IRAM buffers
>> [1]. This BE buffer would only be needed if DMA dev-to-mem or mem-to-dev
>> transfers are needed, between FE and BE. I agree that it could be
>> handled differently, I added it here mostly to express my goal, which is
>> to use the generic dmaengine for BEs. My DMA has no dev-to-dev DMA
>> capability, so I need a buffer to move the data between FE and BE.
> 
> Ah, right, I overlooked that part...  It's confusing.  Maybe it's in
> that style just because it falls back to SNDRV_DMA_TYPE_DEV?
> It's another discussion, in anyway.

Right, it falls back to SNDRV_DMA_TYPE_DEV. [1]

Thanks and best regards,
Codrin

[1] 
https://elixir.bootlin.com/linux/v5.13-rc2/source/sound/core/memalloc.c#L154