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

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

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?

-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)
> -- 
> 2.5.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx