On Wed, 28 Sep 2022 10:09:40 +0200, Takashi Iwai wrote:
On Wed, 28 Sep 2022 09:14:30 +0200, Takashi Iwai wrote:
On Wed, 28 Sep 2022 04:06:45 +0200, Lu, Brent wrote:
During resolution change, display driver would disable HDMI audio then enable it in a short time. There is possibility that eld notify for HDMI audio enable is called when previous runtime suspend is still running. In this case, the elf nofity just returns and not updating the status of corresponding HDMI pin/port. Here we move the eld nofity to a delay work so we don't lose it.
Signed-off-by: Brent Lu brent.lu@intel.com
We have already a dedicated per-pin work for the delayed ELD check. Can we reuse it instead of inventing yet another work? More work needs more cares, and better to avoid unless really needed (e.g. you forgot cleanup at suspend/removal in this patch).
thanks,
Takashi
Hi Takashi,
I've checked the hdmi_repoll_eld() and check_presence_and_report() function to see if we can reuse the per-pin work. I've some questions about reusing the per-pin work:
- hdmi_repoll_eld() calls snd_hda_jack_tbl_get_mst() function while check_presence_and_report() doesn't. Is it ok?
For the system with the audio component, there is no jack entry, hence this will be ignored.
- snd_hdac_i915_set_bclk() is called in intel_pin_eld_notify() function. Since it's skipped, we need to call it in the per-pin work. Need to add a flag in hdmi_spec_per_pin to indicate this situation.
Yeah, I guess this was already a bug. It implies that the set_bclk() call is missing in the suspend/resume case, too. We need to call it more consistently.
- We can schedule the per-pin work in intel_pin_eld_notify() when snd_hdac_is_in_pm() returns true but there is no guarantee the runtime suspend will finished when the per-pin
work is schedule to run.
On the second thought, we may simply proceed the notification if it's in a valid context. The only period to prohibit the update is during the suspend/resume until the ELD is updated by the resume itself. So, something like below may work instead. Could you give it a try?
A correction in the patch, it still has to check in-pm state; otherwise it won't be handled when runtime-suspended.
... and on the further consideration, I believe the best solution is to just get rid of the whole check.
It was introduced by the commit eb399d3c99d8 along with the 8ae743e82f0b that checks the suspend state. The latter is still meaningful (we should skip the bogus notification at suspend). However, the former -- the code path we're dealing with -- doesn't help much in the recent code. That fix was required because the driver probed the ELD bits via HD-audio verb at the time of the fix commit; that is, the driver had to wake up the codec for updating the ELD. OTOH, now ELD is read directly from the graphics chip without the codec wakeup. So the skip makes little sense.
The fix patch is below.
Takashi
-- 8< -- From: Takashi Iwai tiwai@suse.de Subject: [PATCH] ALSA: hda/hdmi: Don't skip notification handling during PM operation
The HDMI driver skips the notification handling from the graphics driver when the codec driver is being in the PM operation. This behavior was introduced by the commit eb399d3c99d8 ("ALSA: hda - Skip ELD notification during PM process"). This skip may cause a problem, as we may miss the ELD update when the connection/disconnection happens right at the runtime-PM operation of the audio codec.
Although this workaround was valid at that time, it's no longer true; the fix was required just because the ELD update procedure needed to wake up the audio codec, which had lead to a runtime-resume during a runtime-suspend. Meanwhile, the ELD update procedure doesn't need a codec wake up any longer since the commit 788d441a164c ("ALSA: hda - Use component ops for i915 HDMI/DP audio jack handling"); i.e. there is no much reason for skipping the notification.
Let's drop those checks for addressing the missing notification.
Signed-off-by: Takashi Iwai tiwai@suse.de --- sound/pci/hda/patch_hdmi.c | 6 ------ 1 file changed, 6 deletions(-)
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index c172640c8a41..21edf7a619f0 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -2666,9 +2666,6 @@ static void generic_acomp_pin_eld_notify(void *audio_ptr, int port, int dev_id) */ if (codec->core.dev.power.power_state.event == PM_EVENT_SUSPEND) return; - /* ditto during suspend/resume process itself */ - if (snd_hdac_is_in_pm(&codec->core)) - return;
check_presence_and_report(codec, pin_nid, dev_id); } @@ -2852,9 +2849,6 @@ static void intel_pin_eld_notify(void *audio_ptr, int port, int pipe) */ if (codec->core.dev.power.power_state.event == PM_EVENT_SUSPEND) return; - /* ditto during suspend/resume process itself */ - if (snd_hdac_is_in_pm(&codec->core)) - return;
snd_hdac_i915_set_bclk(&codec->bus->core); check_presence_and_report(codec, pin_nid, dev_id);