[alsa-devel] [PATCH] ALSA: hda/hdmi - Don't report Jack event if no need to do that

[Takashi Iwai]

On Thu, 02 May 2019 04:52:46 +0200,
Hui Wang wrote:
> 
> 
> On 2019/4/30 下午5:02, Takashi Iwai wrote:
> > On Tue, 30 Apr 2019 10:42:55 +0200,
> > Hui Wang wrote:
> >>
> >> On 2019/4/30 下午3:35, Takashi Iwai wrote:
> >>> On Tue, 30 Apr 2019 08:57:11 +0200,
> >>> Hui Wang wrote:
> >>>> On the machines with AMD GPU or Nvidia GPU, we often meet this issues:
> >>>> after s3, there are 4 HDMI/DP audio devices in the gnome-sound-setting
> >>>> even there is no any monitors plugged.
> >>>>
> >>>> When this problem happens, we check the /proc/asound/cardX/eld#N.M, we
> >>>> will find the monitor_present=1, eld_valid=0.
> >>>>
> >>>> The root cause is somehow the pin_sense reports the monitor is present
> >>>> and eld is valid when there is no monitor plugged.
> >>>>
> >>>> The current driver will read the eld data if the pin_sense reports the
> >>>> eld is valid, because of no monitor is plugged, there is no valid eld
> >>>> data, then the eld->valid is set to 0.
> >>>>
> >>>> If we don't let driver report Jack event when monitor_present=1 while
> >>>> eld_valid=0, there will be no this issue.
> >>>>
> >>>> After this change, the driver only reports Jack event with one of the
> >>>> below 2 conditons:
> >>>>    eld->monitor_present=1 and eld->eld_valid=1 (a valid monitor detect)
> >>>>    eld->monitor_present=0 (a monitor is unplugged)
> >>>>
> >>>> Signed-off-by: Hui Wang <hui.wang at canonical.com>
> >>> Well, if the eld_valid=1 is mandatory, basically we can use it as the
> >>> condition of jack=1, like the patch below.  The return value from
> >>> hdmi_present_sense() indicates only whether we may sync jack state or
> >>> not, and it's not about the jack state itself.
> >>>
> >>>
> >>> thanks,
> >>>
> >>> Takashi
> >>>
> >>> ---
> >>> --- a/sound/pci/hda/patch_hdmi.c
> >>> +++ b/sound/pci/hda/patch_hdmi.c
> >>> @@ -1625,7 +1625,7 @@ static void sync_eld_via_acomp(struct hda_codec *codec,
> >>>    	if (jack == NULL)
> >>>    		goto unlock;
> >>>    	snd_jack_report(jack,
> >>> -			eld->monitor_present ? SND_JACK_AVOUT : 0);
> >>> +			(eld->monitor_present && eld->eld_valid) ? SND_JACK_AVOUT : 0);
> >>>     unlock:
> >>>    	mutex_unlock(&per_pin->lock);
> >>>    }
> >> All Intel machines which get eld via acomp don't have this issue, so
> >> no need to change this function.
> > Yes, but the HDMI audio won't work without the valid ELD no matter
> > whether it's reported via audio-component or verbs.  So this change
> > also corrects a corner case of Intel platforms, too.
> Yes, indeed it is.
> >
> >> For those machines (with nv gpu card or amd gpu card)  which need to
> >> call hdmi_present_sense_via_verbs(), they have this issue. if
> >> hdmi_present_sense_via_verbs() returns true, the hdmi_present_sense()
> >> returns true, then snd_hda_jack_report_sync() will be called. So if we
> >> want to fix this issue, we need to let hdmi_present_sense_via_verbs()
> >> not return true or we change the read_pin_sense() in the hda_jack.c
> > The report_sync() doesn't report immediately; it updates the state
> > then notifies *only if* the state change happened.  That is, the root
> > cause is the false state change as if it were worth for reporting.
> Yes, before suspend, the monitor_present=0, while after resume, the
> monitor_present=1 (I guess it is the graphic driver like amdgpu or
> nvidia/nouveau or BIOS make the PRESENSE register change).
> >
> > IOW, the state "monitor_detected=1 and eld_valid=0" can happen at any
> > time, not only at resume.  User may put a polling work, for example.
> > And, reporting this as the jack=1 causes the trouble, no matter
> > whether it's resume path or not.  So we rather need to fix this false
> > state reporting instead.
> 
> You are right.
> 
> So the fix is putting your change and my change together like below?

Well, removing "!repoll" should be treated as a separate patch, and
I'm not sure whether we want to get rid of it.

The "!repoll" check is a bit misleading.  This condition indicates
that the jack status sync is required, e.g. it's the case where
polling goes to timeout or at the beginning of the probe.  For
example, in the case of resume, it starts with repoll=1.  If an
incomplete state (monitor=1, eld_valid=0) is seen at this point, the
code schedules for the retry at a later point.  And if it reaches to
the max count, it clears to repoll=0 and try the last attempt, which
won't schedule any longer.

So, at this point, we *have to* notify the result no matter whether
it's satisfying or not.


thanks,

Takashi

> 
> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> index 8b3ac690efa3..14b799a85873 100644
> --- a/sound/pci/hda/patch_hdmi.c
> +++ b/sound/pci/hda/patch_hdmi.c
> @@ -1548,7 +1548,7 @@ static bool hdmi_present_sense_via_verbs(struct
> hdmi_spec_per_pin *per_pin,
>         else
>                 update_eld(codec, per_pin, eld);
> 
> -       ret = !repoll || !eld->monitor_present || eld->eld_valid;
> +       ret = !eld->monitor_present || eld->eld_valid;
> 
>         jack = snd_hda_jack_tbl_get(codec, pin_nid);
>         if (jack)
> @@ -1625,7 +1625,7 @@ static void sync_eld_via_acomp(struct hda_codec
> *codec,
>         if (jack == NULL)
>                 goto unlock;
>         snd_jack_report(jack,
> -                       eld->monitor_present ? SND_JACK_AVOUT : 0);
> +                       (eld->monitor_present && eld->eld_valid) ?
> SND_JACK_AVOUT : 0);
>   unlock:
>         mutex_unlock(&per_pin->lock);
> 
> >
> > thanks,
> >
> > Takashi
> > _______________________________________________
> > Alsa-devel mailing list
> > Alsa-devel at alsa-project.org
> > https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>