Imre Deak <imre.deak@xxxxxxxxx> 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@xxxxxxxxx>
> CC: Patrik Jakobsson <patrik.jakobsson@xxxxxxxxxxxxxxx>
> Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
