| Message ID | 20230331142217.19791-1-tiwai@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ALSA: hda/hdmi: Preserve the previous PCM device upon re-enablement | expand |
On 31. 03. 23 16:22, Takashi Iwai wrote: > When a DRM driver turns on or off the screen with the audio > capability, it notifies the ELD to HD-audio HDMI codec driver via > component ops. HDMI codec driver, in turn, attaches or detaches the > PCM stream for the given port on the fly. > > The problem is that, since the recent code change, the HDMI driver > always treats the PCM stream assignment dynamically; this ended up the > confusion of the PCM device appearance. e.g. when a screen goes once > off and on again, it may appear on a different PCM device before the > screen-off. Although the application should treat such a change, it > doesn't seem working gracefully with the current pipewire (maybe > PulseAudio, too). > > As a workaround, this patch changes the HDMI codec driver behavior > slightly to be more consistent. Now it remembers the previous PCM > slot for the given port and try to assign to it. That is, if a port > is re-enabled, the driver tries to use the same PCM slot that was > assigned to that port previously. If it conflicts, a new slot is > searched and used like before, instead. I don't like this workaround so much, because the devices should be assigned from the first PCM device. The sound server should combine the persistent path only from the ELD information for the HDMI devices (if present). The problem may be when multiple similar monitors are connected to the machine. It would be probably nice to have an unique PCM device name for this usage with the serial number of the connected monitor (but I don't think it's in ELD? - it is in the EDID spec thought). > @@ -1399,6 +1408,7 @@ static void hdmi_detach_hda_pcm(struct hdmi_spec *spec, > idx = per_pin->pcm_idx; > per_pin->pcm_idx = -1; > per_pin->pcm = NULL; > + per_pin->prev_pcm_idx = idx; Maybe this line should be moved up before 'pcm = NULL'. The pcm_idx and prev_pcm_idx members are related. For now: Reviewed-by: Jaroslav Kysela <perex@perex.cz> Jaroslav
On Fri, 31 Mar 2023 17:18:44 +0200, Jaroslav Kysela wrote: > > On 31. 03. 23 16:22, Takashi Iwai wrote: > > When a DRM driver turns on or off the screen with the audio > > capability, it notifies the ELD to HD-audio HDMI codec driver via > > component ops. HDMI codec driver, in turn, attaches or detaches the > > PCM stream for the given port on the fly. > > > > The problem is that, since the recent code change, the HDMI driver > > always treats the PCM stream assignment dynamically; this ended up the > > confusion of the PCM device appearance. e.g. when a screen goes once > > off and on again, it may appear on a different PCM device before the > > screen-off. Although the application should treat such a change, it > > doesn't seem working gracefully with the current pipewire (maybe > > PulseAudio, too). > > > > As a workaround, this patch changes the HDMI codec driver behavior > > slightly to be more consistent. Now it remembers the previous PCM > > slot for the given port and try to assign to it. That is, if a port > > is re-enabled, the driver tries to use the same PCM slot that was > > assigned to that port previously. If it conflicts, a new slot is > > searched and used like before, instead. > > I don't like this workaround so much, because the devices should be > assigned from the first PCM device. Normally the device is assigned to the first PCM slot. And, it'll be always so, as long as you connect a single device. In this change, we don't keep the old empty slot, but just tries to use the previous slot at first. So a single device connection always leads to the first PCM slot. That said, the slot preservation becomes effective only when multiple devices are connected and they are on/off at the same time. In such a case, the device order may be changed at each screen on/off with the current code, while this patch allows keeping the order. Maybe I should emphasize about the multiple devices in the patch description. > The sound server should combine the persistent path only from the ELD > information for the HDMI devices (if present). The problem may be when > multiple similar monitors are connected to the machine. It would be > probably nice to have an unique PCM device name for this usage with > the serial number of the connected monitor (but I don't think it's in > ELD? - it is in the EDID spec thought). The name string appears actually in the dialog. But the automatic switch doesn't happen. So we need some improvement / fix. > > @@ -1399,6 +1408,7 @@ static void hdmi_detach_hda_pcm(struct hdmi_spec *spec, > > idx = per_pin->pcm_idx; > > per_pin->pcm_idx = -1; > > per_pin->pcm = NULL; > > + per_pin->prev_pcm_idx = idx; > > Maybe this line should be moved up before 'pcm = NULL'. The pcm_idx > and prev_pcm_idx members are related. OK, I don't mind either position. > For now: > > Reviewed-by: Jaroslav Kysela <perex@perex.cz> thanks, Takashi
On 31. 03. 23 17:30, Takashi Iwai wrote: > That said, the slot preservation becomes effective only when multiple > devices are connected and they are on/off at the same time. In such a > case, the device order may be changed at each screen on/off with the > current code, while this patch allows keeping the order. Maybe I > should emphasize about the multiple devices in the patch description. I though about possibility to remove a monitor completely. Then you can have a gap in the PCM device list. >> The sound server should combine the persistent path only from the ELD >> information for the HDMI devices (if present). The problem may be when >> multiple similar monitors are connected to the machine. It would be >> probably nice to have an unique PCM device name for this usage with >> the serial number of the connected monitor (but I don't think it's in >> ELD? - it is in the EDID spec thought). > > The name string appears actually in the dialog. But the automatic > switch doesn't happen. So we need some improvement / fix. The switch does not happen because PA/PW use the sink/output path which use the ALSA PCM device identification. This sink/output path also identifies the volume/port preservation. It is not ideal for the hotplug audio devices like HDMI, so we should have another way to identify those devices. The ALSA's PCM name / subdevice name fields in the info structure may be usable for this. Jaroslav
On Fri, 31 Mar 2023 17:40:46 +0200, Jaroslav Kysela wrote: > > On 31. 03. 23 17:30, Takashi Iwai wrote: > > > That said, the slot preservation becomes effective only when multiple > > devices are connected and they are on/off at the same time. In such a > > case, the device order may be changed at each screen on/off with the > > current code, while this patch allows keeping the order. Maybe I > > should emphasize about the multiple devices in the patch description. > > I though about possibility to remove a monitor completely. Then you > can have a gap in the PCM device list. Do you mean the situation where multiple monitors were connected and the system is moved afterward to a single monitor environment? Then it might be assigned to the second PCM slot, and that's the designed behavior. In other cases, as long as only a single monitor is used, it'll still be assigned to the first PCM slot even with this patch. > >> The sound server should combine the persistent path only from the ELD > >> information for the HDMI devices (if present). The problem may be when > >> multiple similar monitors are connected to the machine. It would be > >> probably nice to have an unique PCM device name for this usage with > >> the serial number of the connected monitor (but I don't think it's in > >> ELD? - it is in the EDID spec thought). > > > > The name string appears actually in the dialog. But the automatic > > switch doesn't happen. So we need some improvement / fix. > > The switch does not happen because PA/PW use the sink/output path > which use the ALSA PCM device identification. This sink/output path > also identifies the volume/port preservation. It is not ideal for the > hotplug audio devices like HDMI, so we should have another way to > identify those devices. The ALSA's PCM name / subdevice name fields in > the info structure may be usable for this. Well, at least, the jack disconnection and re-connection should be notified in that case, and I thought the switching may happen. But apparently not. Takashi
Hi, On Fri, 31 Mar 2023, Takashi Iwai wrote: > On Fri, 31 Mar 2023 17:18:44 +0200, Jaroslav Kysela wrote: > That said, the slot preservation becomes effective only when multiple > devices are connected and they are on/off at the same time. In such a > case, the device order may be changed at each screen on/off with the > current code, while this patch allows keeping the order. Maybe I > should emphasize about the multiple devices in the patch description. ... and specifically you need multiple audio enabled displays to have this problem. Anyways, even if a bit hacky, I think the patch is ok and helps with the user-experience in these cases. And if application correctly checks the ELD after each jack connection event, those applicatios will continue to work (so this patch does not encourage bad app behaviour). I think this still leaves (rare but possible) cases that need to be handled by application. E.g. if you have multiple (audio enabled) displays connected to a DP-MST dock and the monitors get turned on/off at the same time, it is possible for them to appear with different DP-MST ids (depends on the dock). I.e. the PCM may change. For these cases, the application would have to look up the correct display/receiver (the user wants to stream to) via ELD, every time there's jack connection event. Br, Kai
On 03. 04. 23 18:09, Kai Vehmanen wrote: > Hi, > > On Fri, 31 Mar 2023, Takashi Iwai wrote: > >> On Fri, 31 Mar 2023 17:18:44 +0200, Jaroslav Kysela wrote: >> That said, the slot preservation becomes effective only when multiple >> devices are connected and they are on/off at the same time. In such a >> case, the device order may be changed at each screen on/off with the >> current code, while this patch allows keeping the order. Maybe I >> should emphasize about the multiple devices in the patch description. > > ... and specifically you need multiple audio enabled displays to have > this problem. > > Anyways, even if a bit hacky, I think the patch is ok and helps with the > user-experience in these cases. And if application correctly checks the > ELD after each jack connection event, those applicatios will continue to > work (so this patch does not encourage bad app behaviour). > > I think this still leaves (rare but possible) cases that need to be > handled by application. E.g. if you have multiple (audio enabled) displays > connected to a DP-MST dock and the monitors get turned on/off at the same > time, it is possible for them to appear with different DP-MST ids (depends > on the dock). I.e. the PCM may change. For these cases, the application > would have to look up the correct display/receiver (the user wants to > stream to) via ELD, every time there's jack connection event. The question is, if ELD contains a serial number of the monitor. If there is no unique identification for similar monitors (one model), then we have no way to identify the audio devices properly. If I look to drm_edid_to_eld() function in drivers/gpu/drm/drm_edid.c I don't see any unique information to be copied for the identical monitors. Jaroslav
Hi, On Mon, 3 Apr 2023, Jaroslav Kysela wrote: > The question is, if ELD contains a serial number of the monitor. If there is > no unique identification for similar monitors (one model), then we have no way > to identify the audio devices properly. [...] > If I look to drm_edid_to_eld() function in drivers/gpu/drm/drm_edid.c I don't > see any unique information to be copied for the identical monitors. right, if you have two identical monitors, and to make things worse, you connect them via a DP-MST hub, we have a problem. ELD certainly does not have the serial info. It does have a PortID field, but that is an implementation specific field (in the HDA spec) and at least based on quick check, seem to be zero filled on most systems. Plus, existing apps (e.g. both Pulseaudio and Pipewire) only consider Monitor_Name_String in ELD. EDID does have fields for serial number and date of mfg. Not sure how widely implemented the serial number is, but I can see at least org.gnome.Mutter.DisplayConfig ([1] as used by e.g. GNOME Display Settings panel) do expose it and use it to identify displays (and e.g. remember the display layout). In practise, Takashi's patch will probably cover a large amount of cases. If you don't have a hub in between, the pin NID will be stable. If you have identical monitors, you might have to guess the right monitor on the first time, but subsequently the pin NID reuse will pick the same PCM for same monitor. With a hub in between, we are at mercy of how the DP-MST device ids are allocated (if you have monitors of same type). So indeed next step would be to expose the EDID serial number, and make that available via ALSA so that apps could link a PCM to a monitor name plus serial number. As this is not part of ELD, this is somewhat larger task (need to extend drm_audio_component.h to pass this info, need to extend the ALSA user-space interface and get updates to clients). [1] https://gitlab.gnome.org/GNOME/mutter/-/blob/main/data/dbus-interfaces/org.gnome.Mutter.DisplayConfig.xml , https://wiki.gnome.org/Initiatives/Wayland/Gaps/DisplayConfig Br, Kai
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 9ea633fe9339..90352dbc8052 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -81,6 +81,7 @@ struct hdmi_spec_per_pin { struct delayed_work work; struct hdmi_pcm *pcm; /* pointer to spec->pcm_rec[n] dynamically*/ int pcm_idx; /* which pcm is attached. -1 means no pcm is attached */ + int prev_pcm_idx; /* previously assigned pcm index */ int repoll_count; bool setup; /* the stream has been set up by prepare callback */ bool silent_stream; @@ -1380,9 +1381,17 @@ static void hdmi_attach_hda_pcm(struct hdmi_spec *spec, /* pcm already be attached to the pin */ if (per_pin->pcm) return; + /* try the previously used slot at first */ + idx = per_pin->prev_pcm_idx; + if (idx >= 0) { + if (!test_bit(idx, &spec->pcm_bitmap)) + goto found; + per_pin->prev_pcm_idx = -1; /* no longer valid, clear it */ + } idx = hdmi_find_pcm_slot(spec, per_pin); if (idx == -EBUSY) return; + found: per_pin->pcm_idx = idx; per_pin->pcm = get_hdmi_pcm(spec, idx); set_bit(idx, &spec->pcm_bitmap); @@ -1399,6 +1408,7 @@ static void hdmi_detach_hda_pcm(struct hdmi_spec *spec, idx = per_pin->pcm_idx; per_pin->pcm_idx = -1; per_pin->pcm = NULL; + per_pin->prev_pcm_idx = idx; if (idx >= 0 && idx < spec->pcm_used) clear_bit(idx, &spec->pcm_bitmap); } @@ -1924,6 +1934,7 @@ static int hdmi_add_pin(struct hda_codec *codec, hda_nid_t pin_nid) per_pin->pcm = NULL; per_pin->pcm_idx = -1; + per_pin->prev_pcm_idx = -1; per_pin->pin_nid = pin_nid; per_pin->pin_nid_idx = spec->num_nids; per_pin->dev_id = i;
When a DRM driver turns on or off the screen with the audio capability, it notifies the ELD to HD-audio HDMI codec driver via component ops. HDMI codec driver, in turn, attaches or detaches the PCM stream for the given port on the fly. The problem is that, since the recent code change, the HDMI driver always treats the PCM stream assignment dynamically; this ended up the confusion of the PCM device appearance. e.g. when a screen goes once off and on again, it may appear on a different PCM device before the screen-off. Although the application should treat such a change, it doesn't seem working gracefully with the current pipewire (maybe PulseAudio, too). As a workaround, this patch changes the HDMI codec driver behavior slightly to be more consistent. Now it remembers the previous PCM slot for the given port and try to assign to it. That is, if a port is re-enabled, the driver tries to use the same PCM slot that was assigned to that port previously. If it conflicts, a new slot is searched and used like before, instead. Fixes: ef6f5494faf6 ("ALSA: hda/hdmi: Use only dynamic PCM device allocation") Link: https://bugzilla.kernel.org/show_bug.cgi?id=217259 Signed-off-by: Takashi Iwai <tiwai@suse.de> --- sound/pci/hda/patch_hdmi.c | 11 +++++++++++ 1 file changed, 11 insertions(+)