On Tue, Sep 15, 2020 at 07:44:10PM -0700, José Roberto de Souza wrote: > Due to the debugfs flag, has_psr2 in CRTC state could have a different > value than psr.psr2_enabled and it was causing PSR2 subfeatures(DC3CO > and selective fetch) to be set to not a expected state. > > So here only taking in consideration the parameter and debugfs flag > when computing PSR state, this way the CRTC state will also have > the correct state. > > intel_psr_fastset_force() was already broken as > intel_psr_compute_config() was already only enabling PSR when > psr_global_enabled() and all other PSR requirements are met. > So some changes was required in this function, now it iterates over > all connectors, if it is a eDP connector and is active force a modeset > in the CRTC driving this connector, what will cause the new PSR state > to be set based on the debugfs flag. > > Cc: Gwan-gyeong Mun <[email protected]> > Cc: Ville Syrjälä <[email protected]> > Signed-off-by: José Roberto de Souza <[email protected]>
Looks sensible. Reviewed-by: Ville Syrjälä <[email protected]> > --- > drivers/gpu/drm/i915/display/intel_psr.c | 65 ++++++++++++++---------- > 1 file changed, 37 insertions(+), 28 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c > b/drivers/gpu/drm/i915/display/intel_psr.c > index 4e09ae61d4aa..383b66d9f2f2 100644 > --- a/drivers/gpu/drm/i915/display/intel_psr.c > +++ b/drivers/gpu/drm/i915/display/intel_psr.c > @@ -91,19 +91,14 @@ static bool psr_global_enabled(struct drm_i915_private > *i915) > } > } > > -static bool intel_psr2_enabled(struct drm_i915_private *dev_priv, > - const struct intel_crtc_state *crtc_state) > +static bool psr2_global_enabled(struct drm_i915_private *dev_priv) > { > - /* Cannot enable DSC and PSR2 simultaneously */ > - drm_WARN_ON(&dev_priv->drm, crtc_state->dsc.compression_enable && > - crtc_state->has_psr2); > - > switch (dev_priv->psr.debug & I915_PSR_DEBUG_MODE_MASK) { > case I915_PSR_DEBUG_DISABLE: > case I915_PSR_DEBUG_FORCE_PSR1: > return false; > default: > - return crtc_state->has_psr2; > + return true; > } > } > > @@ -729,6 +724,11 @@ static bool intel_psr2_config_valid(struct intel_dp > *intel_dp, > return false; > } > > + if (!psr2_global_enabled(dev_priv)) { > + drm_dbg_kms(&dev_priv->drm, "PSR2 disabled by flag\n"); > + return false; > + } > + > /* > * DSC and PSR2 cannot be enabled simultaneously. If a requested > * resolution requires DSC to be enabled, priority is given to DSC > @@ -817,8 +817,11 @@ void intel_psr_compute_config(struct intel_dp *intel_dp, > if (intel_dp != dev_priv->psr.dp) > return; > > - if (!psr_global_enabled(dev_priv)) > + if (!psr_global_enabled(dev_priv)) { > + drm_dbg_kms(&dev_priv->drm, "PSR disabled by flag\n"); > return; > + } > + > /* > * HSW spec explicitly says PSR is tied to port A. > * BDW+ platforms have a instance of PSR registers per transcoder but > @@ -959,7 +962,7 @@ static void intel_psr_enable_locked(struct > drm_i915_private *dev_priv, > > drm_WARN_ON(&dev_priv->drm, dev_priv->psr.enabled); > > - dev_priv->psr.psr2_enabled = intel_psr2_enabled(dev_priv, crtc_state); > + dev_priv->psr.psr2_enabled = crtc_state->has_psr2; > dev_priv->psr.busy_frontbuffer_bits = 0; > dev_priv->psr.pipe = to_intel_crtc(crtc_state->uapi.crtc)->pipe; > dev_priv->psr.dc3co_enabled = !!crtc_state->dc3co_exitline; > @@ -1029,15 +1032,7 @@ void intel_psr_enable(struct intel_dp *intel_dp, > drm_WARN_ON(&dev_priv->drm, dev_priv->drrs.dp); > > mutex_lock(&dev_priv->psr.lock); > - > - if (!psr_global_enabled(dev_priv)) { > - drm_dbg_kms(&dev_priv->drm, "PSR disabled by flag\n"); > - goto unlock; > - } > - > intel_psr_enable_locked(dev_priv, crtc_state, conn_state); > - > -unlock: > mutex_unlock(&dev_priv->psr.lock); > } > > @@ -1222,8 +1217,8 @@ void intel_psr_update(struct intel_dp *intel_dp, > > mutex_lock(&dev_priv->psr.lock); > > - enable = crtc_state->has_psr && psr_global_enabled(dev_priv); > - psr2_enable = intel_psr2_enabled(dev_priv, crtc_state); > + enable = crtc_state->has_psr; > + psr2_enable = crtc_state->has_psr2; > > if (enable == psr->enabled && psr2_enable == psr->psr2_enabled) { > /* Force a PSR exit when enabling CRC to avoid CRC timeouts */ > @@ -1320,10 +1315,11 @@ static bool __psr_wait_for_idle_locked(struct > drm_i915_private *dev_priv) > > static int intel_psr_fastset_force(struct drm_i915_private *dev_priv) > { > + struct drm_connector_list_iter conn_iter; > struct drm_device *dev = &dev_priv->drm; > struct drm_modeset_acquire_ctx ctx; > struct drm_atomic_state *state; > - struct intel_crtc *crtc; > + struct drm_connector *conn; > int err; > > state = drm_atomic_state_alloc(dev); > @@ -1334,21 +1330,34 @@ static int intel_psr_fastset_force(struct > drm_i915_private *dev_priv) > state->acquire_ctx = &ctx; > > retry: > - for_each_intel_crtc(dev, crtc) { > - struct intel_crtc_state *crtc_state = > - intel_atomic_get_crtc_state(state, crtc); > > + drm_connector_list_iter_begin(dev, &conn_iter); > + drm_for_each_connector_iter(conn, &conn_iter) { > + struct drm_connector_state *conn_state; > + struct drm_crtc_state *crtc_state; > + > + if (conn->connector_type != DRM_MODE_CONNECTOR_eDP) > + continue; > + > + conn_state = drm_atomic_get_connector_state(state, conn); > + if (IS_ERR(conn_state)) { > + err = PTR_ERR(conn_state); > + goto error; > + } > + > + if (!conn_state->crtc) > + continue; > + > + crtc_state = drm_atomic_get_crtc_state(state, conn_state->crtc); > if (IS_ERR(crtc_state)) { > err = PTR_ERR(crtc_state); > goto error; > } > > - if (crtc_state->hw.active && crtc_state->has_psr) { > - /* Mark mode as changed to trigger a pipe->update() */ > - crtc_state->uapi.mode_changed = true; > - break; > - } > + /* Mark mode as changed to trigger a pipe->update() */ > + crtc_state->mode_changed = true; > } > + drm_connector_list_iter_end(&conn_iter); > > err = drm_atomic_commit(state); > > -- > 2.28.0 -- Ville Syrjälä Intel _______________________________________________ Intel-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/intel-gfx
