[Intel-gfx] [PATCH 1/3] drm/i915/cnl+: Reload CSR firmware when coming back from low power states

[Imre Deak]

On Mon, Jul 30, 2018 at 08:23:08PM +0300, Souza, Jose wrote:
> On Sat, 2018-07-28 at 11:40 +0300, Imre Deak wrote:
> > On Fri, Jul 27, 2018 at 10:19:42PM -0700, Rodrigo Vivi wrote:
> > > On Fri, Jul 27, 2018 at 04:36:24PM -0700, José Roberto de Souza
> > > wrote:
> > > > When returning from low power states the CSR firmware was not
> > > > being
> > > > loaded again in CNL and ICL.
> > 
> > Which power states are you referring to? We should already restore
> > the
> > firmware from all relevant states in i915_drm_resume_early(). The
> > patch
> > to load the firmware on ICL is not yet merged to drm-tip.
> 
> Exactly this one, i915_drm_resume_early() calls
> intel_power_domains_init_hw().
> Yes it is still not loading becasue the firmware name is missing for
> ICL but everything else is there.

I still don't understand how this fixes things. CNL already reloads the
firmware via i915_drm_resume_early() -> cnl_display_core_init(). On
ICL the same will happen once support for loading the firmware is added.
That same patch should also add the call to intel_csr_load_program() from
icl_display_core_init().

I would prefer to keep the call to intel_csr_load_program() in
*_display_core_init() functions to keep the runtime and system suspend
paths similar.

> 
> > 
> > > > Also taking the opportunity to share the load call for gen >= 9,
> > > > instead of calling it from each display_core_init() function.
> > > > 
> > > > Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > > > Cc: Anusha Srivatsa <anusha.srivatsa at intel.com>
> > > 
> > > Cc: Imre Deak <imre.deak at intel.com>
> > > 
> > > > Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_drv.c         |  9 ++++++---
> > > >  drivers/gpu/drm/i915/intel_runtime_pm.c | 13 ++++---------
> > > >  2 files changed, 10 insertions(+), 12 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> > > > b/drivers/gpu/drm/i915/i915_drv.c
> > > > index 18a45e7a3d7c..a42f0dfe19da 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > > @@ -2699,9 +2699,12 @@ static int intel_runtime_resume(struct
> > > > device *kdev)
> > > >  	if (IS_GEN9_LP(dev_priv)) {
> > > >  		bxt_disable_dc9(dev_priv);
> > > >  		bxt_display_core_init(dev_priv, true);
> > > > -		if (dev_priv->csr.dmc_payload &&
> > > > -		    (dev_priv->csr.allowed_dc_mask &
> > > > DC_STATE_EN_UPTO_DC5))
> > > > -			gen9_enable_dc5(dev_priv);
> > > > +		if (dev_priv->csr.dmc_payload) {
> > > > +			intel_csr_load_program(dev_priv);
> > > > +			if (dev_priv->csr.allowed_dc_mask &
> > > > +			    DC_STATE_EN_UPTO_DC5)
> > > > +				gen9_enable_dc5(dev_priv);
> > > > +		}
> > > >  	} else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv)) {
> > > >  		hsw_disable_pc8(dev_priv);
> > > >  	} else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > > {
> > > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > index cf89141b2281..8fdcffe023fe 100644
> > > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > @@ -3075,9 +3075,6 @@ static void skl_display_core_init(struct
> > > > drm_i915_private *dev_priv,
> > > >  	skl_init_cdclk(dev_priv);
> > > >  
> > > >  	gen9_dbuf_enable(dev_priv);
> > > > -
> > > > -	if (resume && dev_priv->csr.dmc_payload)
> > > > -		intel_csr_load_program(dev_priv);
> > > 
> > > and resume becomes unsed, so please kill it.
> > > 
> > > >  }
> > > >  
> > > >  static void skl_display_core_uninit(struct drm_i915_private
> > > > *dev_priv)
> > > > @@ -3140,9 +3137,6 @@ void bxt_display_core_init(struct
> > > > drm_i915_private *dev_priv,
> > > >  	bxt_init_cdclk(dev_priv);
> > > >  
> > > >  	gen9_dbuf_enable(dev_priv);
> > > > -
> > > > -	if (resume && dev_priv->csr.dmc_payload)
> > > > -		intel_csr_load_program(dev_priv);
> > > 
> > > same
> > > 
> > > >  }
> > > >  
> > > >  void bxt_display_core_uninit(struct drm_i915_private *dev_priv)
> > > > @@ -3283,9 +3277,6 @@ static void cnl_display_core_init(struct
> > > > drm_i915_private *dev_priv, bool resume
> > > >  
> > > >  	/* 6. Enable DBUF */
> > > >  	gen9_dbuf_enable(dev_priv);
> > > > -
> > > > -	if (resume && dev_priv->csr.dmc_payload)
> > > > -		intel_csr_load_program(dev_priv);
> > > 
> > > same
> > > 
> > > >  }
> > > >  
> > > >  static void cnl_display_core_uninit(struct drm_i915_private
> > > > *dev_priv)
> > > > @@ -3561,6 +3552,10 @@ void intel_power_domains_init_hw(struct
> > > > drm_i915_private *dev_priv, bool resume)
> > > >  
> > > >  	/* For now, we need the power well to be always enabled. */
> > > >  	intel_display_set_init_power(dev_priv, true);
> > > > +
> > > > +	if (INTEL_GEN(dev_priv) >= 9 && resume && dev_priv-
> > > > >csr.dmc_payload)
> > > > +		intel_csr_load_program(dev_priv);
> > > > +
> > > >  	/* Disable power support if the user asked so. */
> > > >  	if (!i915_modparams.disable_power_well)
> > > >  		intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
> > > > -- 
> > > > 2.18.0
> > > > 
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx at lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx