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

[Anssi Hannula <anssi.hannula@iki.fi>]

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