Re: [PATCH v2] drm/i915/gen9: Fix DMC firmware initialization

[Imre Deak <imre.deak@intel.com>]

On ma, 2016-03-07 at 13:58 +0200, Mika Kuoppala wrote:
> Imre Deak <imre.deak@intel.com> writes:
> 
> > [ text/plain ]
> > In commit 1e657ad7 we moved the last step of firmware
> > initialization to
> > skl_display_core_init(), where it will be run only during system
> > resume,
> > but not during driver loading. Since this init step needs to be
> > done
> > whenever we program the firmware fix this by moving the
> > initialization
> > to the end of intel_csr_load_program().
> > 
> > While at it simplify a bit csr_load_work_fn().
> > 
> > This issue prevented DC5/6 transitions, this change will re-enable
> > those.
> > 
> > v2:
> > - remove debugging left-over and redundant comment in
> > csr_load_work_fn()
> > 
> > Fixes: 1e657ad7a48f ("drm/i915/gen9: Write dc state debugmask bits
> > only once")
> > CC: Mika Kuoppala <mika.kuoppala@intel.com>
> > CC: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_csr.c        | 40
> > +++++++++++++++++++++------------
> >  drivers/gpu/drm/i915/intel_drv.h        |  2 +-
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 22 ++----------------
> >  3 files changed, 29 insertions(+), 35 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_csr.c
> > b/drivers/gpu/drm/i915/intel_csr.c
> > index 902054e..d417d9a 100644
> > --- a/drivers/gpu/drm/i915/intel_csr.c
> > +++ b/drivers/gpu/drm/i915/intel_csr.c
> > @@ -212,6 +212,24 @@ static const struct stepping_info
> > *intel_get_stepping_info(struct drm_device *de
> >  	return NULL;
> >  }
> >  
> > +static void gen9_set_dc_state_debugmask(struct drm_i915_private
> > *dev_priv)
> > +{
> > +	uint32_t val, mask;
> > +
> > +	mask = DC_STATE_DEBUG_MASK_MEMORY_UP;
> > +
> > +	if (IS_BROXTON(dev_priv))
> > +		mask |= DC_STATE_DEBUG_MASK_CORES;
> > +
> > +	/* The below bit doesn't need to be cleared ever
> > afterwards */
> > +	val = I915_READ(DC_STATE_DEBUG);
> > +	if ((val & mask) != mask) {
> > +		val |= mask;
> > +		I915_WRITE(DC_STATE_DEBUG, val);
> > +		POSTING_READ(DC_STATE_DEBUG);
> > +	}
> > +}
> > +
> >  /**
> >   * intel_csr_load_program() - write the firmware from memory to
> > register.
> >   * @dev_priv: i915 drm device.
> > @@ -220,19 +238,19 @@ static const struct stepping_info
> > *intel_get_stepping_info(struct drm_device *de
> >   * Everytime display comes back from low power state this function
> > is called to
> >   * copy the firmware from internal memory to registers.
> >   */
> > -bool intel_csr_load_program(struct drm_i915_private *dev_priv)
> > +void intel_csr_load_program(struct drm_i915_private *dev_priv)
> >  {
> >  	u32 *payload = dev_priv->csr.dmc_payload;
> >  	uint32_t i, fw_size;
> >  
> >  	if (!IS_GEN9(dev_priv)) {
> >  		DRM_ERROR("No CSR support available for this
> > platform\n");
> > -		return false;
> > +		return;
> >  	}
> >  
> >  	if (!dev_priv->csr.dmc_payload) {
> >  		DRM_ERROR("Tried to program CSR with empty
> > payload\n");
> > -		return false;
> > +		return;
> >  	}
> >  
> >  	fw_size = dev_priv->csr.dmc_fw_size;
> > @@ -246,7 +264,7 @@ bool intel_csr_load_program(struct
> > drm_i915_private *dev_priv)
> >  
> >  	dev_priv->csr.dc_state = 0;
> >  
> > -	return true;
> > +	gen9_set_dc_state_debugmask(dev_priv);
> >  }
> >  
> >  static uint32_t *parse_csr_fw(struct drm_i915_private *dev_priv,
> > @@ -388,18 +406,12 @@ static void csr_load_work_fn(struct
> > work_struct *work)
> >  
> >  	ret = request_firmware(&fw, dev_priv->csr.fw_path,
> >  			       &dev_priv->dev->pdev->dev);
> > -	if (!fw)
> > -		goto out;
> > +	if (fw)
> > +		dev_priv->csr.dmc_payload = parse_csr_fw(dev_priv,
> > fw);
> >  
> 
> Why we check the fw pointer here and not the return value?

Yep, I noticed this too, it was an overlook in the the original code.
Since it happens to work I didn't want to change it in this patch. But
we should fix this as a follow-up.

--Imre

> 
> -Mika
> 
> 
> > -	dev_priv->csr.dmc_payload = parse_csr_fw(dev_priv, fw);
> > -	if (!dev_priv->csr.dmc_payload)
> > -		goto out;
> > -
> > -	/* load csr program during system boot, as needed for DC
> > states */
> > -	intel_csr_load_program(dev_priv);
> > -
> > -out:
> >  	if (dev_priv->csr.dmc_payload) {
> > +		intel_csr_load_program(dev_priv);
> > +
> >  		intel_display_power_put(dev_priv,
> > POWER_DOMAIN_INIT);
> >  
> >  		DRM_INFO("Finished loading %s (v%u.%u)\n",
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index cd0b4ea..3daf1e3 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1279,7 +1279,7 @@ u32 skl_plane_ctl_rotation(unsigned int
> > rotation);
> >  
> >  /* intel_csr.c */
> >  void intel_csr_ucode_init(struct drm_i915_private *);
> > -bool intel_csr_load_program(struct drm_i915_private *);
> > +void intel_csr_load_program(struct drm_i915_private *);
> >  void intel_csr_ucode_fini(struct drm_i915_private *);
> >  
> >  /* intel_dp.c */
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 09c52b1..5adf4b3 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -470,24 +470,6 @@ static void assert_can_disable_dc9(struct
> > drm_i915_private *dev_priv)
> >  	  */
> >  }
> >  
> > -static void gen9_set_dc_state_debugmask(struct drm_i915_private
> > *dev_priv)
> > -{
> > -	uint32_t val, mask;
> > -
> > -	mask = DC_STATE_DEBUG_MASK_MEMORY_UP;
> > -
> > -	if (IS_BROXTON(dev_priv))
> > -		mask |= DC_STATE_DEBUG_MASK_CORES;
> > -
> > -	/* The below bit doesn't need to be cleared ever
> > afterwards */
> > -	val = I915_READ(DC_STATE_DEBUG);
> > -	if ((val & mask) != mask) {
> > -		val |= mask;
> > -		I915_WRITE(DC_STATE_DEBUG, val);
> > -		POSTING_READ(DC_STATE_DEBUG);
> > -	}
> > -}
> > -
> >  static void gen9_write_dc_state(struct drm_i915_private *dev_priv,
> >  				u32 state)
> >  {
> > @@ -2141,8 +2123,8 @@ static void skl_display_core_init(struct
> > drm_i915_private *dev_priv,
> >  
> >  	skl_init_cdclk(dev_priv);
> >  
> > -	if (dev_priv->csr.dmc_payload &&
> > intel_csr_load_program(dev_priv))
> > -		gen9_set_dc_state_debugmask(dev_priv);
> > +	if (dev_priv->csr.dmc_payload)
> > +		intel_csr_load_program(dev_priv);
> >  }
> >  
> >  static void skl_display_core_uninit(struct drm_i915_private
> > *dev_priv)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx