| Message ID | 20220927135807.4097052-1-brent.lu@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | ALSA: hda/hdmi: run eld notify in delay work | expand |
On Tue, 27 Sep 2022 15:58:07 +0200, Brent Lu 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
> > > > 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: 1. hdmi_repoll_eld() calls snd_hda_jack_tbl_get_mst() function while check_presence_and_report() doesn't. Is it ok? 2. 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. 3. 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. static void hdmi_repoll_eld(struct work_struct *work) { struct hdmi_spec_per_pin *per_pin = container_of(to_delayed_work(work), struct hdmi_spec_per_pin, work); struct hda_codec *codec = per_pin->codec; struct hdmi_spec *spec = codec->spec; struct hda_jack_tbl *jack; jack = snd_hda_jack_tbl_get_mst(codec, per_pin->pin_nid, per_pin->dev_id); if (jack) jack->jack_dirty = 1; if (per_pin->repoll_count++ > 6) per_pin->repoll_count = 0; mutex_lock(&spec->pcm_lock); hdmi_present_sense(per_pin, per_pin->repoll_count); mutex_unlock(&spec->pcm_lock); } static void check_presence_and_report(struct hda_codec *codec, hda_nid_t nid, int dev_id) { struct hdmi_spec *spec = codec->spec; int pin_idx = pin_id_to_pin_index(codec, nid, dev_id); if (pin_idx < 0) return; mutex_lock(&spec->pcm_lock); hdmi_present_sense(get_pin(spec, pin_idx), 1); mutex_unlock(&spec->pcm_lock); } Regards, Brent
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: > > > > 1. 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. > > > 2. 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. > > > 3. 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. Takashi -- 8< -- --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -83,6 +83,7 @@ struct hdmi_spec_per_pin { int pcm_idx; /* which pcm is attached. -1 means no pcm is attached */ int repoll_count; bool setup; /* the stream has been set up by prepare callback */ + bool eld_update_frozen; bool silent_stream; int channels; /* current number of channels */ bool non_pcm; @@ -788,16 +789,28 @@ static void hdmi_setup_audio_infoframe(struct hda_codec *codec, static void hdmi_present_sense(struct hdmi_spec_per_pin *per_pin, int repoll); -static void check_presence_and_report(struct hda_codec *codec, hda_nid_t nid, - int dev_id) +static struct hdmi_spec_per_pin * +get_pin_from_nid(struct hda_codec *codec, hda_nid_t nid, int dev_id) { struct hdmi_spec *spec = codec->spec; int pin_idx = pin_id_to_pin_index(codec, nid, dev_id); if (pin_idx < 0) + return NULL; + return get_pin(spec, pin_idx); +} + +static void check_presence_and_report(struct hda_codec *codec, hda_nid_t nid, + int dev_id) +{ + struct hdmi_spec *spec = codec->spec; + struct hdmi_spec_per_pin *per_pin; + + per_pin = get_pin_from_nid(codec, nid, dev_id); + if (!per_pin) return; mutex_lock(&spec->pcm_lock); - hdmi_present_sense(get_pin(spec, pin_idx), 1); + hdmi_present_sense(per_pin, 1); mutex_unlock(&spec->pcm_lock); } @@ -1582,6 +1595,7 @@ static void update_eld(struct hda_codec *codec, snd_jack_report(pcm_jack, (eld->monitor_present && eld->eld_valid) ? SND_JACK_AVOUT : 0); + per_pin->eld_update_frozen = false; } /* update ELD and jack state via HD-audio verbs */ @@ -2494,6 +2508,7 @@ static int generic_hdmi_suspend(struct hda_codec *codec) for (pin_idx = 0; pin_idx < spec->num_pins; pin_idx++) { struct hdmi_spec_per_pin *per_pin = get_pin(spec, pin_idx); cancel_delayed_work_sync(&per_pin->work); + per_pin->eld_update_frozen = true; } return 0; } @@ -2656,6 +2671,7 @@ static void generic_acomp_pin_eld_notify(void *audio_ptr, int port, int dev_id) struct hda_codec *codec = audio_ptr; struct hdmi_spec *spec = codec->spec; hda_nid_t pin_nid = spec->port2pin(codec, port); + struct hdmi_spec_per_pin *per_pin; if (!pin_nid) return; @@ -2667,7 +2683,9 @@ 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)) + per_pin = get_pin_from_nid(codec, pin_nid, dev_id); + if (!per_pin || (per_pin->eld_update_frozen && + snd_hdac_is_in_pm(&codec->core))) return; check_presence_and_report(codec, pin_nid, dev_id); @@ -2841,6 +2859,7 @@ static int intel_port2pin(struct hda_codec *codec, int port) static void intel_pin_eld_notify(void *audio_ptr, int port, int pipe) { struct hda_codec *codec = audio_ptr; + struct hdmi_spec_per_pin *per_pin; int pin_nid; int dev_id = pipe; @@ -2853,7 +2872,9 @@ 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)) + per_pin = get_pin_from_nid(codec, pin_nid, dev_id); + if (!per_pin || (per_pin->eld_update_frozen && + snd_hdac_is_in_pm(&codec->core))) return; snd_hdac_i915_set_bclk(&codec->bus->core);
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: > > > > > > 1. 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. > > > > > 2. 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. > > > > > 3. 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);
diff --git a/sound/pci/hda/patch_hdmi.c b/sound/pci/hda/patch_hdmi.c index 6c209cd26c0c..a4c305ee8ff9 100644 --- a/sound/pci/hda/patch_hdmi.c +++ b/sound/pci/hda/patch_hdmi.c @@ -2907,13 +2907,21 @@ static int intel_port2pin(struct hda_codec *codec, int port) return spec->port_map[port]; } -static void intel_pin_eld_notify(void *audio_ptr, int port, int pipe) +struct pin_eld_notify { + void *audio_ptr; + int port; + int pipe; + struct delayed_work notify_work; +}; + +static void pin_eld_notify_work(struct work_struct *work) { - struct hda_codec *codec = audio_ptr; + struct pin_eld_notify *notify = container_of(work, struct pin_eld_notify, notify_work.work); + struct hda_codec *codec = notify->audio_ptr; int pin_nid; - int dev_id = pipe; + int dev_id = notify->pipe; - pin_nid = intel_port2pin(codec, port); + pin_nid = intel_port2pin(codec, notify->port); if (!pin_nid) return; /* skip notification during system suspend (but not in runtime PM); @@ -2922,13 +2930,33 @@ 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)) + if (snd_hdac_is_in_pm(&codec->core)) { + schedule_delayed_work(¬ify->notify_work, msecs_to_jiffies(10)); return; + } snd_hdac_i915_set_bclk(&codec->bus->core); check_presence_and_report(codec, pin_nid, dev_id); } +static void intel_pin_eld_notify(void *audio_ptr, int port, int pipe) +{ + struct hda_codec *codec = audio_ptr; + struct device *dev = hda_codec_dev(codec); + struct pin_eld_notify *notify; + + notify = devm_kzalloc(dev, sizeof(struct pin_eld_notify), GFP_KERNEL); + if (!notify) + return; + + notify->audio_ptr = audio_ptr; + notify->port = port; + notify->pipe = pipe; + INIT_DELAYED_WORK(¬ify->notify_work, pin_eld_notify_work); + + schedule_delayed_work(¬ify->notify_work, 0); +} + static const struct drm_audio_component_audio_ops intel_audio_ops = { .pin2port = intel_pin2port, .pin_eld_notify = intel_pin_eld_notify,
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> --- sound/pci/hda/patch_hdmi.c | 38 +++++++++++++++++++++++++++++++++----- 1 file changed, 33 insertions(+), 5 deletions(-)