[PATCH] ALSA: hda/hdmi: Fix the converter reuse for the silent stream

[PATCH] ALSA: hda/hdmi: Fix the converter reuse for the silent stream

[Jaroslav Kysela]

When the user space pcm stream uses the silent stream converter,
it is no longer allocated for the silent stream. Clear the appropriate
flag in the hdmi_pcm_open() function. The silent stream setup may
be applied in hdmi_pcm_close() (and the error path - open fcn) again.

If the flag is not cleared, the reuse conditions for the silent
stream converter in hdmi_choose_cvt() may improperly share
this converter.

Cc: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Signed-off-by: Jaroslav Kysela <perex@perex.cz>
---
 sound/pci/hda/patch_hdmi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c
index 6c209cd26c0c..22766810635b 100644
--- a/sound/pci/hda/patch_hdmi.c
+++ b/sound/pci/hda/patch_hdmi.c
@@ -1267,6 +1267,7 @@ static int hdmi_pcm_open(struct hda_pcm_stream *hinfo,
 	set_bit(pcm_idx, &spec->pcm_in_use);
 	per_pin = get_pin(spec, pin_idx);
 	per_pin->cvt_nid = per_cvt->cvt_nid;
+	per_pin->silent_stream = false;
 	hinfo->nid = per_cvt->cvt_nid;
 
 	/* flip stripe flag for the assigned stream if supported */
-- 
2.35.3

Re: [PATCH] ALSA: hda/hdmi: Fix the converter reuse for the silent stream

[Takashi Iwai]

On Tue, 13 Sep 2022 09:02:16 +0200,
Jaroslav Kysela wrote:
> 
> When the user space pcm stream uses the silent stream converter,
> it is no longer allocated for the silent stream. Clear the appropriate
> flag in the hdmi_pcm_open() function. The silent stream setup may
> be applied in hdmi_pcm_close() (and the error path - open fcn) again.
> 
> If the flag is not cleared, the reuse conditions for the silent
> stream converter in hdmi_choose_cvt() may improperly share
> this converter.
> 
> Cc: Kai Vehmanen <kai.vehmanen@linux.intel.com>
> Signed-off-by: Jaroslav Kysela <perex@perex.cz>

Applied now.


thanks,

Takashi

Re: [PATCH] ALSA: hda/hdmi: Fix the converter reuse for the silent stream

[Kai Vehmanen]

Hi,

sorry for the late review (as Takashi already applied).

On Tue, 13 Sep 2022 09:02:16 +0200, Jaroslav Kysela wrote:
> When the user space pcm stream uses the silent stream converter,
> it is no longer allocated for the silent stream. Clear the appropriate
> flag in the hdmi_pcm_open() function. The silent stream setup may
> be applied in hdmi_pcm_close() (and the error path - open fcn) again.

Jaroslav, did you hit a real-life problem with a particular sequence? The 
converter allocation was done in a sticky fashion on purpose. The silent 
stream setup (clearing KAE and/or letting codec to power down) was only 
released if the receiver is disconnected from the system. I.e. no actions 
were done in PCM close (on purpose).

Now if there were issues, I wonder whether we should modify the pcm_close 
instead, and avoid some of the cleanup in this case, as a better fix.

I think your patch will create a new problem:
 - receiver A connected to CVT X
 - silent stream enabled for X
 - app opens a PCM, connected to A+X, silent stream disabled  
 - receiver A disconnected
 -> actions in silent_stream_disable() not run correctly for CVT X

Br, Kai

Re: [PATCH] ALSA: hda/hdmi: Fix the converter reuse for the silent stream

[Jaroslav Kysela]

On 19. 09. 22 13:33, Kai Vehmanen wrote:
> Hi,
> 
> sorry for the late review (as Takashi already applied).
> 
> On Tue, 13 Sep 2022 09:02:16 +0200, Jaroslav Kysela wrote:
>> When the user space pcm stream uses the silent stream converter, it is no
>> longer allocated for the silent stream. Clear the appropriate flag in the
>> hdmi_pcm_open() function. The silent stream setup may be applied in
>> hdmi_pcm_close() (and the error path - open fcn) again.
> 
> Jaroslav, did you hit a real-life problem with a particular sequence? The 
> converter allocation was done in a sticky fashion on purpose. The silent 
> stream setup (clearing KAE and/or letting codec to power down) was only 
> released if the receiver is disconnected from the system. I.e. no actions 
> were done in PCM close (on purpose).

Thank you for your comments.

I just wrote a small test utility which shows the bad converter use:

https://gist.github.com/perexg/4977fcb33101148e578c34bd25db8ce4

For the first use (with one monitor connected) I can open 4 HDMI PCM devices,
but later, only 3 HDMI PCM devices can be opened, because the converter
allocation code (3 converters in my case) does a wrong job.

I though that the silent stream is turned off when new S/PDIF frame parameters 
are applied, but it seems that this setup is independent as you noted.

> Now if there were issues, I wonder whether we should modify the pcm_close 
> instead, and avoid some of the cleanup in this case, as a better fix.

Yes, I think that we should add a new silent_stream flag to struct 
hdmi_spec_per_cvt and do not set assigned in this struct when the silent 
stream is managed. The assigned flag should be used only for open/close calls 
to track the used converters correctly. Does it sound right for you?

> I think your patch will create a new problem: - receiver A connected to CVT
> X - silent stream enabled for X - app opens a PCM, connected to A+X, silent
> stream disabled - receiver A disconnected -> actions in
> silent_stream_disable() not run correctly for CVT X

Definitely. The silent stream setup won't be deactivated with this simple patch.

					Jaroslav



-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.