| Message ID | 20210519104842.977895-1-codrin.ciubotariu@microchip.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | soc-pcm: Add separate snd_pcm_runtime for BEs | expand |
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? Also, the last patch allows only IRAM type, which sounds already doubtful. The dmaengine code should be generic. Last but not least, one minor nitpick: please use EXPORT_SYMBOL_GPL() for the newly introduced symbols. thanks, Takashi
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. > 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. > > Last but not least, one minor nitpick: please use EXPORT_SYMBOL_GPL() > for the newly introduced symbols. Sure, this is an oversight on my side. I will make all of them EXPORT_SYMBOL_GPL. Thank you very much for your review! Best regards, Codrin [1] https://elixir.bootlin.com/linux/v5.13-rc2/source/sound/soc/soc-generic-dmaengine-pcm.c#L266
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. > > 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. thanks, Takashi
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
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
On 21.05.2021 17:26, Takashi Iwai wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > 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? Yes, I am thinking that the constraints should be grouped for one BE, since the HW parameters are the same for all the DAIs(CPUs and codecs) in a BE. Also, I think that the separation of snd_pcm_hardware is important. From what I understand now, the BE CPUs snd_soc_pcm_stream structures are ignored entirely and the codecs snd_soc_pcm_stream structures are considered only if fe->dai_link->dpcm_merged_* flags are set [1]. This means that be_hw_params_fixup() callback (BE HW params) is not affected by the BE drivers' snd_soc_pcm_stream. We are relying on the fact that developers know what capabilities the BE DAIs have, in order to set be_hw_params_fixup. Even though HW capabilities are subject to change if HW bugs are discovered... Am I making sense? I still prefer the separate snd_pcm_runtime, since, for my case, I am able to set different runtime->(rate, channels, subformat, frame_bits, sample_bits, period/buffer_size, etc.) and later use bytes_to_* and *_to_bytes() callbacks freely, but if this step is too big, maybe we can split it up and start with something we can all agree on the HW constraints. Thank you for your time! Best regards, Codrin [1] https://elixir.bootlin.com/linux/v5.13-rc3/source/sound/soc/soc-pcm.c#L1768
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. Codrin Ciubotariu (6): ALSA: core: pcm: Create helpers to allocate/free struct snd_pcm_runtime ALSA: pcm: Export constraints initialization functions ALSA: pcm: Check for substream->ops before substream->ops->mmap ALSA: pcm: Create function for snd_pcm_runtime initialization ASoC: soc-pcm: Create new snd_pcm_runtime for BE DAIs ASoC: dmaengine: Allocate buffer if substream is unmanaged include/sound/pcm.h | 7 ++ sound/core/pcm.c | 82 ++++++++++------- sound/core/pcm_native.c | 116 +++++++++++++----------- sound/soc/soc-generic-dmaengine-pcm.c | 18 ++++ sound/soc/soc-pcm.c | 126 ++++++++++++++++++-------- 5 files changed, 229 insertions(+), 120 deletions(-)