Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 21 May 2021 16:26:00 +0200
Message-ID: <s5hr1i0f9jb.wl-tiwai@suse.de>
From:   Takashi Iwai <tiwai@suse.de>
To:     <Codrin.Ciubotariu@microchip.com>
Cc:     <alsa-devel@alsa-project.org>, <linux-kernel@vger.kernel.org>,
        <perex@perex.cz>, <tiwai@suse.com>,
        <pierre-louis.bossart@linux.intel.com>, <broonie@kernel.org>,
        <joe@perches.com>, <lgirdwood@gmail.com>, <lars@metafoo.de>,
        <kuninori.morimoto.gx@renesas.com>, <Nicolas.Ferre@microchip.com>,
        <Cristian.Birsan@microchip.com>
Subject: Re: [RFC PATCH 0/6] soc-pcm: Add separate snd_pcm_runtime for BEs
In-Reply-To: <2c2661d3-78a8-de55-e976-b87f3658a093@microchip.com>
References: <20210519104842.977895-1-codrin.ciubotariu@microchip.com>
        <s5him3eizdf.wl-tiwai@suse.de>
        <056e560e-d06d-23bc-b041-60890fa51e63@microchip.com>
        <s5h4keyivdh.wl-tiwai@suse.de>
        <2c2661d3-78a8-de55-e976-b87f3658a093@microchip.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI/1.14.6 (Maruoka)
 FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 Emacs/25.3
 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

On Thu, 20 May 2021 15:59:02 +0200,
<Codrin.Ciubotariu@microchip.com> wrote:
> 
> 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?

Well, if each BE needs a different set of hw constraint rules, it
needs its own unique copies instead of sharing the rules.  Is it your
requirement?


Takashi