On Mon, Dec 18, 2023 at 7:03 PM Jerome Brunet <jbrunet@xxxxxxxxxxxx> wrote:
>
> On Fri 15 Dec 2023 at 14:55, Hsin-Yi Wang <hsinyi@xxxxxxxxxx> wrote:
>
> > 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.
>
> This is what thought
>
> >
> > 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?
>
> No, I have overlooked that.
> I'm preparing a fix. I'll Cc you.
>
> Eventually, I still would like to make it easier for cards to use the
> HDMI jack and have to codec do that registration on its own. It will
> require some rework of cards already doing it.
>
> I've only seen 2 cards doing that at the moment:
> * imx-hdmi.c
> * mt8188-mt6359.c
>
> Could you point me to yours ?
it is mt8186-mt6366-rt1019-rt5682s.c :
https://source.chromium.org/chromiumos/chromiumos/codesearch/+/main:src/third_party/kernel/v5.15/sound/soc/mediatek/mt8186/mt8186-mt6366-rt1019-rt5682s.c
>
>
>
> >> >
> >> > 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
>
>
> --
> Jerome
>
