HDMI regression on close (was: ALSA: hda - Always turn on pins for HDMI/DP)

HDMI regression on close (was: ALSA: hda - Always turn on pins for HDMI/DP)

[Anssi Hannula]

[-- Attachment #1: Type: text/plain, Size: 6509 bytes --]

Hi!

> From: Takashi Iwai <tiwai@suse.de>
> Date: Fri, 14 Dec 2012 09:22:35 +0000 (+0100)
> Subject: ALSA: hda - Always turn on pins for HDMI/DP
> X-Git-Tag: v3.8-rc1~29^2~7
> X-Git-Url: http://git.kernel.org/?p=linux%2Fkernel%2Fgit%2Fstable%2Flinux-stable.git;a=commitdiff_plain;h=6169b673618bf0b2518ce413b54925782a603f06
> 
> ALSA: hda - Always turn on pins for HDMI/DP
> 
> We've seen the broken HDMI *video* output on some machines with GM965,
> and the debugging session pointed that the culprit is the disabled
> audio output pins.  Toggling these pins dynamically on demand caused
> flickering of HDMI TV.
> 
> This patch changes the behavior to keep the pin ON constantly.

Unfortunately this seems to cause a regression on at least some "NVIDIA
ION2" systems.

When running snd_pcm_open() and snd_pcm_close() on an hdmi device on an
affected system, the HDMI port seemingly continues to transmit an empty
audio stream.

However, when hw_params have been set and snd_pcm_prepare() called
before closing the device (and optionally audio has been output), the
HDMI port stops transmitting at snd_pcm_close() time.

(I use "transmitting" here to mean that the HDMI sink/receiver
receives/shows a an active audio stream)

On systems _not_ affected by the issue, the HDMI port never starts
transmitting if just snd_pcm_open()+snd_pcm_close() have been called
(but note [1]).

HDMI port continuing transmitting has the unfortunate effect that when
the user has connected the system to an amplifier via HDMI (for video
playback purposes), the amplifier may continue to play back empty audio
from HDMI even when there is actual audio data on its S/PDIF input.
An snd_pcm_open()+snd_pcm_close() sequence can be caused by e.g. device
probing by an application.

Several OpenELEC users have reported this issue on stable 3.7.x, here's
some alsainfo:
http://onse.fi/files/ion-regression-alsainfo-1.txt
http://onse.fi/files/ion-regression-alsainfo-2.txt
Both of these have the Nvidia GPU 0b HDMI/DP codecs:
Vendor Id: 0x10de000b
Subsystem Id: 0x10de0101
Revision Id: 0x100200

However, e.g. my non-ION2 laptop with 0b codecs (but different rev) is
_not_ affected:
Vendor Id: 0x10de000b
Subsystem Id: 0x10de0101
Revision Id: 0x100100
http://www.alsa-project.org/db/?f=bc940d23d19ae7e2ee1e83895d85f711c2d0e5b1

I'm still working with the OpenELEC devs and users to pinpoint the issue
further, but it is progressing rather slowly due to not many people
having affected systems (especially in the affected dual HDMI + S/PDIF
cabled configuration).

Feel free to suggest anything we should try out if you have some ideas :)
In the meantime, I'll continue investigating exactly why snd_pcm_close()
doesn't stop the HDMI output on the affected systems (or why does a
simple snd_pcm_open() even start outputting the audio, since that does
not seem to happen on other systems).

I'll keep you posted.


[1] It seems that on my Intel HDMI system (
http://www.alsa-project.org/db/?f=bb3fc8680b372c0475719d42d7fc8b2bb7bfb4eb
) the HDMI port keeps transmitting in approximately reversed situation
than in the above ION2 case. That is, if audio has been output in the
port e.g. with a speaker-test command, closing the device will not cause
the stream to stop (and therefore the above issue happens again,
receives ignores S/PDIF stream as HDMI stream is given higher priority
by default). I found out that the attached patch solves that one.
What are your thoughts on that?
(obviously the patch will not solve the ION2 regression, since cleanup()
is not called if prepare() has not been called; or at least I assume so,
way too tired to check that now...)
Note that closing the device after audio has been output stops HDMI port
transmission even without the patch on both the NVIDIA ION2 systems and
on my NVIDIA laptop, it is just the Intel HDMI codec where that does not
happen (well, of the tested ones, at least).


> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=51421
> 
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
> 
> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> index 0fcfa6f..37dd06d 100644
> --- a/sound/pci/hda/patch_hdmi.c
> +++ b/sound/pci/hda/patch_hdmi.c
> @@ -431,9 +431,11 @@ static void hdmi_init_pin(struct hda_codec *codec, hda_nid_t pin_nid)
>  	if (get_wcaps(codec, pin_nid) & AC_WCAP_OUT_AMP)
>  		snd_hda_codec_write(codec, pin_nid, 0,
>  				AC_VERB_SET_AMP_GAIN_MUTE, AMP_OUT_UNMUTE);
> -	/* Disable pin out until stream is active*/
> +	/* Enable pin out: some machines with GM965 gets broken output when
> +	 * the pin is disabled or changed while using with HDMI
> +	 */
>  	snd_hda_codec_write(codec, pin_nid, 0,
> -			    AC_VERB_SET_PIN_WIDGET_CONTROL, 0);
> +			    AC_VERB_SET_PIN_WIDGET_CONTROL, PIN_OUT);
>  }
>  
>  static int hdmi_get_channel_count(struct hda_codec *codec, hda_nid_t cvt_nid)
> @@ -1341,7 +1343,6 @@ static int generic_hdmi_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
>  	struct hdmi_spec *spec = codec->spec;
>  	int pin_idx = hinfo_to_pin_index(spec, hinfo);
>  	hda_nid_t pin_nid = spec->pins[pin_idx].pin_nid;
> -	int pinctl;
>  	bool non_pcm;
>  
>  	non_pcm = check_non_pcm_per_cvt(codec, cvt_nid);
> @@ -1350,11 +1351,6 @@ static int generic_hdmi_playback_pcm_prepare(struct hda_pcm_stream *hinfo,
>  
>  	hdmi_setup_audio_infoframe(codec, pin_idx, non_pcm, substream);
>  
> -	pinctl = snd_hda_codec_read(codec, pin_nid, 0,
> -				    AC_VERB_GET_PIN_WIDGET_CONTROL, 0);
> -	snd_hda_codec_write(codec, pin_nid, 0,
> -			    AC_VERB_SET_PIN_WIDGET_CONTROL, pinctl | PIN_OUT);
> -
>  	return hdmi_setup_stream(codec, cvt_nid, pin_nid, stream_tag, format);
>  }
>  
> @@ -1374,7 +1370,6 @@ static int hdmi_pcm_close(struct hda_pcm_stream *hinfo,
>  	int cvt_idx, pin_idx;
>  	struct hdmi_spec_per_cvt *per_cvt;
>  	struct hdmi_spec_per_pin *per_pin;
> -	int pinctl;
>  
>  	if (hinfo->nid) {
>  		cvt_idx = cvt_nid_to_cvt_index(spec, hinfo->nid);
> @@ -1391,11 +1386,6 @@ static int hdmi_pcm_close(struct hda_pcm_stream *hinfo,
>  			return -EINVAL;
>  		per_pin = &spec->pins[pin_idx];
>  
> -		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);
>  		per_pin->chmap_set = false;
>  		memset(per_pin->chmap, 0, sizeof(per_pin->chmap));


-- 
Anssi Hannula

[-- Attachment #2: 0001-ALSA-hda-Always-clean-up-HDMI-streams-in-cleanup-cal.patch --]
[-- Type: text/x-patch, Size: 1590 bytes --]

>From de006e0d6ccfa3709447757f1f4084621eb84dd7 Mon Sep 17 00:00:00 2001
From: Anssi Hannula <anssi.hannula@iki.fi>
Date: Sat, 16 Feb 2013 03:41:35 +0200
Subject: [PATCH] ALSA: hda - Always clean up HDMI streams in cleanup callback

Currently actual HDMI stream cleanup (resetting CHANNEL_STREAMID and
STREAM_FORMAT to zero) is deferred to a later time by
snd_hda_codec_cleanup_stream() like with other HDA streams.

On at least some Intel codecs (but not in at least some NVIDIA ones)
this will cause the HDMI port to continue transmitting an audio stream
after the device has been closed. This has the effect that a receiver
connected to such a port may give the HDMI port audio higher priority
than e.g. an actually active S/PDIF input port, causing silence to be
heard instead of audio of the S/PDIF input port of the receiver.

Change the cleanup callback to always clean up the streams to avoid such
issues.

Signed-off-by: Anssi Hannula <anssi.hannula@iki.fi>
---
 sound/pci/hda/patch_hdmi.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 807a2aa..283ed55 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -1358,7 +1358,8 @@ static int generic_hdmi_playback_pcm_cleanup(struct hda_pcm_stream *hinfo,
 					     struct hda_codec *codec,
 					     struct snd_pcm_substream *substream)
 {
-	snd_hda_codec_cleanup_stream(codec, hinfo->nid);
+	/* Avoid outputting silence after cleanup */
+	__snd_hda_codec_cleanup_stream(codec, hinfo->nid, 1);
 	return 0;
 }
 
-- 
1.7.10


[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

[PATCH] ALSA: hda - Avoid outputting HDMI audio before prepare() and after close()

[Anssi Hannula]

Some HDMI codecs (at least NVIDIA 0x10de000b:0x10de0101:0x100100) start
transmitting an empty audio stream as soon as PIN_OUT and AC_DIG1_ENABLE
are enabled.

Since commit 6169b673618bf0b2518ce413b54925782a603f06 ("ALSA: hda -
Always turn on pins for HDMI/DP") this happens at first open() time, and
will continue even after close().

Additionally, some codecs (at least Intel PantherPoint HDMI) currently
continue transmitting HDMI audio even after close() in case some actual
audio was output after open() (this happens regardless of PIN_OUT).

Empty HDMI audio transmission when not intended has the effect that a
possible HDMI audio sink/receiver may prefer the empty HDMI audio stream
over an actual audio stream on its S/PDIF inputs.

To avoid the issue before first prepare(), set stream format to 0 on
codec initialization. 0 is not a valid format value for HDMI and will
prevent the audio stream from being output.

Additionally, at close() time, make sure that the stream is cleaned up.
This will ensure that the format is reset to 0 at that time, preventing
audio from being output in that case.

Thanks to OpenELEC developers and users for their help in investigating
this issue on the affected NVIDIA "ION2" hardware, and for the initial
bug report of the issue. Testing of the final version on NVIDIA ION2 was
done by OpenELEC user "MrXIII". Testing on Intel PantherPoint was done
by myself.

Signed-off-by: Anssi Hannula <anssi.hannula@iki.fi>
Cc: stable@vger.kernel.org
---

This also supersedes the patch I attached yesterday as it fixes both
cases.

I guess the alternative to this approach would be to fiddle with
AC_DIG1_ENABLE when preparing and closing the device, but with a quick
look that seemed to be possibly more complex since AC_DIG1_ENABLE is
already meddled with at quite a few places in hda_codec.c.

WDYT, is this OK or should we try a AC_DIG1_ENABLE based approach or
something else?


 sound/pci/hda/patch_hdmi.c |   14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 807a2aa..bcb83c7 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -1253,6 +1253,14 @@ static int hdmi_add_cvt(struct hda_codec *codec, hda_nid_t cvt_nid)
 	if (err < 0)
 		return err;
 
+	/*
+	 * Some HDMI codecs (at least NVIDIA 0x10de000b:0x10de0101:0x100100)
+	 * start transmitting an empty audio stream as soon as PIN_OUT and
+	 * AC_DIG1_ENABLE are enabled, which happens at open() time.
+	 * To avoid that, set format to 0, which is not valid for HDMI.
+	 */
+	snd_hda_codec_write(codec, cvt_nid, 0, AC_VERB_SET_STREAM_FORMAT, 0);
+
 	spec->num_cvts++;
 
 	return 0;
@@ -1372,6 +1380,12 @@ static int hdmi_pcm_close(struct hda_pcm_stream *hinfo,
 	struct hdmi_spec_per_pin *per_pin;
 
 	if (hinfo->nid) {
+		/*
+		 * Make sure no empty audio is output after this point by
+		 * setting stream format to 0, which is not valid for HDMI.
+		 */
+		__snd_hda_codec_cleanup_stream(codec, hinfo->nid, 1);
+
 		cvt_idx = cvt_nid_to_cvt_index(spec, hinfo->nid);
 		if (snd_BUG_ON(cvt_idx < 0))
 			return -EINVAL;
-- 
1.7.10

Re: [PATCH] ALSA: hda - Avoid outputting HDMI audio before prepare() and after close()

[Anssi Hannula]

16.02.2013 18:40, Anssi Hannula kirjoitti:
> Some HDMI codecs (at least NVIDIA 0x10de000b:0x10de0101:0x100100) start
> transmitting an empty audio stream as soon as PIN_OUT and AC_DIG1_ENABLE
> are enabled.
> 
> Since commit 6169b673618bf0b2518ce413b54925782a603f06 ("ALSA: hda -
> Always turn on pins for HDMI/DP") this happens at first open() time, and
> will continue even after close().
> 
> Additionally, some codecs (at least Intel PantherPoint HDMI) currently
> continue transmitting HDMI audio even after close() in case some actual
> audio was output after open() (this happens regardless of PIN_OUT).
> 
> Empty HDMI audio transmission when not intended has the effect that a
> possible HDMI audio sink/receiver may prefer the empty HDMI audio stream
> over an actual audio stream on its S/PDIF inputs.
> 
> To avoid the issue before first prepare(), set stream format to 0 on
> codec initialization. 0 is not a valid format value for HDMI and will
> prevent the audio stream from being output.
> 
> Additionally, at close() time, make sure that the stream is cleaned up.
> This will ensure that the format is reset to 0 at that time, preventing
> audio from being output in that case.
> 
> Thanks to OpenELEC developers and users for their help in investigating
> this issue on the affected NVIDIA "ION2" hardware, and for the initial
> bug report of the issue. Testing of the final version on NVIDIA ION2 was
> done by OpenELEC user "MrXIII". Testing on Intel PantherPoint was done
> by myself.
> 
> Signed-off-by: Anssi Hannula <anssi.hannula@iki.fi>
> Cc: stable@vger.kernel.org
> ---
> 
> This also supersedes the patch I attached yesterday as it fixes both
> cases.
> 
> I guess the alternative to this approach would be to fiddle with
> AC_DIG1_ENABLE when preparing and closing the device, but with a quick
> look that seemed to be possibly more complex since AC_DIG1_ENABLE is
> already meddled with at quite a few places in hda_codec.c.

Hmm... actually, that would not be so much more complex, just making
sure AC_DIG1_ENABLE is off in hdmi_add_cvt() (by e.g. zeroing
AC_VERB_SET_DIGI_CONVERT_1) and making snd_hda_spdif_ctls_unassign()
call "set_spdif_ctls(codec, spdif->nid, 0, -1);".

However, it would still mean that just a simple snd_pcm_open() could
cause an empty stream to be output (if format is valid), since iec958
controls are assigned at open() time, and iec958 mute controls
AC_DIG1_ENABLE. Or we could add additional code in hda_codec.c to
prevent AC_DIG1_ENABLE being set if prepare() has not been called.


> WDYT, is this OK or should we try a AC_DIG1_ENABLE based approach or
> something else?
> 
> 
>  sound/pci/hda/patch_hdmi.c |   14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
> index 807a2aa..bcb83c7 100644
> --- a/sound/pci/hda/patch_hdmi.c
> +++ b/sound/pci/hda/patch_hdmi.c
> @@ -1253,6 +1253,14 @@ static int hdmi_add_cvt(struct hda_codec *codec, hda_nid_t cvt_nid)
>  	if (err < 0)
>  		return err;
>  
> +	/*
> +	 * Some HDMI codecs (at least NVIDIA 0x10de000b:0x10de0101:0x100100)
> +	 * start transmitting an empty audio stream as soon as PIN_OUT and
> +	 * AC_DIG1_ENABLE are enabled, which happens at open() time.
> +	 * To avoid that, set format to 0, which is not valid for HDMI.
> +	 */
> +	snd_hda_codec_write(codec, cvt_nid, 0, AC_VERB_SET_STREAM_FORMAT, 0);
> +
>  	spec->num_cvts++;
>  
>  	return 0;
> @@ -1372,6 +1380,12 @@ static int hdmi_pcm_close(struct hda_pcm_stream *hinfo,
>  	struct hdmi_spec_per_pin *per_pin;
>  
>  	if (hinfo->nid) {
> +		/*
> +		 * Make sure no empty audio is output after this point by
> +		 * setting stream format to 0, which is not valid for HDMI.
> +		 */
> +		__snd_hda_codec_cleanup_stream(codec, hinfo->nid, 1);
> +
>  		cvt_idx = cvt_nid_to_cvt_index(spec, hinfo->nid);
>  		if (snd_BUG_ON(cvt_idx < 0))
>  			return -EINVAL;
> 


-- 
Anssi Hannula

Re: [PATCH] ALSA: hda - Avoid outputting HDMI audio before prepare() and after close()

[Takashi Iwai]

At Sat, 16 Feb 2013 22:59:40 +0200,
Anssi Hannula wrote:
> 
> 16.02.2013 18:40, Anssi Hannula kirjoitti:
> > Some HDMI codecs (at least NVIDIA 0x10de000b:0x10de0101:0x100100) start
> > transmitting an empty audio stream as soon as PIN_OUT and AC_DIG1_ENABLE
> > are enabled.
> > 
> > Since commit 6169b673618bf0b2518ce413b54925782a603f06 ("ALSA: hda -
> > Always turn on pins for HDMI/DP") this happens at first open() time, and
> > will continue even after close().
> > 
> > Additionally, some codecs (at least Intel PantherPoint HDMI) currently
> > continue transmitting HDMI audio even after close() in case some actual
> > audio was output after open() (this happens regardless of PIN_OUT).
> > 
> > Empty HDMI audio transmission when not intended has the effect that a
> > possible HDMI audio sink/receiver may prefer the empty HDMI audio stream
> > over an actual audio stream on its S/PDIF inputs.
> > 
> > To avoid the issue before first prepare(), set stream format to 0 on
> > codec initialization. 0 is not a valid format value for HDMI and will
> > prevent the audio stream from being output.
> > 
> > Additionally, at close() time, make sure that the stream is cleaned up.
> > This will ensure that the format is reset to 0 at that time, preventing
> > audio from being output in that case.
> > 
> > Thanks to OpenELEC developers and users for their help in investigating
> > this issue on the affected NVIDIA "ION2" hardware, and for the initial
> > bug report of the issue. Testing of the final version on NVIDIA ION2 was
> > done by OpenELEC user "MrXIII". Testing on Intel PantherPoint was done
> > by myself.
> > 
> > Signed-off-by: Anssi Hannula <anssi.hannula@iki.fi>
> > Cc: stable@vger.kernel.org
> > ---
> > 
> > This also supersedes the patch I attached yesterday as it fixes both
> > cases.
> > 
> > I guess the alternative to this approach would be to fiddle with
> > AC_DIG1_ENABLE when preparing and closing the device, but with a quick
> > look that seemed to be possibly more complex since AC_DIG1_ENABLE is
> > already meddled with at quite a few places in hda_codec.c.
> 
> Hmm... actually, that would not be so much more complex, just making
> sure AC_DIG1_ENABLE is off in hdmi_add_cvt() (by e.g. zeroing
> AC_VERB_SET_DIGI_CONVERT_1) and making snd_hda_spdif_ctls_unassign()
> call "set_spdif_ctls(codec, spdif->nid, 0, -1);".
> 
> However, it would still mean that just a simple snd_pcm_open() could
> cause an empty stream to be output (if format is valid), since iec958
> controls are assigned at open() time, and iec958 mute controls
> AC_DIG1_ENABLE. Or we could add additional code in hda_codec.c to
> prevent AC_DIG1_ENABLE being set if prepare() has not been called.

Is the problem fixed if you set codec->no_sticky_stream = 1?

Actually, the current behavior is intentional.  There have been bug
reports that just reopening a device causes the receiver not reacting
immediately.  In the worst case, it takes a few seconds to sync. So we
introduced a mechanism to keep the PCM stream assignment.

If your problem is solved by clearing codec->no_stick_stream, we may
provide an additional mixer control to turn it on/off, for example,
because we can't blindly disable/enable it globally if user's receiver
has a problem with the stream sync.


thanks,

Takashi

Re: [PATCH] ALSA: hda - Avoid outputting HDMI audio before prepare() and after close()

[Anssi Hannula]

17.02.2013 10:24, Takashi Iwai kirjoitti:
> At Sat, 16 Feb 2013 22:59:40 +0200,
> Anssi Hannula wrote:
>>
>> 16.02.2013 18:40, Anssi Hannula kirjoitti:
>>> Some HDMI codecs (at least NVIDIA 0x10de000b:0x10de0101:0x100100) start
>>> transmitting an empty audio stream as soon as PIN_OUT and AC_DIG1_ENABLE
>>> are enabled.
>>>
>>> Since commit 6169b673618bf0b2518ce413b54925782a603f06 ("ALSA: hda -
>>> Always turn on pins for HDMI/DP") this happens at first open() time, and
>>> will continue even after close().
>>>
>>> Additionally, some codecs (at least Intel PantherPoint HDMI) currently
>>> continue transmitting HDMI audio even after close() in case some actual
>>> audio was output after open() (this happens regardless of PIN_OUT).
>>>
>>> Empty HDMI audio transmission when not intended has the effect that a
>>> possible HDMI audio sink/receiver may prefer the empty HDMI audio stream
>>> over an actual audio stream on its S/PDIF inputs.
>>>
>>> To avoid the issue before first prepare(), set stream format to 0 on
>>> codec initialization. 0 is not a valid format value for HDMI and will
>>> prevent the audio stream from being output.
>>>
>>> Additionally, at close() time, make sure that the stream is cleaned up.
>>> This will ensure that the format is reset to 0 at that time, preventing
>>> audio from being output in that case.
>>>
>>> Thanks to OpenELEC developers and users for their help in investigating
>>> this issue on the affected NVIDIA "ION2" hardware, and for the initial
>>> bug report of the issue. Testing of the final version on NVIDIA ION2 was
>>> done by OpenELEC user "MrXIII". Testing on Intel PantherPoint was done
>>> by myself.
>>>
>>> Signed-off-by: Anssi Hannula <anssi.hannula@iki.fi>
>>> Cc: stable@vger.kernel.org
>>> ---
>>>
>>> This also supersedes the patch I attached yesterday as it fixes both
>>> cases.
>>>
>>> I guess the alternative to this approach would be to fiddle with
>>> AC_DIG1_ENABLE when preparing and closing the device, but with a quick
>>> look that seemed to be possibly more complex since AC_DIG1_ENABLE is
>>> already meddled with at quite a few places in hda_codec.c.
>>
>> Hmm... actually, that would not be so much more complex, just making
>> sure AC_DIG1_ENABLE is off in hdmi_add_cvt() (by e.g. zeroing
>> AC_VERB_SET_DIGI_CONVERT_1) and making snd_hda_spdif_ctls_unassign()
>> call "set_spdif_ctls(codec, spdif->nid, 0, -1);".
>>
>> However, it would still mean that just a simple snd_pcm_open() could
>> cause an empty stream to be output (if format is valid), since iec958
>> controls are assigned at open() time, and iec958 mute controls
>> AC_DIG1_ENABLE. Or we could add additional code in hda_codec.c to
>> prevent AC_DIG1_ENABLE being set if prepare() has not been called.
> 
> Is the problem fixed if you set codec->no_sticky_stream = 1?

The issue on NVIDIA reported by users, no, that flag is not used in a
simple open()+close() path. My issue on Intel, yes.

> Actually, the current behavior is intentional.  There have been bug
> reports that just reopening a device causes the receiver not reacting
> immediately.  In the worst case, it takes a few seconds to sync. So we
> introduced a mechanism to keep the PCM stream assignment.

Hmm.. If the intention is to keep a silent stream playing even after
device has been closed, why was PIN_OUT disabled on close until "ALSA:
hda - Always turn on pins for HDMI/DP" in December?

Anyway, I can understand the desire to keep an empty stream running
after device has been closed. However, IMO a simple
snd_pcm_open+snd_pcm_close triggering a start of empty stream (as on the
affected NVIDIA hw) seems wrong, since this can happen without actual
audio being output by application device probing (e.g. pulseaudio) and
the HDMI audio supersedes actual audio, as reported by several users
(during beta/rc testing of OpenELEC - it is also not very obvious what
is causing the sudden brokenness of audio, it took quite a bit of
debugging before we found out the cause).

Also, as noted, this "current behavior" doesn't actually work on the
affected NVIDIA 0b hardware (nor it does on my non-affected NVIDIA 0b
hardware); when the actual audio stream stops, sync is lost regardless
of the kept stream assignment.

> If your problem is solved by clearing codec->no_stick_stream, we may
> provide an additional mixer control to turn it on/off, for example,
> because we can't blindly disable/enable it globally if user's receiver
> has a problem with the stream sync.

I think we definitely should make "no empty hdmi stream after close or
before first open" an option at least.

However, while writing this I made some tests on my hw with NVIDIA 15
codec, and it seems to have a fourth different set of behavior related
to this.

So we now have at least (unless I made a mistake at some point, which is
not too unlikely considering the amount of variables here... I can
retest stuff if needed):

NVIDIA 0b non-"ION2" (rev 0x100100) hardware:
- HDMI output becomes active when the real audio stream starts
  (plus non-tested other conditions, at least AC_DIG1_ENABLE)
- HDMI output stops after the real audio stream stops
- on cold start: format is valid, AC_DIG1_ENABLE is off
=> currently HDMI becomes active when actual audio is output, and
   is stopped when audio stops (e.g. even when pausing playback without
   closing)

NVIDIA 0b "ION2" (rev 0x100200) hardware:
- HDMI output becomes active when PIN_OUT + AC_DIG1_ENABLE + valid
  stream format
- HDMI output stops after the real audio stream stops
  (it would follow that flipping AC_DIG1_ENABLE off and on again
   would start HDMI output again - however, not tested)
- on cold start: format is valid, AC_DIG1_ENABLE is off
=> currently HDMI becomes active immediately at open(), and is stopped
   when actual audio stream stops. However, in case of open()+close(),
   the actual audio stream never starts and therefore never stops,
   leaving the HDMI audio active with empty stream

NVIDIA 15 hardware:
- HDMI output becomes active when PIN_OUT + AC_DIG1_ENABLE
  (no valid stream format needed)
- HDMI output stops after the above conditions stop
- on cold start: format is valid, AC_DIG1_ENABLE is off
=> currently HDMI becomes active immediately at open(),
   and is never stopped unless it is muted during playback
   (and my earlier patch would not actually change that)

Intel PantherPoint HDMI hardware:
- HDMI output becomes active when AC_DIG1_ENABLE + valid stream format
  (regardless of PIN_OUT)
- HDMI output stops after the above conditions stop (or, in a strange
  just-trying-to-confuse-me way, when stopping a 32kHz stream for some
  reason)
- on cold start: format is invalid, AC_DIG1_ENABLE is on
=> currently HDMI becomes active when the stream format is set for
   the first time, i.e. something is played back. It is never stopped
   unless it is muted during playback.


Note that e.g. pulseaudio opens+closes devices immediately, so on NVIDIA
0b "ION2" and NVIDIA 15 cases HDMI output becomes active immediately.
Also note that muting the IEC958 control won't stop the HDMI output
unless/until the device is actually opened, since the IEC958 ctl is
nowadays only assigned to the device when opened.

Now, whatever is the behavior that we want, according the above clearly
there is space for improvement in any case.

For example, when exactly do we want the HDMI port to become active,
preferably?
- immediately, always on initialization
- on first open()
- on first prepare()

And when to stop?
- never
- on close()
- after timeout after close()

As per the above test data most hardware can be made to follow any of
the behaviors (except that NVIDIA 0b non-ION2 cannot seemingly support
keeping empty HDMI stream open at all, at least per current tests), but
the current behavior is rather mixed.

As you probably have gathered, I'd support HDMI port to become active at
prepare() (after all we don't have the stream parameters yet at open
time or earlier), and to become inactive at close(). I guess I can
settle for an option, though, if you insist :)


(There is also the power saving stuff to be considered, but I'm ignoring
that simply because I have no information about the effects there...)

-- 
Anssi Hannula

Re: [PATCH] ALSA: hda - Avoid outputting HDMI audio before prepare() and after close()

[Takashi Iwai]

At Sun, 17 Feb 2013 14:23:21 +0200,
Anssi Hannula wrote:
> 
> 17.02.2013 10:24, Takashi Iwai kirjoitti:
> > At Sat, 16 Feb 2013 22:59:40 +0200,
> > Anssi Hannula wrote:
> >>
> >> 16.02.2013 18:40, Anssi Hannula kirjoitti:
> >>> Some HDMI codecs (at least NVIDIA 0x10de000b:0x10de0101:0x100100) start
> >>> transmitting an empty audio stream as soon as PIN_OUT and AC_DIG1_ENABLE
> >>> are enabled.
> >>>
> >>> Since commit 6169b673618bf0b2518ce413b54925782a603f06 ("ALSA: hda -
> >>> Always turn on pins for HDMI/DP") this happens at first open() time, and
> >>> will continue even after close().
> >>>
> >>> Additionally, some codecs (at least Intel PantherPoint HDMI) currently
> >>> continue transmitting HDMI audio even after close() in case some actual
> >>> audio was output after open() (this happens regardless of PIN_OUT).
> >>>
> >>> Empty HDMI audio transmission when not intended has the effect that a
> >>> possible HDMI audio sink/receiver may prefer the empty HDMI audio stream
> >>> over an actual audio stream on its S/PDIF inputs.
> >>>
> >>> To avoid the issue before first prepare(), set stream format to 0 on
> >>> codec initialization. 0 is not a valid format value for HDMI and will
> >>> prevent the audio stream from being output.
> >>>
> >>> Additionally, at close() time, make sure that the stream is cleaned up.
> >>> This will ensure that the format is reset to 0 at that time, preventing
> >>> audio from being output in that case.
> >>>
> >>> Thanks to OpenELEC developers and users for their help in investigating
> >>> this issue on the affected NVIDIA "ION2" hardware, and for the initial
> >>> bug report of the issue. Testing of the final version on NVIDIA ION2 was
> >>> done by OpenELEC user "MrXIII". Testing on Intel PantherPoint was done
> >>> by myself.
> >>>
> >>> Signed-off-by: Anssi Hannula <anssi.hannula@iki.fi>
> >>> Cc: stable@vger.kernel.org
> >>> ---
> >>>
> >>> This also supersedes the patch I attached yesterday as it fixes both
> >>> cases.
> >>>
> >>> I guess the alternative to this approach would be to fiddle with
> >>> AC_DIG1_ENABLE when preparing and closing the device, but with a quick
> >>> look that seemed to be possibly more complex since AC_DIG1_ENABLE is
> >>> already meddled with at quite a few places in hda_codec.c.
> >>
> >> Hmm... actually, that would not be so much more complex, just making
> >> sure AC_DIG1_ENABLE is off in hdmi_add_cvt() (by e.g. zeroing
> >> AC_VERB_SET_DIGI_CONVERT_1) and making snd_hda_spdif_ctls_unassign()
> >> call "set_spdif_ctls(codec, spdif->nid, 0, -1);".
> >>
> >> However, it would still mean that just a simple snd_pcm_open() could
> >> cause an empty stream to be output (if format is valid), since iec958
> >> controls are assigned at open() time, and iec958 mute controls
> >> AC_DIG1_ENABLE. Or we could add additional code in hda_codec.c to
> >> prevent AC_DIG1_ENABLE being set if prepare() has not been called.
> > 
> > Is the problem fixed if you set codec->no_sticky_stream = 1?
> 
> The issue on NVIDIA reported by users, no, that flag is not used in a
> simple open()+close() path.

It is.  The flag is used in hda_codec.c.  It's a flag for setting
FORMAT stuff, not the SPDIF status.

> My issue on Intel, yes.
>
> > Actually, the current behavior is intentional.  There have been bug
> > reports that just reopening a device causes the receiver not reacting
> > immediately.  In the worst case, it takes a few seconds to sync. So we
> > introduced a mechanism to keep the PCM stream assignment.
> 
> Hmm.. If the intention is to keep a silent stream playing even after
> device has been closed, why was PIN_OUT disabled on close until "ALSA:
> hda - Always turn on pins for HDMI/DP" in December?

The intention isn't to keep the stream playing, but it keeps the
STREAM tag.  It's just a weird hardware implementation that it keeps
playing.  And, the feature was implemented far before the new dynamic
SPDIF status control assignment and the dynamic pin down.  So, it got
broken by that stuff sometime ago.

> Anyway, I can understand the desire to keep an empty stream running
> after device has been closed. However, IMO a simple
> snd_pcm_open+snd_pcm_close triggering a start of empty stream (as on the
> affected NVIDIA hw) seems wrong, since this can happen without actual
> audio being output by application device probing (e.g. pulseaudio) and
> the HDMI audio supersedes actual audio, as reported by several users
> (during beta/rc testing of OpenELEC - it is also not very obvious what
> is causing the sudden brokenness of audio, it took quite a bit of
> debugging before we found out the cause).

Again, the intention isn't about keep stream sending.  It was a
workaround to avoid the unnecessary out-of-sync situation at each open
or prepare.  Otherwise you'll get a few seconds silence at each
stream.

> Also, as noted, this "current behavior" doesn't actually work on the
> affected NVIDIA 0b hardware (nor it does on my non-affected NVIDIA 0b
> hardware); when the actual audio stream stops, sync is lost regardless
> of the kept stream assignment.

Yes, this got broken by the update sometime ago, unfortunately.

And, remember that user may still use OSS emulation -- in that case,
there are frequent open/close sequences.  So, keeping sync after close
makes sense for such.

In other words, there is no black/white solution for this problem.
It must be somehow configurable for each use case.


> > If your problem is solved by clearing codec->no_stick_stream, we may
> > provide an additional mixer control to turn it on/off, for example,
> > because we can't blindly disable/enable it globally if user's receiver
> > has a problem with the stream sync.
> 
> I think we definitely should make "no empty hdmi stream after close or
> before first open" an option at least.
> 
> However, while writing this I made some tests on my hw with NVIDIA 15
> codec, and it seems to have a fourth different set of behavior related
> to this.
> 
> So we now have at least (unless I made a mistake at some point, which is
> not too unlikely considering the amount of variables here... I can
> retest stuff if needed):
> 
> NVIDIA 0b non-"ION2" (rev 0x100100) hardware:
> - HDMI output becomes active when the real audio stream starts
>   (plus non-tested other conditions, at least AC_DIG1_ENABLE)
> - HDMI output stops after the real audio stream stops
> - on cold start: format is valid, AC_DIG1_ENABLE is off
> => currently HDMI becomes active when actual audio is output, and
>    is stopped when audio stops (e.g. even when pausing playback without
>    closing)
> 
> NVIDIA 0b "ION2" (rev 0x100200) hardware:
> - HDMI output becomes active when PIN_OUT + AC_DIG1_ENABLE + valid
>   stream format
> - HDMI output stops after the real audio stream stops
>   (it would follow that flipping AC_DIG1_ENABLE off and on again
>    would start HDMI output again - however, not tested)
> - on cold start: format is valid, AC_DIG1_ENABLE is off
> => currently HDMI becomes active immediately at open(), and is stopped
>    when actual audio stream stops. However, in case of open()+close(),
>    the actual audio stream never starts and therefore never stops,
>    leaving the HDMI audio active with empty stream
> 
> NVIDIA 15 hardware:
> - HDMI output becomes active when PIN_OUT + AC_DIG1_ENABLE
>   (no valid stream format needed)
> - HDMI output stops after the above conditions stop
> - on cold start: format is valid, AC_DIG1_ENABLE is off
> => currently HDMI becomes active immediately at open(),
>    and is never stopped unless it is muted during playback
>    (and my earlier patch would not actually change that)
> 
> Intel PantherPoint HDMI hardware:
> - HDMI output becomes active when AC_DIG1_ENABLE + valid stream format
>   (regardless of PIN_OUT)
> - HDMI output stops after the above conditions stop (or, in a strange
>   just-trying-to-confuse-me way, when stopping a 32kHz stream for some
>   reason)
> - on cold start: format is invalid, AC_DIG1_ENABLE is on
> => currently HDMI becomes active when the stream format is set for
>    the first time, i.e. something is played back. It is never stopped
>    unless it is muted during playback.
> 
> 
> Note that e.g. pulseaudio opens+closes devices immediately, so on NVIDIA
> 0b "ION2" and NVIDIA 15 cases HDMI output becomes active immediately.
> Also note that muting the IEC958 control won't stop the HDMI output
> unless/until the device is actually opened, since the IEC958 ctl is
> nowadays only assigned to the device when opened.
> 
> Now, whatever is the behavior that we want, according the above clearly
> there is space for improvement in any case.
> 
> For example, when exactly do we want the HDMI port to become active,
> preferably?
> - immediately, always on initialization
> - on first open()
> - on first prepare()
>
> And when to stop?
> - never
> - on close()
> - after timeout after close()

We just need to try.  I guess the setting at open() is the cleanest
way of implementation.  The delayed cleanup sounds interesting, and if
it can be implemented without too many mess, this looks like the best
option for stopping.


> As per the above test data most hardware can be made to follow any of
> the behaviors (except that NVIDIA 0b non-ION2 cannot seemingly support
> keeping empty HDMI stream open at all, at least per current tests), but
> the current behavior is rather mixed.
> 
> As you probably have gathered, I'd support HDMI port to become active at
> prepare() (after all we don't have the stream parameters yet at open
> time or earlier), and to become inactive at close(). I guess I can
> settle for an option, though, if you insist :)
> 
> 
> (There is also the power saving stuff to be considered, but I'm ignoring
> that simply because I have no information about the effects there...)

It shouldn't influence so much on the power.  The empty audio stream
has nothing to do with the sound driver side.  The sound component can
be even completely powered off during the empty audio stream is
embedded into HDMI/DP frame.


thanks,

Takashi

Re: [PATCH] ALSA: hda - Avoid outputting HDMI audio before prepare() and after close()

[Anssi Hannula]

On Sunday, February 17, 2013, Takashi Iwai <tiwai@suse.de> wrote:
> At Sun, 17 Feb 2013 14:23:21 +0200,
> Anssi Hannula wrote:
>>
>> 17.02.2013 10:24, Takashi Iwai kirjoitti:
>> > At Sat, 16 Feb 2013 22:59:40 +0200,
>> > Anssi Hannula wrote:
>> >>
>> >> 16.02.2013 18:40, Anssi Hannula kirjoitti:
>> >>> Some HDMI codecs (at least NVIDIA 0x10de000b:0x10de0101:0x100100)
start
>> >>> transmitting an empty audio stream as soon as PIN_OUT and
AC_DIG1_ENABLE
>> >>> are enabled.
>> >>>
>> >>> Since commit 6169b673618bf0b2518ce413b54925782a603f06 ("ALSA: hda -
>> >>> Always turn on pins for HDMI/DP") this happens at first open() time,
and
>> >>> will continue even after close().
>> >>>
>> >>> Additionally, some codecs (at least Intel PantherPoint HDMI)
currently
>> >>> continue transmitting HDMI audio even after close() in case some
actual
>> >>> audio was output after open() (this happens regardless of PIN_OUT).
>> >>>
>> >>> Empty HDMI audio transmission when not intended has the effect that a
>> >>> possible HDMI audio sink/receiver may prefer the empty HDMI audio
stream
>> >>> over an actual audio stream on its S/PDIF inputs.
>> >>>
>> >>> To avoid the issue before first prepare(), set stream format to 0 on
>> >>> codec initialization. 0 is not a valid format value for HDMI and will
>> >>> prevent the audio stream from being output.
>> >>>
>> >>> Additionally, at close() time, make sure that the stream is cleaned
up.
>> >>> This will ensure that the format is reset to 0 at that time,
preventing
>> >>> audio from being output in that case.
>> >>>
>> >>> Thanks to OpenELEC developers and users for their help in
investigating
>> >>> this issue on the affected NVIDIA "ION2" hardware, and for the
initial
>> >>> bug report of the issue. Testing of the final version on NVIDIA ION2
was
>> >>> done by OpenELEC user "MrXIII". Testing on Intel PantherPoint was
done
>> >>> by myself.
>> >>>
>> >>> Signed-off-by: Anssi Hannula <anssi.hannula@iki.fi>
>> >>> Cc: stable@vger.kernel.org
>> >>> ---
>> >>>
>> >>> This also supersedes the patch I attached yesterday as it fixes both
>> >>> cases.
>> >>>
>> >>> I guess the alternative to this approach would be to fiddle with
>> >>> AC_DIG1_ENABLE when preparing and closing the device, but with a
quick
>> >>> look that seemed to be possibly more complex since AC_DIG1_ENABLE is
>> >>> already meddled with at quite a few places in hda_codec.c.
>> >>
>> >> Hmm... actually, that would not be so much more complex, just making
>> >> sure AC_DIG1_ENABLE is off in hdmi_add_cvt() (by e.g. zeroing
>> >> AC_VERB_SET_DIGI_CONVERT_1) and making snd_hda_spdif_ctls_unassign()
>> >> call "set_spdif_ctls(codec, spdif->nid, 0, -1);".
>> >>
>> >> However, it would still mean that just a simple snd_pcm_open() could
>> >> cause an empty stream to be output (if format is valid), since iec958
>> >> controls are assigned at open() time, and iec958 mute controls
>> >> AC_DIG1_ENABLE. Or we could add additional code in hda_codec.c to
>> >> prevent AC_DIG1_ENABLE being set if prepare() has not been called.
>> >
>> > Is the problem fixed if you set codec->no_sticky_stream = 1?
>>
>> The issue on NVIDIA reported by users, no, that flag is not used in a
>> simple open()+close() path.
>
> It is.  The flag is used in hda_codec.c.  It's a flag for setting
> FORMAT stuff, not the SPDIF status.

The only use I see there is in __snd_hda_codec_cleanup_stream(), but AFAICS
that is not called by open/close callbacks of hdmi, since the stream is
only allocated in prepare(). Am I missing something?

>> My issue on Intel, yes.
>>
>> > Actually, the current behavior is intentional.  There have been bug
>> > reports that just reopening a device causes the receiver not reacting
>> > immediately.  In the worst case, it takes a few seconds to sync. So we
>> > introduced a mechanism to keep the PCM stream assignment.
>>
>> Hmm.. If the intention is to keep a silent stream playing even after
>> device has been closed, why was PIN_OUT disabled on close until "ALSA:
>> hda - Always turn on pins for HDMI/DP" in December?
>
> The intention isn't to keep the stream playing, but it keeps the
> STREAM tag.  It's just a weird hardware implementation that it keeps
> playing.  And, the feature was implemented far before the new dynamic
> SPDIF status control assignment and the dynamic pin down.  So, it got
> broken by that stuff sometime ago.

Ah, so you mean something different with „lost sync„, I used that
interchangeably with „hdmi stream stopped„.  Can you explain the
difference, to get us on the same page? :)

I was able to see only two states on the receiver. Either the HDMI icon is
on (and any simultaneously active s/pdif is ignored by default unless
otherwise configured), or it is off (after which starting the playback
again takes approx. 0.8sec). But is there a more subtle 3rd state? (Or
maybe the behavior depends on the receiver, and my receiver handles more
situations like the stream is active).

>> Anyway, I can understand the desire to keep an empty stream running
>> after device has been closed. However, IMO a simple
>> snd_pcm_open+snd_pcm_close triggering a start of empty stream (as on the
>> affected NVIDIA hw) seems wrong, since this can happen without actual
>> audio being output by application device probing (e.g. pulseaudio) and
>> the HDMI audio supersedes actual audio, as reported by several users
>> (during beta/rc testing of OpenELEC - it is also not very obvious what
>> is causing the sudden brokenness of audio, it took quite a bit of
>> debugging before we found out the cause).
>
> Again, the intention isn't about keep stream sending.  It was a
> workaround to avoid the unnecessary out-of-sync situation at each open
> or prepare.  Otherwise you'll get a few seconds silence at each
> stream.

OK, I thought not sending stream and out-of-sync are the same thing.

>> Also, as noted, this "current behavior" doesn't actually work on the
>> affected NVIDIA 0b hardware (nor it does on my non-affected NVIDIA 0b
>> hardware); when the actual audio stream stops, sync is lost regardless
>> of the kept stream assignment.
>
> Yes, this got broken by the update sometime ago, unfortunately.
>
> And, remember that user may still use OSS emulation -- in that case,
> there are frequent open/close sequences.  So, keeping sync after close
> makes sense for such.
>
> In other words, there is no black/white solution for this problem.
> It must be somehow configurable for each use case.
>
>
>> > If your problem is solved by clearing codec->no_stick_stream, we may
>> > provide an additional mixer control to turn it on/off, for example,
>> > because we can't blindly disable/enable it globally if user's receiver
>> > has a problem with the stream sync.
>>
>> I think we definitely should make "no empty hdmi stream after close or
>> before first open" an option at least.
>>
>> However, while writing this I made some tests on my hw with NVIDIA 15
>> codec, and it seems to have a fourth different set of behavior related
>> to this.
>>
>> So we now have at least (unless I made a mistake at some point, which is
>> not too unlikely considering the amount of variables here... I can
>> retest stuff if needed):
>>
>> NVIDIA 0b non-"ION2" (rev 0x100100) hardware:
>> - HDMI output becomes active when the real audio stream starts
>>   (plus non-tested other conditions, at least AC_DIG1_ENABLE)
>> - HDMI output stops after the real audio stream stops
>> - on cold start: format is valid, AC_DIG1_ENABLE is off
>> => currently HDMI becomes active when actual audio is output, and
>>    is stopped when audio stops (e.g. even when pausing playback without
>>    closing)
>>
>> NVIDIA 0b "ION2" (rev 0x100200) hardware:
>> - HDMI output becomes active when PIN_OUT + AC_DIG1_ENABLE + valid
>>   stream format
>> - HDMI output stops after the real audio stream stops
>>   (it would follow that flipping AC_DIG1_ENABLE off and on again
>>    would start HDMI output again - however, not tested)
>> - on cold start: format is valid, AC_DIG1_ENABLE is off
>> => currently HDMI becomes active immediately at open(), and is stopped
>>    when actual audio stream stops. However, in case of open()+close(),
>>    the actual audio stream never starts and therefore never stops,
>>    leaving the HDMI audio active with empty stream
>>
>> NVIDIA 15 hardware:
>> - HDMI output becomes active when PIN_OUT + AC_DIG1_ENABLE
>>   (no valid stream format needed)
>> - HDMI output stops after the above conditions stop
>> - on cold start: format is valid, AC_DIG1_ENABLE is off
>> => currently HDMI becomes active immediately at open(),
>>    and is never stopped unless it is muted during playback
>>    (and my earlier patch would not actually change that)
>>
>> Intel PantherPoint HDMI hardware:
>> - HDMI output becomes active when AC_DIG1_ENABLE + valid stream format
>>   (regardless of PIN_OUT)
>> - HDMI output stops after the above conditions stop (or, in a strange
>>   just-trying-to-confuse-me way, when stopping a 32kHz stream for some
>>   reason)
>> - on cold start: format is invalid, AC_DIG1_ENABLE is on
>> => currently HDMI becomes active when the stream format is set for
>>    the first time, i.e. something is played back. It is never stopped
>>    unless it is muted during playback.
>>
>>
>> Note that e.g. pulseaudio opens+closes devices immediately, so on NVIDIA
>> 0b "ION2" and NVIDIA 15 cases HDMI output becomes active immediately.
>> Also note that muting the IEC958 control won't stop the HDMI output
>> unless/until the device is actually opened, since the IEC958 ctl is
>> nowadays only assigned to the device when opened.
>>
>> Now, whatever is the behavior that we want, according the above clearly
>> there is space for improvement in any case.
>>
>> For example, when exactly do we want the HDMI port to become active,
>> preferably?
>> - immediately, always on initialization
>> - on first open()
>> - on first prepare()
>>
>> And when to stop?
>> - never
>> - on close()
>> - after timeout after close()
>
> We just need to try.  I guess the setting at open() is the cleanest
> way of implementation.  The delayed cleanup sounds interesting, and if
> it can be implemented without too many mess, this looks like the best
> option for stopping.
>
>
>> As per the above test data most hardware can be made to follow any of
>> the behaviors (except that NVIDIA 0b non-ION2 cannot seemingly support
>> keeping empty HDMI stream open at all, at least per current tests), but
>> the current behavior is rather mixed.
>>
>> As you probably have gathered, I'd support HDMI port to become active at
>> prepare() (after all we don't have the stream parameters yet at open
>> time or earlier), and to become inactive at close(). I guess I can
>> settle for an option, though, if you insist :)
>>
>>
>> (There is also the power saving stuff to be considered, but I'm ignoring
>> that simply because I have no information about the effects there...)
>
> It shouldn't influence so much on the power.  The empty audio stream
> has nothing to do with the sound driver side.  The sound component can
> be even completely powered off during the empty audio stream is
> embedded into HDMI/DP frame.
>
>
> thanks,
>
> Takashi
>

-- 
Anssi Hannula

Re: [PATCH] ALSA: hda - Avoid outputting HDMI audio before prepare() and after close()

[Takashi Iwai]

At Sun, 17 Feb 2013 19:57:57 +0200,
Anssi Hannula wrote:
> 
> On Sunday, February 17, 2013, Takashi Iwai <tiwai@suse.de> wrote:
> > At Sun, 17 Feb 2013 14:23:21 +0200,
> > Anssi Hannula wrote:
> >>
> >> 17.02.2013 10:24, Takashi Iwai kirjoitti:
> >> > At Sat, 16 Feb 2013 22:59:40 +0200,
> >> > Anssi Hannula wrote:
> >> >>
> >> >> 16.02.2013 18:40, Anssi Hannula kirjoitti:
> >> >>> Some HDMI codecs (at least NVIDIA 0x10de000b:0x10de0101:0x100100)
> start
> >> >>> transmitting an empty audio stream as soon as PIN_OUT and
> AC_DIG1_ENABLE
> >> >>> are enabled.
> >> >>>
> >> >>> Since commit 6169b673618bf0b2518ce413b54925782a603f06 ("ALSA: hda -
> >> >>> Always turn on pins for HDMI/DP") this happens at first open() time,
> and
> >> >>> will continue even after close().
> >> >>>
> >> >>> Additionally, some codecs (at least Intel PantherPoint HDMI)
> currently
> >> >>> continue transmitting HDMI audio even after close() in case some
> actual
> >> >>> audio was output after open() (this happens regardless of PIN_OUT).
> >> >>>
> >> >>> Empty HDMI audio transmission when not intended has the effect that a
> >> >>> possible HDMI audio sink/receiver may prefer the empty HDMI audio
> stream
> >> >>> over an actual audio stream on its S/PDIF inputs.
> >> >>>
> >> >>> To avoid the issue before first prepare(), set stream format to 0 on
> >> >>> codec initialization. 0 is not a valid format value for HDMI and will
> >> >>> prevent the audio stream from being output.
> >> >>>
> >> >>> Additionally, at close() time, make sure that the stream is cleaned
> up.
> >> >>> This will ensure that the format is reset to 0 at that time,
> preventing
> >> >>> audio from being output in that case.
> >> >>>
> >> >>> Thanks to OpenELEC developers and users for their help in
> investigating
> >> >>> this issue on the affected NVIDIA "ION2" hardware, and for the
> initial
> >> >>> bug report of the issue. Testing of the final version on NVIDIA ION2
> was
> >> >>> done by OpenELEC user "MrXIII". Testing on Intel PantherPoint was
> done
> >> >>> by myself.
> >> >>>
> >> >>> Signed-off-by: Anssi Hannula <anssi.hannula@iki.fi>
> >> >>> Cc: stable@vger.kernel.org
> >> >>> ---
> >> >>>
> >> >>> This also supersedes the patch I attached yesterday as it fixes both
> >> >>> cases.
> >> >>>
> >> >>> I guess the alternative to this approach would be to fiddle with
> >> >>> AC_DIG1_ENABLE when preparing and closing the device, but with a
> quick
> >> >>> look that seemed to be possibly more complex since AC_DIG1_ENABLE is
> >> >>> already meddled with at quite a few places in hda_codec.c.
> >> >>
> >> >> Hmm... actually, that would not be so much more complex, just making
> >> >> sure AC_DIG1_ENABLE is off in hdmi_add_cvt() (by e.g. zeroing
> >> >> AC_VERB_SET_DIGI_CONVERT_1) and making snd_hda_spdif_ctls_unassign()
> >> >> call "set_spdif_ctls(codec, spdif->nid, 0, -1);".
> >> >>
> >> >> However, it would still mean that just a simple snd_pcm_open() could
> >> >> cause an empty stream to be output (if format is valid), since iec958
> >> >> controls are assigned at open() time, and iec958 mute controls
> >> >> AC_DIG1_ENABLE. Or we could add additional code in hda_codec.c to
> >> >> prevent AC_DIG1_ENABLE being set if prepare() has not been called.
> >> >
> >> > Is the problem fixed if you set codec->no_sticky_stream = 1?
> >>
> >> The issue on NVIDIA reported by users, no, that flag is not used in a
> >> simple open()+close() path.
> >
> > It is.  The flag is used in hda_codec.c.  It's a flag for setting
> > FORMAT stuff, not the SPDIF status.
> 
> The only use I see there is in __snd_hda_codec_cleanup_stream(), but AFAICS
> that is not called by open/close callbacks of hdmi, since the stream is
> only allocated in prepare(). Am I missing something?

It changes the behavior of cleanup.  The cleanup is implicitly called
before close via hw_free.

When cleanup callback isn't defined, snd_hda_codec_cleanup_stream() is
invoked as default.  And no_sticky_stream flag is evaluated there.
Hence, it must influence on simple_hdmi case, too.

> 
> >> My issue on Intel, yes.
> >>
> >> > Actually, the current behavior is intentional.  There have been bug
> >> > reports that just reopening a device causes the receiver not reacting
> >> > immediately.  In the worst case, it takes a few seconds to sync. So we
> >> > introduced a mechanism to keep the PCM stream assignment.
> >>
> >> Hmm.. If the intention is to keep a silent stream playing even after
> >> device has been closed, why was PIN_OUT disabled on close until "ALSA:
> >> hda - Always turn on pins for HDMI/DP" in December?
> >
> > The intention isn't to keep the stream playing, but it keeps the
> > STREAM tag.  It's just a weird hardware implementation that it keeps
> > playing.  And, the feature was implemented far before the new dynamic
> > SPDIF status control assignment and the dynamic pin down.  So, it got
> > broken by that stuff sometime ago.
> 
> Ah, so you mean something different with „lost sync„, I used that
> interchangeably with „hdmi stream stopped„.  Can you explain the
> difference, to get us on the same page? :)

Some receivers seem to keep sync even if you stop the stream as long
as the FORMAT tag isn't changed.  That's the only point of the sticky
PCM stream.  It's not intended to keep the stream running.

I'd say it's rather a bug of hardware or driver doing that.  Maybe
it's just for avoiding the out-of-sync situation.

What makes harder in the case of HDMI/DP, it's not only about the
hardware but related also with the video driver.  I won't be surprised
if different behavior is seen between the open-source and binary-only
drivers, or between different driver versions.  And, plus, the
behavior depends on the receiver, too.

So, we always need to think of three things if we debug this kind of
problems:
- HDMI audio codec
- Video driver, version
- Receiver hardware


Takashi
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] ALSA: hda - Avoid outputting HDMI audio before prepare() and after close()

[Anssi Hannula]

On 18.02.2013 13:03, Takashi Iwai wrote:
> At Sun, 17 Feb 2013 19:57:57 +0200,
> Anssi Hannula wrote:
>>
>> On Sunday, February 17, 2013, Takashi Iwai <tiwai@suse.de> wrote:
>> > At Sun, 17 Feb 2013 14:23:21 +0200,
>> > Anssi Hannula wrote:
>> >>
>> >> 17.02.2013 10:24, Takashi Iwai kirjoitti:
>> >> > At Sat, 16 Feb 2013 22:59:40 +0200,
>> >> > Anssi Hannula wrote:
>> >> >>
>> >> >> 16.02.2013 18:40, Anssi Hannula kirjoitti:
>> >> >>> Some HDMI codecs (at least NVIDIA 
>> 0x10de000b:0x10de0101:0x100100)
>> start
>> >> >>> transmitting an empty audio stream as soon as PIN_OUT and
>> AC_DIG1_ENABLE
>> >> >>> are enabled.
>> >> >>>
>> >> >>> Since commit 6169b673618bf0b2518ce413b54925782a603f06 ("ALSA: 
>> hda -
>> >> >>> Always turn on pins for HDMI/DP") this happens at first 
>> open() time,
>> and
>> >> >>> will continue even after close().
>> >> >>>
>> >> >>> Additionally, some codecs (at least Intel PantherPoint HDMI)
>> currently
>> >> >>> continue transmitting HDMI audio even after close() in case 
>> some
>> actual
>> >> >>> audio was output after open() (this happens regardless of 
>> PIN_OUT).
>> >> >>>
>> >> >>> Empty HDMI audio transmission when not intended has the 
>> effect that a
>> >> >>> possible HDMI audio sink/receiver may prefer the empty HDMI 
>> audio
>> stream
>> >> >>> over an actual audio stream on its S/PDIF inputs.
>> >> >>>
>> >> >>> To avoid the issue before first prepare(), set stream format 
>> to 0 on
>> >> >>> codec initialization. 0 is not a valid format value for HDMI 
>> and will
>> >> >>> prevent the audio stream from being output.
>> >> >>>
>> >> >>> Additionally, at close() time, make sure that the stream is 
>> cleaned
>> up.
>> >> >>> This will ensure that the format is reset to 0 at that time,
>> preventing
>> >> >>> audio from being output in that case.
>> >> >>>
>> >> >>> Thanks to OpenELEC developers and users for their help in
>> investigating
>> >> >>> this issue on the affected NVIDIA "ION2" hardware, and for 
>> the
>> initial
>> >> >>> bug report of the issue. Testing of the final version on 
>> NVIDIA ION2
>> was
>> >> >>> done by OpenELEC user "MrXIII". Testing on Intel PantherPoint 
>> was
>> done
>> >> >>> by myself.
>> >> >>>
>> >> >>> Signed-off-by: Anssi Hannula <anssi.hannula@iki.fi>
>> >> >>> Cc: stable@vger.kernel.org
>> >> >>> ---
>> >> >>>
>> >> >>> This also supersedes the patch I attached yesterday as it 
>> fixes both
>> >> >>> cases.
>> >> >>>
>> >> >>> I guess the alternative to this approach would be to fiddle 
>> with
>> >> >>> AC_DIG1_ENABLE when preparing and closing the device, but 
>> with a
>> quick
>> >> >>> look that seemed to be possibly more complex since 
>> AC_DIG1_ENABLE is
>> >> >>> already meddled with at quite a few places in hda_codec.c.
>> >> >>
>> >> >> Hmm... actually, that would not be so much more complex, just 
>> making
>> >> >> sure AC_DIG1_ENABLE is off in hdmi_add_cvt() (by e.g. zeroing
>> >> >> AC_VERB_SET_DIGI_CONVERT_1) and making 
>> snd_hda_spdif_ctls_unassign()
>> >> >> call "set_spdif_ctls(codec, spdif->nid, 0, -1);".
>> >> >>
>> >> >> However, it would still mean that just a simple snd_pcm_open() 
>> could
>> >> >> cause an empty stream to be output (if format is valid), since 
>> iec958
>> >> >> controls are assigned at open() time, and iec958 mute controls
>> >> >> AC_DIG1_ENABLE. Or we could add additional code in hda_codec.c 
>> to
>> >> >> prevent AC_DIG1_ENABLE being set if prepare() has not been 
>> called.
>> >> >
>> >> > Is the problem fixed if you set codec->no_sticky_stream = 1?
>> >>
>> >> The issue on NVIDIA reported by users, no, that flag is not used 
>> in a
>> >> simple open()+close() path.
>> >
>> > It is.  The flag is used in hda_codec.c.  It's a flag for setting
>> > FORMAT stuff, not the SPDIF status.
>>
>> The only use I see there is in __snd_hda_codec_cleanup_stream(), but 
>> AFAICS
>> that is not called by open/close callbacks of hdmi, since the stream 
>> is
>> only allocated in prepare(). Am I missing something?
>
> It changes the behavior of cleanup.  The cleanup is implicitly called
> before close via hw_free.

Ah, indeed, that's what I was missing. Somehow I thought cleanup() was 
only
called if prepare() was.

> When cleanup callback isn't defined, snd_hda_codec_cleanup_stream() 
> is
> invoked as default.  And no_sticky_stream flag is evaluated there.
> Hence, it must influence on simple_hdmi case, too.

Right (though with "simple" I meant "prepare() not called", everything 
I wrote
was about generic_hdmi).

>>
>> >> My issue on Intel, yes.
>> >>
>> >> > Actually, the current behavior is intentional.  There have been 
>> bug
>> >> > reports that just reopening a device causes the receiver not 
>> reacting
>> >> > immediately.  In the worst case, it takes a few seconds to 
>> sync. So we
>> >> > introduced a mechanism to keep the PCM stream assignment.
>> >>
>> >> Hmm.. If the intention is to keep a silent stream playing even 
>> after
>> >> device has been closed, why was PIN_OUT disabled on close until 
>> "ALSA:
>> >> hda - Always turn on pins for HDMI/DP" in December?
>> >
>> > The intention isn't to keep the stream playing, but it keeps the
>> > STREAM tag.  It's just a weird hardware implementation that it 
>> keeps
>> > playing.  And, the feature was implemented far before the new 
>> dynamic
>> > SPDIF status control assignment and the dynamic pin down.  So, it 
>> got
>> > broken by that stuff sometime ago.
>>
>> Ah, so you mean something different with „lost sync„, I used that
>> interchangeably with „hdmi stream stopped„.  Can you explain the
>> difference, to get us on the same page? :)
>
> Some receivers seem to keep sync even if you stop the stream as long
> as the FORMAT tag isn't changed.  That's the only point of the sticky
> PCM stream.  It's not intended to keep the stream running.

I wonder what is the actual difference in the actual HDMI transmission
between "stream running" and "sync kept". Any idea?

> I'd say it's rather a bug of hardware or driver doing that.  Maybe
> it's just for avoiding the out-of-sync situation.

Of course I can't tell if it is really just my receiver that handles 
your
"sync kept" state as if the stream was running.

> What makes harder in the case of HDMI/DP, it's not only about the
> hardware but related also with the video driver.  I won't be 
> surprised
> if different behavior is seen between the open-source and binary-only
> drivers, or between different driver versions.  And, plus, the
> behavior depends on the receiver, too.
>
> So, we always need to think of three things if we debug this kind of
> problems:
> - HDMI audio codec
> - Video driver, version
> - Receiver hardware

Indeed.

-- 
Anssi Hannula

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel