ALSA: hda/hdmi - add a parameter to let users decide if checking the eld_valid
diff mbox series

Message ID 20191111144502.22910-1-hui.wang@canonical.com
State New
Headers show
Series
  • ALSA: hda/hdmi - add a parameter to let users decide if checking the eld_valid
Related show

Commit Message

Hui Wang Nov. 11, 2019, 2:45 p.m. UTC
With the commit 7f641e26a6df ("ALSA: hda/hdmi - Consider eld_valid
when reporting jack event"), the driver checks eld_valid before
reporting Jack state, this fixes the 4 HDMI/DP audio devices issue.

But recently some users complained that the hdmi audio on their
machines couldn't work anymore with this commit. On their machines,
the monitor_present is 1 while the eld_valid is 0 when plugging a
monitor, and the hdmi audio could work even the eld_valid is 0.

To make the hdmi audio work again on those machines, adding a module
parameter, if usrs want to skip the checking eld_valid, they
could set checking_eld_valid=0 when loading the module. And this
parameter only applies to sense_via_verbs, for those getting eld via
component, no need to apply this parameter since it is impossible
that present is 1 while eld_valid is 0.

BugLink: https://bugs.launchpad.net/bugs/1834771
Fixes: 7f641e26a6df ("ALSA: hda/hdmi - Consider eld_valid when reporting jack event")
Cc: <stable@vger.kernel.org>
Signed-off-by: Hui Wang <hui.wang@canonical.com>
---
 sound/pci/hda/patch_hdmi.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

Comments

Takashi Iwai Nov. 11, 2019, 3:33 p.m. UTC | #1
On Mon, 11 Nov 2019 15:45:02 +0100,
Hui Wang wrote:
> 
> With the commit 7f641e26a6df ("ALSA: hda/hdmi - Consider eld_valid
> when reporting jack event"), the driver checks eld_valid before
> reporting Jack state, this fixes the 4 HDMI/DP audio devices issue.
> 
> But recently some users complained that the hdmi audio on their
> machines couldn't work anymore with this commit. On their machines,
> the monitor_present is 1 while the eld_valid is 0 when plugging a
> monitor, and the hdmi audio could work even the eld_valid is 0.
> 
> To make the hdmi audio work again on those machines, adding a module
> parameter, if usrs want to skip the checking eld_valid, they
> could set checking_eld_valid=0 when loading the module. And this
> parameter only applies to sense_via_verbs, for those getting eld via
> component, no need to apply this parameter since it is impossible
> that present is 1 while eld_valid is 0.
> 
> BugLink: https://bugs.launchpad.net/bugs/1834771
> Fixes: 7f641e26a6df ("ALSA: hda/hdmi - Consider eld_valid when reporting jack event")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Hui Wang <hui.wang@canonical.com>

Well, this sort of module option is rather a last resort, so I
hesitate to apply this.

The bug reports in the above are a bit hard to digest quickly.
Could you tell exactly which hardware (and drivers) show the problem?

FWIW, amdgpu driver already got the audio-component binding recently,
so this problem shouldn't be triggered, at least in this code path.
And, for nouveau and radeon, I already submitted the patches to
support the audio-component binding, but by some reason they haven't
been merged to the upstream.  In that case, we'd need to ping DRM
guys.


thanks,

Takashi


> ---
>  sound/pci/hda/patch_hdmi.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> index be8a977fc684..d70fca4f4411 100644
> --- a/sound/pci/hda/patch_hdmi.c
> +++ b/sound/pci/hda/patch_hdmi.c
> @@ -37,6 +37,11 @@ static bool static_hdmi_pcm;
>  module_param(static_hdmi_pcm, bool, 0644);
>  MODULE_PARM_DESC(static_hdmi_pcm, "Don't restrict PCM parameters per ELD info");
>  
> +static bool checking_eld_valid = true;
> +module_param(checking_eld_valid, bool, 0644);
> +MODULE_PARM_DESC(checking_eld_valid, "Checking eld_valid before reporting Jack "
> +		 "state (default = 1, using verbs only)");
> +
>  #define is_haswell(codec)  ((codec)->core.vendor_id == 0x80862807)
>  #define is_broadwell(codec)    ((codec)->core.vendor_id == 0x80862808)
>  #define is_skylake(codec) ((codec)->core.vendor_id == 0x80862809)
> @@ -1557,8 +1562,9 @@ static bool hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin,
>  	jack = snd_hda_jack_tbl_get(codec, pin_nid);
>  	if (jack) {
>  		jack->block_report = !ret;
> -		jack->pin_sense = (eld->monitor_present && eld->eld_valid) ?
> -			AC_PINSENSE_PRESENCE : 0;
> +		if (checking_eld_valid)
> +			jack->pin_sense = (eld->monitor_present && eld->eld_valid) ?
> +				AC_PINSENSE_PRESENCE : 0;
>  	}
>  	mutex_unlock(&per_pin->lock);
>  	return ret;
> -- 
> 2.17.1
>
Takashi Iwai Nov. 11, 2019, 4:04 p.m. UTC | #2
On Mon, 11 Nov 2019 16:33:45 +0100,
Takashi Iwai wrote:
> 
> On Mon, 11 Nov 2019 15:45:02 +0100,
> Hui Wang wrote:
> > 
> > With the commit 7f641e26a6df ("ALSA: hda/hdmi - Consider eld_valid
> > when reporting jack event"), the driver checks eld_valid before
> > reporting Jack state, this fixes the 4 HDMI/DP audio devices issue.
> > 
> > But recently some users complained that the hdmi audio on their
> > machines couldn't work anymore with this commit. On their machines,
> > the monitor_present is 1 while the eld_valid is 0 when plugging a
> > monitor, and the hdmi audio could work even the eld_valid is 0.
> > 
> > To make the hdmi audio work again on those machines, adding a module
> > parameter, if usrs want to skip the checking eld_valid, they
> > could set checking_eld_valid=0 when loading the module. And this
> > parameter only applies to sense_via_verbs, for those getting eld via
> > component, no need to apply this parameter since it is impossible
> > that present is 1 while eld_valid is 0.
> > 
> > BugLink: https://bugs.launchpad.net/bugs/1834771
> > Fixes: 7f641e26a6df ("ALSA: hda/hdmi - Consider eld_valid when reporting jack event")
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Hui Wang <hui.wang@canonical.com>
> 
> Well, this sort of module option is rather a last resort, so I
> hesitate to apply this.
> 
> The bug reports in the above are a bit hard to digest quickly.
> Could you tell exactly which hardware (and drivers) show the problem?
> 
> FWIW, amdgpu driver already got the audio-component binding recently,
> so this problem shouldn't be triggered, at least in this code path.
> And, for nouveau and radeon, I already submitted the patches to
> support the audio-component binding, but by some reason they haven't
> been merged to the upstream.  In that case, we'd need to ping DRM
> guys.

On the second thought, I wonder whether eld_valid would be corrected
later by the graphics side at all.  If yes, it's a timing issue, and
it can be corrected with the repolling.

A totally untested patch is below.


thanks,

Takashi

--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -1549,19 +1549,25 @@ static bool hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin,
 			do_repoll = true;
 	}
 
-	if (do_repoll)
+	do_repoll |= repoll && eld->eld_valid != eld->monitor_present;
+	if (do_repoll) {
 		schedule_delayed_work(&per_pin->work, msecs_to_jiffies(300));
-	else
+		ret = false;
+	} else {
 		update_eld(codec, per_pin, eld);
-
-	ret = !repoll || !eld->monitor_present || eld->eld_valid;
+		per_pin->repoll_count = 0;
+		ret = true;
+	}
 
 	jack = snd_hda_jack_tbl_get(codec, pin_nid);
 	if (jack) {
 		jack->block_report = !ret;
-		jack->pin_sense = (eld->monitor_present && eld->eld_valid) ?
-			AC_PINSENSE_PRESENCE : 0;
+		if (ret) {
+			jack->pin_sense = (eld->monitor_present && eld->eld_valid) ?
+				AC_PINSENSE_PRESENCE : 0;
+		}
 	}
+
 	mutex_unlock(&per_pin->lock);
 	return ret;
 }
Hui Wang Nov. 12, 2019, 12:21 p.m. UTC | #3
On 2019/11/12 上午12:04, Takashi Iwai wrote:
> On Mon, 11 Nov 2019 16:33:45 +0100,
> Takashi Iwai wrote:
>> On Mon, 11 Nov 2019 15:45:02 +0100,
>> Hui Wang wrote:
>>> With the commit 7f641e26a6df ("ALSA: hda/hdmi - Consider eld_valid
>>> when reporting jack event"), the driver checks eld_valid before
>>> reporting Jack state, this fixes the 4 HDMI/DP audio devices issue.
>>>
>>> But recently some users complained that the hdmi audio on their
>>> machines couldn't work anymore with this commit. On their machines,
>>> the monitor_present is 1 while the eld_valid is 0 when plugging a
>>> monitor, and the hdmi audio could work even the eld_valid is 0.
>>>
>>> To make the hdmi audio work again on those machines, adding a module
>>> parameter, if usrs want to skip the checking eld_valid, they
>>> could set checking_eld_valid=0 when loading the module. And this
>>> parameter only applies to sense_via_verbs, for those getting eld via
>>> component, no need to apply this parameter since it is impossible
>>> that present is 1 while eld_valid is 0.
>>>
>>> BugLink: https://bugs.launchpad.net/bugs/1834771
>>> Fixes: 7f641e26a6df ("ALSA: hda/hdmi - Consider eld_valid when reporting jack event")
>>> Cc: <stable@vger.kernel.org>
>>> Signed-off-by: Hui Wang <hui.wang@canonical.com>
>> Well, this sort of module option is rather a last resort, so I
>> hesitate to apply this.
>>
>> The bug reports in the above are a bit hard to digest quickly.
>> Could you tell exactly which hardware (and drivers) show the problem?
>>
>> FWIW, amdgpu driver already got the audio-component binding recently,
>> so this problem shouldn't be triggered, at least in this code path.
>> And, for nouveau and radeon, I already submitted the patches to
>> support the audio-component binding, but by some reason they haven't
>> been merged to the upstream.  In that case, we'd need to ping DRM
>> guys.

I know the amdgpu driver already supported audio-component, I guess it 
will fix this problem. But it is not easy to backport the related 
patches to ubuntu 4.15 and 5.0 kernels. It will be better if we could 
figure out a fix for sense_via_verbs() too. :-)


> On the second thought, I wonder whether eld_valid would be corrected
> later by the graphics side at all.  If yes, it's a timing issue, and
> it can be corrected with the repolling.
>
> A totally untested patch is below.

I will build a testing kernel with this patch and let the bug reporter 
test it.

Thanks,

Hui.


>
> thanks,
>
> Takashi
>
> --- a/sound/pci/hda/patch_hdmi.c
> +++ b/sound/pci/hda/patch_hdmi.c
> @@ -1549,19 +1549,25 @@ static bool hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin,
>   			do_repoll = true;
>   	}
>   
> -	if (do_repoll)
> +	do_repoll |= repoll && eld->eld_valid != eld->monitor_present;
> +	if (do_repoll) {
>   		schedule_delayed_work(&per_pin->work, msecs_to_jiffies(300));
> -	else
> +		ret = false;
> +	} else {
>   		update_eld(codec, per_pin, eld);
> -
> -	ret = !repoll || !eld->monitor_present || eld->eld_valid;
> +		per_pin->repoll_count = 0;
> +		ret = true;
> +	}
>   
>   	jack = snd_hda_jack_tbl_get(codec, pin_nid);
>   	if (jack) {
>   		jack->block_report = !ret;
> -		jack->pin_sense = (eld->monitor_present && eld->eld_valid) ?
> -			AC_PINSENSE_PRESENCE : 0;
> +		if (ret) {
> +			jack->pin_sense = (eld->monitor_present && eld->eld_valid) ?
> +				AC_PINSENSE_PRESENCE : 0;
> +		}
>   	}
> +
>   	mutex_unlock(&per_pin->lock);
>   	return ret;
>   }
>
Hui Wang Nov. 18, 2019, 4:40 a.m. UTC | #4
On 2019/11/12 下午8:21, Hui Wang wrote:
>
> On 2019/11/12 上午12:04, Takashi Iwai wrote:
>> On Mon, 11 Nov 2019 16:33:45 +0100,
>> Takashi Iwai wrote:
>>> On Mon, 11 Nov 2019 15:45:02 +0100,
>>> Hui Wang wrote:
>
[snip]
>> On the second thought, I wonder whether eld_valid would be corrected
>> later by the graphics side at all.  If yes, it's a timing issue, and
>> it can be corrected with the repolling.
>>
>> A totally untested patch is below.
>
> I will build a testing kernel with this patch and let the bug reporter 
> test it.
>
> Thanks,
>
> Hui.

Hello Takashi,

Tested the patch,  it didn't work. The driver always failed to read the 
speaker allocation from snd_hdmi_get_eld_ati().

This is the dmesg after adding the patch:

https://launchpadlibrarian.net/451420819/dmesg (both presence and 
eld_valid bits are set, but can't get the speaker_alloc)

Regards,

Hui.


>
>
>>
>> thanks,
>>
>> Takashi
>>
>> --- a/sound/pci/hda/patch_hdmi.c
>> +++ b/sound/pci/hda/patch_hdmi.c
>> @@ -1549,19 +1549,25 @@ static bool 
>> hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin,
>>               do_repoll = true;
>>       }
>>   -    if (do_repoll)
>> +    do_repoll |= repoll && eld->eld_valid != eld->monitor_present;
>> +    if (do_repoll) {
>>           schedule_delayed_work(&per_pin->work, msecs_to_jiffies(300));
>> -    else
>> +        ret = false;
>> +    } else {
>>           update_eld(codec, per_pin, eld);
>> -
>> -    ret = !repoll || !eld->monitor_present || eld->eld_valid;
>> +        per_pin->repoll_count = 0;
>> +        ret = true;
>> +    }
>>         jack = snd_hda_jack_tbl_get(codec, pin_nid);
>>       if (jack) {
>>           jack->block_report = !ret;
>> -        jack->pin_sense = (eld->monitor_present && eld->eld_valid) ?
>> -            AC_PINSENSE_PRESENCE : 0;
>> +        if (ret) {
>> +            jack->pin_sense = (eld->monitor_present && 
>> eld->eld_valid) ?
>> +                AC_PINSENSE_PRESENCE : 0;
>> +        }
>>       }
>> +
>>       mutex_unlock(&per_pin->lock);
>>       return ret;
>>   }
>>
Takashi Iwai Nov. 18, 2019, 7:12 a.m. UTC | #5
On Mon, 18 Nov 2019 05:40:52 +0100,
Hui Wang wrote:
> 
> 
> On 2019/11/12 下午8:21, Hui Wang wrote:
> >
> > On 2019/11/12 上午12:04, Takashi Iwai wrote:
> >> On Mon, 11 Nov 2019 16:33:45 +0100,
> >> Takashi Iwai wrote:
> >>> On Mon, 11 Nov 2019 15:45:02 +0100,
> >>> Hui Wang wrote:
> >
> [snip]
> >> On the second thought, I wonder whether eld_valid would be corrected
> >> later by the graphics side at all.  If yes, it's a timing issue, and
> >> it can be corrected with the repolling.
> >>
> >> A totally untested patch is below.
> >
> > I will build a testing kernel with this patch and let the bug
> > reporter test it.
> >
> > Thanks,
> >
> > Hui.
> 
> Hello Takashi,
> 
> Tested the patch,  it didn't work. The driver always failed to read
> the speaker allocation from snd_hdmi_get_eld_ati().
> 
> This is the dmesg after adding the patch:
> 
> https://launchpadlibrarian.net/451420819/dmesg (both presence and
> eld_valid bits are set, but can't get the speaker_alloc)

So it's likely a bug in the graphics driver :)

In anyway, it indicates that it's not about eld_valid check itself.
The eld_valid was returned correctly together with the monitor_present
flag.

I guess the system worked casually with your patch to ignore eld_valid
because we don't care much about the channel mapping if channels <= 2.
IOW, another workaround would be to ignore the error if channels <=
2.

But I wonder whether this state persists after this resume moment.
Could you check what happens if you unload / reload the HD-audio
driver?  Does the read of spk_alloc still fail?


thanks,

Takashi

> 
> Regards,
> 
> Hui.
> 
> 
> >
> >
> >>
> >> thanks,
> >>
> >> Takashi
> >>
> >> --- a/sound/pci/hda/patch_hdmi.c
> >> +++ b/sound/pci/hda/patch_hdmi.c
> >> @@ -1549,19 +1549,25 @@ static bool
> >> hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin,
> >>               do_repoll = true;
> >>       }
> >>   -    if (do_repoll)
> >> +    do_repoll |= repoll && eld->eld_valid != eld->monitor_present;
> >> +    if (do_repoll) {
> >>           schedule_delayed_work(&per_pin->work, msecs_to_jiffies(300));
> >> -    else
> >> +        ret = false;
> >> +    } else {
> >>           update_eld(codec, per_pin, eld);
> >> -
> >> -    ret = !repoll || !eld->monitor_present || eld->eld_valid;
> >> +        per_pin->repoll_count = 0;
> >> +        ret = true;
> >> +    }
> >>         jack = snd_hda_jack_tbl_get(codec, pin_nid);
> >>       if (jack) {
> >>           jack->block_report = !ret;
> >> -        jack->pin_sense = (eld->monitor_present && eld->eld_valid) ?
> >> -            AC_PINSENSE_PRESENCE : 0;
> >> +        if (ret) {
> >> +            jack->pin_sense = (eld->monitor_present &&
> >> eld->eld_valid) ?
> >> +                AC_PINSENSE_PRESENCE : 0;
> >> +        }
> >>       }
> >> +
> >>       mutex_unlock(&per_pin->lock);
> >>       return ret;
> >>   }
> >>
>
Hui Wang Nov. 18, 2019, 7:52 a.m. UTC | #6
On 2019/11/18 下午3:12, Takashi Iwai wrote:
> On Mon, 18 Nov 2019 05:40:52 +0100,
> Hui Wang wrote:
>>
>> On 2019/11/12 下午8:21, Hui Wang wrote:
>>> On 2019/11/12 上午12:04, Takashi Iwai wrote:
>>>> On Mon, 11 Nov 2019 16:33:45 +0100,
>>>> Takashi Iwai wrote:
>>>>> On Mon, 11 Nov 2019 15:45:02 +0100,
>>>>> Hui Wang wrote:
>> [snip]
>>>> On the second thought, I wonder whether eld_valid would be corrected
>>>> later by the graphics side at all.  If yes, it's a timing issue, and
>>>> it can be corrected with the repolling.
>>>>
>>>> A totally untested patch is below.
>>> I will build a testing kernel with this patch and let the bug
>>> reporter test it.
>>>
>>> Thanks,
>>>
>>> Hui.
>> Hello Takashi,
>>
>> Tested the patch,  it didn't work. The driver always failed to read
>> the speaker allocation from snd_hdmi_get_eld_ati().
>>
>> This is the dmesg after adding the patch:
>>
>> https://launchpadlibrarian.net/451420819/dmesg (both presence and
>> eld_valid bits are set, but can't get the speaker_alloc)
> So it's likely a bug in the graphics driver :)
>
> In anyway, it indicates that it's not about eld_valid check itself.
> The eld_valid was returned correctly together with the monitor_present
> flag.
>
> I guess the system worked casually with your patch to ignore eld_valid
> because we don't care much about the channel mapping if channels <= 2.
> IOW, another workaround would be to ignore the error if channels <=
> 2.
>
> But I wonder whether this state persists after this resume moment.
> Could you check what happens if you unload / reload the HD-audio
> driver?  Does the read of spk_alloc still fail?

OK, will test it.

Thanks,

Hui.

>
>
> thanks,
>
> Takashi
>
>> Regards,
>>
>> Hui.
>>
>>
>>>
>>>> thanks,
>>>>
>>>> Takashi
>>>>
>>>> --- a/sound/pci/hda/patch_hdmi.c
>>>> +++ b/sound/pci/hda/patch_hdmi.c
>>>> @@ -1549,19 +1549,25 @@ static bool
>>>> hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin,
>>>>                do_repoll = true;
>>>>        }
>>>>    -    if (do_repoll)
>>>> +    do_repoll |= repoll && eld->eld_valid != eld->monitor_present;
>>>> +    if (do_repoll) {
>>>>            schedule_delayed_work(&per_pin->work, msecs_to_jiffies(300));
>>>> -    else
>>>> +        ret = false;
>>>> +    } else {
>>>>            update_eld(codec, per_pin, eld);
>>>> -
>>>> -    ret = !repoll || !eld->monitor_present || eld->eld_valid;
>>>> +        per_pin->repoll_count = 0;
>>>> +        ret = true;
>>>> +    }
>>>>          jack = snd_hda_jack_tbl_get(codec, pin_nid);
>>>>        if (jack) {
>>>>            jack->block_report = !ret;
>>>> -        jack->pin_sense = (eld->monitor_present && eld->eld_valid) ?
>>>> -            AC_PINSENSE_PRESENCE : 0;
>>>> +        if (ret) {
>>>> +            jack->pin_sense = (eld->monitor_present &&
>>>> eld->eld_valid) ?
>>>> +                AC_PINSENSE_PRESENCE : 0;
>>>> +        }
>>>>        }
>>>> +
>>>>        mutex_unlock(&per_pin->lock);
>>>>        return ret;
>>>>    }
>>>>
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Patch
diff mbox series

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index be8a977fc684..d70fca4f4411 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -37,6 +37,11 @@  static bool static_hdmi_pcm;
 module_param(static_hdmi_pcm, bool, 0644);
 MODULE_PARM_DESC(static_hdmi_pcm, "Don't restrict PCM parameters per ELD info");
 
+static bool checking_eld_valid = true;
+module_param(checking_eld_valid, bool, 0644);
+MODULE_PARM_DESC(checking_eld_valid, "Checking eld_valid before reporting Jack "
+		 "state (default = 1, using verbs only)");
+
 #define is_haswell(codec)  ((codec)->core.vendor_id == 0x80862807)
 #define is_broadwell(codec)    ((codec)->core.vendor_id == 0x80862808)
 #define is_skylake(codec) ((codec)->core.vendor_id == 0x80862809)
@@ -1557,8 +1562,9 @@  static bool hdmi_present_sense_via_verbs(struct hdmi_spec_per_pin *per_pin,
 	jack = snd_hda_jack_tbl_get(codec, pin_nid);
 	if (jack) {
 		jack->block_report = !ret;
-		jack->pin_sense = (eld->monitor_present && eld->eld_valid) ?
-			AC_PINSENSE_PRESENCE : 0;
+		if (checking_eld_valid)
+			jack->pin_sense = (eld->monitor_present && eld->eld_valid) ?
+				AC_PINSENSE_PRESENCE : 0;
 	}
 	mutex_unlock(&per_pin->lock);
 	return ret;