Re: [PATCH v2 4/4] drm/i915/gen9: Fix runtime PM refcounting in case DMC firmware isn't loaded

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Apr 18, 2016 at 02:48:21PM +0300, Imre Deak wrote:
> While we disable runtime PM and with that display power well support if
> the DMC firmware isn't loaded, we still want to disable power wells
> during system suspend and driver unload. So drop/reacquire the
> corresponding power refcount during suspend/resume and driver unloading.
> This also means we have to check if DMC is not loaded and skip enabling
> DC states in the power well code.
> 
> v2:
> - Reuse intel_csr_ucode_suspend() in intel_csr_ucode_fini() instead of
>   opencoding the former. (Chris)
> - Add docbook comment to the public resume and suspend functions.
> 
> CC: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> 
> ---
> 
> As this fixes the special case when firmware loading failed with the
> result of possibly leaving power wells enabled during suspend/resume and
> driver unloading it's not for -fixes or stable.

Do we even care about this to bother fixing it? We pretty much agreed that
we need dmc to make power management work on skl, so if it doesn't work
too well over suspend/resume (soix), meh.

And your patch here does make the suspend/resume code even more fragile
and filled with special cases, so not too sure we should apply this one.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_drv.c         |  5 ++--
>  drivers/gpu/drm/i915/intel_csr.c        | 44 +++++++++++++++++++++++++++++++--
>  drivers/gpu/drm/i915/intel_drv.h        |  2 ++
>  drivers/gpu/drm/i915/intel_runtime_pm.c |  3 +++
>  4 files changed, 50 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 3b79e97..9a016f1 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -640,8 +640,7 @@ static int i915_drm_suspend(struct drm_device *dev)
>  
>  	intel_display_set_init_power(dev_priv, false);
>  
> -	if (HAS_CSR(dev_priv))
> -		flush_work(&dev_priv->csr.work);
> +	intel_csr_ucode_suspend(dev_priv);
>  
>  out:
>  	enable_rpm_wakeref_asserts(dev_priv);
> @@ -733,6 +732,8 @@ static int i915_drm_resume(struct drm_device *dev)
>  
>  	disable_rpm_wakeref_asserts(dev_priv);
>  
> +	intel_csr_ucode_resume(dev_priv);
> +
>  	mutex_lock(&dev->struct_mutex);
>  	i915_gem_restore_gtt_mappings(dev);
>  	mutex_unlock(&dev->struct_mutex);
> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
> index d57b00e..a34c23e 100644
> --- a/drivers/gpu/drm/i915/intel_csr.c
> +++ b/drivers/gpu/drm/i915/intel_csr.c
> @@ -467,10 +467,50 @@ void intel_csr_ucode_init(struct drm_i915_private *dev_priv)
>  }
>  
>  /**
> + * intel_csr_ucode_suspend() - prepare CSR firmware before system suspend
> + * @dev_priv: i915 drm device
> + *
> + * Prepare the DMC firmware before entering system suspend. This includes
> + * flushing pending work items and releasing any resources acquired during
> + * init.
> + */
> +void intel_csr_ucode_suspend(struct drm_i915_private *dev_priv)
> +{
> +	if (!HAS_CSR(dev_priv))
> +		return;
> +
> +	flush_work(&dev_priv->csr.work);
> +
> +	/* Drop the reference held in case DMC isn't loaded. */
> +	if (!dev_priv->csr.dmc_payload)
> +		intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
> +}
> +
> +/**
> + * intel_csr_ucode_resume() - init CSR firmware during system resume
> + * @dev_priv: i915 drm device
> + *
> + * Reinitialize the DMC firmware during system resume, reacquiring any
> + * resources released in intel_csr_ucode_suspend().
> + */
> +void intel_csr_ucode_resume(struct drm_i915_private *dev_priv)
> +{
> +	if (!HAS_CSR(dev_priv))
> +		return;
> +
> +	/*
> +	 * Reacquire the reference to keep RPM disabled in case DMC isn't
> +	 * loaded.
> +	 */
> +	if (!dev_priv->csr.dmc_payload)
> +		intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
> +}
> +
> +/**
>   * intel_csr_ucode_fini() - unload the CSR firmware.
>   * @dev_priv: i915 drm device.
>   *
> - * Firmmware unloading includes freeing the internal momory and reset the
> + * Firmmware unloading includes freeing the internal memory and reset the
>   * firmware loading status.
>   */
>  void intel_csr_ucode_fini(struct drm_i915_private *dev_priv)
> @@ -478,7 +518,7 @@ void intel_csr_ucode_fini(struct drm_i915_private *dev_priv)
>  	if (!HAS_CSR(dev_priv))
>  		return;
>  
> -	flush_work(&dev_priv->csr.work);
> +	intel_csr_ucode_suspend(dev_priv);
>  
>  	kfree(dev_priv->csr.dmc_payload);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 10dfe72..beed9e8 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1275,6 +1275,8 @@ u32 skl_plane_ctl_rotation(unsigned int rotation);
>  void intel_csr_ucode_init(struct drm_i915_private *);
>  void intel_csr_load_program(struct drm_i915_private *);
>  void intel_csr_ucode_fini(struct drm_i915_private *);
> +void intel_csr_ucode_suspend(struct drm_i915_private *);
> +void intel_csr_ucode_resume(struct drm_i915_private *);
>  
>  /* intel_dp.c */
>  void intel_dp_init(struct drm_device *dev, i915_reg_t output_reg, enum port port);
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 259f66f..bfa8548 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -809,6 +809,9 @@ static void gen9_dc_off_power_well_enable(struct drm_i915_private *dev_priv,
>  static void gen9_dc_off_power_well_disable(struct drm_i915_private *dev_priv,
>  					   struct i915_power_well *power_well)
>  {
> +	if (!dev_priv->csr.dmc_payload)
> +		return;
> +
>  	if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC6)
>  		skl_enable_dc6(dev_priv);
>  	else if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5)
> -- 
> 2.5.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx