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?
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);