| Message ID | 5166C939.80003@canonical.com |
|---|---|
| State | New |
| Headers | show |
On 04/11/2013 04:31 PM, Tim Gardner wrote: > On 04/11/2013 07:28 AM, Leann Ogasawara wrote: >> On 04/11/2013 06:09 AM, Leann Ogasawara wrote: >>> Applied to Raring master-next. >> >> Hi David, >> >> After I'd applied this to Raring master-next, there was some concern >> raised about the patch via the #ubuntu-kernel IRC channel. We are >> hoping to get some clarification and feedback from you. >> >> [06:12:36] <rtg_> ogasawara, actually, I was gonna question David on >> that patch. it seems over engineered to me. >> [06:14:13] <ogasawara> rtg_: I thought it seemed fairly well contained >> and tested >> [06:14:40] <ogasawara> rtg_: it's not too late to yank it if you have >> reservations >> [06:15:12] <rtg_> ogasawara, in that it only affects haswell. however, >> why not just force the D0 and mute settings rather then interrogate the >> HW ? less code I think. >> [06:17:24] <ogasawara> rtg_: it's a valid question, want me to punt it >> till we get clarification? >> [06:18:54] <rtg_> ogasawara, if we can get ahold of him today. >> [06:20:23] <rtg_> ogasawara, by collapsing the code into a more >> definitive form I might worry about audio artifacts such as pops and >> clicks whilst programming those registers. thats really what I'd like to >> confirm from him. >> >>> >>> On 04/11/2013 12:38 AM, David Henningsson wrote: >>>> BugLink: https://bugs.launchpad.net/bugs/1167270 >>>> >>>> This patch has been tested on two machines, and found to resolve one >>>> of the HDMI audio issues on Haswell. >>>> >>>> The root cause is lack of synchronisation between the video driver and >>>> the audio driver. Upstream wants to resolve this by adding >>>> synchronisation mechanisms, so they won't take this patch. But since >>>> kernel freeze is today, I wanted to ask you to accept this >>>> intermediate workaround, which at every playback start checks for the >>>> error and corrects it if found. >>>> >>>> >>>> >>>> >>> >> > > Perhaps something like the attached. > Thanks for the review. There are two things here: The D0 and the mute. For the D0 part, I *think* setting it always is harmless and shouldn't cause any clicks or similar. After all, this is a digital connection so artifacts should be avoided by the recipient (HDMI monitor/receiver) anyway. The gain to keep it as it is would be to avoid the 40 ms delay in case this does not happen (which is does not in the majority of cases). But probably just enabling it, skipping the delay, and not read it back for debugging would work too, I just haven't tested it (and can't do so since the hw is in Asia), and I know you like patches to be tested before you apply them :-) For the mute part; I think the patch suggested by rtg is buggy since it always enables audio, without taking the mute kcontrol into affect: there is a "IEC958 Playback Switch" control that user could set to turn both channels off legitimately. So the mute part needs to stay the way it was in my version of the patch.
On 04/11/2013 09:25 PM, David Henningsson wrote: > On 04/11/2013 04:31 PM, Tim Gardner wrote: >> On 04/11/2013 07:28 AM, Leann Ogasawara wrote: >>> On 04/11/2013 06:09 AM, Leann Ogasawara wrote: >>>> Applied to Raring master-next. >>> >>> Hi David, >>> >>> After I'd applied this to Raring master-next, there was some concern >>> raised about the patch via the #ubuntu-kernel IRC channel. We are >>> hoping to get some clarification and feedback from you. >>> >>> [06:12:36] <rtg_> ogasawara, actually, I was gonna question David on >>> that patch. it seems over engineered to me. >>> [06:14:13] <ogasawara> rtg_: I thought it seemed fairly well contained >>> and tested >>> [06:14:40] <ogasawara> rtg_: it's not too late to yank it if you have >>> reservations >>> [06:15:12] <rtg_> ogasawara, in that it only affects haswell. however, >>> why not just force the D0 and mute settings rather then interrogate the >>> HW ? less code I think. >>> [06:17:24] <ogasawara> rtg_: it's a valid question, want me to punt it >>> till we get clarification? >>> [06:18:54] <rtg_> ogasawara, if we can get ahold of him today. >>> [06:20:23] <rtg_> ogasawara, by collapsing the code into a more >>> definitive form I might worry about audio artifacts such as pops and >>> clicks whilst programming those registers. thats really what I'd like to >>> confirm from him. >>> >>>> >>>> On 04/11/2013 12:38 AM, David Henningsson wrote: >>>>> BugLink: https://bugs.launchpad.net/bugs/1167270 >>>>> >>>>> This patch has been tested on two machines, and found to resolve one >>>>> of the HDMI audio issues on Haswell. >>>>> >>>>> The root cause is lack of synchronisation between the video driver and >>>>> the audio driver. Upstream wants to resolve this by adding >>>>> synchronisation mechanisms, so they won't take this patch. But since >>>>> kernel freeze is today, I wanted to ask you to accept this >>>>> intermediate workaround, which at every playback start checks for the >>>>> error and corrects it if found. >>>>> >>>>> >>>>> >>>>> >>>> >>> >> >> Perhaps something like the attached. >> > > Thanks for the review. > > There are two things here: The D0 and the mute. > > For the D0 part, I *think* setting it always is harmless and shouldn't > cause any clicks or similar. After all, this is a digital connection so > artifacts should be avoided by the recipient (HDMI monitor/receiver) > anyway. > The gain to keep it as it is would be to avoid the 40 ms delay in case > this does not happen (which is does not in the majority of cases). But > probably just enabling it, skipping the delay, and not read it back for > debugging would work too, I just haven't tested it (and can't do so > since the hw is in Asia), and I know you like patches to be tested > before you apply them :-) > > For the mute part; I think the patch suggested by rtg is buggy since it > always enables audio, without taking the mute kcontrol into affect: > there is a "IEC958 Playback Switch" control that user could set to turn > both channels off legitimately. So the mute part needs to stay the way > it was in my version of the patch. It looks like this patch did not make it into Raring before the kernel freeze?
On 04/12/2013 03:15 AM, David Henningsson wrote: > On 04/11/2013 09:25 PM, David Henningsson wrote: >> On 04/11/2013 04:31 PM, Tim Gardner wrote: >>> On 04/11/2013 07:28 AM, Leann Ogasawara wrote: >>>> On 04/11/2013 06:09 AM, Leann Ogasawara wrote: >>>>> Applied to Raring master-next. >>>> >>>> Hi David, >>>> >>>> After I'd applied this to Raring master-next, there was some concern >>>> raised about the patch via the #ubuntu-kernel IRC channel. We are >>>> hoping to get some clarification and feedback from you. >>>> >>>> [06:12:36] <rtg_> ogasawara, actually, I was gonna question David on >>>> that patch. it seems over engineered to me. >>>> [06:14:13] <ogasawara> rtg_: I thought it seemed fairly well contained >>>> and tested >>>> [06:14:40] <ogasawara> rtg_: it's not too late to yank it if you have >>>> reservations >>>> [06:15:12] <rtg_> ogasawara, in that it only affects haswell. however, >>>> why not just force the D0 and mute settings rather then interrogate the >>>> HW ? less code I think. >>>> [06:17:24] <ogasawara> rtg_: it's a valid question, want me to punt it >>>> till we get clarification? >>>> [06:18:54] <rtg_> ogasawara, if we can get ahold of him today. >>>> [06:20:23] <rtg_> ogasawara, by collapsing the code into a more >>>> definitive form I might worry about audio artifacts such as pops and >>>> clicks whilst programming those registers. thats really what I'd >>>> like to >>>> confirm from him. >>>> >>>>> >>>>> On 04/11/2013 12:38 AM, David Henningsson wrote: >>>>>> BugLink: https://bugs.launchpad.net/bugs/1167270 >>>>>> >>>>>> This patch has been tested on two machines, and found to resolve one >>>>>> of the HDMI audio issues on Haswell. >>>>>> >>>>>> The root cause is lack of synchronisation between the video driver >>>>>> and >>>>>> the audio driver. Upstream wants to resolve this by adding >>>>>> synchronisation mechanisms, so they won't take this patch. But since >>>>>> kernel freeze is today, I wanted to ask you to accept this >>>>>> intermediate workaround, which at every playback start checks for the >>>>>> error and corrects it if found. >>>>>> >>>>>> >>>>>> >>>>>> >>>>> >>>> >>> >>> Perhaps something like the attached. >>> >> >> Thanks for the review. >> >> There are two things here: The D0 and the mute. >> >> For the D0 part, I *think* setting it always is harmless and shouldn't >> cause any clicks or similar. After all, this is a digital connection so >> artifacts should be avoided by the recipient (HDMI monitor/receiver) >> anyway. >> The gain to keep it as it is would be to avoid the 40 ms delay in case >> this does not happen (which is does not in the majority of cases). But >> probably just enabling it, skipping the delay, and not read it back for >> debugging would work too, I just haven't tested it (and can't do so >> since the hw is in Asia), and I know you like patches to be tested >> before you apply them :-) >> >> For the mute part; I think the patch suggested by rtg is buggy since it >> always enables audio, without taking the mute kcontrol into affect: >> there is a "IEC958 Playback Switch" control that user could set to turn >> both channels off legitimately. So the mute part needs to stay the way >> it was in my version of the patch. > > It looks like this patch did not make it into Raring before the kernel > freeze? > > It'll make it into the final upload.
On 04/16/2013 02:39 PM, Tim Gardner wrote: > On 04/12/2013 03:15 AM, David Henningsson wrote: >> On 04/11/2013 09:25 PM, David Henningsson wrote: >>> On 04/11/2013 04:31 PM, Tim Gardner wrote: >>>> On 04/11/2013 07:28 AM, Leann Ogasawara wrote: >>>>> On 04/11/2013 06:09 AM, Leann Ogasawara wrote: >>>>>> Applied to Raring master-next. >>>>> >>>>> Hi David, >>>>> >>>>> After I'd applied this to Raring master-next, there was some concern >>>>> raised about the patch via the #ubuntu-kernel IRC channel. We are >>>>> hoping to get some clarification and feedback from you. >>>>> >>>>> [06:12:36] <rtg_> ogasawara, actually, I was gonna question David on >>>>> that patch. it seems over engineered to me. >>>>> [06:14:13] <ogasawara> rtg_: I thought it seemed fairly well contained >>>>> and tested >>>>> [06:14:40] <ogasawara> rtg_: it's not too late to yank it if you have >>>>> reservations >>>>> [06:15:12] <rtg_> ogasawara, in that it only affects haswell. however, >>>>> why not just force the D0 and mute settings rather then interrogate the >>>>> HW ? less code I think. >>>>> [06:17:24] <ogasawara> rtg_: it's a valid question, want me to punt it >>>>> till we get clarification? >>>>> [06:18:54] <rtg_> ogasawara, if we can get ahold of him today. >>>>> [06:20:23] <rtg_> ogasawara, by collapsing the code into a more >>>>> definitive form I might worry about audio artifacts such as pops and >>>>> clicks whilst programming those registers. thats really what I'd >>>>> like to >>>>> confirm from him. >>>>> >>>>>> >>>>>> On 04/11/2013 12:38 AM, David Henningsson wrote: >>>>>>> BugLink: https://bugs.launchpad.net/bugs/1167270 >>>>>>> >>>>>>> This patch has been tested on two machines, and found to resolve one >>>>>>> of the HDMI audio issues on Haswell. >>>>>>> >>>>>>> The root cause is lack of synchronisation between the video driver >>>>>>> and >>>>>>> the audio driver. Upstream wants to resolve this by adding >>>>>>> synchronisation mechanisms, so they won't take this patch. But since >>>>>>> kernel freeze is today, I wanted to ask you to accept this >>>>>>> intermediate workaround, which at every playback start checks for the >>>>>>> error and corrects it if found. >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>>> Perhaps something like the attached. >>>> >>> >>> Thanks for the review. >>> >>> There are two things here: The D0 and the mute. >>> >>> For the D0 part, I *think* setting it always is harmless and shouldn't >>> cause any clicks or similar. After all, this is a digital connection so >>> artifacts should be avoided by the recipient (HDMI monitor/receiver) >>> anyway. >>> The gain to keep it as it is would be to avoid the 40 ms delay in case >>> this does not happen (which is does not in the majority of cases). But >>> probably just enabling it, skipping the delay, and not read it back for >>> debugging would work too, I just haven't tested it (and can't do so >>> since the hw is in Asia), and I know you like patches to be tested >>> before you apply them :-) >>> >>> For the mute part; I think the patch suggested by rtg is buggy since it >>> always enables audio, without taking the mute kcontrol into affect: >>> there is a "IEC958 Playback Switch" control that user could set to turn >>> both channels off legitimately. So the mute part needs to stay the way >>> it was in my version of the patch. >> >> It looks like this patch did not make it into Raring before the kernel >> freeze? >> >> > > It'll make it into the final upload. > Thanks. Also, FYI, upstream decided to take it for 3.10 because they won't finish the advanced synchronisation stuff in the 3.10 kernel cycle.
From 418a4f3f415198a076e1b080a57de5c06707a7e4 Mon Sep 17 00:00:00 2001 From: David Henningsson <david.henningsson@canonical.com> Date: Wed, 10 Apr 2013 12:26:07 +0200 Subject: [PATCH] ALSA: hda - fixup D3 pin and right channel mute on Haswell HDMI audio When graphics initializes the HDMI chip, sometimes this leads to pins going into D3 and right channel being muted. If the audio driver finishes initialization before the graphic driver does, this situation becomes permanent. This is a workaround that checks for this situation and corrects it on playback prepare. It has been verified working on at least one machine. BugLink: https://bugs.launchpad.net/bugs/1167270 Signed-off-by: David Henningsson <david.henningsson@canonical.com> Signed-off-by: Leann Ogasawara <leann.ogasawara@canonical.com> --- sound/pci/hda/patch_hdmi.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 53755d6..db3f31f 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -998,6 +998,28 @@ static void hdmi_unsol_event(struct hda_codec *codec, unsigned int res) hdmi_non_intrinsic_event(codec, res); } +static void haswell_verify_pin_D0(struct hda_codec *codec, hda_nid_t nid) +{ + int pwr, lamp, ramp; + + snd_hda_codec_write(codec, nid, 0, AC_VERB_SET_POWER_STATE, AC_PWRST_D0); + msleep(40); + pwr = snd_hda_codec_read(codec, nid, 0, AC_VERB_GET_POWER_STATE, 0); + pwr = (pwr & AC_PWRST_ACTUAL) >> AC_PWRST_ACTUAL_SHIFT; + snd_printd("Haswell HDMI audio: Power for pin 0x%x is now D%d\n", nid, pwr); + + snd_hda_codec_write(codec, nid, 0, AC_VERB_SET_AMP_GAIN_MUTE, + AC_AMP_SET_RIGHT | AC_AMP_SET_OUTPUT); + snd_hda_codec_write(codec, nid, 0, AC_VERB_SET_AMP_GAIN_MUTE, + AC_AMP_SET_LEFT | AC_AMP_SET_OUTPUT); + + lamp = snd_hda_codec_read(codec, nid, 0, AC_VERB_GET_AMP_GAIN_MUTE, + AC_AMP_GET_LEFT | AC_AMP_GET_OUTPUT); + ramp = snd_hda_codec_read(codec, nid, 0, AC_VERB_GET_AMP_GAIN_MUTE, + AC_AMP_GET_RIGHT | AC_AMP_GET_OUTPUT); + snd_printd("Haswell HDMI audio: Mute after set on pin 0x%x: [0x%x 0x%x]\n", nid, lamp, ramp); +} + /* * Callbacks */ @@ -1012,6 +1034,9 @@ static int hdmi_setup_stream(struct hda_codec *codec, hda_nid_t cvt_nid, int pinctl; int new_pinctl = 0; + if (codec->vendor_id == 0x80862807) + haswell_verify_pin_D0(codec, pin_nid); + if (snd_hda_query_pin_caps(codec, pin_nid) & AC_PINCAP_HBR) { pinctl = snd_hda_codec_read(codec, pin_nid, 0, AC_VERB_GET_PIN_WIDGET_CONTROL, 0); -- 1.7.9.5