| Message ID | 20191111144502.22910-1-hui.wang@canonical.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ALSA: hda/hdmi - add a parameter to let users decide if checking the eld_valid | expand |
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 >
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; }
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; > } >
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; >> } >>
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; > >> } > >> >
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
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;
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(-)