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

From: Jaroslav Kysela <perex@perex.cz>
To: Kai Vehmanen <kai.vehmanen@linux.intel.com>
Cc: Takashi Iwai <tiwai@suse.de>,
	ALSA development <alsa-devel@alsa-project.org>
Subject: Re: [PATCH] ALSA: hda/hdmi: Fix the converter reuse for the silent stream
Date: Mon, 19 Sep 2022 15:17:23 +0200	[thread overview]
Message-ID: <a846e337-fd8c-f2bd-431f-348900b6f382@perex.cz> (raw)
In-Reply-To: <alpine.DEB.2.22.394.2209191430310.2722275@eliteleevi.tm.intel.com>

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.