| Message ID | 1343702825-15439-5-git-send-email-xingchao.wang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
At Tue, 31 Jul 2012 10:47:05 +0800, Wang Xingchao wrote: > > There's one issue for HDMI pins, even the related pin will be enabled > when the stream is active but the GPU registers show the PIN is not in > active state, so we force all pins in active state and donot close it > when the stream is closed. > > Signed-off-by: Wang Xingchao <xingchao.wang@intel.com> Oh no, this is a bad workaround. Please be more exact at which timing the pin must be set. Setting all the time is just a wrong approach. Also, at the next time, please Cc to alsa-devel and other developers. I don't want that such a patch is merged secretly through drm tree :) > --- > sound/pci/hda/patch_hdmi.c | 15 ++++++++------- > 1 file changed, 8 insertions(+), 7 deletions(-) > > diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c > index ad319d4..64cc9e0 100644 > --- a/sound/pci/hda/patch_hdmi.c > +++ b/sound/pci/hda/patch_hdmi.c > @@ -421,13 +421,17 @@ static void hdmi_write_dip_byte(struct hda_codec *codec, hda_nid_t pin_nid, > > static void hdmi_init_pin(struct hda_codec *codec, hda_nid_t pin_nid) > { > + /* Force all output Pins enabled */ > + snd_printk(KERN_DEBUG "HDMI: enable all output\n"); > + snd_hda_codec_write(codec, pin_nid, 0, > + AC_VERB_SET_PIN_WIDGET_CONTROL, 0x40); Don't put debug prints unconditionally. > + > /* Unmute */ > - if (get_wcaps(codec, pin_nid) & AC_WCAP_OUT_AMP) > + if (get_wcaps(codec, pin_nid) & AC_WCAP_OUT_AMP) { > + snd_printk(KERN_DEBUG "HDMI: unmute pin %d\n", pin_nid); Ditto. thanks, Takashi > snd_hda_codec_write(codec, pin_nid, 0, > AC_VERB_SET_AMP_GAIN_MUTE, AMP_OUT_UNMUTE); > - /* Disable pin out until stream is active*/ > - snd_hda_codec_write(codec, pin_nid, 0, > - AC_VERB_SET_PIN_WIDGET_CONTROL, 0); > + } > } > > static int hdmi_get_channel_count(struct hda_codec *codec, hda_nid_t cvt_nid) > @@ -1190,9 +1194,6 @@ static int generic_hdmi_playback_pcm_cleanup(struct hda_pcm_stream *hinfo, > > pinctl = snd_hda_codec_read(codec, per_pin->pin_nid, 0, > AC_VERB_GET_PIN_WIDGET_CONTROL, 0); > - snd_hda_codec_write(codec, per_pin->pin_nid, 0, > - AC_VERB_SET_PIN_WIDGET_CONTROL, > - pinctl & ~PIN_OUT); > snd_hda_spdif_ctls_unassign(codec, pin_idx); > } > > -- > 1.7.9.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
Hi Takashi, > -----Original Message----- > From: Takashi Iwai [mailto:tiwai@suse.de] > Sent: Tuesday, July 31, 2012 7:18 PM > To: Wang, Xingchao > Cc: intel-gfx@lists.freedesktop.org; Zanoni, Paulo R > Subject: Re: [Intel-gfx] [PATCH V2 4/4] ALSA HDA: Force HDMI pins enabled > > At Tue, 31 Jul 2012 10:47:05 +0800, > Wang Xingchao wrote: > > > > There's one issue for HDMI pins, even the related pin will be enabled > > when the stream is active but the GPU registers show the PIN is not in > > active state, so we force all pins in active state and donot close it > > when the stream is closed. > > > > Signed-off-by: Wang Xingchao <xingchao.wang@intel.com> > > Oh no, this is a bad workaround. > > Please be more exact at which timing the pin must be set. > Setting all the time is just a wrong approach. Sorry for that, I am still trying to find out the root cause and the patch was intended for test only. Basically the HDA verb made the change but GPU side does not reflect the correct state, did you ever meet same phenomenon? There's one similar bug: HBR sound passthrough (DTS-HS-MA, Dolby-Turuhd) not working on Intel Sandy Bridge https://bugs.freedesktop.org/show_bug.cgi?id=49055 > > Also, at the next time, please Cc to alsa-devel and other developers. > I don't want that such a patch is merged secretly through drm tree :) Hmm, sure. i will cc to alsa-devel when send out the final version for review. Thanks your clarification. I will take more care when send out alsa patches. :) --xingchao > > > --- > > sound/pci/hda/patch_hdmi.c | 15 ++++++++------- > > 1 file changed, 8 insertions(+), 7 deletions(-) > > > > diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c > > index ad319d4..64cc9e0 100644 > > --- a/sound/pci/hda/patch_hdmi.c > > +++ b/sound/pci/hda/patch_hdmi.c > > @@ -421,13 +421,17 @@ static void hdmi_write_dip_byte(struct hda_codec > > *codec, hda_nid_t pin_nid, > > > > static void hdmi_init_pin(struct hda_codec *codec, hda_nid_t pin_nid) > > { > > + /* Force all output Pins enabled */ > > + snd_printk(KERN_DEBUG "HDMI: enable all output\n"); > > + snd_hda_codec_write(codec, pin_nid, 0, > > + AC_VERB_SET_PIN_WIDGET_CONTROL, 0x40); > > Don't put debug prints unconditionally. > > > + > > /* Unmute */ > > - if (get_wcaps(codec, pin_nid) & AC_WCAP_OUT_AMP) > > + if (get_wcaps(codec, pin_nid) & AC_WCAP_OUT_AMP) { > > + snd_printk(KERN_DEBUG "HDMI: unmute pin %d\n", pin_nid); > > Ditto. > > > thanks, > > Takashi > > > > snd_hda_codec_write(codec, pin_nid, 0, > > AC_VERB_SET_AMP_GAIN_MUTE, AMP_OUT_UNMUTE); > > - /* Disable pin out until stream is active*/ > > - snd_hda_codec_write(codec, pin_nid, 0, > > - AC_VERB_SET_PIN_WIDGET_CONTROL, 0); > > + } > > } > > > > static int hdmi_get_channel_count(struct hda_codec *codec, hda_nid_t > > cvt_nid) @@ -1190,9 +1194,6 @@ static int > > generic_hdmi_playback_pcm_cleanup(struct hda_pcm_stream *hinfo, > > > > pinctl = snd_hda_codec_read(codec, per_pin->pin_nid, 0, > > AC_VERB_GET_PIN_WIDGET_CONTROL, 0); > > - snd_hda_codec_write(codec, per_pin->pin_nid, 0, > > - AC_VERB_SET_PIN_WIDGET_CONTROL, > > - pinctl & ~PIN_OUT); > > snd_hda_spdif_ctls_unassign(codec, pin_idx); > > } > > > > -- > > 1.7.9.5 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > >
At Wed, 1 Aug 2012 00:27:02 +0000, Wang, Xingchao wrote: > > Hi Takashi, > > > -----Original Message----- > > From: Takashi Iwai [mailto:tiwai@suse.de] > > Sent: Tuesday, July 31, 2012 7:18 PM > > To: Wang, Xingchao > > Cc: intel-gfx@lists.freedesktop.org; Zanoni, Paulo R > > Subject: Re: [Intel-gfx] [PATCH V2 4/4] ALSA HDA: Force HDMI pins enabled > > > > At Tue, 31 Jul 2012 10:47:05 +0800, > > Wang Xingchao wrote: > > > > > > There's one issue for HDMI pins, even the related pin will be enabled > > > when the stream is active but the GPU registers show the PIN is not in > > > active state, so we force all pins in active state and donot close it > > > when the stream is closed. > > > > > > Signed-off-by: Wang Xingchao <xingchao.wang@intel.com> > > > > Oh no, this is a bad workaround. > > > > Please be more exact at which timing the pin must be set. > > Setting all the time is just a wrong approach. > > Sorry for that, I am still trying to find out the root cause and the patch was intended for test only. I see. > Basically the HDA verb made the change but GPU side does not reflect the correct state, did you ever meet same phenomenon? > There's one similar bug: > HBR sound passthrough (DTS-HS-MA, Dolby-Turuhd) not working on Intel Sandy Bridge > https://bugs.freedesktop.org/show_bug.cgi?id=49055 Well, I myself can't check it due to lack of a test device. But if any, this should be applied to only Intel codecs, as Nvidia (and likely AMD) apparently works as is now. And another more interesting thing is to know _when_ you need to set the pin at all. There must be some timing that GPU checks the codec pin state. If it's just once before sending ELD, then yes, the pin has to be set persistently. thanks, Takashi > > Also, at the next time, please Cc to alsa-devel and other developers. > > I don't want that such a patch is merged secretly through drm tree :) > > Hmm, sure. i will cc to alsa-devel when send out the final version for review. > > Thanks your clarification. I will take more care when send out alsa patches. :) > > --xingchao > > > > > > --- > > > sound/pci/hda/patch_hdmi.c | 15 ++++++++------- > > > 1 file changed, 8 insertions(+), 7 deletions(-) > > > > > > diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c > > > index ad319d4..64cc9e0 100644 > > > --- a/sound/pci/hda/patch_hdmi.c > > > +++ b/sound/pci/hda/patch_hdmi.c > > > @@ -421,13 +421,17 @@ static void hdmi_write_dip_byte(struct hda_codec > > > *codec, hda_nid_t pin_nid, > > > > > > static void hdmi_init_pin(struct hda_codec *codec, hda_nid_t pin_nid) > > > { > > > + /* Force all output Pins enabled */ > > > + snd_printk(KERN_DEBUG "HDMI: enable all output\n"); > > > + snd_hda_codec_write(codec, pin_nid, 0, > > > + AC_VERB_SET_PIN_WIDGET_CONTROL, 0x40); > > > > Don't put debug prints unconditionally. > > > > > + > > > /* Unmute */ > > > - if (get_wcaps(codec, pin_nid) & AC_WCAP_OUT_AMP) > > > + if (get_wcaps(codec, pin_nid) & AC_WCAP_OUT_AMP) { > > > + snd_printk(KERN_DEBUG "HDMI: unmute pin %d\n", pin_nid); > > > > Ditto. > > > > > > thanks, > > > > Takashi > > > > > > > snd_hda_codec_write(codec, pin_nid, 0, > > > AC_VERB_SET_AMP_GAIN_MUTE, AMP_OUT_UNMUTE); > > > - /* Disable pin out until stream is active*/ > > > - snd_hda_codec_write(codec, pin_nid, 0, > > > - AC_VERB_SET_PIN_WIDGET_CONTROL, 0); > > > + } > > > } > > > > > > static int hdmi_get_channel_count(struct hda_codec *codec, hda_nid_t > > > cvt_nid) @@ -1190,9 +1194,6 @@ static int > > > generic_hdmi_playback_pcm_cleanup(struct hda_pcm_stream *hinfo, > > > > > > pinctl = snd_hda_codec_read(codec, per_pin->pin_nid, 0, > > > AC_VERB_GET_PIN_WIDGET_CONTROL, 0); > > > - snd_hda_codec_write(codec, per_pin->pin_nid, 0, > > > - AC_VERB_SET_PIN_WIDGET_CONTROL, > > > - pinctl & ~PIN_OUT); > > > snd_hda_spdif_ctls_unassign(codec, pin_idx); > > > } > > > > > > -- > > > 1.7.9.5 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > >
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index ad319d4..64cc9e0 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -421,13 +421,17 @@ static void hdmi_write_dip_byte(struct hda_codec *codec, hda_nid_t pin_nid, static void hdmi_init_pin(struct hda_codec *codec, hda_nid_t pin_nid) { + /* Force all output Pins enabled */ + snd_printk(KERN_DEBUG "HDMI: enable all output\n"); + snd_hda_codec_write(codec, pin_nid, 0, + AC_VERB_SET_PIN_WIDGET_CONTROL, 0x40); + /* Unmute */ - if (get_wcaps(codec, pin_nid) & AC_WCAP_OUT_AMP) + if (get_wcaps(codec, pin_nid) & AC_WCAP_OUT_AMP) { + snd_printk(KERN_DEBUG "HDMI: unmute pin %d\n", pin_nid); snd_hda_codec_write(codec, pin_nid, 0, AC_VERB_SET_AMP_GAIN_MUTE, AMP_OUT_UNMUTE); - /* Disable pin out until stream is active*/ - snd_hda_codec_write(codec, pin_nid, 0, - AC_VERB_SET_PIN_WIDGET_CONTROL, 0); + } } static int hdmi_get_channel_count(struct hda_codec *codec, hda_nid_t cvt_nid) @@ -1190,9 +1194,6 @@ static int generic_hdmi_playback_pcm_cleanup(struct hda_pcm_stream *hinfo, pinctl = snd_hda_codec_read(codec, per_pin->pin_nid, 0, AC_VERB_GET_PIN_WIDGET_CONTROL, 0); - snd_hda_codec_write(codec, per_pin->pin_nid, 0, - AC_VERB_SET_PIN_WIDGET_CONTROL, - pinctl & ~PIN_OUT); snd_hda_spdif_ctls_unassign(codec, pin_idx); }
There's one issue for HDMI pins, even the related pin will be enabled when the stream is active but the GPU registers show the PIN is not in active state, so we force all pins in active state and donot close it when the stream is closed. Signed-off-by: Wang Xingchao <xingchao.wang@intel.com> --- sound/pci/hda/patch_hdmi.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)