Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 28 Sep 2022 09:14:30 +0200
Message-ID: <875yh8ezs9.wl-tiwai@suse.de>
From:   Takashi Iwai <tiwai@suse.de>
To:     "Lu, Brent" <brent.lu@intel.com>
Cc:     "alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>,
        "Jaroslav Kysela" <perex@perex.cz>, Takashi Iwai <tiwai@suse.com>,
        Kai Vehmanen <kai.vehmanen@linux.intel.com>,
        Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>,
        Mohan Kumar <mkumard@nvidia.com>,
        Ville =?ISO-8859-1?Q?Syrj=E4l=E4?= 
        <ville.syrjala@linux.intel.com>, "Zhi, Yong" <yong.zhi@intel.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] ALSA: hda/hdmi: run eld notify in delay work
In-Reply-To: <CY5PR11MB6257CB33E1EDA90CE2B2F99D97549@CY5PR11MB6257.namprd11.prod.outlook.com>
References: <20220927135807.4097052-1-brent.lu@intel.com>
        <87ill8gb5c.wl-tiwai@suse.de>
        <CY5PR11MB6257CB33E1EDA90CE2B2F99D97549@CY5PR11MB6257.namprd11.prod.outlook.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) Emacs/27.2 Mule/6.0
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

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?


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