On ma, 2016-03-07 at 13:58 +0200, Mika Kuoppala wrote:
> 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?
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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
