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

[Hui Wang]

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?

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