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

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

[Hui Wang]

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@canonical.com>
---
 sound/pci/hda/patch_hdmi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 8b3ac690efa3..e5a34fa6f358 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)
-- 
2.17.1

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

[Takashi Iwai]

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@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);
 }

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

[Hui Wang]

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@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.

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



> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Takashi Iwai]

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@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.

> 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.

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.


thanks,

Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [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@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@alsa-project.org
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [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@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@alsa-project.org
> > https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Hui Wang]

On 2019/5/2 下午11:49, Takashi Iwai wrote:
> 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.
>>>>>>

>>>>>>
>>>>>> 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

So let us do sth in the hda_jack.c, adding two members check_eld and 
eld_valid in the struct hda_jack_tbl{},

When building new jacks, since there is no eld_data yet, just follow old 
rules (only checking AC_PINSENSE_PRESENCE)

After building new jacks, if it is not a phantom one, set check_eld to 1

every time we get eld (via verbs or acomp), we will sync jack->eld_valid 
to eld->eld_valid.

in the report_sync(), we will check check_eld and eld_valid, not depends 
on AC_PINSENSE_PRESENCE only.

No matter interrupt, s3 or poll,  the jack->eld_valid will be synced and 
the report_sync() will report jack state depeneds on both 
AC_PINSENSE_PRESENCE and eld_valid

Does it sound good?

Thanks,

Hui.

diff --git a/sound/pci/hda/hda_jack.c b/sound/pci/hda/hda_jack.c
index 74b46952fc98..4bdf842bd915 100644
--- a/sound/pci/hda/hda_jack.c
+++ b/sound/pci/hda/hda_jack.c
@@ -341,7 +341,8 @@ void snd_hda_jack_report_sync(struct hda_codec *codec)
                                 continue;
                         state = jack->button_state;
                         if (get_jack_plug_state(jack->pin_sense))
-                               state |= jack->type;
+                               if (!jack->check_eld || jack->eld_valid)
+                                       state |= jack->type;
                         snd_jack_report(jack->jack, state);
                         if (jack->button_state) {
                                 snd_jack_report(jack->jack,
diff --git a/sound/pci/hda/hda_jack.h b/sound/pci/hda/hda_jack.h
index 1d713201c160..57d7245b8961 100644
--- a/sound/pci/hda/hda_jack.h
+++ b/sound/pci/hda/hda_jack.h
@@ -40,6 +40,8 @@ struct hda_jack_tbl {
         unsigned int jack_dirty:1;      /* needs to update? */
         unsigned int phantom_jack:1;    /* a fixed, always present port? */
         unsigned int block_report:1;    /* in a transitional state - do 
not report to userspace */
+       unsigned int check_eld:1;       /* check eld flag when deciding 
pin presence */
+       unsigned int eld_valid:1;       /* eld data valid or not */
         hda_nid_t gating_jack;          /* valid when gating jack 
plugged */
         hda_nid_t gated_jack;           /* gated is dependent on this 
jack */
         int type;
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 8b3ac690efa3..56357e17d17d 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -1551,8 +1551,10 @@ static bool hdmi_present_sense_via_verbs(struct 
hdmi_spec_per_pin *per_pin,
         ret = !repoll || !eld->monitor_present || eld->eld_valid;

         jack = snd_hda_jack_tbl_get(codec, pin_nid);
-       if (jack)
+       if (jack) {
                 jack->block_report = !ret;
+               jack->eld_valid = eld->eld_valid;
+       }

         mutex_unlock(&per_pin->lock);
         return ret;
@@ -1593,6 +1595,7 @@ static void sync_eld_via_acomp(struct hda_codec 
*codec,
         struct hdmi_spec *spec = codec->spec;
         struct hdmi_eld *eld = &spec->temp_eld;
         struct snd_jack *jack = NULL;
+       struct hda_jack_tbl *jack_tbl;
         int size;

         mutex_lock(&per_pin->lock);
@@ -1615,6 +1618,10 @@ static void sync_eld_via_acomp(struct hda_codec 
*codec,
                 eld->eld_size = 0;
         }

+       jack_tbl = snd_hda_jack_tbl_get(codec, per_pin->pin_nid);
+       if (jack_tbl)
+               jack_tbl->eld_valid = eld->eld_valid;
+
         /* pcm_idx >=0 before update_eld() means it is in monitor
          * disconnected event. Jack must be fetched before update_eld()
          */
@@ -1625,7 +1632,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);
  }
@@ -2163,6 +2170,8 @@ static int generic_hdmi_build_jack(struct 
hda_codec *codec, int pcm_idx)
          * align with dyn_pcm_assign mode
          */
         spec->pcm_rec[pcm_idx].jack = jack->jack;
+       if (!jack->phantom_jack)
+               jack->check_eld = 1;
         return 0;
  }

>
>> 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
>>
>>
>>> thanks,
>>>
>>> Takashi
>>> _______________________________________________
>>> Alsa-devel mailing list
>>> Alsa-devel@alsa-project.org
>>> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Takashi Iwai]

On Fri, 03 May 2019 06:05:09 +0200,
Hui Wang wrote:
> 
> 
> On 2019/5/2 下午11:49, Takashi Iwai wrote:
> > 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.
> >>>>>>
> 
> >>>>>>
> >>>>>> 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
> 
> So let us do sth in the hda_jack.c, adding two members check_eld and
> eld_valid in the struct hda_jack_tbl{},
> 
> When building new jacks, since there is no eld_data yet, just follow
> old rules (only checking AC_PINSENSE_PRESENCE)
> 
> After building new jacks, if it is not a phantom one, set check_eld to 1
> 
> every time we get eld (via verbs or acomp), we will sync
> jack->eld_valid to eld->eld_valid.
> 
> in the report_sync(), we will check check_eld and eld_valid, not
> depends on AC_PINSENSE_PRESENCE only.
> 
> No matter interrupt, s3 or poll,  the jack->eld_valid will be synced
> and the report_sync() will report jack state depeneds on both
> AC_PINSENSE_PRESENCE and eld_valid
> 
> Does it sound good?

OK, I was confused -- I overlooked the fact that hda_jack.c updates
the pin sense locally.  And thanks for the suggestion, we're heading
to the right direction.

One thing that doesn't fit is introducing HDMI-specific things like
eld_valid and check_eld.  Basically this is about the falsely reported
jack detection, after all.  So, what we need is the correction of the
incorrect jack detection -- or a way to override the value.

Actually, for HDMI, there is no reason to read the pin sense at all.
All we need is already provided by the HDMI codec driver (monitor
present and eld valid).

Then I can think of adding a new flag hda_jack_tbl.manual_pin_sense,
for example, indicating that the pin sense is updated by the driver.
That's a change something like below.


thanks,

Takashi

--- a/sound/pci/hda/hda_jack.c
+++ b/sound/pci/hda/hda_jack.c
@@ -189,7 +189,7 @@ void snd_hda_jack_set_dirty_all(struct hda_codec *codec)
 	int i;
 
 	for (i = 0; i < codec->jacktbl.used; i++, jack++)
-		if (jack->nid)
+		if (jack->nid && !jack->manual_pin_sense)
 			jack->jack_dirty = 1;
 }
 EXPORT_SYMBOL_GPL(snd_hda_jack_set_dirty_all);
@@ -568,7 +568,8 @@ void snd_hda_jack_unsol_event(struct hda_codec *codec, unsigned int res)
 	event = snd_hda_jack_tbl_get_from_tag(codec, tag);
 	if (!event)
 		return;
-	event->jack_dirty = 1;
+	if (!event->manual_pin_sense)
+		event->jack_dirty = 1;
 
 	call_jack_callback(codec, res, event);
 	snd_hda_jack_report_sync(codec);
--- a/sound/pci/hda/hda_jack.h
+++ b/sound/pci/hda/hda_jack.h
@@ -40,6 +40,7 @@ struct hda_jack_tbl {
 	unsigned int jack_dirty:1;	/* needs to update? */
 	unsigned int phantom_jack:1;    /* a fixed, always present port? */
 	unsigned int block_report:1;    /* in a transitional state - do not report to userspace */
+	unsigned int manual_pin_sense:1; /* pin_sense is updated by codec driver */
 	hda_nid_t gating_jack;		/* valid when gating jack plugged */
 	hda_nid_t gated_jack;		/* gated is dependent on this jack */
 	int type;
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -170,6 +170,7 @@ struct hdmi_spec {
 
 	bool dyn_pin_out;
 	bool dyn_pcm_assign;
+	bool use_pin_sense;
 	/*
 	 * Non-generic VIA/NVIDIA specific
 	 */
@@ -797,7 +798,6 @@ static void hdmi_intrinsic_event(struct hda_codec *codec, unsigned int res)
 	jack = snd_hda_jack_tbl_get_from_tag(codec, tag);
 	if (!jack)
 		return;
-	jack->jack_dirty = 1;
 
 	codec_dbg(codec,
 		"HDMI hot plug event: Codec=%d Pin=%d Device=%d Inactive=%d Presence_Detect=%d ELD_Valid=%d\n",
@@ -1551,8 +1551,11 @@ static bool hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin,
 	ret = !repoll || !eld->monitor_present || eld->eld_valid;
 
 	jack = snd_hda_jack_tbl_get(codec, pin_nid);
-	if (jack)
+	if (jack) {
 		jack->block_report = !ret;
+		jack->pin_sense = (eld->monitor_present && eld->eld_valid) ?
+			AC_PINSENSE_PRESENCE : 0;
+	}
 
 	mutex_unlock(&per_pin->lock);
 	return ret;
@@ -2163,6 +2166,7 @@ static int generic_hdmi_build_jack(struct hda_codec *codec, int pcm_idx)
 	 * align with dyn_pcm_assign mode
 	 */
 	spec->pcm_rec[pcm_idx].jack = jack->jack;
+	jack->manual_pin_sense = !spec->use_pin_sense;
 	return 0;
 }
 
@@ -2972,6 +2976,7 @@ static int patch_simple_hdmi(struct hda_codec *codec,
 	spec->multiout.dig_out_nid = cvt_nid;
 	spec->num_cvts = 1;
 	spec->num_pins = 1;
+	spec->use_pin_sense = true;
 	per_pin = snd_array_new(&spec->pins);
 	per_cvt = snd_array_new(&spec->cvts);
 	if (!per_pin || !per_cvt) {
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Hui Wang]

On 2019/5/3 下午11:57, Takashi Iwai wrote:
> On Fri, 03 May 2019 06:05:09 +0200,
> Hui Wang wrote:
>>
>> On 2019/5/2 下午11:49, Takashi Iwai wrote:
>>> 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.
>>>>>>>>
>>>>>>>> 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
>> So let us do sth in the hda_jack.c, adding two members check_eld and
>> eld_valid in the struct hda_jack_tbl{},
>>
>> When building new jacks, since there is no eld_data yet, just follow
>> old rules (only checking AC_PINSENSE_PRESENCE)
>>
>> After building new jacks, if it is not a phantom one, set check_eld to 1
>>
>> every time we get eld (via verbs or acomp), we will sync
>> jack->eld_valid to eld->eld_valid.
>>
>> in the report_sync(), we will check check_eld and eld_valid, not
>> depends on AC_PINSENSE_PRESENCE only.
>>
>> No matter interrupt, s3 or poll,  the jack->eld_valid will be synced
>> and the report_sync() will report jack state depeneds on both
>> AC_PINSENSE_PRESENCE and eld_valid
>>
>> Does it sound good?
> OK, I was confused -- I overlooked the fact that hda_jack.c updates
> the pin sense locally.  And thanks for the suggestion, we're heading
> to the right direction.
>
> One thing that doesn't fit is introducing HDMI-specific things like
> eld_valid and check_eld.  Basically this is about the falsely reported
> jack detection, after all.  So, what we need is the correction of the
> incorrect jack detection -- or a way to override the value.
>
> Actually, for HDMI, there is no reason to read the pin sense at all.
> All we need is already provided by the HDMI codec driver (monitor
> present and eld valid).
>
> Then I can think of adding a new flag hda_jack_tbl.manual_pin_sense,
> for example, indicating that the pin sense is updated by the driver.
> That's a change something like below.
>
Looks like the hdmi_present_sense_via_verbs() still has some issue. At 
the entry of this function,  it calls snd_hda_pin_sense() and read the 
eld_data depending on the return value,  here it expects reading the pin 
sense register, after applying your change, this function returns a 
shadow value instead of a register value, if we set the pin_sense to 0, 
then it is possible that the pin_get_eld() will not be called anymore.

And inspired by your change, maybe we just make this change, then it is 
enough to fix the falsely report issue here.

@@ -1551,8 +1551,11 @@ static bool hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin,
  	ret = !repoll || !eld->monitor_present || eld->eld_valid;
  
  	jack = snd_hda_jack_tbl_get(codec, pin_nid);
-	if (jack)
+	if (jack) {
  		jack->block_report = !ret;
+		jack->pin_sense = (eld->monitor_present && eld->eld_valid) ?
+			AC_PINSENSE_PRESENCE : 0;
+	}

because in the snd_hda_pin_sense(), the jack_dirty is set to 0, then we 
change the jack->pin_sense,  and in the report_sync() it will decide the 
jack state according to the jack->pin_sense we changed.

Thanks,

Hui.


> thanks,
>
> Takashi
>
> --- a/sound/pci/hda/hda_jack.c
> +++ b/sound/pci/hda/hda_jack.c
> @@ -189,7 +189,7 @@ void snd_hda_jack_set_dirty_all(struct hda_codec *codec)
>   	int i;
>   
>   	for (i = 0; i < codec->jacktbl.used; i++, jack++)
> -		if (jack->nid)
> +		if (jack->nid && !jack->manual_pin_sense)
>   			jack->jack_dirty = 1;
>   }
>   EXPORT_SYMBOL_GPL(snd_hda_jack_set_dirty_all);
> @@ -568,7 +568,8 @@ void snd_hda_jack_unsol_event(struct hda_codec *codec, unsigned int res)
>   	event = snd_hda_jack_tbl_get_from_tag(codec, tag);
>   	if (!event)
>   		return;
> -	event->jack_dirty = 1;
> +	if (!event->manual_pin_sense)
> +		event->jack_dirty = 1;
>   
>   	call_jack_callback(codec, res, event);
>   	snd_hda_jack_report_sync(codec);
> --- a/sound/pci/hda/hda_jack.h
> +++ b/sound/pci/hda/hda_jack.h
> @@ -40,6 +40,7 @@ struct hda_jack_tbl {
>   	unsigned int jack_dirty:1;	/* needs to update? */
>   	unsigned int phantom_jack:1;    /* a fixed, always present port? */
>   	unsigned int block_report:1;    /* in a transitional state - do not report to userspace */
> +	unsigned int manual_pin_sense:1; /* pin_sense is updated by codec driver */
>   	hda_nid_t gating_jack;		/* valid when gating jack plugged */
>   	hda_nid_t gated_jack;		/* gated is dependent on this jack */
>   	int type;
> --- a/sound/pci/hda/patch_hdmi.c
> +++ b/sound/pci/hda/patch_hdmi.c
> @@ -170,6 +170,7 @@ struct hdmi_spec {
>   
>   	bool dyn_pin_out;
>   	bool dyn_pcm_assign;
> +	bool use_pin_sense;
>   	/*
>   	 * Non-generic VIA/NVIDIA specific
>   	 */
> @@ -797,7 +798,6 @@ static void hdmi_intrinsic_event(struct hda_codec *codec, unsigned int res)
>   	jack = snd_hda_jack_tbl_get_from_tag(codec, tag);
>   	if (!jack)
>   		return;
> -	jack->jack_dirty = 1;
>   
>   	codec_dbg(codec,
>   		"HDMI hot plug event: Codec=%d Pin=%d Device=%d Inactive=%d Presence_Detect=%d ELD_Valid=%d\n",
> @@ -1551,8 +1551,11 @@ static bool hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin,
>   	ret = !repoll || !eld->monitor_present || eld->eld_valid;
>   
>   	jack = snd_hda_jack_tbl_get(codec, pin_nid);
> -	if (jack)
> +	if (jack) {
>   		jack->block_report = !ret;
> +		jack->pin_sense = (eld->monitor_present && eld->eld_valid) ?
> +			AC_PINSENSE_PRESENCE : 0;
> +	}
>   
>   	mutex_unlock(&per_pin->lock);
>   	return ret;
> @@ -2163,6 +2166,7 @@ static int generic_hdmi_build_jack(struct hda_codec *codec, int pcm_idx)
>   	 * align with dyn_pcm_assign mode
>   	 */
>   	spec->pcm_rec[pcm_idx].jack = jack->jack;
> +	jack->manual_pin_sense = !spec->use_pin_sense;
>   	return 0;
>   }
>   
> @@ -2972,6 +2976,7 @@ static int patch_simple_hdmi(struct hda_codec *codec,
>   	spec->multiout.dig_out_nid = cvt_nid;
>   	spec->num_cvts = 1;
>   	spec->num_pins = 1;
> +	spec->use_pin_sense = true;
>   	per_pin = snd_array_new(&spec->pins);
>   	per_cvt = snd_array_new(&spec->cvts);
>   	if (!per_pin || !per_cvt) {
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Takashi Iwai]

On Sat, 04 May 2019 04:45:36 +0200,
Hui Wang wrote:
> 
> 
> On 2019/5/3 下午11:57, Takashi Iwai wrote:
> > On Fri, 03 May 2019 06:05:09 +0200,
> > Hui Wang wrote:
> >>
> >> On 2019/5/2 下午11:49, Takashi Iwai wrote:
> >>> 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.
> >>>>>>>>
> >>>>>>>> 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
> >> So let us do sth in the hda_jack.c, adding two members check_eld and
> >> eld_valid in the struct hda_jack_tbl{},
> >>
> >> When building new jacks, since there is no eld_data yet, just follow
> >> old rules (only checking AC_PINSENSE_PRESENCE)
> >>
> >> After building new jacks, if it is not a phantom one, set check_eld to 1
> >>
> >> every time we get eld (via verbs or acomp), we will sync
> >> jack->eld_valid to eld->eld_valid.
> >>
> >> in the report_sync(), we will check check_eld and eld_valid, not
> >> depends on AC_PINSENSE_PRESENCE only.
> >>
> >> No matter interrupt, s3 or poll,  the jack->eld_valid will be synced
> >> and the report_sync() will report jack state depeneds on both
> >> AC_PINSENSE_PRESENCE and eld_valid
> >>
> >> Does it sound good?
> > OK, I was confused -- I overlooked the fact that hda_jack.c updates
> > the pin sense locally.  And thanks for the suggestion, we're heading
> > to the right direction.
> >
> > One thing that doesn't fit is introducing HDMI-specific things like
> > eld_valid and check_eld.  Basically this is about the falsely reported
> > jack detection, after all.  So, what we need is the correction of the
> > incorrect jack detection -- or a way to override the value.
> >
> > Actually, for HDMI, there is no reason to read the pin sense at all.
> > All we need is already provided by the HDMI codec driver (monitor
> > present and eld valid).
> >
> > Then I can think of adding a new flag hda_jack_tbl.manual_pin_sense,
> > for example, indicating that the pin sense is updated by the driver.
> > That's a change something like below.
> >
> Looks like the hdmi_present_sense_via_verbs() still has some issue. At
> the entry of this function,  it calls snd_hda_pin_sense() and read the
> eld_data depending on the return value,  here it expects reading the
> pin sense register, after applying your change, this function returns
> a shadow value instead of a register value, if we set the pin_sense to
> 0, then it is possible that the pin_get_eld() will not be called
> anymore.

Right...

> And inspired by your change, maybe we just make this change, then it
> is enough to fix the falsely report issue here.
> 
> @@ -1551,8 +1551,11 @@ static bool hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin,
>  	ret = !repoll || !eld->monitor_present || eld->eld_valid;
>  	jack = snd_hda_jack_tbl_get(codec, pin_nid);
> -	if (jack)
> +	if (jack) {
>  		jack->block_report = !ret;
> +		jack->pin_sense = (eld->monitor_present && eld->eld_valid) ?
> +			AC_PINSENSE_PRESENCE : 0;
> +	}
> 
> because in the snd_hda_pin_sense(), the jack_dirty is set to 0, then
> we change the jack->pin_sense,  and in the report_sync() it will
> decide the jack state according to the jack->pin_sense we changed.

OK, that should work, just overriding and correcting the pin_sense.

I guess the only missing code path is the case where jack->dirty is
set manually without the callback call via
snd_hda_jack_set_dirty_all().  Through a quick glance, it's called
from the common resume code, hda_call_codec_resume().  But, again,
HDMI codec has its own resume to refresh pin detection
(generic_hdmi_resume()), and the changed code-path should be
involved.

The rest callers of snd_hda_jack_set_dirty_all() are the polling mode
and the unsol event handler for the old HDMI codecs, both of which we
don't care much.

That said, your minimal change looks good to me, and I'll happily
apply as long as it's tested.  Of course, it needs some more careful
comments about the behavior.


Thanks!

Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

[Hui Wang]

On 2019/5/4 下午3:18, Takashi Iwai wrote:
> On Sat, 04 May 2019 04:45:36 +0200,
> Hui Wang wrote:
>>
>> On 2019/5/3 下午11:57, Takashi Iwai wrote:
>>> On Fri, 03 May 2019 06:05:09 +0200,
>>> Hui Wang wrote:
>>
> Right...
>
>> And inspired by your change, maybe we just make this change, then it
>> is enough to fix the falsely report issue here.
>>
>> @@ -1551,8 +1551,11 @@ static bool hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin,
>>   	ret = !repoll || !eld->monitor_present || eld->eld_valid;
>>   	jack = snd_hda_jack_tbl_get(codec, pin_nid);
>> -	if (jack)
>> +	if (jack) {
>>   		jack->block_report = !ret;
>> +		jack->pin_sense = (eld->monitor_present && eld->eld_valid) ?
>> +			AC_PINSENSE_PRESENCE : 0;
>> +	}
>>
>> because in the snd_hda_pin_sense(), the jack_dirty is set to 0, then
>> we change the jack->pin_sense,  and in the report_sync() it will
>> decide the jack state according to the jack->pin_sense we changed.
> OK, that should work, just overriding and correcting the pin_sense.
>
> I guess the only missing code path is the case where jack->dirty is
> set manually without the callback call via
> snd_hda_jack_set_dirty_all().  Through a quick glance, it's called
> from the common resume code, hda_call_codec_resume().  But, again,
> HDMI codec has its own resume to refresh pin detection
> (generic_hdmi_resume()), and the changed code-path should be
> involved.
>
> The rest callers of snd_hda_jack_set_dirty_all() are the polling mode
> and the unsol event handler for the old HDMI codecs, both of which we
> don't care much.
>
> That said, your minimal change looks good to me, and I'll happily
> apply as long as it's tested.  Of course, it needs some more careful
> comments about the behavior.

OK,  after testing it, I will send the patch to review.

Thanks.


>
>
> Thanks!
>
> Takashi
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel