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 10:37:21 +0200
Message-ID: <87y1u3evy6.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: <871qrvgbsr.wl-tiwai@suse.de>
References: <20220927135807.4097052-1-brent.lu@intel.com>
        <87ill8gb5c.wl-tiwai@suse.de>
        <CY5PR11MB6257CB33E1EDA90CE2B2F99D97549@CY5PR11MB6257.namprd11.prod.outlook.com>
        <875yh8ezs9.wl-tiwai@suse.de>
        <871qrvgbsr.wl-tiwai@suse.de>
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 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);
-- 
2.35.3