* [PATCH] ALSA: hda - Re-setup HDMI pin and audio infoframe on stream switches
@ 2013-09-03 9:57 Takashi Iwai
2013-09-03 11:10 ` David Henningsson
0 siblings, 1 reply; 8+ messages in thread
From: Takashi Iwai @ 2013-09-03 9:57 UTC (permalink / raw)
To: alsa-devel; +Cc: Mengdong Lin
When the transcoder:port mapping on Haswell HDMI/DP audio is changed
during the stream playback, the sound gets lost. Typically this
problem is seen when the user switches the graphics mode from eDP+DP
to DP-only configuration, where CRTC 1 is used for DP in the former
while CRTC 0 is used for the latter.
The graphics controller notifies the change via the normal ELD update
procedure, so we get the intrinsic event. For enabling the sound
again, the HDMI audio driver needs to reset the pin and set up the
audio infoframe again.
This patch achieves it by:
- keep the current status of channels and info frame setup in per_pin
struct,
- check the reconnection in the intrinsic event handler,
- reset the pin and the re-invoke hdmi_setup_audio_infoframe()
accordingly.
The hdmi_setup_audio_infoframe() function has been changed, too, so
that it can be invoked without passing the substream instance.
The patch is mostly based on the work by Mengdong Lin.
Cc: Mengdong Lin <mengdong.lin@intel.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
sound/pci/hda/patch_hdmi.c | 41 +++++++++++++++++++++++++++++++----------
1 file changed, 31 insertions(+), 10 deletions(-)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index b83b14f..22b5089 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -67,6 +67,8 @@ struct hdmi_spec_per_pin {
struct delayed_work work;
struct snd_kcontrol *eld_ctl;
int repoll_count;
+ bool setup; /* the stream has been set up by prepare callback */
+ int channels; /* current number of channels */
bool non_pcm;
bool chmap_set; /* channel-map override by ALSA API? */
unsigned char chmap[8]; /* ALSA API channel-map */
@@ -879,18 +881,19 @@ static bool hdmi_infoframe_uptodate(struct hda_codec *codec, hda_nid_t pin_nid,
return true;
}
-static void hdmi_setup_audio_infoframe(struct hda_codec *codec, int pin_idx,
- bool non_pcm,
- struct snd_pcm_substream *substream)
+static void hdmi_setup_audio_infoframe(struct hda_codec *codec,
+ struct hdmi_spec_per_pin *per_pin,
+ bool non_pcm)
{
- struct hdmi_spec *spec = codec->spec;
- struct hdmi_spec_per_pin *per_pin = get_pin(spec, pin_idx);
hda_nid_t pin_nid = per_pin->pin_nid;
- int channels = substream->runtime->channels;
+ int channels = per_pin->channels;
struct hdmi_eld *eld;
int ca;
union audio_infoframe ai;
+ if (!channels)
+ return;
+
eld = &per_pin->sink_eld;
if (!eld->monitor_present)
return;
@@ -1341,6 +1344,7 @@ static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
eld_changed = true;
}
if (update_eld) {
+ bool old_eld_valid = pin_eld->eld_valid;
pin_eld->eld_valid = eld->eld_valid;
eld_changed = pin_eld->eld_size != eld->eld_size ||
memcmp(pin_eld->eld_buffer, eld->eld_buffer,
@@ -1350,6 +1354,18 @@ static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
eld->eld_size);
pin_eld->eld_size = eld->eld_size;
pin_eld->info = eld->info;
+
+ /* 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);
+ hdmi_setup_audio_infoframe(codec, per_pin,
+ per_pin->non_pcm);
+ }
}
mutex_unlock(&pin_eld->lock);
@@ -1522,14 +1538,17 @@ static int generic_hdmi_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
hda_nid_t cvt_nid = hinfo->nid;
struct hdmi_spec *spec = codec->spec;
int pin_idx = hinfo_to_pin_index(spec, hinfo);
- hda_nid_t pin_nid = get_pin(spec, pin_idx)->pin_nid;
+ struct hdmi_spec_per_pin *per_pin = get_pin(spec, pin_idx);
+ hda_nid_t pin_nid = per_pin->pin_nid;
bool non_pcm;
non_pcm = check_non_pcm_per_cvt(codec, cvt_nid);
+ per_pin->channels = substream->runtime->channels;
+ per_pin->setup = true;
hdmi_set_channel_count(codec, cvt_nid, substream->runtime->channels);
- hdmi_setup_audio_infoframe(codec, pin_idx, non_pcm, substream);
+ hdmi_setup_audio_infoframe(codec, per_pin, non_pcm);
return hdmi_setup_stream(codec, cvt_nid, pin_nid, stream_tag, format);
}
@@ -1569,6 +1588,9 @@ static int hdmi_pcm_close(struct hda_pcm_stream *hinfo,
snd_hda_spdif_ctls_unassign(codec, pin_idx);
per_pin->chmap_set = false;
memset(per_pin->chmap, 0, sizeof(per_pin->chmap));
+
+ per_pin->setup = false;
+ per_pin->channels = 0;
}
return 0;
@@ -1704,8 +1726,7 @@ static int hdmi_chmap_ctl_put(struct snd_kcontrol *kcontrol,
per_pin->chmap_set = true;
memcpy(per_pin->chmap, chmap, sizeof(chmap));
if (prepared)
- hdmi_setup_audio_infoframe(codec, pin_idx, per_pin->non_pcm,
- substream);
+ hdmi_setup_audio_infoframe(codec, per_pin, per_pin->non_pcm);
return 0;
}
--
1.8.3.4
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] ALSA: hda - Re-setup HDMI pin and audio infoframe on stream switches
2013-09-03 9:57 [PATCH] ALSA: hda - Re-setup HDMI pin and audio infoframe on stream switches Takashi Iwai
@ 2013-09-03 11:10 ` David Henningsson
2013-09-03 12:06 ` Takashi Iwai
0 siblings, 1 reply; 8+ messages in thread
From: David Henningsson @ 2013-09-03 11:10 UTC (permalink / raw)
To: Takashi Iwai; +Cc: Mengdong Lin, alsa-devel
On 09/03/2013 11:57 AM, Takashi Iwai wrote:
> When the transcoder:port mapping on Haswell HDMI/DP audio is changed
> during the stream playback, the sound gets lost. Typically this
> problem is seen when the user switches the graphics mode from eDP+DP
> to DP-only configuration, where CRTC 1 is used for DP in the former
> while CRTC 0 is used for the latter.
>
> The graphics controller notifies the change via the normal ELD update
> procedure, so we get the intrinsic event. For enabling the sound
> again, the HDMI audio driver needs to reset the pin and set up the
> audio infoframe again.
Thanks for working on this!
See a review comment below.
>
> This patch achieves it by:
> - keep the current status of channels and info frame setup in per_pin
> struct,
> - check the reconnection in the intrinsic event handler,
> - reset the pin and the re-invoke hdmi_setup_audio_infoframe()
> accordingly.
>
> The hdmi_setup_audio_infoframe() function has been changed, too, so
> that it can be invoked without passing the substream instance.
>
> The patch is mostly based on the work by Mengdong Lin.
>
> Cc: Mengdong Lin <mengdong.lin@intel.com>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
> sound/pci/hda/patch_hdmi.c | 41 +++++++++++++++++++++++++++++++----------
> 1 file changed, 31 insertions(+), 10 deletions(-)
>
> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> index b83b14f..22b5089 100644
> --- a/sound/pci/hda/patch_hdmi.c
> +++ b/sound/pci/hda/patch_hdmi.c
> @@ -67,6 +67,8 @@ struct hdmi_spec_per_pin {
> struct delayed_work work;
> struct snd_kcontrol *eld_ctl;
> int repoll_count;
> + bool setup; /* the stream has been set up by prepare callback */
> + int channels; /* current number of channels */
> bool non_pcm;
> bool chmap_set; /* channel-map override by ALSA API? */
> unsigned char chmap[8]; /* ALSA API channel-map */
> @@ -879,18 +881,19 @@ static bool hdmi_infoframe_uptodate(struct hda_codec *codec, hda_nid_t pin_nid,
> return true;
> }
>
> -static void hdmi_setup_audio_infoframe(struct hda_codec *codec, int pin_idx,
> - bool non_pcm,
> - struct snd_pcm_substream *substream)
> +static void hdmi_setup_audio_infoframe(struct hda_codec *codec,
> + struct hdmi_spec_per_pin *per_pin,
> + bool non_pcm)
> {
> - struct hdmi_spec *spec = codec->spec;
> - struct hdmi_spec_per_pin *per_pin = get_pin(spec, pin_idx);
> hda_nid_t pin_nid = per_pin->pin_nid;
> - int channels = substream->runtime->channels;
> + int channels = per_pin->channels;
> struct hdmi_eld *eld;
> int ca;
> union audio_infoframe ai;
>
> + if (!channels)
> + return;
> +
> eld = &per_pin->sink_eld;
> if (!eld->monitor_present)
> return;
> @@ -1341,6 +1344,7 @@ static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
> eld_changed = true;
> }
> if (update_eld) {
> + bool old_eld_valid = pin_eld->eld_valid;
> pin_eld->eld_valid = eld->eld_valid;
> eld_changed = pin_eld->eld_size != eld->eld_size ||
> memcmp(pin_eld->eld_buffer, eld->eld_buffer,
> @@ -1350,6 +1354,18 @@ static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
> eld->eld_size);
> pin_eld->eld_size = eld->eld_size;
> pin_eld->info = eld->info;
> +
> + /* 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 the system is deliberately muted by turning off "IEC958 Playback
Switch", are we now ignoring that and turning it back on?
Also, this looks a bit like the workaround I added a while back
(83f26ad2c909083fa6 - ALSA: hda - fixup D3 pin and right channel mute on
Haswell HDMI audio) is there a possibility we need to fix up D3 as well
here? I e, call haswell_verify_pin_D0 rather than just setting the mute?
Or perhaps move the call to haswell_verify_pin_D0 from hdmi_setup_stream
to hdme_setup_audio_infoframe ?
> + hdmi_setup_audio_infoframe(codec, per_pin,
> + per_pin->non_pcm);
> + }
> }
> mutex_unlock(&pin_eld->lock);
>
> @@ -1522,14 +1538,17 @@ static int generic_hdmi_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
> hda_nid_t cvt_nid = hinfo->nid;
> struct hdmi_spec *spec = codec->spec;
> int pin_idx = hinfo_to_pin_index(spec, hinfo);
> - hda_nid_t pin_nid = get_pin(spec, pin_idx)->pin_nid;
> + struct hdmi_spec_per_pin *per_pin = get_pin(spec, pin_idx);
> + hda_nid_t pin_nid = per_pin->pin_nid;
> bool non_pcm;
>
> non_pcm = check_non_pcm_per_cvt(codec, cvt_nid);
> + per_pin->channels = substream->runtime->channels;
> + per_pin->setup = true;
>
> hdmi_set_channel_count(codec, cvt_nid, substream->runtime->channels);
>
> - hdmi_setup_audio_infoframe(codec, pin_idx, non_pcm, substream);
> + hdmi_setup_audio_infoframe(codec, per_pin, non_pcm);
>
> return hdmi_setup_stream(codec, cvt_nid, pin_nid, stream_tag, format);
> }
> @@ -1569,6 +1588,9 @@ static int hdmi_pcm_close(struct hda_pcm_stream *hinfo,
> snd_hda_spdif_ctls_unassign(codec, pin_idx);
> per_pin->chmap_set = false;
> memset(per_pin->chmap, 0, sizeof(per_pin->chmap));
> +
> + per_pin->setup = false;
> + per_pin->channels = 0;
> }
>
> return 0;
> @@ -1704,8 +1726,7 @@ static int hdmi_chmap_ctl_put(struct snd_kcontrol *kcontrol,
> per_pin->chmap_set = true;
> memcpy(per_pin->chmap, chmap, sizeof(chmap));
> if (prepared)
> - hdmi_setup_audio_infoframe(codec, pin_idx, per_pin->non_pcm,
> - substream);
> + hdmi_setup_audio_infoframe(codec, per_pin, per_pin->non_pcm);
>
> return 0;
> }
>
--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ALSA: hda - Re-setup HDMI pin and audio infoframe on stream switches
2013-09-03 11:10 ` David Henningsson
@ 2013-09-03 12:06 ` Takashi Iwai
2013-09-03 12:36 ` David Henningsson
0 siblings, 1 reply; 8+ messages in thread
From: Takashi Iwai @ 2013-09-03 12:06 UTC (permalink / raw)
To: David Henningsson; +Cc: Mengdong Lin, alsa-devel
At Tue, 03 Sep 2013 13:10:43 +0200,
David Henningsson wrote:
>
> On 09/03/2013 11:57 AM, Takashi Iwai wrote:
> > When the transcoder:port mapping on Haswell HDMI/DP audio is changed
> > during the stream playback, the sound gets lost. Typically this
> > problem is seen when the user switches the graphics mode from eDP+DP
> > to DP-only configuration, where CRTC 1 is used for DP in the former
> > while CRTC 0 is used for the latter.
> >
> > The graphics controller notifies the change via the normal ELD update
> > procedure, so we get the intrinsic event. For enabling the sound
> > again, the HDMI audio driver needs to reset the pin and set up the
> > audio infoframe again.
>
> Thanks for working on this!
>
> See a review comment below.
>
> >
> > This patch achieves it by:
> > - keep the current status of channels and info frame setup in per_pin
> > struct,
> > - check the reconnection in the intrinsic event handler,
> > - reset the pin and the re-invoke hdmi_setup_audio_infoframe()
> > accordingly.
> >
> > The hdmi_setup_audio_infoframe() function has been changed, too, so
> > that it can be invoked without passing the substream instance.
> >
> > The patch is mostly based on the work by Mengdong Lin.
> >
> > Cc: Mengdong Lin <mengdong.lin@intel.com>
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> > sound/pci/hda/patch_hdmi.c | 41 +++++++++++++++++++++++++++++++----------
> > 1 file changed, 31 insertions(+), 10 deletions(-)
> >
> > diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> > index b83b14f..22b5089 100644
> > --- a/sound/pci/hda/patch_hdmi.c
> > +++ b/sound/pci/hda/patch_hdmi.c
> > @@ -67,6 +67,8 @@ struct hdmi_spec_per_pin {
> > struct delayed_work work;
> > struct snd_kcontrol *eld_ctl;
> > int repoll_count;
> > + bool setup; /* the stream has been set up by prepare callback */
> > + int channels; /* current number of channels */
> > bool non_pcm;
> > bool chmap_set; /* channel-map override by ALSA API? */
> > unsigned char chmap[8]; /* ALSA API channel-map */
> > @@ -879,18 +881,19 @@ static bool hdmi_infoframe_uptodate(struct hda_codec *codec, hda_nid_t pin_nid,
> > return true;
> > }
> >
> > -static void hdmi_setup_audio_infoframe(struct hda_codec *codec, int pin_idx,
> > - bool non_pcm,
> > - struct snd_pcm_substream *substream)
> > +static void hdmi_setup_audio_infoframe(struct hda_codec *codec,
> > + struct hdmi_spec_per_pin *per_pin,
> > + bool non_pcm)
> > {
> > - struct hdmi_spec *spec = codec->spec;
> > - struct hdmi_spec_per_pin *per_pin = get_pin(spec, pin_idx);
> > hda_nid_t pin_nid = per_pin->pin_nid;
> > - int channels = substream->runtime->channels;
> > + int channels = per_pin->channels;
> > struct hdmi_eld *eld;
> > int ca;
> > union audio_infoframe ai;
> >
> > + if (!channels)
> > + return;
> > +
> > eld = &per_pin->sink_eld;
> > if (!eld->monitor_present)
> > return;
> > @@ -1341,6 +1344,7 @@ static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
> > eld_changed = true;
> > }
> > if (update_eld) {
> > + bool old_eld_valid = pin_eld->eld_valid;
> > pin_eld->eld_valid = eld->eld_valid;
> > eld_changed = pin_eld->eld_size != eld->eld_size ||
> > memcmp(pin_eld->eld_buffer, eld->eld_buffer,
> > @@ -1350,6 +1354,18 @@ static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
> > eld->eld_size);
> > pin_eld->eld_size = eld->eld_size;
> > pin_eld->info = eld->info;
> > +
> > + /* 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 the system is deliberately muted by turning off "IEC958 Playback
> Switch", are we now ignoring that and turning it back on?
The pin amp is always unmuted. The IEC958 Playback Switch is
controlled via IEC958 status bits instead.
> Also, this looks a bit like the workaround I added a while back
> (83f26ad2c909083fa6 - ALSA: hda - fixup D3 pin and right channel mute on
> Haswell HDMI audio) is there a possibility we need to fix up D3 as well
> here? I e, call haswell_verify_pin_D0 rather than just setting the mute?
> Or perhaps move the call to haswell_verify_pin_D0 from hdmi_setup_stream
> to hdme_setup_audio_infoframe ?
The power state is fine in this case, but just to reset the amp state
by some reason. It's not enough to update the info frame, as far as I
tested.
thanks,
Takashi
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ALSA: hda - Re-setup HDMI pin and audio infoframe on stream switches
2013-09-03 12:06 ` Takashi Iwai
@ 2013-09-03 12:36 ` David Henningsson
2013-09-03 12:47 ` Takashi Iwai
2013-09-04 1:29 ` Lin, Mengdong
0 siblings, 2 replies; 8+ messages in thread
From: David Henningsson @ 2013-09-03 12:36 UTC (permalink / raw)
To: Takashi Iwai; +Cc: Mengdong Lin, alsa-devel
On 09/03/2013 02:06 PM, Takashi Iwai wrote:
> At Tue, 03 Sep 2013 13:10:43 +0200,
> David Henningsson wrote:
>>
>> On 09/03/2013 11:57 AM, Takashi Iwai wrote:
>>> When the transcoder:port mapping on Haswell HDMI/DP audio is changed
>>> during the stream playback, the sound gets lost. Typically this
>>> problem is seen when the user switches the graphics mode from eDP+DP
>>> to DP-only configuration, where CRTC 1 is used for DP in the former
>>> while CRTC 0 is used for the latter.
>>>
>>> The graphics controller notifies the change via the normal ELD update
>>> procedure, so we get the intrinsic event. For enabling the sound
>>> again, the HDMI audio driver needs to reset the pin and set up the
>>> audio infoframe again.
>>
>> Thanks for working on this!
>>
>> See a review comment below.
>>
>>>
>>> This patch achieves it by:
>>> - keep the current status of channels and info frame setup in per_pin
>>> struct,
>>> - check the reconnection in the intrinsic event handler,
>>> - reset the pin and the re-invoke hdmi_setup_audio_infoframe()
>>> accordingly.
>>>
>>> The hdmi_setup_audio_infoframe() function has been changed, too, so
>>> that it can be invoked without passing the substream instance.
>>>
>>> The patch is mostly based on the work by Mengdong Lin.
>>>
>>> Cc: Mengdong Lin <mengdong.lin@intel.com>
>>> Cc: <stable@vger.kernel.org>
>>> Signed-off-by: Takashi Iwai <tiwai@suse.de>
>>> ---
>>> sound/pci/hda/patch_hdmi.c | 41 +++++++++++++++++++++++++++++++----------
>>> 1 file changed, 31 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
>>> index b83b14f..22b5089 100644
>>> --- a/sound/pci/hda/patch_hdmi.c
>>> +++ b/sound/pci/hda/patch_hdmi.c
>>> @@ -67,6 +67,8 @@ struct hdmi_spec_per_pin {
>>> struct delayed_work work;
>>> struct snd_kcontrol *eld_ctl;
>>> int repoll_count;
>>> + bool setup; /* the stream has been set up by prepare callback */
>>> + int channels; /* current number of channels */
>>> bool non_pcm;
>>> bool chmap_set; /* channel-map override by ALSA API? */
>>> unsigned char chmap[8]; /* ALSA API channel-map */
>>> @@ -879,18 +881,19 @@ static bool hdmi_infoframe_uptodate(struct hda_codec *codec, hda_nid_t pin_nid,
>>> return true;
>>> }
>>>
>>> -static void hdmi_setup_audio_infoframe(struct hda_codec *codec, int pin_idx,
>>> - bool non_pcm,
>>> - struct snd_pcm_substream *substream)
>>> +static void hdmi_setup_audio_infoframe(struct hda_codec *codec,
>>> + struct hdmi_spec_per_pin *per_pin,
>>> + bool non_pcm)
>>> {
>>> - struct hdmi_spec *spec = codec->spec;
>>> - struct hdmi_spec_per_pin *per_pin = get_pin(spec, pin_idx);
>>> hda_nid_t pin_nid = per_pin->pin_nid;
>>> - int channels = substream->runtime->channels;
>>> + int channels = per_pin->channels;
>>> struct hdmi_eld *eld;
>>> int ca;
>>> union audio_infoframe ai;
>>>
>>> + if (!channels)
>>> + return;
>>> +
>>> eld = &per_pin->sink_eld;
>>> if (!eld->monitor_present)
>>> return;
>>> @@ -1341,6 +1344,7 @@ static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
>>> eld_changed = true;
>>> }
>>> if (update_eld) {
>>> + bool old_eld_valid = pin_eld->eld_valid;
>>> pin_eld->eld_valid = eld->eld_valid;
>>> eld_changed = pin_eld->eld_size != eld->eld_size ||
>>> memcmp(pin_eld->eld_buffer, eld->eld_buffer,
>>> @@ -1350,6 +1354,18 @@ static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
>>> eld->eld_size);
>>> pin_eld->eld_size = eld->eld_size;
>>> pin_eld->info = eld->info;
>>> +
>>> + /* 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 the system is deliberately muted by turning off "IEC958 Playback
>> Switch", are we now ignoring that and turning it back on?
>
> The pin amp is always unmuted. The IEC958 Playback Switch is
> controlled via IEC958 status bits instead.
Ok.
>
>> Also, this looks a bit like the workaround I added a while back
>> (83f26ad2c909083fa6 - ALSA: hda - fixup D3 pin and right channel mute on
>> Haswell HDMI audio) is there a possibility we need to fix up D3 as well
>> here? I e, call haswell_verify_pin_D0 rather than just setting the mute?
>> Or perhaps move the call to haswell_verify_pin_D0 from hdmi_setup_stream
>> to hdme_setup_audio_infoframe ?
>
> The power state is fine in this case, but just to reset the amp state
> by some reason. It's not enough to update the info frame, as far as I
> tested.
Ok - maybe Mengdong can comment more on this, if it's the same scenario
as I was working around earlier (right channel muted and pin in D3), or
if this is something different.
If it's the same issue, I was thinking that we could
1) skip the extra snd_hda_codec_write here, and
2) move the call to haswell_verify_pin_D0 from hdmi_setup_stream to
hdmi_setup_audio_infoframe?
Also, if the pin amp is always unmuted, we could speed up
haswell_verify_pin_D0 to always unmute instead of read-then-write.
--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ALSA: hda - Re-setup HDMI pin and audio infoframe on stream switches
2013-09-03 12:36 ` David Henningsson
@ 2013-09-03 12:47 ` Takashi Iwai
2013-09-03 13:09 ` David Henningsson
2013-09-04 1:29 ` Lin, Mengdong
1 sibling, 1 reply; 8+ messages in thread
From: Takashi Iwai @ 2013-09-03 12:47 UTC (permalink / raw)
To: David Henningsson; +Cc: Mengdong Lin, alsa-devel
At Tue, 03 Sep 2013 14:36:13 +0200,
David Henningsson wrote:
>
> On 09/03/2013 02:06 PM, Takashi Iwai wrote:
> > At Tue, 03 Sep 2013 13:10:43 +0200,
> > David Henningsson wrote:
> >>
> >> On 09/03/2013 11:57 AM, Takashi Iwai wrote:
> >>> When the transcoder:port mapping on Haswell HDMI/DP audio is changed
> >>> during the stream playback, the sound gets lost. Typically this
> >>> problem is seen when the user switches the graphics mode from eDP+DP
> >>> to DP-only configuration, where CRTC 1 is used for DP in the former
> >>> while CRTC 0 is used for the latter.
> >>>
> >>> The graphics controller notifies the change via the normal ELD update
> >>> procedure, so we get the intrinsic event. For enabling the sound
> >>> again, the HDMI audio driver needs to reset the pin and set up the
> >>> audio infoframe again.
> >>
> >> Thanks for working on this!
> >>
> >> See a review comment below.
> >>
> >>>
> >>> This patch achieves it by:
> >>> - keep the current status of channels and info frame setup in per_pin
> >>> struct,
> >>> - check the reconnection in the intrinsic event handler,
> >>> - reset the pin and the re-invoke hdmi_setup_audio_infoframe()
> >>> accordingly.
> >>>
> >>> The hdmi_setup_audio_infoframe() function has been changed, too, so
> >>> that it can be invoked without passing the substream instance.
> >>>
> >>> The patch is mostly based on the work by Mengdong Lin.
> >>>
> >>> Cc: Mengdong Lin <mengdong.lin@intel.com>
> >>> Cc: <stable@vger.kernel.org>
> >>> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> >>> ---
> >>> sound/pci/hda/patch_hdmi.c | 41 +++++++++++++++++++++++++++++++----------
> >>> 1 file changed, 31 insertions(+), 10 deletions(-)
> >>>
> >>> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> >>> index b83b14f..22b5089 100644
> >>> --- a/sound/pci/hda/patch_hdmi.c
> >>> +++ b/sound/pci/hda/patch_hdmi.c
> >>> @@ -67,6 +67,8 @@ struct hdmi_spec_per_pin {
> >>> struct delayed_work work;
> >>> struct snd_kcontrol *eld_ctl;
> >>> int repoll_count;
> >>> + bool setup; /* the stream has been set up by prepare callback */
> >>> + int channels; /* current number of channels */
> >>> bool non_pcm;
> >>> bool chmap_set; /* channel-map override by ALSA API? */
> >>> unsigned char chmap[8]; /* ALSA API channel-map */
> >>> @@ -879,18 +881,19 @@ static bool hdmi_infoframe_uptodate(struct hda_codec *codec, hda_nid_t pin_nid,
> >>> return true;
> >>> }
> >>>
> >>> -static void hdmi_setup_audio_infoframe(struct hda_codec *codec, int pin_idx,
> >>> - bool non_pcm,
> >>> - struct snd_pcm_substream *substream)
> >>> +static void hdmi_setup_audio_infoframe(struct hda_codec *codec,
> >>> + struct hdmi_spec_per_pin *per_pin,
> >>> + bool non_pcm)
> >>> {
> >>> - struct hdmi_spec *spec = codec->spec;
> >>> - struct hdmi_spec_per_pin *per_pin = get_pin(spec, pin_idx);
> >>> hda_nid_t pin_nid = per_pin->pin_nid;
> >>> - int channels = substream->runtime->channels;
> >>> + int channels = per_pin->channels;
> >>> struct hdmi_eld *eld;
> >>> int ca;
> >>> union audio_infoframe ai;
> >>>
> >>> + if (!channels)
> >>> + return;
> >>> +
> >>> eld = &per_pin->sink_eld;
> >>> if (!eld->monitor_present)
> >>> return;
> >>> @@ -1341,6 +1344,7 @@ static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
> >>> eld_changed = true;
> >>> }
> >>> if (update_eld) {
> >>> + bool old_eld_valid = pin_eld->eld_valid;
> >>> pin_eld->eld_valid = eld->eld_valid;
> >>> eld_changed = pin_eld->eld_size != eld->eld_size ||
> >>> memcmp(pin_eld->eld_buffer, eld->eld_buffer,
> >>> @@ -1350,6 +1354,18 @@ static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
> >>> eld->eld_size);
> >>> pin_eld->eld_size = eld->eld_size;
> >>> pin_eld->info = eld->info;
> >>> +
> >>> + /* 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 the system is deliberately muted by turning off "IEC958 Playback
> >> Switch", are we now ignoring that and turning it back on?
> >
> > The pin amp is always unmuted. The IEC958 Playback Switch is
> > controlled via IEC958 status bits instead.
>
> Ok.
>
> >
> >> Also, this looks a bit like the workaround I added a while back
> >> (83f26ad2c909083fa6 - ALSA: hda - fixup D3 pin and right channel mute on
> >> Haswell HDMI audio) is there a possibility we need to fix up D3 as well
> >> here? I e, call haswell_verify_pin_D0 rather than just setting the mute?
> >> Or perhaps move the call to haswell_verify_pin_D0 from hdmi_setup_stream
> >> to hdme_setup_audio_infoframe ?
> >
> > The power state is fine in this case, but just to reset the amp state
> > by some reason. It's not enough to update the info frame, as far as I
> > tested.
>
> Ok - maybe Mengdong can comment more on this, if it's the same scenario
> as I was working around earlier (right channel muted and pin in D3), or
> if this is something different.
>
> If it's the same issue, I was thinking that we could
> 1) skip the extra snd_hda_codec_write here, and
> 2) move the call to haswell_verify_pin_D0 from hdmi_setup_stream to
> hdmi_setup_audio_infoframe?
That makes sense. Though, I'd like to merge the working patch now and
improve later on, as it's already in 3.12 merge window now.
> Also, if the pin amp is always unmuted, we could speed up
> haswell_verify_pin_D0 to always unmute instead of read-then-write.
Yes.
Takashi
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ALSA: hda - Re-setup HDMI pin and audio infoframe on stream switches
2013-09-03 12:47 ` Takashi Iwai
@ 2013-09-03 13:09 ` David Henningsson
0 siblings, 0 replies; 8+ messages in thread
From: David Henningsson @ 2013-09-03 13:09 UTC (permalink / raw)
To: Takashi Iwai; +Cc: Mengdong Lin, alsa-devel
On 09/03/2013 02:47 PM, Takashi Iwai wrote:
> At Tue, 03 Sep 2013 14:36:13 +0200,
> David Henningsson wrote:
>>
>> On 09/03/2013 02:06 PM, Takashi Iwai wrote:
>>> At Tue, 03 Sep 2013 13:10:43 +0200,
>>> David Henningsson wrote:
>>>>
>>>> On 09/03/2013 11:57 AM, Takashi Iwai wrote:
>>>>> When the transcoder:port mapping on Haswell HDMI/DP audio is changed
>>>>> during the stream playback, the sound gets lost. Typically this
>>>>> problem is seen when the user switches the graphics mode from eDP+DP
>>>>> to DP-only configuration, where CRTC 1 is used for DP in the former
>>>>> while CRTC 0 is used for the latter.
>>>>>
>>>>> The graphics controller notifies the change via the normal ELD update
>>>>> procedure, so we get the intrinsic event. For enabling the sound
>>>>> again, the HDMI audio driver needs to reset the pin and set up the
>>>>> audio infoframe again.
>>>>
>>>> Thanks for working on this!
>>>>
>>>> See a review comment below.
>>>>
>>>>>
>>>>> This patch achieves it by:
>>>>> - keep the current status of channels and info frame setup in per_pin
>>>>> struct,
>>>>> - check the reconnection in the intrinsic event handler,
>>>>> - reset the pin and the re-invoke hdmi_setup_audio_infoframe()
>>>>> accordingly.
>>>>>
>>>>> The hdmi_setup_audio_infoframe() function has been changed, too, so
>>>>> that it can be invoked without passing the substream instance.
>>>>>
>>>>> The patch is mostly based on the work by Mengdong Lin.
>>>>>
>>>>> Cc: Mengdong Lin <mengdong.lin@intel.com>
>>>>> Cc: <stable@vger.kernel.org>
>>>>> Signed-off-by: Takashi Iwai <tiwai@suse.de>
>>>>> ---
>>>>> sound/pci/hda/patch_hdmi.c | 41 +++++++++++++++++++++++++++++++----------
>>>>> 1 file changed, 31 insertions(+), 10 deletions(-)
>>>>>
>>>>> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
>>>>> index b83b14f..22b5089 100644
>>>>> --- a/sound/pci/hda/patch_hdmi.c
>>>>> +++ b/sound/pci/hda/patch_hdmi.c
>>>>> @@ -67,6 +67,8 @@ struct hdmi_spec_per_pin {
>>>>> struct delayed_work work;
>>>>> struct snd_kcontrol *eld_ctl;
>>>>> int repoll_count;
>>>>> + bool setup; /* the stream has been set up by prepare callback */
>>>>> + int channels; /* current number of channels */
>>>>> bool non_pcm;
>>>>> bool chmap_set; /* channel-map override by ALSA API? */
>>>>> unsigned char chmap[8]; /* ALSA API channel-map */
>>>>> @@ -879,18 +881,19 @@ static bool hdmi_infoframe_uptodate(struct hda_codec *codec, hda_nid_t pin_nid,
>>>>> return true;
>>>>> }
>>>>>
>>>>> -static void hdmi_setup_audio_infoframe(struct hda_codec *codec, int pin_idx,
>>>>> - bool non_pcm,
>>>>> - struct snd_pcm_substream *substream)
>>>>> +static void hdmi_setup_audio_infoframe(struct hda_codec *codec,
>>>>> + struct hdmi_spec_per_pin *per_pin,
>>>>> + bool non_pcm)
>>>>> {
>>>>> - struct hdmi_spec *spec = codec->spec;
>>>>> - struct hdmi_spec_per_pin *per_pin = get_pin(spec, pin_idx);
>>>>> hda_nid_t pin_nid = per_pin->pin_nid;
>>>>> - int channels = substream->runtime->channels;
>>>>> + int channels = per_pin->channels;
>>>>> struct hdmi_eld *eld;
>>>>> int ca;
>>>>> union audio_infoframe ai;
>>>>>
>>>>> + if (!channels)
>>>>> + return;
>>>>> +
>>>>> eld = &per_pin->sink_eld;
>>>>> if (!eld->monitor_present)
>>>>> return;
>>>>> @@ -1341,6 +1344,7 @@ static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
>>>>> eld_changed = true;
>>>>> }
>>>>> if (update_eld) {
>>>>> + bool old_eld_valid = pin_eld->eld_valid;
>>>>> pin_eld->eld_valid = eld->eld_valid;
>>>>> eld_changed = pin_eld->eld_size != eld->eld_size ||
>>>>> memcmp(pin_eld->eld_buffer, eld->eld_buffer,
>>>>> @@ -1350,6 +1354,18 @@ static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll)
>>>>> eld->eld_size);
>>>>> pin_eld->eld_size = eld->eld_size;
>>>>> pin_eld->info = eld->info;
>>>>> +
>>>>> + /* 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 the system is deliberately muted by turning off "IEC958 Playback
>>>> Switch", are we now ignoring that and turning it back on?
>>>
>>> The pin amp is always unmuted. The IEC958 Playback Switch is
>>> controlled via IEC958 status bits instead.
>>
>> Ok.
>>
>>>
>>>> Also, this looks a bit like the workaround I added a while back
>>>> (83f26ad2c909083fa6 - ALSA: hda - fixup D3 pin and right channel mute on
>>>> Haswell HDMI audio) is there a possibility we need to fix up D3 as well
>>>> here? I e, call haswell_verify_pin_D0 rather than just setting the mute?
>>>> Or perhaps move the call to haswell_verify_pin_D0 from hdmi_setup_stream
>>>> to hdme_setup_audio_infoframe ?
>>>
>>> The power state is fine in this case, but just to reset the amp state
>>> by some reason. It's not enough to update the info frame, as far as I
>>> tested.
>>
>> Ok - maybe Mengdong can comment more on this, if it's the same scenario
>> as I was working around earlier (right channel muted and pin in D3), or
>> if this is something different.
>>
>> If it's the same issue, I was thinking that we could
>> 1) skip the extra snd_hda_codec_write here, and
>> 2) move the call to haswell_verify_pin_D0 from hdmi_setup_stream to
>> hdmi_setup_audio_infoframe?
>
> That makes sense. Though, I'd like to merge the working patch now and
> improve later on, as it's already in 3.12 merge window now.
Sure, no problem.
>
>> Also, if the pin amp is always unmuted, we could speed up
>> haswell_verify_pin_D0 to always unmute instead of read-then-write.
>
> Yes.
>
>
> Takashi
>
--
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ALSA: hda - Re-setup HDMI pin and audio infoframe on stream switches
2013-09-03 12:36 ` David Henningsson
2013-09-03 12:47 ` Takashi Iwai
@ 2013-09-04 1:29 ` Lin, Mengdong
2013-09-04 4:53 ` Lin, Mengdong
1 sibling, 1 reply; 8+ messages in thread
From: Lin, Mengdong @ 2013-09-04 1:29 UTC (permalink / raw)
To: David Henningsson, Takashi Iwai; +Cc: alsa-devel@alsa-project.org
Hi David and Takashi,
> -----Original Message-----
> From: alsa-devel-bounces@alsa-project.org
> [mailto:alsa-devel-bounces@alsa-project.org] On Behalf Of David Henningsson
> Sent: Tuesday, September 03, 2013 8:36 PM
> >> Also, this looks a bit like the workaround I added a while back
> >> (83f26ad2c909083fa6 - ALSA: hda - fixup D3 pin and right channel mute
> >> on Haswell HDMI audio) is there a possibility we need to fix up D3 as
> >> well here? I e, call haswell_verify_pin_D0 rather than just setting the mute?
> >> Or perhaps move the call to haswell_verify_pin_D0 from
> >> hdmi_setup_stream to hdme_setup_audio_infoframe ?
> >
> > The power state is fine in this case, but just to reset the amp state
> > by some reason. It's not enough to update the info frame, as far as I
> > tested.
>
> Ok - maybe Mengdong can comment more on this, if it's the same scenario as I
> was working around earlier (right channel muted and pin in D3), or if this is
> something different.
They're same in nature, although the pin power state can be affected or not.
S3, hot-plug and Gfx mode switch can all make Gfx reconnect the port, maybe to the same or a different transcoder.
During this process , audio will be disabled and re-enabled again in Gfx driver side.
As a result, pin mute status, inforframe transmission, power state need to be re-setup.
Till now, we only observed pin in D3 after boot. But considering Gfx is implement runtime PM, it would be better to always check/fix pin power state.
Since the ELD valid unsolicited response is a signal to indicate Gfx driver has finished audio re-enabling, the audio driver could fix things in hdmi_present_sense() triggered by the HDMI intrinsic event handler.
>
> If it's the same issue, I was thinking that we could
> 1) skip the extra snd_hda_codec_write here, and
> 2) move the call to haswell_verify_pin_D0 from hdmi_setup_stream to
> hdmi_setup_audio_infoframe?
>
> Also, if the pin amp is always unmuted, we could speed up
> haswell_verify_pin_D0 to always unmute instead of read-then-write.
I'll try to merge the Haswell fixing into hdmi_present_sense() and verify it today.
BTW, I've asked Canonical test engineers through Lauchpad to verify current patch.
Thanks
Mengdong
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ALSA: hda - Re-setup HDMI pin and audio infoframe on stream switches
2013-09-04 1:29 ` Lin, Mengdong
@ 2013-09-04 4:53 ` Lin, Mengdong
0 siblings, 0 replies; 8+ messages in thread
From: Lin, Mengdong @ 2013-09-04 4:53 UTC (permalink / raw)
To: David Henningsson, Takashi Iwai; +Cc: alsa-devel@alsa-project.org
> -----Original Message-----
> From: alsa-devel-bounces@alsa-project.org
> [mailto:alsa-devel-bounces@alsa-project.org] On Behalf Of Lin, Mengdong
> Sent: Wednesday, September 04, 2013 9:30 AM
>
> > >> Also, this looks a bit like the workaround I added a while back
> > >> (83f26ad2c909083fa6 - ALSA: hda - fixup D3 pin and right channel
> > >> mute on Haswell HDMI audio) is there a possibility we need to fix
> > >> up D3 as well here? I e, call haswell_verify_pin_D0 rather than just setting
> the mute?
> > >> Or perhaps move the call to haswell_verify_pin_D0 from
> > >> hdmi_setup_stream to hdme_setup_audio_infoframe ?
> > >
> > > The power state is fine in this case, but just to reset the amp
> > > state by some reason. It's not enough to update the info frame, as
> > > far as I tested.
> >
> > Ok - maybe Mengdong can comment more on this, if it's the same
> > scenario as I was working around earlier (right channel muted and pin
> > in D3), or if this is something different.
>
> They're same in nature, although the pin power state can be affected or not.
> S3, hot-plug and Gfx mode switch can all make Gfx reconnect the port, maybe
> to the same or a different transcoder.
> During this process , audio will be disabled and re-enabled again in Gfx driver
> side.
> As a result, pin mute status, inforframe transmission, power state need to be
> re-setup.
>
> Till now, we only observed pin in D3 after boot. But considering Gfx is
> implement runtime PM, it would be better to always check/fix pin power state.
Fix typo: We only observe pin in D3 after boot and S3, i.e. the pin (port) is used for 1st time after GPU change state to D0.
In this display mode switch case, the pin power state remains in D0, although Gfx driver disables and re-enables the port.
> Since the ELD valid unsolicited response is a signal to indicate Gfx driver has
> finished audio re-enabling, the audio driver could fix things in
> hdmi_present_sense() triggered by the HDMI intrinsic event handler.
> >
> > If it's the same issue, I was thinking that we could
> > 1) skip the extra snd_hda_codec_write here, and
> > 2) move the call to haswell_verify_pin_D0 from hdmi_setup_stream to
> > hdmi_setup_audio_infoframe?
> >
> > Also, if the pin amp is always unmuted, we could speed up
> > haswell_verify_pin_D0 to always unmute instead of read-then-write.
I've submitted a patch for this, with subject 'ALSA: hda - unmute pin amplifier in infoframe setup for Haswell'. Could you have a look?
But haswell_verify_pin_D0 is still called in hdmi_setup_stream() and only unmuting pin amp is moved to hdmi_setup_audio_infoframe().
It's because ELDV unsol event may come in codec D3, and pin should not be set to D0 in the event handler for this case.
So the new patch still fixes pin power state when preparing the stream.
Thanks
Mengdong
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2013-09-04 4:53 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-03 9:57 [PATCH] ALSA: hda - Re-setup HDMI pin and audio infoframe on stream switches Takashi Iwai
2013-09-03 11:10 ` David Henningsson
2013-09-03 12:06 ` Takashi Iwai
2013-09-03 12:36 ` David Henningsson
2013-09-03 12:47 ` Takashi Iwai
2013-09-03 13:09 ` David Henningsson
2013-09-04 1:29 ` Lin, Mengdong
2013-09-04 4:53 ` 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.