* [PATCH] ALSA: hda - unmute pin amplifier in infoframe setup for Haswell @ 2013-09-03 20:38 mengdong.lin 2013-09-04 9:55 ` David Henningsson 0 siblings, 1 reply; 4+ messages in thread From: mengdong.lin @ 2013-09-03 20:38 UTC (permalink / raw) To: alsa-devel, tiwai; +Cc: Mengdong Lin, David Henningsson From: Mengdong Lin <mengdong.lin@intel.com> When Gfx driver reconnects a port and transcoder, the pin amplifier will be muted. To enable sound, the pin amp need to be unmuted. This patch - moves pin amp unmuting from stream preparing to hdmi_setup_audio_infoframe(). So if port:transcoder reconnection happens during stream playback, the ELDV unsol event can stil trigger pin's amp unmuting when re-setting up audio info frame. - remove reading pin amp status before unmuting for speed-up, since pin amp should always be unmuted. - rename haswell_verify_pin_D0() to haswell_verify_pin_cvt_D0(), since the convertor power state is also fixed here. - define is_haswell() to replace checking Haswell vendor ID everywhere. This patch is mostly based on suggestion of David Henningsson. Cc: David Henningsson <david.henningsson@canonical.com> Signed-off-by: Mengdong Lin <mengdong.lin@intel.com> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 9a58893..471c158 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -44,6 +44,8 @@ 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"); +#define is_haswell(codec) ((codec)->vendor_id == 0x80862807) + struct hdmi_spec_per_cvt { hda_nid_t cvt_nid; int assigned; @@ -894,6 +896,11 @@ static void hdmi_setup_audio_infoframe(struct hda_codec *codec, if (!channels) return; + if (is_haswell(codec)) + snd_hda_codec_write(codec, pin_nid, 0, + AC_VERB_SET_AMP_GAIN_MUTE, + AMP_OUT_UNMUTE); + eld = &per_pin->sink_eld; if (!eld->monitor_present) return; @@ -1033,10 +1040,10 @@ 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, +static void haswell_verify_pin_cvt_D0(struct hda_codec *codec, hda_nid_t cvt_nid, hda_nid_t nid) { - int pwr, lamp, ramp; + int pwr; /* For Haswell, the converter 1/2 may keep in D3 state after bootup, * thus pins could only choose converter 0 for use. Make sure the @@ -1052,25 +1059,6 @@ static void haswell_verify_pin_D0(struct hda_codec *codec, 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); } - - 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); - if (lamp != ramp) { - snd_hda_codec_write(codec, nid, 0, AC_VERB_SET_AMP_GAIN_MUTE, - AC_AMP_SET_RIGHT | AC_AMP_SET_OUTPUT | lamp); - - 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); - } } /* @@ -1087,8 +1075,8 @@ 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, cvt_nid, pin_nid); + if (is_haswell(codec)) + haswell_verify_pin_cvt_D0(codec, cvt_nid, pin_nid); if (snd_hda_query_pin_caps(codec, pin_nid) & AC_PINCAP_HBR) { pinctl = snd_hda_codec_read(codec, pin_nid, 0, @@ -1227,7 +1215,7 @@ static int hdmi_pcm_open(struct hda_pcm_stream *hinfo, mux_idx); /* configure unused pins to choose other converters */ - if (codec->vendor_id == 0x80862807) + if (is_haswell(codec)) haswell_config_cvts(codec, pin_idx, mux_idx); snd_hda_spdif_ctls_assign(codec, pin_idx, per_cvt->cvt_nid); @@ -1358,14 +1346,10 @@ static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll) /* Haswell-specific workaround: re-setup when the transcoder is * changed during the stream playback */ - if (codec->vendor_id == 0x80862807 && - eld->eld_valid && !old_eld_valid && per_pin->setup) { - snd_hda_codec_write(codec, pin_nid, 0, - AC_VERB_SET_AMP_GAIN_MUTE, - AMP_OUT_UNMUTE); + if (is_haswell(codec) && + eld->eld_valid && !old_eld_valid && per_pin->setup) hdmi_setup_audio_infoframe(codec, per_pin, per_pin->non_pcm); - } } mutex_unlock(&pin_eld->lock); @@ -1405,7 +1389,7 @@ static int hdmi_add_pin(struct hda_codec *codec, hda_nid_t pin_nid) if (get_defcfg_connect(config) == AC_JACK_PORT_NONE) return 0; - if (codec->vendor_id == 0x80862807) + if (is_haswell(codec)) intel_haswell_fixup_connect_list(codec, pin_nid); pin_idx = spec->num_pins; @@ -2014,7 +1998,7 @@ static int patch_generic_hdmi(struct hda_codec *codec) codec->spec = spec; hdmi_array_init(spec, 4); - if (codec->vendor_id == 0x80862807) { + if (is_haswell(codec)) { intel_haswell_enable_all_pins(codec, true); intel_haswell_fixup_enable_dp12(codec); } @@ -2025,7 +2009,7 @@ static int patch_generic_hdmi(struct hda_codec *codec) return -EINVAL; } codec->patch_ops = generic_hdmi_patch_ops; - if (codec->vendor_id == 0x80862807) { + if (is_haswell(codec)) { codec->patch_ops.set_power_state = haswell_set_power_state; codec->dp_mst = true; } -- 1.8.1.2 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] ALSA: hda - unmute pin amplifier in infoframe setup for Haswell 2013-09-03 20:38 [PATCH] ALSA: hda - unmute pin amplifier in infoframe setup for Haswell mengdong.lin @ 2013-09-04 9:55 ` David Henningsson 2013-09-04 10:04 ` Takashi Iwai 0 siblings, 1 reply; 4+ messages in thread From: David Henningsson @ 2013-09-04 9:55 UTC (permalink / raw) To: mengdong.lin; +Cc: tiwai, alsa-devel On 09/03/2013 10:38 PM, mengdong.lin@intel.com wrote: > From: Mengdong Lin <mengdong.lin@intel.com> > > When Gfx driver reconnects a port and transcoder, the pin amplifier will be > muted. To enable sound, the pin amp need to be unmuted. > > This patch > - moves pin amp unmuting from stream preparing to hdmi_setup_audio_infoframe(). > So if port:transcoder reconnection happens during stream playback, the ELDV > unsol event can stil trigger pin's amp unmuting when re-setting up audio > info frame. > > - remove reading pin amp status before unmuting for speed-up, since pin amp > should always be unmuted. > > - rename haswell_verify_pin_D0() to haswell_verify_pin_cvt_D0(), since the > convertor power state is also fixed here. Or just haswell_verify_D0. Anyway, looks good to me. > - define is_haswell() to replace checking Haswell vendor ID everywhere. I agree with this, but Takashi might want refactoring and behavioural changes in different patches. Side note: One can also wonder what happens in future generations of Intel hardware (Broadwell etc), if we end up with a is_haswell_or_broadwell_or_somethingwell_or_verywell() after a while :-) > > This patch is mostly based on suggestion of David Henningsson. > > Cc: David Henningsson <david.henningsson@canonical.com> > Signed-off-by: Mengdong Lin <mengdong.lin@intel.com> > > diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c > index 9a58893..471c158 100644 > --- a/sound/pci/hda/patch_hdmi.c > +++ b/sound/pci/hda/patch_hdmi.c > @@ -44,6 +44,8 @@ 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"); > > +#define is_haswell(codec) ((codec)->vendor_id == 0x80862807) > + > struct hdmi_spec_per_cvt { > hda_nid_t cvt_nid; > int assigned; > @@ -894,6 +896,11 @@ static void hdmi_setup_audio_infoframe(struct hda_codec *codec, > if (!channels) > return; > > + if (is_haswell(codec)) > + snd_hda_codec_write(codec, pin_nid, 0, > + AC_VERB_SET_AMP_GAIN_MUTE, > + AMP_OUT_UNMUTE); > + > eld = &per_pin->sink_eld; > if (!eld->monitor_present) > return; > @@ -1033,10 +1040,10 @@ 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, > +static void haswell_verify_pin_cvt_D0(struct hda_codec *codec, > hda_nid_t cvt_nid, hda_nid_t nid) > { > - int pwr, lamp, ramp; > + int pwr; > > /* For Haswell, the converter 1/2 may keep in D3 state after bootup, > * thus pins could only choose converter 0 for use. Make sure the > @@ -1052,25 +1059,6 @@ static void haswell_verify_pin_D0(struct hda_codec *codec, > 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); > } > - > - 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); > - if (lamp != ramp) { > - snd_hda_codec_write(codec, nid, 0, AC_VERB_SET_AMP_GAIN_MUTE, > - AC_AMP_SET_RIGHT | AC_AMP_SET_OUTPUT | lamp); > - > - 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); > - } > } > > /* > @@ -1087,8 +1075,8 @@ 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, cvt_nid, pin_nid); > + if (is_haswell(codec)) > + haswell_verify_pin_cvt_D0(codec, cvt_nid, pin_nid); > > if (snd_hda_query_pin_caps(codec, pin_nid) & AC_PINCAP_HBR) { > pinctl = snd_hda_codec_read(codec, pin_nid, 0, > @@ -1227,7 +1215,7 @@ static int hdmi_pcm_open(struct hda_pcm_stream *hinfo, > mux_idx); > > /* configure unused pins to choose other converters */ > - if (codec->vendor_id == 0x80862807) > + if (is_haswell(codec)) > haswell_config_cvts(codec, pin_idx, mux_idx); > > snd_hda_spdif_ctls_assign(codec, pin_idx, per_cvt->cvt_nid); > @@ -1358,14 +1346,10 @@ static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll) > /* Haswell-specific workaround: re-setup when the transcoder is > * changed during the stream playback > */ > - if (codec->vendor_id == 0x80862807 && > - eld->eld_valid && !old_eld_valid && per_pin->setup) { > - snd_hda_codec_write(codec, pin_nid, 0, > - AC_VERB_SET_AMP_GAIN_MUTE, > - AMP_OUT_UNMUTE); > + if (is_haswell(codec) && > + eld->eld_valid && !old_eld_valid && per_pin->setup) > hdmi_setup_audio_infoframe(codec, per_pin, > per_pin->non_pcm); > - } > } > mutex_unlock(&pin_eld->lock); > > @@ -1405,7 +1389,7 @@ static int hdmi_add_pin(struct hda_codec *codec, hda_nid_t pin_nid) > if (get_defcfg_connect(config) == AC_JACK_PORT_NONE) > return 0; > > - if (codec->vendor_id == 0x80862807) > + if (is_haswell(codec)) > intel_haswell_fixup_connect_list(codec, pin_nid); > > pin_idx = spec->num_pins; > @@ -2014,7 +1998,7 @@ static int patch_generic_hdmi(struct hda_codec *codec) > codec->spec = spec; > hdmi_array_init(spec, 4); > > - if (codec->vendor_id == 0x80862807) { > + if (is_haswell(codec)) { > intel_haswell_enable_all_pins(codec, true); > intel_haswell_fixup_enable_dp12(codec); > } > @@ -2025,7 +2009,7 @@ static int patch_generic_hdmi(struct hda_codec *codec) > return -EINVAL; > } > codec->patch_ops = generic_hdmi_patch_ops; > - if (codec->vendor_id == 0x80862807) { > + if (is_haswell(codec)) { > codec->patch_ops.set_power_state = haswell_set_power_state; > codec->dp_mst = true; > } > -- David Henningsson, Canonical Ltd. https://launchpad.net/~diwic ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ALSA: hda - unmute pin amplifier in infoframe setup for Haswell 2013-09-04 9:55 ` David Henningsson @ 2013-09-04 10:04 ` Takashi Iwai 2013-09-05 1:41 ` Lin, Mengdong 0 siblings, 1 reply; 4+ messages in thread From: Takashi Iwai @ 2013-09-04 10:04 UTC (permalink / raw) To: David Henningsson; +Cc: mengdong.lin, alsa-devel At Wed, 04 Sep 2013 11:55:27 +0200, David Henningsson wrote: > > On 09/03/2013 10:38 PM, mengdong.lin@intel.com wrote: > > From: Mengdong Lin <mengdong.lin@intel.com> > > > > When Gfx driver reconnects a port and transcoder, the pin amplifier will be > > muted. To enable sound, the pin amp need to be unmuted. > > > > This patch > > - moves pin amp unmuting from stream preparing to hdmi_setup_audio_infoframe(). > > So if port:transcoder reconnection happens during stream playback, the ELDV > > unsol event can stil trigger pin's amp unmuting when re-setting up audio > > info frame. > > > > - remove reading pin amp status before unmuting for speed-up, since pin amp > > should always be unmuted. > > > > - rename haswell_verify_pin_D0() to haswell_verify_pin_cvt_D0(), since the > > convertor power state is also fixed here. > > Or just haswell_verify_D0. Anyway, looks good to me. > > > - define is_haswell() to replace checking Haswell vendor ID everywhere. > > I agree with this, but Takashi might want refactoring and behavioural > changes in different patches. Have no patches right now. Feel free to go forward. > Side note: One can also wonder what happens in future generations of > Intel hardware (Broadwell etc), if we end up with a > is_haswell_or_broadwell_or_somethingwell_or_verywell() after a while :-) Yeah, but we'll keep maintaining the driver code, so changing this would be trivial ;) Though, it'd be cleaner to split this patch: one for defining and replacing with is_hawell() (or whatever), and another actually changes haswell_veriy_pin_D0(). Takashi > > This patch is mostly based on suggestion of David Henningsson. > > > > Cc: David Henningsson <david.henningsson@canonical.com> > > Signed-off-by: Mengdong Lin <mengdong.lin@intel.com> > > > > diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c > > index 9a58893..471c158 100644 > > --- a/sound/pci/hda/patch_hdmi.c > > +++ b/sound/pci/hda/patch_hdmi.c > > @@ -44,6 +44,8 @@ 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"); > > > > +#define is_haswell(codec) ((codec)->vendor_id == 0x80862807) > > + > > struct hdmi_spec_per_cvt { > > hda_nid_t cvt_nid; > > int assigned; > > @@ -894,6 +896,11 @@ static void hdmi_setup_audio_infoframe(struct hda_codec *codec, > > if (!channels) > > return; > > > > + if (is_haswell(codec)) > > + snd_hda_codec_write(codec, pin_nid, 0, > > + AC_VERB_SET_AMP_GAIN_MUTE, > > + AMP_OUT_UNMUTE); > > + > > eld = &per_pin->sink_eld; > > if (!eld->monitor_present) > > return; > > @@ -1033,10 +1040,10 @@ 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, > > +static void haswell_verify_pin_cvt_D0(struct hda_codec *codec, > > hda_nid_t cvt_nid, hda_nid_t nid) > > { > > - int pwr, lamp, ramp; > > + int pwr; > > > > /* For Haswell, the converter 1/2 may keep in D3 state after bootup, > > * thus pins could only choose converter 0 for use. Make sure the > > @@ -1052,25 +1059,6 @@ static void haswell_verify_pin_D0(struct hda_codec *codec, > > 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); > > } > > - > > - 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); > > - if (lamp != ramp) { > > - snd_hda_codec_write(codec, nid, 0, AC_VERB_SET_AMP_GAIN_MUTE, > > - AC_AMP_SET_RIGHT | AC_AMP_SET_OUTPUT | lamp); > > - > > - 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); > > - } > > } > > > > /* > > @@ -1087,8 +1075,8 @@ 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, cvt_nid, pin_nid); > > + if (is_haswell(codec)) > > + haswell_verify_pin_cvt_D0(codec, cvt_nid, pin_nid); > > > > if (snd_hda_query_pin_caps(codec, pin_nid) & AC_PINCAP_HBR) { > > pinctl = snd_hda_codec_read(codec, pin_nid, 0, > > @@ -1227,7 +1215,7 @@ static int hdmi_pcm_open(struct hda_pcm_stream *hinfo, > > mux_idx); > > > > /* configure unused pins to choose other converters */ > > - if (codec->vendor_id == 0x80862807) > > + if (is_haswell(codec)) > > haswell_config_cvts(codec, pin_idx, mux_idx); > > > > snd_hda_spdif_ctls_assign(codec, pin_idx, per_cvt->cvt_nid); > > @@ -1358,14 +1346,10 @@ static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll) > > /* Haswell-specific workaround: re-setup when the transcoder is > > * changed during the stream playback > > */ > > - if (codec->vendor_id == 0x80862807 && > > - eld->eld_valid && !old_eld_valid && per_pin->setup) { > > - snd_hda_codec_write(codec, pin_nid, 0, > > - AC_VERB_SET_AMP_GAIN_MUTE, > > - AMP_OUT_UNMUTE); > > + if (is_haswell(codec) && > > + eld->eld_valid && !old_eld_valid && per_pin->setup) > > hdmi_setup_audio_infoframe(codec, per_pin, > > per_pin->non_pcm); > > - } > > } > > mutex_unlock(&pin_eld->lock); > > > > @@ -1405,7 +1389,7 @@ static int hdmi_add_pin(struct hda_codec *codec, hda_nid_t pin_nid) > > if (get_defcfg_connect(config) == AC_JACK_PORT_NONE) > > return 0; > > > > - if (codec->vendor_id == 0x80862807) > > + if (is_haswell(codec)) > > intel_haswell_fixup_connect_list(codec, pin_nid); > > > > pin_idx = spec->num_pins; > > @@ -2014,7 +1998,7 @@ static int patch_generic_hdmi(struct hda_codec *codec) > > codec->spec = spec; > > hdmi_array_init(spec, 4); > > > > - if (codec->vendor_id == 0x80862807) { > > + if (is_haswell(codec)) { > > intel_haswell_enable_all_pins(codec, true); > > intel_haswell_fixup_enable_dp12(codec); > > } > > @@ -2025,7 +2009,7 @@ static int patch_generic_hdmi(struct hda_codec *codec) > > return -EINVAL; > > } > > codec->patch_ops = generic_hdmi_patch_ops; > > - if (codec->vendor_id == 0x80862807) { > > + if (is_haswell(codec)) { > > codec->patch_ops.set_power_state = haswell_set_power_state; > > codec->dp_mst = true; > > } > > > > > > -- > David Henningsson, Canonical Ltd. > https://launchpad.net/~diwic > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ALSA: hda - unmute pin amplifier in infoframe setup for Haswell 2013-09-04 10:04 ` Takashi Iwai @ 2013-09-05 1:41 ` Lin, Mengdong 0 siblings, 0 replies; 4+ messages in thread From: Lin, Mengdong @ 2013-09-05 1:41 UTC (permalink / raw) To: Takashi Iwai, David Henningsson; +Cc: alsa-devel@alsa-project.org Hi Takashi and David, Thanks for your comments! I'll split the patch. Broadwell has the same HW behavior as Haswell. Maybe in the future, is_haswell() can be renamed to is_haswell_plus(), 'plus' means including the successor processors. This is the naming convention used in our documents. "is_haswell_or_broadwell_or_somethingwell_or_verywell()" really sounds like "is_everywhere", considering these specific fixings :-) Thanks Mengdong > -----Original Message----- > From: Takashi Iwai [mailto:tiwai@suse.de] > Sent: Wednesday, September 04, 2013 6:04 PM > To: David Henningsson > Cc: Lin, Mengdong; alsa-devel@alsa-project.org > Subject: Re: [PATCH] ALSA: hda - unmute pin amplifier in infoframe setup for > Haswell > > At Wed, 04 Sep 2013 11:55:27 +0200, > David Henningsson wrote: > > > > On 09/03/2013 10:38 PM, mengdong.lin@intel.com wrote: > > > From: Mengdong Lin <mengdong.lin@intel.com> > > > > > > When Gfx driver reconnects a port and transcoder, the pin amplifier > > > will be muted. To enable sound, the pin amp need to be unmuted. > > > > > > This patch > > > - moves pin amp unmuting from stream preparing to > hdmi_setup_audio_infoframe(). > > > So if port:transcoder reconnection happens during stream playback, the > ELDV > > > unsol event can stil trigger pin's amp unmuting when re-setting up audio > > > info frame. > > > > > > - remove reading pin amp status before unmuting for speed-up, since pin > amp > > > should always be unmuted. > > > > > > - rename haswell_verify_pin_D0() to haswell_verify_pin_cvt_D0(), since the > > > convertor power state is also fixed here. > > > > Or just haswell_verify_D0. Anyway, looks good to me. > > > > > - define is_haswell() to replace checking Haswell vendor ID everywhere. > > > > I agree with this, but Takashi might want refactoring and behavioural > > changes in different patches. > > Have no patches right now. Feel free to go forward. > > > Side note: One can also wonder what happens in future generations of > > Intel hardware (Broadwell etc), if we end up with a > > is_haswell_or_broadwell_or_somethingwell_or_verywell() after a while > > :-) > > Yeah, but we'll keep maintaining the driver code, so changing this would be > trivial ;) > > Though, it'd be cleaner to split this patch: one for defining and replacing with > is_hawell() (or whatever), and another actually changes > haswell_veriy_pin_D0(). > > > Takashi > > > > This patch is mostly based on suggestion of David Henningsson. > > > > > > Cc: David Henningsson <david.henningsson@canonical.com> > > > Signed-off-by: Mengdong Lin <mengdong.lin@intel.com> > > > > > > diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c > > > index 9a58893..471c158 100644 > > > --- a/sound/pci/hda/patch_hdmi.c > > > +++ b/sound/pci/hda/patch_hdmi.c > > > @@ -44,6 +44,8 @@ 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"); > > > > > > +#define is_haswell(codec) ((codec)->vendor_id == 0x80862807) > > > + > > > struct hdmi_spec_per_cvt { > > > hda_nid_t cvt_nid; > > > int assigned; > > > @@ -894,6 +896,11 @@ static void hdmi_setup_audio_infoframe(struct > hda_codec *codec, > > > if (!channels) > > > return; > > > > > > + if (is_haswell(codec)) > > > + snd_hda_codec_write(codec, pin_nid, 0, > > > + AC_VERB_SET_AMP_GAIN_MUTE, > > > + AMP_OUT_UNMUTE); > > > + > > > eld = &per_pin->sink_eld; > > > if (!eld->monitor_present) > > > return; > > > @@ -1033,10 +1040,10 @@ 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, > > > +static void haswell_verify_pin_cvt_D0(struct hda_codec *codec, > > > hda_nid_t cvt_nid, hda_nid_t nid) { > > > - int pwr, lamp, ramp; > > > + int pwr; > > > > > > /* For Haswell, the converter 1/2 may keep in D3 state after bootup, > > > * thus pins could only choose converter 0 for use. Make sure the > > > @@ -1052,25 +1059,6 @@ static void haswell_verify_pin_D0(struct > hda_codec *codec, > > > 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); > > > } > > > - > > > - 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); > > > - if (lamp != ramp) { > > > - snd_hda_codec_write(codec, nid, 0, > AC_VERB_SET_AMP_GAIN_MUTE, > > > - AC_AMP_SET_RIGHT | AC_AMP_SET_OUTPUT | > lamp); > > > - > > > - 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); > > > - } > > > } > > > > > > /* > > > @@ -1087,8 +1075,8 @@ 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, cvt_nid, pin_nid); > > > + if (is_haswell(codec)) > > > + haswell_verify_pin_cvt_D0(codec, cvt_nid, pin_nid); > > > > > > if (snd_hda_query_pin_caps(codec, pin_nid) & AC_PINCAP_HBR) { > > > pinctl = snd_hda_codec_read(codec, pin_nid, 0, @@ -1227,7 > +1215,7 > > > @@ static int hdmi_pcm_open(struct hda_pcm_stream *hinfo, > > > mux_idx); > > > > > > /* configure unused pins to choose other converters */ > > > - if (codec->vendor_id == 0x80862807) > > > + if (is_haswell(codec)) > > > haswell_config_cvts(codec, pin_idx, mux_idx); > > > > > > snd_hda_spdif_ctls_assign(codec, pin_idx, per_cvt->cvt_nid); @@ > > > -1358,14 +1346,10 @@ static void hdmi_present_sense(struct > hdmi_spec_per_pin *per_pin, int repoll) > > > /* Haswell-specific workaround: re-setup when the transcoder is > > > * changed during the stream playback > > > */ > > > - if (codec->vendor_id == 0x80862807 && > > > - eld->eld_valid && !old_eld_valid && per_pin->setup) { > > > - snd_hda_codec_write(codec, pin_nid, 0, > > > - AC_VERB_SET_AMP_GAIN_MUTE, > > > - AMP_OUT_UNMUTE); > > > + if (is_haswell(codec) && > > > + eld->eld_valid && !old_eld_valid && per_pin->setup) > > > hdmi_setup_audio_infoframe(codec, per_pin, > > > per_pin->non_pcm); > > > - } > > > } > > > mutex_unlock(&pin_eld->lock); > > > > > > @@ -1405,7 +1389,7 @@ static int hdmi_add_pin(struct hda_codec > *codec, hda_nid_t pin_nid) > > > if (get_defcfg_connect(config) == AC_JACK_PORT_NONE) > > > return 0; > > > > > > - if (codec->vendor_id == 0x80862807) > > > + if (is_haswell(codec)) > > > intel_haswell_fixup_connect_list(codec, pin_nid); > > > > > > pin_idx = spec->num_pins; > > > @@ -2014,7 +1998,7 @@ static int patch_generic_hdmi(struct hda_codec > *codec) > > > codec->spec = spec; > > > hdmi_array_init(spec, 4); > > > > > > - if (codec->vendor_id == 0x80862807) { > > > + if (is_haswell(codec)) { > > > intel_haswell_enable_all_pins(codec, true); > > > intel_haswell_fixup_enable_dp12(codec); > > > } > > > @@ -2025,7 +2009,7 @@ static int patch_generic_hdmi(struct hda_codec > *codec) > > > return -EINVAL; > > > } > > > codec->patch_ops = generic_hdmi_patch_ops; > > > - if (codec->vendor_id == 0x80862807) { > > > + if (is_haswell(codec)) { > > > codec->patch_ops.set_power_state = > haswell_set_power_state; > > > codec->dp_mst = true; > > > } > > > > > > > > > > > -- > > David Henningsson, Canonical Ltd. > > https://launchpad.net/~diwic > > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-09-05 1:41 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-09-03 20:38 [PATCH] ALSA: hda - unmute pin amplifier in infoframe setup for Haswell mengdong.lin 2013-09-04 9:55 ` David Henningsson 2013-09-04 10:04 ` Takashi Iwai 2013-09-05 1:41 ` Lin, Mengdong
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.