RE: [PATCH] ALSA: hda/hdmi: run eld notify in delay work

From: "Lu, Brent" <brent.lu@intel.com>
To: Takashi Iwai <tiwai@suse.de>
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 Syrjälä" <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
Date: Wed, 5 Oct 2022 08:14:19 +0000	[thread overview]
Message-ID: <CY5PR11MB62571E282C37DDFBD4058D99975D9@CY5PR11MB6257.namprd11.prod.outlook.com> (raw)
In-Reply-To: <87y1u3evy6.wl-tiwai@suse.de>

> 
> ... 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.
Hi Takashi,

I've got the test result from ODM which is positive. During 60 test runs, the elf notify
running in suspend happened 10 times and the audio is normal. The patch is looking
good.

Regards,
Brent

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