Re: [Intel-gfx] [PATCH v2 22/22] drm/i915/audio: Resume HSW/BDW HDA controller around ELD access

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Oct 12, 2022 at 01:49:36PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> On HSW/BDW the hardware ELD buffer does not work if the controller
> is suspended. I'm not 100% which thing in there is needed to make it
> work (at least just forcing the controller into D0 with setpci is
> not enough). But a full runtime resume seems to do the trick here
> at least, and so far it looks like this doesn't even deadlock or
> anything.

So this apparently works for evrything else except module reload,
where the ELD buffer isn't ready by the time we do the first modeset.
Strangely the same thing works fine at boot time when we first load
the drivers. Not sure what the difference is here.

I also tried to disable runtime pm for all the hda stuff via sysfs
(echo on > power/control) before unloading the modules, but that
doesn't help either. And I didn't immediately spot any explicit
stuff to power things off when unloading the hda driver. But maybe
I missed something?

> 
> Cc: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com>
> Cc: Kai Vehmanen <kai.vehmanen@linux.intel.com>
> Cc: Takashi Iwai <tiwai@suse.de>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_audio.c | 38 ++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_audio.c b/drivers/gpu/drm/i915/display/intel_audio.c
> index 328c47719fd8..467f3f260969 100644
> --- a/drivers/gpu/drm/i915/display/intel_audio.c
> +++ b/drivers/gpu/drm/i915/display/intel_audio.c
> @@ -23,6 +23,7 @@
>  
>  #include <linux/component.h>
>  #include <linux/kernel.h>
> +#include <linux/pm_runtime.h>
>  
>  #include <drm/drm_edid.h>
>  #include <drm/i915_component.h>
> @@ -480,6 +481,16 @@ hsw_audio_config_update(struct intel_encoder *encoder,
>  		hsw_hdmi_audio_config_update(encoder, crtc_state);
>  }
>  
> +static struct pci_dev *hsw_hda_controller(struct drm_i915_private *i915)
> +{
> +	int domain = pci_domain_nr(to_pci_dev(i915->drm.dev)->bus);
> +
> +	if (!IS_HASWELL(i915) && !IS_BROADWELL(i915))
> +		return NULL;
> +
> +	return pci_get_domain_bus_and_slot(domain, 0, PCI_DEVFN(3, 0));
> +}
> +
>  static int hsw_eld_buffer_size(struct drm_i915_private *i915,
>  			       enum transcoder cpu_transcoder)
>  {
> @@ -497,8 +508,14 @@ static void hsw_audio_codec_get_config(struct intel_encoder *encoder,
>  	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
>  	u32 *eld = (u32 *)crtc_state->eld;
>  	int eld_buffer_size, len, i;
> +	struct pci_dev *hsw_hdac;
>  	u32 tmp;
>  
> +	/* on HSW/BDW ELD access doesn't work if the HDA controller is supended */
> +	hsw_hdac = hsw_hda_controller(i915);
> +	if (hsw_hdac)
> +		pm_runtime_get_sync(&hsw_hdac->dev);
> +
>  	tmp = intel_de_read(i915, HSW_AUD_PIN_ELD_CP_VLD);
>  	if ((tmp & AUDIO_ELD_VALID(cpu_transcoder)) == 0)
>  		return;
> @@ -511,6 +528,9 @@ static void hsw_audio_codec_get_config(struct intel_encoder *encoder,
>  
>  	for (i = 0; i < len; i++)
>  		eld[i] = intel_de_read(i915, HSW_AUD_EDID_DATA(cpu_transcoder));
> +
> +	if (hsw_hdac)
> +		pm_runtime_put(&hsw_hdac->dev);
>  }
>  
>  static void hsw_audio_codec_disable(struct intel_encoder *encoder,
> @@ -520,6 +540,12 @@ static void hsw_audio_codec_disable(struct intel_encoder *encoder,
>  	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
>  	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->uapi.crtc);
>  	enum transcoder cpu_transcoder = old_crtc_state->cpu_transcoder;
> +	struct pci_dev *hsw_hdac;
> +
> +	/* on HSW/BDW ELD access doesn't work if the HDA controller is supended */
> +	hsw_hdac = hsw_hda_controller(i915);
> +	if (hsw_hdac)
> +		pm_runtime_get_sync(&hsw_hdac->dev);
>  
>  	mutex_lock(&i915->display.audio.mutex);
>  
> @@ -544,6 +570,9 @@ static void hsw_audio_codec_disable(struct intel_encoder *encoder,
>  		     AUDIO_OUTPUT_ENABLE(cpu_transcoder), 0);
>  
>  	mutex_unlock(&i915->display.audio.mutex);
> +
> +	if (hsw_hdac)
> +		pm_runtime_put(&hsw_hdac->dev);
>  }
>  
>  static unsigned int calc_hblank_early_prog(struct intel_encoder *encoder,
> @@ -664,6 +693,12 @@ static void hsw_audio_codec_enable(struct intel_encoder *encoder,
>  	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
>  	const u32 *eld = (const u32 *)crtc_state->eld;
>  	int eld_buffer_size, len, i;
> +	struct pci_dev *hsw_hdac;
> +
> +	/* on HSW/BDW ELD access doesn't work if the HDA controller is supended */
> +	hsw_hdac = hsw_hda_controller(i915);
> +	if (hsw_hdac)
> +		pm_runtime_get_sync(&hsw_hdac->dev);
>  
>  	mutex_lock(&i915->display.audio.mutex);
>  
> @@ -705,6 +740,9 @@ static void hsw_audio_codec_enable(struct intel_encoder *encoder,
>  	hsw_audio_config_update(encoder, crtc_state);
>  
>  	mutex_unlock(&i915->display.audio.mutex);
> +
> +	if (hsw_hdac)
> +		pm_runtime_put(&hsw_hdac->dev);
>  }
>  
>  struct ilk_audio_regs {
> -- 
> 2.35.1

-- 
Ville Syrjälä
Intel