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@xxxxxxxxx> > > > > 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? 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,8 @@ 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) return; check_presence_and_report(codec, pin_nid, dev_id); @@ -2841,6 +2858,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 +2871,8 @@ 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) return; snd_hdac_i915_set_bclk(&codec->bus->core);