meson-aiu: HDMI codec .prepare() callback not called

Commit Message

Martin Blumenstingl Dec. 31, 2024, 6:44 p.m. UTC

Hi Jerome,

I am still working on a HDMI controller driver for Meson8/8b/8m2.
HDMI output would be incomplete without audio.
There's a great series from Dmitry [0] which simplifies the HDMI
controller driver implementation by moving all the hdmi-codec related
management to a generic framework.

I tried integrating Dmitry's work [0] into my HDMI controller driver.
While testing I found that hdmi-codec's .prepare() callback is not
called at all and asked Dmitry for help [1]. All other callbacks
(.hw_params, .startup, .shutdown, ...) however are called.
On his board (Qualcomm SDM845, sdm845-db845c.dts) hdmi_codec_prepare()
is called by snd_soc_pcm_dai_prepare() (core sound framework, not
platform specific).
However, on my Odroid-C1 this is not happening.

Looking further snd_soc_pcm_dai_prepare() I can see that
for_each_rtd_dais() has three entries:
  snd_soc_pcm_dai_prepare(), dai name=I2S Encoder, id=2
  snd_soc_pcm_dai_prepare(), dai name=CODEC CTRL HDMI I2S IN, id=0
  snd_soc_pcm_dai_prepare(), dai name=I2S FIFO, id=0
What I'm missing here is a dai name "i2s-hifi" (from
sound/soc/codecs/hdmi-codec.c, see hdmi_i2s_dai)

My hdmi_tx node looks like this (abbreviated):
  hdmi_tx: hdmi-tx@42000 {
      compatible = "amlogic,meson8b-hdmi-tx";
      reg = <0x42000 0xc>;
      #sound-dai-cells = <1>;
       ...
  };

Then I have a "amlogic,gx-sound-card" instance with the following dai-links:
  dai-link-0 {
      sound-dai = <&aiu AIU_CPU CPU_I2S_FIFO>;
  };
  dai-link-1 {
      sound-dai = <&aiu AIU_CPU CPU_I2S_ENCODER>;
      dai-format = "i2s";
      mclk-fs = <256>;

      codec-0 {
          sound-dai = <&aiu AIU_HDMI CTRL_I2S>;
    };
  };
  dai-link-2 {
      sound-dai = <&aiu AIU_HDMI CTRL_OUT>;

      codec-0 {
          sound-dai = <&hdmi_tx 0>;
      };
  };
So apart from the additional cell in the sound-dai towards hdmi_tx
(Meson8/8b/8m2's HDMI controller has two inputs: I2C and SPDIF. I2C is
the first one, hence &hdmi_tx 0) this is identical to
arch/arm64/boot/dts/amlogic/meson-gxbb-odroidc2.dts.

I have further verified that the gx-card parsing does find the HDMi
controller and links it correctly.
To me it's odd that only the .prepare() callback is not called, all
others (as mentioned above: .hw_params, .startup, ...) are working
fine.

I'm not familiar with the sound stack and I'm hoping that you have
some ideas on how to either debug or fix it.


Thanks in advance and best regards,
Martin


[0] https://lore.kernel.org/dri-devel/[e-mail]/
[1] https://lore.kernel.org/dri-devel/[e-mail]/
[2] https://lore.kernel.org/dri-devel/l3u3wtnxyhrwjynevkwfjwarisc4yt4xy2rbzf5kb7k5l5dw3n@lxqtimymyjg6/

Comments

Jerome Brunet Jan. 6, 2025, 10:44 a.m. UTC | #1

On Tue 31 Dec 2024 at 19:44, Martin Blumenstingl <[e-mail]> wrote:

> Hi Jerome,
>
> I am still working on a HDMI controller driver for Meson8/8b/8m2.
> HDMI output would be incomplete without audio.
> There's a great series from Dmitry [0] which simplifies the HDMI
> controller driver implementation by moving all the hdmi-codec related
> management to a generic framework.
>
> I tried integrating Dmitry's work [0] into my HDMI controller driver.
> While testing I found that hdmi-codec's .prepare() callback is not
> called at all and asked Dmitry for help [1]. All other callbacks
> (.hw_params, .startup, .shutdown, ...) however are called.
> On his board (Qualcomm SDM845, sdm845-db845c.dts) hdmi_codec_prepare()
> is called by snd_soc_pcm_dai_prepare() (core sound framework, not
> platform specific).
> However, on my Odroid-C1 this is not happening.
>
> Looking further snd_soc_pcm_dai_prepare() I can see that
> for_each_rtd_dais() has three entries:
>   snd_soc_pcm_dai_prepare(), dai name=I2S Encoder, id=2
>   snd_soc_pcm_dai_prepare(), dai name=CODEC CTRL HDMI I2S IN, id=0
>   snd_soc_pcm_dai_prepare(), dai name=I2S FIFO, id=0
> What I'm missing here is a dai name "i2s-hifi" (from
> sound/soc/codecs/hdmi-codec.c, see hdmi_i2s_dai)
>
> My hdmi_tx node looks like this (abbreviated):
>   hdmi_tx: hdmi-tx@42000 {
>       compatible = "amlogic,meson8b-hdmi-tx";
>       reg = <0x42000 0xc>;
>       #sound-dai-cells = <1>;
>        ...
>   };
>
> Then I have a "amlogic,gx-sound-card" instance with the following dai-links:
>   dai-link-0 {
>       sound-dai = <&aiu AIU_CPU CPU_I2S_FIFO>;
>   };
>   dai-link-1 {
>       sound-dai = <&aiu AIU_CPU CPU_I2S_ENCODER>;
>       dai-format = "i2s";
>       mclk-fs = <256>;
>
>       codec-0 {
>           sound-dai = <&aiu AIU_HDMI CTRL_I2S>;
>     };
>   };
>   dai-link-2 {
>       sound-dai = <&aiu AIU_HDMI CTRL_OUT>;
>
>       codec-0 {
>           sound-dai = <&hdmi_tx 0>;
>       };
>   };
> So apart from the additional cell in the sound-dai towards hdmi_tx
> (Meson8/8b/8m2's HDMI controller has two inputs: I2C and SPDIF. I2C is
> the first one, hence &hdmi_tx 0) this is identical to
> arch/arm64/boot/dts/amlogic/meson-gxbb-odroidc2.dts.
>
> I have further verified that the gx-card parsing does find the HDMi
> controller and links it correctly.
> To me it's odd that only the .prepare() callback is not called, all
> others (as mentioned above: .hw_params, .startup, ...) are working
> fine.

I think the problem you are seeing comes from the quirk of
codec-to-codec links. The hdmi codec link is such a link on Amlogic
because further digital routing is required after the backend.

Those type of links are not used much beside some
CPU offloading on Samsung and Amlogic, as far as I know.
It is possible, even likely, that things are still missing there.

So those C2C links are operated by the DAPM events, not the regualar
ASoC code. You can start here:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/sound/soc/soc-dapm.c#n3995

You'll see that .prepare() is not called, same as .trigger()
That should propably be fixed :/

Side Note:
While this type of link is not used much, I think that - fully
implemented - it could hold the key to proper DAI chaining ... possibly
allowing to get rid of DPCM.

>
> I'm not familiar with the sound stack and I'm hoping that you have
> some ideas on how to either debug or fix it.
>
>
> Thanks in advance and best regards,
> Martin
>
>
> [0] https://lore.kernel.org/dri-devel/[e-mail]/
> [1] https://lore.kernel.org/dri-devel/[e-mail]/
> [2] https://lore.kernel.org/dri-devel/l3u3wtnxyhrwjynevkwfjwarisc4yt4xy2rbzf5kb7k5l5dw3n@lxqtimymyjg6/
>
> [2. text/x-patch; sound-soc-soc-dai-snd_soc_pcm_dai_prepare-info.diff]...

Martin Blumenstingl Jan. 6, 2025, 12:49 p.m. UTC | #2

Hi Jerome,

On Mon, Jan 6, 2025 at 11:44 AM Jerome Brunet <[e-mail]> wrote:
[...]
> > I have further verified that the gx-card parsing does find the HDMi
> > controller and links it correctly.
> > To me it's odd that only the .prepare() callback is not called, all
> > others (as mentioned above: .hw_params, .startup, ...) are working
> > fine.
>
> I think the problem you are seeing comes from the quirk of
> codec-to-codec links. The hdmi codec link is such a link on Amlogic
> because further digital routing is required after the backend.
>
> Those type of links are not used much beside some
> CPU offloading on Samsung and Amlogic, as far as I know.
> It is possible, even likely, that things are still missing there.
>
> So those C2C links are operated by the DAPM events, not the regualar
> ASoC code. You can start here:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/sound/soc/soc-dapm.c#n3995
Thank you - that is indeed the root cause!

> You'll see that .prepare() is not called, same as .trigger()
> That should propably be fixed :/
Since I'm still very much clueless about all of this I just came up
with an experimental patch. Any feedback on it is welcome (I can send
it as RFC patch - but prepare for me needing support).
With the attached patch I now see hdmi-codec's .prepare callback being called:
$ speaker-test -c2

speaker-test 1.2.13

Playback device is default
Stream parameters are 48000Hz, S16_LE, 2 channels
Using 16 octaves of pink noise
Rate set to 48000Hz (requested 48000Hz)
Buffer size range from 128 to 262144
Period size range from 64 to 131072
Periods = 4
[   42.043413] hdmi-audio-codec hdmi-audio-codec.0.auto:
hdmi_codec_hw_params() width 16 rate 48000 channels 2
[   42.047916] hdmi-audio-codec hdmi-audio-codec.0.auto:
hdmi_codec_prepare() width 16 rate 48000 channels 2



Best regards,
Martin

Jerome Brunet Jan. 6, 2025, 1:24 p.m. UTC | #3

On Mon 06 Jan 2025 at 13:49, Martin Blumenstingl <[e-mail]> wrote:

> Hi Jerome,
>
> On Mon, Jan 6, 2025 at 11:44 AM Jerome Brunet <[e-mail]> wrote:
> [...]
>> > I have further verified that the gx-card parsing does find the HDMi
>> > controller and links it correctly.
>> > To me it's odd that only the .prepare() callback is not called, all
>> > others (as mentioned above: .hw_params, .startup, ...) are working
>> > fine.
>>
>> I think the problem you are seeing comes from the quirk of
>> codec-to-codec links. The hdmi codec link is such a link on Amlogic
>> because further digital routing is required after the backend.
>>
>> Those type of links are not used much beside some
>> CPU offloading on Samsung and Amlogic, as far as I know.
>> It is possible, even likely, that things are still missing there.
>>
>> So those C2C links are operated by the DAPM events, not the regualar
>> ASoC code. You can start here:
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/sound/soc/soc-dapm.c#n3995
> Thank you - that is indeed the root cause!
>
>> You'll see that .prepare() is not called, same as .trigger()
>> That should propably be fixed :/
> Since I'm still very much clueless about all of this I just came up
> with an experimental patch. Any feedback on it is welcome (I can send
> it as RFC patch - but prepare for me needing support).

It is probably a good idea to send it to get more feedback, yes.

> With the attached patch I now see hdmi-codec's .prepare callback being called:

Looks like a good start :)

Couples of nitpicks:

You might want to check the stream validity in
the function you've introduced, like in soc_dai_trigger()

It may also be nice to update snd_soc_pcm_dai_prepare() with the
introduced function.

> $ speaker-test -c2
>
> speaker-test 1.2.13
>
> Playback device is default
> Stream parameters are 48000Hz, S16_LE, 2 channels
> Using 16 octaves of pink noise
> Rate set to 48000Hz (requested 48000Hz)
> Buffer size range from 128 to 262144
> Period size range from 64 to 131072
> Periods = 4
> [   42.043413] hdmi-audio-codec hdmi-audio-codec.0.auto:
> hdmi_codec_hw_params() width 16 rate 48000 channels 2
> [   42.047916] hdmi-audio-codec hdmi-audio-codec.0.auto:
> hdmi_codec_prepare() width 16 rate 48000 channels 2
>
>
>
> Best regards,
> Martin
>
> [2. text/x-patch; soc-damp-call-prepare.diff]...

Martin Blumenstingl Jan. 6, 2025, 1:38 p.m. UTC | #4

Hi Jerome,

On Mon, Jan 6, 2025 at 2:24 PM Jerome Brunet <[e-mail]> wrote:
>
> On Mon 06 Jan 2025 at 13:49, Martin Blumenstingl <[e-mail]> wrote:
>
> > Hi Jerome,
> >
> > On Mon, Jan 6, 2025 at 11:44 AM Jerome Brunet <[e-mail]> wrote:
> > [...]
> >> > I have further verified that the gx-card parsing does find the HDMi
> >> > controller and links it correctly.
> >> > To me it's odd that only the .prepare() callback is not called, all
> >> > others (as mentioned above: .hw_params, .startup, ...) are working
> >> > fine.
> >>
> >> I think the problem you are seeing comes from the quirk of
> >> codec-to-codec links. The hdmi codec link is such a link on Amlogic
> >> because further digital routing is required after the backend.
> >>
> >> Those type of links are not used much beside some
> >> CPU offloading on Samsung and Amlogic, as far as I know.
> >> It is possible, even likely, that things are still missing there.
> >>
> >> So those C2C links are operated by the DAPM events, not the regualar
> >> ASoC code. You can start here:
> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/sound/soc/soc-dapm.c#n3995
> > Thank you - that is indeed the root cause!
> >
> >> You'll see that .prepare() is not called, same as .trigger()
> >> That should propably be fixed :/
> > Since I'm still very much clueless about all of this I just came up
> > with an experimental patch. Any feedback on it is welcome (I can send
> > it as RFC patch - but prepare for me needing support).
>
> It is probably a good idea to send it to get more feedback, yes.
Sure, I'll do that later and keep you Cc'ed.

> > With the attached patch I now see hdmi-codec's .prepare callback being called:
>
> Looks like a good start :)
>
> Couples of nitpicks:
>
> You might want to check the stream validity in
> the function you've introduced, like in soc_dai_trigger()
snd_soc_dai_hw_params() doesn't check it either but snd_soc_dai_startup() does.

> It may also be nice to update snd_soc_pcm_dai_prepare() with the
> introduced function.
Ah, this one is also checking for substream validity. That explains
your previous suggestion :-)


Best regards,
Martin

Martin Blumenstingl Jan. 6, 2025, 2:15 p.m. UTC | #5

On Mon, Jan 6, 2025 at 2:38 PM Martin Blumenstingl
<[e-mail]> wrote:
>
> Hi Jerome,
>
> On Mon, Jan 6, 2025 at 2:24 PM Jerome Brunet <[e-mail]> wrote:
> >
> > On Mon 06 Jan 2025 at 13:49, Martin Blumenstingl <[e-mail]> wrote:
> >
> > > Hi Jerome,
> > >
> > > On Mon, Jan 6, 2025 at 11:44 AM Jerome Brunet <[e-mail]> wrote:
> > > [...]
> > >> > I have further verified that the gx-card parsing does find the HDMi
> > >> > controller and links it correctly.
> > >> > To me it's odd that only the .prepare() callback is not called, all
> > >> > others (as mentioned above: .hw_params, .startup, ...) are working
> > >> > fine.
> > >>
> > >> I think the problem you are seeing comes from the quirk of
> > >> codec-to-codec links. The hdmi codec link is such a link on Amlogic
> > >> because further digital routing is required after the backend.
> > >>
> > >> Those type of links are not used much beside some
> > >> CPU offloading on Samsung and Amlogic, as far as I know.
> > >> It is possible, even likely, that things are still missing there.
> > >>
> > >> So those C2C links are operated by the DAPM events, not the regualar
> > >> ASoC code. You can start here:
> > >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/sound/soc/soc-dapm.c#n3995
> > > Thank you - that is indeed the root cause!
> > >
> > >> You'll see that .prepare() is not called, same as .trigger()
> > >> That should propably be fixed :/
> > > Since I'm still very much clueless about all of this I just came up
> > > with an experimental patch. Any feedback on it is welcome (I can send
> > > it as RFC patch - but prepare for me needing support).
> >
> > It is probably a good idea to send it to get more feedback, yes.
> Sure, I'll do that later and keep you Cc'ed.
patches are sent (with your feedback applied), let's continue in [0]


[0] https://lore.kernel.org/linux-sound/[e-mail]/

Patch

diff --git a/drivers/gpu/drm/display/drm_hdmi_audio_helper.c b/drivers/gpu/drm/display/drm_hdmi_audio_helper.c
index aa3a5d0329dc..05afc9f0bdd6 100644
--- a/drivers/gpu/drm/display/drm_hdmi_audio_helper.c
+++ b/drivers/gpu/drm/display/drm_hdmi_audio_helper.c
@@ -129,7 +129,6 @@  EXPORT_SYMBOL(drm_connector_hdmi_audio_plugged_notify);
 static const struct hdmi_codec_ops drm_connector_hdmi_audio_ops = {
 	.audio_startup = drm_connector_hdmi_audio_startup,
 	.prepare = drm_connector_hdmi_audio_prepare,
-	.hw_params = drm_connector_hdmi_audio_prepare,
 	.audio_shutdown = drm_connector_hdmi_audio_shutdown,
 	.mute_stream = drm_connector_hdmi_audio_mute_stream,
 	.get_eld = drm_connector_hdmi_audio_get_eld,
diff --git a/sound/soc/codecs/hdmi-codec.c b/sound/soc/codecs/hdmi-codec.c
index 69f98975e14a..a931d05ba89e 100644
--- a/sound/soc/codecs/hdmi-codec.c
+++ b/sound/soc/codecs/hdmi-codec.c
@@ -570,13 +570,13 @@  static int hdmi_codec_hw_params(struct snd_pcm_substream *substream,
 	};
 	int ret;
 
-	if (!hcp->hcd.ops->hw_params)
-		return 0;
-
-	dev_dbg(dai->dev, "%s() width %d rate %d channels %d\n", __func__,
+	dev_info(dai->dev, "%s() width %d rate %d channels %d\n", __func__,
 		params_width(params), params_rate(params),
 		params_channels(params));
 
+	if (!hcp->hcd.ops->hw_params)
+		return 0;
+
 	ret = hdmi_codec_fill_codec_params(dai,
 					   params_width(params),
 					   params_rate(params),
@@ -611,12 +611,12 @@  static int hdmi_codec_prepare(struct snd_pcm_substream *substream,
 	struct hdmi_codec_params hp;
 	int ret;
 
+	dev_info(dai->dev, "%s() width %d rate %d channels %d\n", __func__,
+		width, rate, channels);
+
 	if (!hcp->hcd.ops->prepare)
 		return 0;
 
-	dev_dbg(dai->dev, "%s() width %d rate %d channels %d\n", __func__,
-		width, rate, channels);
-
 	ret = hdmi_codec_fill_codec_params(dai, width, rate, channels, &hp);
 	if (ret < 0)
 		return ret;
diff --git a/sound/soc/soc-dai.c b/sound/soc/soc-dai.c
index 34ba1a93a4c9..030c573753cc 100644
--- a/sound/soc/soc-dai.c
+++ b/sound/soc/soc-dai.c
@@ -577,6 +577,7 @@  int snd_soc_pcm_dai_prepare(struct snd_pcm_substream *substream)
 	int i, ret;
 
 	for_each_rtd_dais(rtd, i, dai) {
+		pr_info("%s(), dai name=%s, id=%d\n", __func__, dai->name, dai->id);
 		if (!snd_soc_dai_stream_valid(dai, substream->stream))
 			continue;
 		if (dai->driver->ops &&