Re: [PATCH] ASoC: hdmi-codec: register hpd callback on component probe
From: Hsin-Yi Wang
Date: Fri Dec 15 2023 - 17:55:38 EST
On Fri, Dec 15, 2023 at 12:40 AM Jerome Brunet <jbrunet@xxxxxxxxxxxx> wrote:
>
>
> On Fri 15 Dec 2023 at 12:51, Zhengqiao Xia <xiazhengqiao@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx> wrote:
>
> > Hi Jerome,
> >
> > After my testing, I found that this patch will cause the audio on the external display to not work properly after
> > restart.
> > You move the plugged_cb to run in hdmi_probe, at this time hcp- > jack = NULL, the driver cannot report `SND_JACK_LINEOUT
> > ` normally.
> > static void hdmi_codec_jack_report(struct hdmi_codec_priv *hcp,
> > unsigned int jack_status)
> > {
> > printk("xzq-866 hdmi_codec_jack_report: jack=%x, jack_status=%d", hcp->jack, jack_status != hcp->jack_status);
> > if (hcp->jack && jack_status != hcp->jack_status) {
> > snd_soc_jack_report(hcp->jack, jack_status, SND_JACK_LINEOUT);
> > hcp->jack_status = jack_status;
> > }
> > }
> > So we must call plugged_cb in hdmi_codec_set_jack, Can you make some changes?
>
> Hi Zhengqiao,
>
> That is unfortunate. Sorry.
>
> This patch has changed when the hpd callback is registered, no when it
> comes in effect. This is still dependent on calling .set_jack() and it
> is not happening any later than it was before. So, in theory, it should
> not have changed anything, if your driver actually relies on the HPD
> event.
>
> Trying to guess what is happening for you, I suppose your HDMI driver is
> "faking" an HPD event to report the initial jack status when the
> hook_plugged_cb() is called. Could you point me to the hdmi driver you
> are using so I can have a look ?
>
> My reference when testing this was dw-hdmi-i2s-audio and it does not do
> that, it just registers the callback. I think this is what it supposed
> to do TBH.
>
> An idea I have been thinking about for a while is have the hdmi-codec
> insert the jack in the card itself, instead of the card doing. That
> would give the jack "for free" to any user of the HDMI codec and might
> also solve your issue. It would require a small rework of the cards doing
> the hdmi jack register, but there are not many of these AFAIK.
>
The driver is it6505. The implementation of hook_plugged_cb():
1. register plugged_cb
2. call plugged_cb(bool plugged)
bridge detect callback it6505_detect would also call plugged_cb, but
only on the first time hpd status changed (eg. changed from connect
<--> disconnect)
it6505_detect() {
status = it6505->hpd_state ...
...
if (it6505->connector_status != status) {
it6505->connector_status = status;
it6505_plugged_status_to_codec(it6505); // this will call plugged_cb
}
}
Unfortunately the first time after boot that hpd status changed was
detected before set_jack. If we replug hdmi, the plugged_cb() was
called by bridge_detect, which is expected.
Prior to this patch, the initial plugged_cb() was called by hook_plugged_cb().
After the patch, plugged_cb() should be called by hpd change (by
bridge detect), but due to the driver logic only calling it on the
first hpd state change, it fails to call plugged_cb() again when jack
is set.
I checked the dw-hdmi.c's bridge_detect, and it's similar in that it
also checks the last_connector_result, so maybe it's due to a timing
difference?
> >
> > On Mon, Nov 6, 2023 at 6:40 PM Jerome Brunet <jbrunet@xxxxxxxxxxxx> wrote:
> >
> > The HDMI hotplug callback to the hdmi-codec is currently registered when
> > jack is set.
> >
> > The hotplug not only serves to report the ASoC jack state but also to get
> > the ELD. It should be registered when the component probes instead, so it
> > does not depend on the card driver registering a jack for the HDMI to
> > properly report the ELD.
> >
> > Fixes: 25ce4f2b3593 ("ASoC: hdmi-codec: Get ELD in before reporting plugged event")
> > Signed-off-by: Jerome Brunet <jbrunet@xxxxxxxxxxxx>
> > ---
> > sound/soc/codecs/hdmi-codec.c | 27 +++++++++++++++++++--------
> > 1 file changed, 19 insertions(+), 8 deletions(-)
> >
> > diff --git a/sound/soc/codecs/hdmi-codec.c b/sound/soc/codecs/hdmi-codec.c
> > index 09eef6042aad..20da1eaa4f1c 100644
> > --- a/sound/soc/codecs/hdmi-codec.c
> > +++ b/sound/soc/codecs/hdmi-codec.c
> > @@ -877,18 +877,13 @@ static int hdmi_codec_set_jack(struct snd_soc_component *component,
> > void *data)
> > {
> > struct hdmi_codec_priv *hcp = snd_soc_component_get_drvdata(component);
> > - int ret = -ENOTSUPP;
> >
> > if (hcp->hcd.ops->hook_plugged_cb) {
> > hcp->jack = jack;
> > - ret = hcp->hcd.ops->hook_plugged_cb(component->dev->parent,
> > - hcp->hcd.data,
> > - plugged_cb,
> > - component->dev);
> > - if (ret)
> > - hcp->jack = NULL;
> > + return 0;
> > }
> > - return ret;
> > +
> > + return -ENOTSUPP;
> > }
> >
> > static int hdmi_dai_spdif_probe(struct snd_soc_dai *dai)
> > @@ -982,6 +977,21 @@ static int hdmi_of_xlate_dai_id(struct snd_soc_component *component,
> > return ret;
> > }
> >
> > +static int hdmi_probe(struct snd_soc_component *component)
> > +{
> > + struct hdmi_codec_priv *hcp = snd_soc_component_get_drvdata(component);
> > + int ret = 0;
> > +
> > + if (hcp->hcd.ops->hook_plugged_cb) {
> > + ret = hcp->hcd.ops->hook_plugged_cb(component->dev->parent,
> > + hcp->hcd.data,
> > + plugged_cb,
> > + component->dev);
> > + }
> > +
> > + return ret;
> > +}
> > +
> > static void hdmi_remove(struct snd_soc_component *component)
> > {
> > struct hdmi_codec_priv *hcp = snd_soc_component_get_drvdata(component);
> > @@ -992,6 +1002,7 @@ static void hdmi_remove(struct snd_soc_component *component)
> > }
> >
> > static const struct snd_soc_component_driver hdmi_driver = {
> > + .probe = hdmi_probe,
> > .remove = hdmi_remove,
> > .dapm_widgets = hdmi_widgets,
> > .num_dapm_widgets = ARRAY_SIZE(hdmi_widgets),
>
>
> --
> Jerome