On Wed, 2019-02-06 at 18:56 -0800, Dhinakaran Pandiyan wrote: > On Wed, 2019-02-06 at 13:18 -0800, José Roberto de Souza wrote: > > Changing the i915_edp_psr_debug was enabling, disabling or > > switching > > PSR version by directly calling intel_psr_disable_locked() and > > intel_psr_enable_locked(), what is not the default PSR path that > > will > > be executed by real users. > > > > So lets force a fastset in the PSR CRTC to trigger a pipe update > > and > > stress the default code path. > > > > Recently a bug was found when switching from PSR2 to PSR1 while > > enable_psr kernel parameter was set to the default parameter, this > > changes fix it and also fixes the bug linked bellow were DRRS was > > left enabled together with PSR when enabling PSR from debugfs. > > > > v2: Handling missing case: disabled to PSR1 > > > > v3: Not duplicating the whole atomic state(Maarten) > > > > v4: Adding back the missing call to > > intel_psr_irq_control(Dhinakaran) > > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx> pushed to dinq, thanks for the review. > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108341 > > Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx> > > Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> > > Signed-off-by: José Roberto de Souza <jose.souza@xxxxxxxxx> > > --- > > drivers/gpu/drm/i915/i915_debugfs.c | 14 +-- > > drivers/gpu/drm/i915/i915_drv.h | 2 +- > > drivers/gpu/drm/i915/intel_ddi.c | 2 +- > > drivers/gpu/drm/i915/intel_drv.h | 6 +- > > drivers/gpu/drm/i915/intel_psr.c | 182 ++++++++++++++++-------- > > ---- > > 5 files changed, 113 insertions(+), 93 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > > b/drivers/gpu/drm/i915/i915_debugfs.c > > index 0bd890c04fe4..20a49cc4a9a1 100644 > > --- a/drivers/gpu/drm/i915/i915_debugfs.c > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > > @@ -2607,7 +2607,6 @@ static int > > i915_edp_psr_debug_set(void *data, u64 val) > > { > > struct drm_i915_private *dev_priv = data; > > - struct drm_modeset_acquire_ctx ctx; > > intel_wakeref_t wakeref; > > int ret; > > > > @@ -2618,18 +2617,7 @@ i915_edp_psr_debug_set(void *data, u64 val) > > > > wakeref = intel_runtime_pm_get(dev_priv); > > > > - drm_modeset_acquire_init(&ctx, > > DRM_MODESET_ACQUIRE_INTERRUPTIBLE); > > - > > -retry: > > - ret = intel_psr_set_debugfs_mode(dev_priv, &ctx, val); > > - if (ret == -EDEADLK) { > > - ret = drm_modeset_backoff(&ctx); > > - if (!ret) > > - goto retry; > > - } > > - > > - drm_modeset_drop_locks(&ctx); > > - drm_modeset_acquire_fini(&ctx); > > + ret = intel_psr_debug_set(dev_priv, val); > > > > intel_runtime_pm_put(dev_priv, wakeref); > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > b/drivers/gpu/drm/i915/i915_drv.h > > index a2293152cb6a..989d1ac72ec8 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -496,7 +496,7 @@ struct i915_psr { > > > > u32 debug; > > bool sink_support; > > - bool prepared, enabled; > > + bool enabled; > > struct intel_dp *dp; > > enum pipe pipe; > > bool active; > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c > > b/drivers/gpu/drm/i915/intel_ddi.c > > index ca705546a0ab..9211e4579489 100644 > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > @@ -3556,7 +3556,7 @@ static void intel_ddi_update_pipe_dp(struct > > intel_encoder *encoder, > > { > > struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); > > > > - intel_psr_enable(intel_dp, crtc_state); > > + intel_psr_update(intel_dp, crtc_state); > > intel_edp_drrs_enable(intel_dp, crtc_state); > > > > intel_panel_update_backlight(encoder, crtc_state, conn_state); > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > b/drivers/gpu/drm/i915/intel_drv.h > > index 9ec3d00fbd19..3eba33a2f951 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -2077,9 +2077,9 @@ void intel_psr_enable(struct intel_dp > > *intel_dp, > > const struct intel_crtc_state *crtc_state); > > void intel_psr_disable(struct intel_dp *intel_dp, > > const struct intel_crtc_state *old_crtc_state); > > -int intel_psr_set_debugfs_mode(struct drm_i915_private *dev_priv, > > - struct drm_modeset_acquire_ctx *ctx, > > - u64 value); > > +void intel_psr_update(struct intel_dp *intel_dp, > > + const struct intel_crtc_state *crtc_state); > > +int intel_psr_debug_set(struct drm_i915_private *dev_priv, u64 > > value); > > void intel_psr_invalidate(struct drm_i915_private *dev_priv, > > unsigned frontbuffer_bits, > > enum fb_op_origin origin); > > diff --git a/drivers/gpu/drm/i915/intel_psr.c > > b/drivers/gpu/drm/i915/intel_psr.c > > index 84a0fb981561..75c1a5deebf5 100644 > > --- a/drivers/gpu/drm/i915/intel_psr.c > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > @@ -51,6 +51,8 @@ > > * must be correctly synchronized/cancelled when shutting down the > > pipe." > > */ > > > > +#include <drm/drmP.h> > > +#include <drm/drm_atomic_helper.h> > > > > #include "intel_drv.h" > > #include "i915_drv.h" > > @@ -718,8 +720,11 @@ static void intel_psr_enable_locked(struct > > drm_i915_private *dev_priv, > > { > > struct intel_dp *intel_dp = dev_priv->psr.dp; > > > > - if (dev_priv->psr.enabled) > > - return; > > + WARN_ON(dev_priv->psr.enabled); > > + > > + dev_priv->psr.psr2_enabled = intel_psr2_enabled(dev_priv, > > crtc_state); > > + dev_priv->psr.busy_frontbuffer_bits = 0; > > + dev_priv->psr.pipe = to_intel_crtc(crtc_state->base.crtc)- > > > pipe; > > > > DRM_DEBUG_KMS("Enabling PSR%s\n", > > dev_priv->psr.psr2_enabled ? "2" : "1"); > > @@ -752,20 +757,13 @@ void intel_psr_enable(struct intel_dp > > *intel_dp, > > WARN_ON(dev_priv->drrs.dp); > > > > mutex_lock(&dev_priv->psr.lock); > > - if (dev_priv->psr.prepared) { > > - DRM_DEBUG_KMS("PSR already in use\n"); > > + > > + if (!psr_global_enabled(dev_priv->psr.debug)) { > > + DRM_DEBUG_KMS("PSR disabled by flag\n"); > > goto unlock; > > } > > > > - dev_priv->psr.psr2_enabled = intel_psr2_enabled(dev_priv, > > crtc_state); > > - dev_priv->psr.busy_frontbuffer_bits = 0; > > - dev_priv->psr.prepared = true; > > - dev_priv->psr.pipe = to_intel_crtc(crtc_state->base.crtc)- > > > pipe; > > - > > - if (psr_global_enabled(dev_priv->psr.debug)) > > - intel_psr_enable_locked(dev_priv, crtc_state); > > - else > > - DRM_DEBUG_KMS("PSR disabled by flag\n"); > > + intel_psr_enable_locked(dev_priv, crtc_state); > > > > unlock: > > mutex_unlock(&dev_priv->psr.lock); > > @@ -848,18 +846,54 @@ void intel_psr_disable(struct intel_dp > > *intel_dp, > > return; > > > > mutex_lock(&dev_priv->psr.lock); > > - if (!dev_priv->psr.prepared) { > > - mutex_unlock(&dev_priv->psr.lock); > > - return; > > - } > > > > intel_psr_disable_locked(intel_dp); > > > > - dev_priv->psr.prepared = false; > > mutex_unlock(&dev_priv->psr.lock); > > cancel_work_sync(&dev_priv->psr.work); > > } > > > > +/** > > + * intel_psr_update - Update PSR state > > + * @intel_dp: Intel DP > > + * @crtc_state: new CRTC state > > + * > > + * This functions will update PSR states, disabling, enabling or > > switching PSR > > + * version when executing fastsets. For full modeset, > > intel_psr_disable() and > > + * intel_psr_enable() should be called instead. > > + */ > > +void intel_psr_update(struct intel_dp *intel_dp, > > + const struct intel_crtc_state *crtc_state) > > +{ > > + struct drm_i915_private *dev_priv = dp_to_i915(intel_dp); > > + struct i915_psr *psr = &dev_priv->psr; > > + bool enable, psr2_enable; > > + > > + if (!CAN_PSR(dev_priv) || READ_ONCE(psr->dp) != intel_dp) > > + return; > > + > > + mutex_lock(&dev_priv->psr.lock); > > + > > + enable = crtc_state->has_psr && psr_global_enabled(psr->debug); > > + psr2_enable = intel_psr2_enabled(dev_priv, crtc_state); > > + > > + if (enable == psr->enabled && psr2_enable == psr->psr2_enabled) > > + goto unlock; > > + > > + if (psr->enabled) { > > + if (!enable || psr2_enable != psr->psr2_enabled) > > + intel_psr_disable_locked(intel_dp); > > + } > > + > > + if (enable) { > > + if (!psr->enabled || psr2_enable != psr->psr2_enabled) > > + intel_psr_enable_locked(dev_priv, crtc_state); > > + } > > + > > +unlock: > > + mutex_unlock(&dev_priv->psr.lock); > > +} > > + > > /** > > * intel_psr_wait_for_idle - wait for PSR1 to idle > > * @new_crtc_state: new CRTC state > > @@ -924,36 +958,64 @@ static bool __psr_wait_for_idle_locked(struct > > drm_i915_private *dev_priv) > > return err == 0 && dev_priv->psr.enabled; > > } > > > > -static bool switching_psr(struct drm_i915_private *dev_priv, > > - struct intel_crtc_state *crtc_state, > > - u32 mode) > > +static int intel_psr_fastset_force(struct drm_i915_private > > *dev_priv) > > { > > - /* Can't switch psr state anyway if PSR2 is not supported. */ > > - if (!crtc_state || !crtc_state->has_psr2) > > - return false; > > + struct drm_device *dev = &dev_priv->drm; > > + struct drm_modeset_acquire_ctx ctx; > > + struct drm_atomic_state *state; > > + struct drm_crtc *crtc; > > + int err; > > > > - if (dev_priv->psr.psr2_enabled && mode == > > I915_PSR_DEBUG_FORCE_PSR1) > > - return true; > > + state = drm_atomic_state_alloc(dev); > > + if (!state) > > + return -ENOMEM; > > > > - if (!dev_priv->psr.psr2_enabled && mode != > > I915_PSR_DEBUG_FORCE_PSR1) > > - return true; > > + drm_modeset_acquire_init(&ctx, > > DRM_MODESET_ACQUIRE_INTERRUPTIBLE); > > + state->acquire_ctx = &ctx; > > + > > +retry: > > + drm_for_each_crtc(crtc, dev) { > > + struct drm_crtc_state *crtc_state; > > + struct intel_crtc_state *intel_crtc_state; > > + > > + crtc_state = drm_atomic_get_crtc_state(state, crtc); > > + if (IS_ERR(crtc_state)) { > > + err = PTR_ERR(crtc_state); > > + goto error; > > + } > > + > > + intel_crtc_state = to_intel_crtc_state(crtc_state); > > + > > + if (intel_crtc_has_type(intel_crtc_state, > > INTEL_OUTPUT_EDP) && > > + intel_crtc_state->has_psr) { > > + /* Mark mode as changed to trigger a pipe- > > > update() */ > > + crtc_state->mode_changed = true; > > + break; > > + } > > + } > > + > > + err = drm_atomic_commit(state); > > > > - return false; > > +error: > > + if (err == -EDEADLK) { > > + drm_atomic_state_clear(state); > > + err = drm_modeset_backoff(&ctx); > > + if (!err) > > + goto retry; > > + } > > + > > + drm_modeset_drop_locks(&ctx); > > + drm_modeset_acquire_fini(&ctx); > > + drm_atomic_state_put(state); > > + > > + return err; > > } > > > > -int intel_psr_set_debugfs_mode(struct drm_i915_private *dev_priv, > > - struct drm_modeset_acquire_ctx *ctx, > > - u64 val) > > +int intel_psr_debug_set(struct drm_i915_private *dev_priv, u64 > > val) > > { > > - struct drm_device *dev = &dev_priv->drm; > > - struct drm_connector_state *conn_state; > > - struct intel_crtc_state *crtc_state = NULL; > > - struct drm_crtc_commit *commit; > > - struct drm_crtc *crtc; > > - struct intel_dp *dp; > > + const u32 mode = val & I915_PSR_DEBUG_MODE_MASK; > > + u32 old_mode; > > int ret; > > - bool enable; > > - u32 mode = val & I915_PSR_DEBUG_MODE_MASK; > > > > if (val & ~(I915_PSR_DEBUG_IRQ | I915_PSR_DEBUG_MODE_MASK) || > > mode > I915_PSR_DEBUG_FORCE_PSR1) { > > @@ -961,49 +1023,19 @@ int intel_psr_set_debugfs_mode(struct > > drm_i915_private *dev_priv, > > return -EINVAL; > > } > > > > - ret = drm_modeset_lock(&dev->mode_config.connection_mutex, > > ctx); > > - if (ret) > > - return ret; > > - > > - /* dev_priv->psr.dp should be set once and then never touched > > again. */ > > - dp = READ_ONCE(dev_priv->psr.dp); > > - conn_state = dp->attached_connector->base.state; > > - crtc = conn_state->crtc; > > - if (crtc) { > > - ret = drm_modeset_lock(&crtc->mutex, ctx); > > - if (ret) > > - return ret; > > - > > - crtc_state = to_intel_crtc_state(crtc->state); > > - commit = crtc_state->base.commit; > > - } else { > > - commit = conn_state->commit; > > - } > > - if (commit) { > > - ret = wait_for_completion_interruptible(&commit- > > > hw_done); > > - if (ret) > > - return ret; > > - } > > - > > ret = mutex_lock_interruptible(&dev_priv->psr.lock); > > if (ret) > > return ret; > > > > - enable = psr_global_enabled(val); > > - > > - if (!enable || switching_psr(dev_priv, crtc_state, mode)) > > - intel_psr_disable_locked(dev_priv->psr.dp); > > - > > + old_mode = dev_priv->psr.debug & I915_PSR_DEBUG_MODE_MASK; > > dev_priv->psr.debug = val; > > - if (crtc) > > - dev_priv->psr.psr2_enabled = > > intel_psr2_enabled(dev_priv, crtc_state); > > - > > intel_psr_irq_control(dev_priv, dev_priv->psr.debug); > > > > - if (dev_priv->psr.prepared && enable) > > - intel_psr_enable_locked(dev_priv, crtc_state); > > - > > mutex_unlock(&dev_priv->psr.lock); > > + > > + if (old_mode != mode) > > + ret = intel_psr_fastset_force(dev_priv); > > + > > return ret; > > } > >
Attachment:
signature.asc
Description: This is a digitally signed message part
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
