mbox series

[0/3] ASoC: grace time for DPCM cleanup

Message ID 87wmo6dyxg.wl-kuninori.morimoto.gx@renesas.com (mailing list archive)
Headers show
Series ASoC: grace time for DPCM cleanup | expand

Message

Kuninori Morimoto May 7, 2024, 4:32 a.m. UTC
Hi Mark, Pierre-Louis
Cc each ASoC driver maintainer

As we discussed in [1], we don't need to use dpcm_playback/capture flag,
so we remove it. But we have been using it for 10 years, some driver might
get damage. The most likely case is that the device/driver can use both
playback/capture, but have only one flag, and not using xxx_only flag.
[1/3] patch indicates warning in such case.

And because of its history, DPCM has been checking CPU side only. But it should
check Codec side too same as non-DPCM. Some device/driver has been bypassed
this check. It should be error. [2/3] patch indicates warning in such case.

Because dpcm_xxx flag is no longer used by [1/3] patch, 
snd_soc_dai_link_set_capabilities() is no longer needed. [3/3] patch remove it.

These adds grace time for DPCM cleanup.
I'm not sure when dpcm_xxx will be removed, and Codec check bypass will be error,
but maybe v6.11 or v6.12 ?
Please check each driver by that time.

[1] https://lore.kernel.org/r/87edaym2cg.wl-kuninori.morimoto.gx@renesas.com

Kuninori Morimoto (3):
  ASoC: soc-pcm: Indicate warning if dpcm_playback/capture were used for availability limition
  ASoC: soc-pcm: Indicate warning if CPU / Codec availability mismatch
  ASoC: remove snd_soc_dai_link_set_capabilities()

 include/sound/soc-dai.h               |  1 -
 include/sound/soc.h                   |  1 +
 sound/soc/fsl/imx-card.c              |  3 -
 sound/soc/generic/audio-graph-card.c  |  2 -
 sound/soc/generic/audio-graph-card2.c |  2 -
 sound/soc/generic/simple-card.c       |  2 -
 sound/soc/meson/axg-card.c            |  1 -
 sound/soc/meson/gx-card.c             |  1 -
 sound/soc/qcom/common.c               |  1 -
 sound/soc/soc-dai.c                   | 38 -----------
 sound/soc/soc-pcm.c                   | 96 ++++++++++++++++++---------
 11 files changed, 67 insertions(+), 81 deletions(-)

Comments

Jerome Brunet May 7, 2024, 8:47 a.m. UTC | #1
On Tue 07 May 2024 at 04:32, Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> wrote:

> Hi Mark, Pierre-Louis
> Cc each ASoC driver maintainer
>
> As we discussed in [1], we don't need to use dpcm_playback/capture flag,
> so we remove it. But we have been using it for 10 years, some driver might
> get damage. The most likely case is that the device/driver can use both
> playback/capture, but have only one flag, and not using xxx_only flag.
> [1/3] patch indicates warning in such case.
>
> And because of its history, DPCM has been checking CPU side only. But it should
> check Codec side too same as non-DPCM. Some device/driver has been bypassed
> this check. It should be error. [2/3] patch indicates warning in such case.
>
> Because dpcm_xxx flag is no longer used by [1/3] patch, 
> snd_soc_dai_link_set_capabilities() is no longer needed. [3/3] patch remove it.
>
> These adds grace time for DPCM cleanup.
> I'm not sure when dpcm_xxx will be removed, and Codec check bypass will be error,
> but maybe v6.11 or v6.12 ?
> Please check each driver by that time.

Hi Kuninori-san,

I have tested this series on an Amlogic device (vim3l)
This brings warnings for cases which are perfectly fine.

For example, one of the DPCM backends is the TDM interface. This
interface is capable of both playback and capture. It can be associated
with any i2s/TDM codec.

The codec may do playback and capture too, but it
may also do a single direction. Then usual example is the hdmi codec
which does playback only.

In this case I get:
 axg-sound-card sound: CPU capture is available but Codec capture is not (be.dai-link-6) Please update Codec driver

I don't think this is right.

>
> [1] https://lore.kernel.org/r/87edaym2cg.wl-kuninori.morimoto.gx@renesas.com
>
> Kuninori Morimoto (3):
>   ASoC: soc-pcm: Indicate warning if dpcm_playback/capture were used for availability limition
>   ASoC: soc-pcm: Indicate warning if CPU / Codec availability mismatch
>   ASoC: remove snd_soc_dai_link_set_capabilities()
>
>  include/sound/soc-dai.h               |  1 -
>  include/sound/soc.h                   |  1 +
>  sound/soc/fsl/imx-card.c              |  3 -
>  sound/soc/generic/audio-graph-card.c  |  2 -
>  sound/soc/generic/audio-graph-card2.c |  2 -
>  sound/soc/generic/simple-card.c       |  2 -
>  sound/soc/meson/axg-card.c            |  1 -
>  sound/soc/meson/gx-card.c             |  1 -
>  sound/soc/qcom/common.c               |  1 -
>  sound/soc/soc-dai.c                   | 38 -----------
>  sound/soc/soc-pcm.c                   | 96 ++++++++++++++++++---------
>  11 files changed, 67 insertions(+), 81 deletions(-)
Kuninori Morimoto May 8, 2024, 12:06 a.m. UTC | #2
Hi Jerome

Thank you for testing

> I have tested this series on an Amlogic device (vim3l)
> This brings warnings for cases which are perfectly fine.
> 
> For example, one of the DPCM backends is the TDM interface. This
> interface is capable of both playback and capture. It can be associated
> with any i2s/TDM codec.
> 
> The codec may do playback and capture too, but it
> may also do a single direction. Then usual example is the hdmi codec
> which does playback only.
> 
> In this case I get:
>  axg-sound-card sound: CPU capture is available but Codec capture is not (be.dai-link-6) Please update Codec driver
> 
> I don't think this is right.

Hmm..., I'm confusing
Does it mean you want to use "playback only" on it ?
If so, did you get below warning too ?
	 "both playback/capture are available, but not using playback_only flag (%s)\n",

If not, can you please fill below ?

Card
	dpcm_playback = (0 or 1)
	dpcm_capture  = (0 or 1)
	playback_only = (0 or 1)
	capture_only  = (0 or 1)
BE.CPU
	playback = (available, not available)
	capture  = (available, not available)
BE.Codec
	playback = (available, not available)
	capture  = (available, not available)
Expect
	playback = (available, not available)
	capture  = (available, not available)

Thank you for your help !!

Best regards
---
Renesas Electronics
Ph.D. Kuninori Morimoto
Kuninori Morimoto May 9, 2024, 5:50 a.m. UTC | #3
Hi Jerome

I need your help

> > I have tested this series on an Amlogic device (vim3l)
> > This brings warnings for cases which are perfectly fine.
> > 
> > For example, one of the DPCM backends is the TDM interface. This
> > interface is capable of both playback and capture. It can be associated
> > with any i2s/TDM codec.
> > 
> > The codec may do playback and capture too, but it
> > may also do a single direction. Then usual example is the hdmi codec
> > which does playback only.
> > 
> > In this case I get:
> >  axg-sound-card sound: CPU capture is available but Codec capture is not (be.dai-link-6) Please update Codec driver
> > 
> > I don't think this is right.
> 
> Hmm..., I'm confusing
> Does it mean you want to use "playback only" on it ?
> If so, did you get below warning too ?
> 	 "both playback/capture are available, but not using playback_only flag (%s)\n",
> 
> If not, can you please fill below ?
> 
> Card
> 	dpcm_playback = (0 or 1)
> 	dpcm_capture  = (0 or 1)
> 	playback_only = (0 or 1)
> 	capture_only  = (0 or 1)
> BE.CPU
> 	playback = (available, not available)
> 	capture  = (available, not available)
> BE.Codec
> 	playback = (available, not available)
> 	capture  = (available, not available)
> Expect
> 	playback = (available, not available)
> 	capture  = (available, not available)

I need feedback from you, it is still not clear for me.
But I noticed that we want to update below. I'm happy if it can solve your
issue.

-	if (has_playback && !has_playback_both)
+	if (has_playback && !has_playback_both && !dai_link->capture_only)
		dev_warn(rtd->card->dev, ...)

-	if (has_capture && !has_capture_both)
+	if (has_capture && !has_capture_both && !dai_link->playback_only)
		dev_warn(rtd->card->dev, ...)



Thank you for your help !!

Best regards
---
Renesas Electronics
Ph.D. Kuninori Morimoto
Jerome Brunet May 9, 2024, 8:51 a.m. UTC | #4
On Thu 09 May 2024 at 05:50, Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> wrote:

> Hi Jerome
>
> I need your help
>
>> > I have tested this series on an Amlogic device (vim3l)
>> > This brings warnings for cases which are perfectly fine.
>> > 
>> > For example, one of the DPCM backends is the TDM interface. This
>> > interface is capable of both playback and capture. It can be associated
>> > with any i2s/TDM codec.
>> > 
>> > The codec may do playback and capture too, but it
>> > may also do a single direction. Then usual example is the hdmi codec
>> > which does playback only.
>> > 
>> > In this case I get:
>> >  axg-sound-card sound: CPU capture is available but Codec capture is not (be.dai-link-6) Please update Codec driver
>> > 
>> > I don't think this is right.
>> 
>> Hmm..., I'm confusing
>> Does it mean you want to use "playback only" on it ?
>> If so, did you get below warning too ?
>> 	 "both playback/capture are available, but not using playback_only flag (%s)\n",
>>

I've checked. No such trace, no.

>> If not, can you please fill below ?
>> 
>> Card
>> 	dpcm_playback = (0 or 1)
>> 	dpcm_capture  = (0 or 1)
>> 	playback_only = (0 or 1)
>> 	capture_only  = (0 or 1)
>> BE.CPU
>> 	playback = (available, not available)
>> 	capture  = (available, not available)
>> BE.Codec
>> 	playback = (available, not available)
>> 	capture  = (available, not available)
>> Expect
>> 	playback = (available, not available)
>> 	capture  = (available, not available)

I'm not too sure I undestand this. I'll try to illustrate the case
raising the warning as precisely as possible bellow

>
> I need feedback from you, it is still not clear for me.

Sorry. There are some national holidays in France. I'm not spending much
time near the keyboard ATM

> But I noticed that we want to update below. I'm happy if it can solve your
> issue.
>
> -	if (has_playback && !has_playback_both)
> +	if (has_playback && !has_playback_both && !dai_link->capture_only)
> 		dev_warn(rtd->card->dev, ...)
>
> -	if (has_capture && !has_capture_both)
> +	if (has_capture && !has_capture_both && !dai_link->playback_only)
> 		dev_warn(rtd->card->dev, ...)
>

Honestly I'm a bit lost in all these flag :/

Some for BE error reported here is the full picture

     PCM
 =====|=====
      V
    --------
    |CPU FE| This CPU (FIFO) does Playback only. 
    --------
      |  Because of the CPU, link a playback only one
      V
    ----------
    |Codec FE| Using Dummy here
    ----------
      |
      V
    ~~~~~~~~~~
    |Routing |
    ~~~~~~~~~~
      ^
      |
      V
    --------
    |CPU BE|  This is the TDM interface. Capable of both Playback and
    --------  Capture. Through routing it can be connected to Playback
      ^       and/or Capture FE CPUs.
      |
      |
      V
    -------------
    |BE Codec(s)| Possibly N codecs, supporting both direction, or a
    ------------- Single one, or one direction each. In this particular case
      |           it is Playback only C2C.
      |
 ---- V ------------------ From Here, it is specific to HDMI -------------
    ~~~~~~~~~~~~~
    |C2C Routing| SoC has routing has mux between the different TDM
    ~~~~~~~~~~~~~ interfaces and the HDMI controller
      |
      |
      V
    ---------
    |C2C CPU|     This is a playback only CPU for HDMI
    ---------
      |
      V
    -----------
    |HDMI Codec|  The usual HDMI codec, playback only
    -----------

Better picture are available in the SoC doc.
There is publicly available datasheet here [1]

Audio paths are displayed in Section 9, page 807, Figure 9-1.
A TDM interface in this figure is combination of a TDMOUT and a TDMIN
(axg-tdm-formatter.c). The axg-tdm-interface.c joins them because they
use the same pads and clocks.

[1]: https://dl.khadas.com/products/vim3l/datasheet/s905d3_datasheet_0.2_wesion.pdf

>
>
> Thank you for your help !!
>
> Best regards
> ---
> Renesas Electronics
> Ph.D. Kuninori Morimoto
Kuninori Morimoto May 9, 2024, 11:42 p.m. UTC | #5
Hi Jerome

Thank you for your reply

> >> If so, did you get below warning too ?
> >> 	 "both playback/capture are available, but not using playback_only flag (%s)\n",
> >>
> 
> I've checked. No such trace, no.

OK, thanks

> >> Card
> >> 	dpcm_playback = (0 or 1)
> >> 	dpcm_capture  = (0 or 1)
> >> 	playback_only = (0 or 1)
> >> 	capture_only  = (0 or 1)
> >> BE.CPU
> >> 	playback = (available, not available)
> >> 	capture  = (available, not available)
> >> BE.Codec
> >> 	playback = (available, not available)
> >> 	capture  = (available, not available)
> >> Expect
> >> 	playback = (available, not available)
> >> 	capture  = (available, not available)
> 
> I'm not too sure I undestand this. I'll try to illustrate the case
> raising the warning as precisely as possible bellow

Thanks

Because you got was

(A)	axg-sound-card sound: CPU capture is available but Codec capture is
	not (be.dai-link-6) Please update Codec driver

It is for BE. And validation check is for each rtd only, this means it checks
BE only, relationship with other rtd is not related here.

>     --------
>     |CPU BE|  This is the TDM interface. Capable of both Playback and
>     --------  Capture. Through routing it can be connected to Playback
>       ^       and/or Capture FE CPUs.
>       |
>       V
>     -------------
>     |BE Codec(s)| Possibly N codecs, supporting both direction, or a
>     ------------- Single one, or one direction each. In this particular case
>       |           it is Playback only C2C.

So, I think the warning happen here.
The validation check is checking this BE only.

As I mentioned above, you use this BE through playback only FE and/or C2C,
but that relationship is not related to here.

According to above explanation, this BE itself is available for both
playback and capture. And you didn't get below warning, I guess this BE
has both dpcm_playback/capture flag, and no xxx_only flag.

	 "both playback/capture are available, but not using playback_only
	  flag (%s)\n",

Before my patch, the validation check is checks CPU-BE only, but it also
checks BE-Codec after my patch, and you got the warning (A).

So, I guess your BE-Codec simply missing capture channels_min settings.
Please double check it, or please tell me which codec driver this BE is
using.

	static struct snd_soc_dai_driver xxx_dai = {
		...
		.playback = {
			...
			.channels_min	= x,
			...
		},
		.capture = {
			...
=>			.channels_min	= x,
			...
		},
	},

> But I noticed that we want to update below. I'm happy if it can solve your
> issue.
>
> -	if (has_playback && !has_playback_both)
> +	if (has_playback && !has_playback_both && !dai_link->capture_only)
> 		dev_warn(rtd->card->dev, ...)
>
> -	if (has_capture && !has_capture_both)
> +	if (has_capture && !has_capture_both && !dai_link->playback_only)
> 		dev_warn(rtd->card->dev, ...)
>
> Honestly I'm a bit lost in all these flag :/

Thanks, no problem, me too :9

Unfortunately and confusingly, there are many combination exist around here.
But because of your feedback, I noticed one missing pattern. Thanks


Thank you for your help !!

Best regards
---
Renesas Electronics
Ph.D. Kuninori Morimoto
Jerome Brunet May 12, 2024, 9:33 a.m. UTC | #6
On Thu 09 May 2024 at 23:42, Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> wrote:

> Hi Jerome
>
> Thank you for your reply
>
>> >> If so, did you get below warning too ?
>> >> 	 "both playback/capture are available, but not using playback_only flag (%s)\n",
>> >>
>> 
>> I've checked. No such trace, no.
>
> OK, thanks
>
>> >> Card
>> >> 	dpcm_playback = (0 or 1)
>> >> 	dpcm_capture  = (0 or 1)
>> >> 	playback_only = (0 or 1)
>> >> 	capture_only  = (0 or 1)
>> >> BE.CPU
>> >> 	playback = (available, not available)
>> >> 	capture  = (available, not available)
>> >> BE.Codec
>> >> 	playback = (available, not available)
>> >> 	capture  = (available, not available)
>> >> Expect
>> >> 	playback = (available, not available)
>> >> 	capture  = (available, not available)
>> 
>> I'm not too sure I undestand this. I'll try to illustrate the case
>> raising the warning as precisely as possible bellow
>
> Thanks
>
> Because you got was
>
> (A)	axg-sound-card sound: CPU capture is available but Codec capture is
> 	not (be.dai-link-6) Please update Codec driver
>
> It is for BE. And validation check is for each rtd only, this means it checks
> BE only, relationship with other rtd is not related here.
>
>>     --------
>>     |CPU BE|  This is the TDM interface. Capable of both Playback and
>>     --------  Capture. Through routing it can be connected to Playback
>>       ^       and/or Capture FE CPUs.
>>       |
>>       V
>>     -------------
>>     |BE Codec(s)| Possibly N codecs, supporting both direction, or a
>>     ------------- Single one, or one direction each. In this particular case
>>       |           it is Playback only C2C.
>
> So, I think the warning happen here.
> The validation check is checking this BE only.
>
> As I mentioned above, you use this BE through playback only FE and/or C2C,
> but that relationship is not related to here.
>
> According to above explanation, this BE itself is available for both
> playback and capture. And you didn't get below warning, I guess this BE
> has both dpcm_playback/capture flag, and no xxx_only flag.
>
> 	 "both playback/capture are available, but not using playback_only
> 	  flag (%s)\n",
>

Apparently so, but it should not.

Before your patchset, axg-card.c (and gx-card.c) relied on
snd_soc_dai_link_set_capabilities() to init dpcm_playback/capture flags.

That was done following another semantic change on those flags:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/sound/soc/meson?h=v6.9-rc7&id=da3f23fde9d7b4a7e0ca9a9a096cec3104df1b82

 - 1st problem: I see that following your removal of
   snd_soc_dai_link_set_capabilities(), the dpcm_playback/capture flags
   are no longer properly initialised in the amlogic card drivers.
   That will need fixing.

Then, for TDM backends (like here), the tdm slots are checked and the
direction is disabled if it has no slots:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/sound/soc/meson/axg-card.c?h=v6.9-rc7#n171

In theory, dpcm_capture should be 0 for this link which has no Rx
slot:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/amlogic/meson-khadas-vim3.dtsi?h=v6.9-rc7#n218

... Sooo, that is 2nd problem

> Before my patch, the validation check is checks CPU-BE only, but it also
> checks BE-Codec after my patch, and you got the warning (A).

Yes I've got that. I was not expecting a failure on this case TBH.

I was more looking for the infamous 2 codecs case, each with a single
direction (which I did not check yet ...)

>
> So, I guess your BE-Codec simply missing capture channels_min settings.
> Please double check it, or please tell me which codec driver this BE is
> using.

Here the codec:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/sound/soc/meson/g12a-tohdmitx.c?h=v6.9-rc7

>
> 	static struct snd_soc_dai_driver xxx_dai = {
> 		...
> 		.playback = {
> 			...
> 			.channels_min	= x,
> 			...
> 		},
> 		.capture = {
> 			...
> =>			.channels_min	= x,
> 			...
> 		},

This codec is not meant to have capture channels.
I think DT description and the card driver settings (before the removal of
snd_soc_dai_link_set_capabilities()) are correct.

IMO, those check become too restrictive. We are adding a lot of code I'm
not sure I understand what we stand by going so far in the
consistency checks.

Initially those dpcm_playback/capture flag could be used to just
forcefully disable a link direction, regardless of the CPUs or codecs present
on the link. It was just another setting and it was not required to be consistent
with anything. It just declared whether the direction was allowed on the
link, or not.

It changed this commit, and the flags suddenly needed to be consistent
with whatever was on link:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/sound/soc?h=v6.9-rc7&id=b73287f0b074

I complained back then and I still don't think this change was good.

If the flags needs to consistent with what is on the link, and we have
the ability to check it, why let card drivers set it and then complain
about it in ASoC checks ? Why not deal with it in the framework directly ?

I think the simplest solution would to just go back to the initial
meaning of these flags which was just the card driver declaring the
direction allowed/disallowed on a link. Then ASoC can mix that
information with whatever it finds with DAI drivers and figure out what
is actually possible.

It would give a clear and separate semantic meaning to those flags
instead something redundant with the DAI driver info.

What we have currently is not straight forward to set and check.
It makes the code unnecessarily complicated and difficult to maintain. I
think the difficulties with this patchset are a good illustration of
that, unfortunately.

> 	},
>
>> But I noticed that we want to update below. I'm happy if it can solve your
>> issue.
>>
>> -	if (has_playback && !has_playback_both)
>> +	if (has_playback && !has_playback_both && !dai_link->capture_only)
>> 		dev_warn(rtd->card->dev, ...)
>>
>> -	if (has_capture && !has_capture_both)
>> +	if (has_capture && !has_capture_both && !dai_link->playback_only)
>> 		dev_warn(rtd->card->dev, ...)
>>
>> Honestly I'm a bit lost in all these flag :/
>
> Thanks, no problem, me too :9
>
> Unfortunately and confusingly, there are many combination exist around here.

My point exactly ;)

> But because of your feedback, I noticed one missing pattern. Thanks
>
>
> Thank you for your help !!

Thanks a lot for all those reworks !

>
> Best regards
> ---
> Renesas Electronics
> Ph.D. Kuninori Morimoto
Kuninori Morimoto May 13, 2024, 12:11 a.m. UTC | #7
Hi Jerome

Thank you for your feedback and analysis !

>  - 1st problem: I see that following your removal of
>    snd_soc_dai_link_set_capabilities(), the dpcm_playback/capture flags
>    are no longer properly initialised in the amlogic card drivers.
>    That will need fixing.
(snip)
> This codec is not meant to have capture channels.
> I think DT description and the card driver settings (before the removal of
> snd_soc_dai_link_set_capabilities()) are correct.

OK, I see. Thank you for your analysis.

The problem was my patch checks CPU direction vs Codec direction only,
thus, it will indicates unexpected warnings, like this case.

Thank you for finding it, I hope v2 patch should be OK for you.

> IMO, those check become too restrictive. We are adding a lot of code I'm
> not sure I understand what we stand by going so far in the
> consistency checks.
> 
> Initially those dpcm_playback/capture flag could be used to just
> forcefully disable a link direction, regardless of the CPUs or codecs present
> on the link. It was just another setting and it was not required to be consistent
> with anything. It just declared whether the direction was allowed on the
> link, or not.
> 
> It changed this commit, and the flags suddenly needed to be consistent
> with whatever was on link:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/sound/soc?h=v6.9-rc7&id=b73287f0b074
> 
> I complained back then and I still don't think this change was good.
> 
> If the flags needs to consistent with what is on the link, and we have
> the ability to check it, why let card drivers set it and then complain
> about it in ASoC checks ? Why not deal with it in the framework directly ?
> 
> I think the simplest solution would to just go back to the initial
> meaning of these flags which was just the card driver declaring the
> direction allowed/disallowed on a link. Then ASoC can mix that
> information with whatever it finds with DAI drivers and figure out what
> is actually possible.
> 
> It would give a clear and separate semantic meaning to those flags
> instead something redundant with the DAI driver info.
> 
> What we have currently is not straight forward to set and check.
> It makes the code unnecessarily complicated and difficult to maintain. I
> think the difficulties with this patchset are a good illustration of
> that, unfortunately.

By this patch-set, it will be handled automatically via CPU and Codec
driver settings, Card driver will no longer need to consider about
direction (like non-DPCM), and I hope people (including you) will be
happy about that.

Thank you for your help !!

Best regards
---
Renesas Electronics
Ph.D. Kuninori Morimoto
Jerome Brunet May 13, 2024, 7:36 a.m. UTC | #8
On Mon 13 May 2024 at 00:11, Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> wrote:

> Hi Jerome
>
> Thank you for your feedback and analysis !
>
>>  - 1st problem: I see that following your removal of
>>    snd_soc_dai_link_set_capabilities(), the dpcm_playback/capture flags
>>    are no longer properly initialised in the amlogic card drivers.
>>    That will need fixing.
> (snip)
>> This codec is not meant to have capture channels.
>> I think DT description and the card driver settings (before the removal of
>> snd_soc_dai_link_set_capabilities()) are correct.
>
> OK, I see. Thank you for your analysis.
>
> The problem was my patch checks CPU direction vs Codec direction only,
> thus, it will indicates unexpected warnings, like this case.
>
> Thank you for finding it, I hope v2 patch should be OK for you.
>

I'll check

>> IMO, those check become too restrictive. We are adding a lot of code I'm
>> not sure I understand what we stand by going so far in the
>> consistency checks.
>> 
>> Initially those dpcm_playback/capture flag could be used to just
>> forcefully disable a link direction, regardless of the CPUs or codecs present
>> on the link. It was just another setting and it was not required to be consistent
>> with anything. It just declared whether the direction was allowed on the
>> link, or not.
>> 
>> It changed this commit, and the flags suddenly needed to be consistent
>> with whatever was on link:
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/sound/soc?h=v6.9-rc7&id=b73287f0b074
>> 
>> I complained back then and I still don't think this change was good.
>> 
>> If the flags needs to consistent with what is on the link, and we have
>> the ability to check it, why let card drivers set it and then complain
>> about it in ASoC checks ? Why not deal with it in the framework directly ?
>> 
>> I think the simplest solution would to just go back to the initial
>> meaning of these flags which was just the card driver declaring the
>> direction allowed/disallowed on a link. Then ASoC can mix that
>> information with whatever it finds with DAI drivers and figure out what
>> is actually possible.
>> 
>> It would give a clear and separate semantic meaning to those flags
>> instead something redundant with the DAI driver info.
>> 
>> What we have currently is not straight forward to set and check.
>> It makes the code unnecessarily complicated and difficult to maintain. I
>> think the difficulties with this patchset are a good illustration of
>> that, unfortunately.
>
> By this patch-set, it will be handled automatically via CPU and Codec
> driver settings, Card driver will no longer need to consider about
> direction (like non-DPCM), and I hope people (including you) will be
> happy about that.
>

If it makes things simpler, I am happy for sure :)

However, I'm a bit confused. If it is handled automatically by the CPUs
and Codecs settings, does it mean dpcm_playback/capture flags are
no-ops from now on ?

Should I update my card drivers to ditch those flags completely ?

May I still disable a direction on a link from the card driver, like in
the case I described above, when a TDM link has no slots for a direction ?

> Thank you for your help !!
>
> Best regards
> ---
> Renesas Electronics
> Ph.D. Kuninori Morimoto
Kuninori Morimoto May 13, 2024, 11:42 p.m. UTC | #9
Hi Jerome

Thank you for your reply

> However, I'm a bit confused. If it is handled automatically by the CPUs
> and Codecs settings, does it mean dpcm_playback/capture flags are
> no-ops from now on ?

Yes.
dpcm_playback/capture flag itself will be exist for a while, but it will be
removed soon (v6.11 ? v6.12 ? not yet fixed).

Some driver might is using dpcm_xxx flag as limitation of direction. For
example HW can use both playback/capture, but driver want to use playback
only, in this case, driver might have dpcm_playback flag only.

In this case, driver authoer need to update to use playback_only flag
instead. [1/3] patch will indicate warning about it, for a while.


> Should I update my card drivers to ditch those flags completely ?

If the driver is using dpcm_xxx flag as limitation of direction,
driver author need to update to use xxx_only flag.
If the driver have no such flag miss, I will remove all dpcm_xxx flags
when end of its support.
Of course we can avoid extra problem if each driver author remove/update
it by themself, instead of me ;P

> May I still disable a direction on a link from the card driver, like in
> the case I described above, when a TDM link has no slots for a direction ?

For example, in case of CPU can handle both playback/capture, and Codec
handles playback only, it will be playback only automatically, no Card
settings is needed.

If both CPU/Codec can handle playback/capture, but you want to enable
playback only, you can use playback_only flag.

Is it help you ?



Thank you for your help !!

Best regards
---
Renesas Electronics
Ph.D. Kuninori Morimoto
Jerome Brunet May 14, 2024, 9:04 a.m. UTC | #10
On Mon 13 May 2024 at 23:42, Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> wrote:

> Hi Jerome
>
> Thank you for your reply
>
>> However, I'm a bit confused. If it is handled automatically by the CPUs
>> and Codecs settings, does it mean dpcm_playback/capture flags are
>> no-ops from now on ?
>
> Yes.
> dpcm_playback/capture flag itself will be exist for a while, but it will be
> removed soon (v6.11 ? v6.12 ? not yet fixed).
>
> Some driver might is using dpcm_xxx flag as limitation of direction. For
> example HW can use both playback/capture, but driver want to use playback
> only, in this case, driver might have dpcm_playback flag only.
>
> In this case, driver authoer need to update to use playback_only flag
> instead. [1/3] patch will indicate warning about it, for a while.
>
>
>> Should I update my card drivers to ditch those flags completely ?
>
> If the driver is using dpcm_xxx flag as limitation of direction,
> driver author need to update to use xxx_only flag.
> If the driver have no such flag miss, I will remove all dpcm_xxx flags
> when end of its support.
> Of course we can avoid extra problem if each driver author remove/update
> it by themself, instead of me ;P

Makes sense. I'll check your v2 and prepare the update of the related
card driver

>
>> May I still disable a direction on a link from the card driver, like in
>> the case I described above, when a TDM link has no slots for a direction ?
>
> For example, in case of CPU can handle both playback/capture, and Codec
> handles playback only, it will be playback only automatically, no Card
> settings is needed.
>
> If both CPU/Codec can handle playback/capture, but you want to enable
> playback only, you can use playback_only flag.
>
> Is it help you ?
>

Path forward is clear. Thanks a lot.

>
>
> Thank you for your help !!
>
> Best regards
> ---
> Renesas Electronics
> Ph.D. Kuninori Morimoto