| Message ID | 20220913070216.3233974-1-perex@perex.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ALSA: hda/hdmi: Fix the converter reuse for the silent stream | expand |
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
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
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
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 */
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(+)