| Message ID | 20190201015958.24692-1-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] drm/i915/psr: Execute the default PSR code path when setting i915_edp_psr_debug | expand |
On Thu, 2019-01-31 at 17:59 -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) > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108341 > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > --- > 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 | 188 +++++++++++++++++--------- > -- > 5 files changed, 119 insertions(+), 93 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > b/drivers/gpu/drm/i915/i915_debugfs.c > index fa2c226fc779..766a5b4ad3d6 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_set_debugfs_mode(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 f11c66d172d3..baee581a0d5b 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 90ba5436370e..4c01decc30d3 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -2067,9 +2067,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_set_debugfs_mode(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..e970ffd7dd0d 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; Since you are removing .prepared, is there a need for psr_enable/psr_enable_locked separation? Same with disable and disable_locked? The whole thing was added before fastset to avoid modesets. > - 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,62 @@ 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); > } > > +#define PSR_MODE_STRING_GET(enabled, psr2_enabled) \ > + enabled ? (psr2_enabled ? "PSR2" : "PSR1") : "disabled" > + > +/** > + * 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; > + const char *old_mode, *new_mode; > + > + 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); This gets computed again in psr_enable_locked(). Move the assignment of dev_priv->psr.psr2_enabled outside of psr_enable_locked() ? Looking at the diff, this patch moved it down to psr_enable_locked(), why? > + > + if (psr->enabled == enable && psr2_enable == psr->psr2_enabled) nit: keeping the order consistent makes it easy to read IMO if (enable == psr->enabled && psr2_enable == psr->psr2_enabled) > + goto unlock; > + > + old_mode = PSR_MODE_STRING_GET(psr->enabled, psr- > >psr2_enabled); > + new_mode = PSR_MODE_STRING_GET(enable, psr2_enable); > + DRM_DEBUG_KMS("Updating from %s to %s", old_mode, new_mode); Both intel_psr_{enable, disable}_locked() print what's happening, doesn't look like this adds a any new information. > + > + 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 +966,63 @@ 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)) { > + /* Mark mode as changed to trigger a pipe- > >update() */ > + crtc_state->mode_changed = true; Add a flag to commit only if we found a crtc with eDP output? And perhaps, check for crtc_state->has_psr instead of eDP output? > + break; > + } > + } > + > + err = drm_atomic_commit(state); > + > +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 false; > + 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_set_debugfs_mode(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 +1030,18 @@ 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); > + mutex_unlock(&dev_priv->psr.lock); > > - if (dev_priv->psr.prepared && enable) > - intel_psr_enable_locked(dev_priv, crtc_state); > + if (old_mode != mode) What if it was the IRQ debug bit that had changed? > + ret = intel_psr_fastset_force(dev_priv); > > - mutex_unlock(&dev_priv->psr.lock); > return ret; > } >
On Tue, 2019-02-05 at 15:50 -0800, Dhinakaran Pandiyan wrote: > On Thu, 2019-01-31 at 17:59 -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) > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108341 > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > --- > > 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 | 188 +++++++++++++++++------- > > -- > > -- > > 5 files changed, 119 insertions(+), 93 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > > b/drivers/gpu/drm/i915/i915_debugfs.c > > index fa2c226fc779..766a5b4ad3d6 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_set_debugfs_mode(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 f11c66d172d3..baee581a0d5b 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 90ba5436370e..4c01decc30d3 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -2067,9 +2067,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_set_debugfs_mode(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..e970ffd7dd0d 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; > > Since you are removing .prepared, is there a need for > psr_enable/psr_enable_locked separation? Same with disable and > disable_locked? The whole thing was added before fastset to avoid > modesets. > psr_enable_locked() now will be called from psr_enable() and psr_update() so we can't drop it, the same for psr_disable_locked() it will be called from psr_disable() and psr_update(). > > > - 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,62 @@ 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); > > } > > > > +#define PSR_MODE_STRING_GET(enabled, psr2_enabled) \ > > + enabled ? (psr2_enabled ? "PSR2" : "PSR1") : "disabled" > > + > > +/** > > + * 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; > > + const char *old_mode, *new_mode; > > + > > + 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); > This gets computed again in psr_enable_locked(). Move the assignment > of > dev_priv->psr.psr2_enabled outside of psr_enable_locked() ? > > > > Looking at the diff, this patch moved it down to psr_enable_locked(), > why? I moved to make psr_enable_locked() be the one resposible to call the necessary functions and change the internal state, otherwise we would have the assignment of dev_priv->psr.psr2_enabled() in intel_psr_enable() and intel_psr_update(). In my opnion call intel_psr2_enabled() it from intel_psr_update() and then in intel_psr_enable_locked() is a very small price that we pay to make code more simple. > > > + > > + if (psr->enabled == enable && psr2_enable == psr->psr2_enabled) > nit: keeping the order consistent makes it easy to read IMO Okay > > if (enable == psr->enabled && psr2_enable == psr->psr2_enabled) > > + goto unlock; > > + > > + old_mode = PSR_MODE_STRING_GET(psr->enabled, psr- > > > psr2_enabled); > > + new_mode = PSR_MODE_STRING_GET(enable, psr2_enable); > > + DRM_DEBUG_KMS("Updating from %s to %s", old_mode, new_mode); > Both intel_psr_{enable, disable}_locked() print what's happening, > doesn't look like this adds a any new information. In the logs we have the information that PSR is being disabled but we don't know the version but anyways if you want I can drop this debug message. > > > + > > + 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 +966,63 @@ 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)) { > > + /* Mark mode as changed to trigger a pipe- > > > update() */ > > + crtc_state->mode_changed = true; > Add a flag to commit only if we found a crtc with eDP output? And > perhaps, check for crtc_state->has_psr instead of eDP output? Huum we could add the crtc_state->has_psr check but maybe there will be a case where it is set to false and a new compute() will set it to false? Anyways there is no down side in not checking it too. > > > > + break; > > + } > > + } > > + > > + err = drm_atomic_commit(state); > > + > > +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 false; > > + 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_set_debugfs_mode(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 +1030,18 @@ 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); > > + mutex_unlock(&dev_priv->psr.lock); > > > > - if (dev_priv->psr.prepared && enable) > > - intel_psr_enable_locked(dev_priv, crtc_state); > > + if (old_mode != mode) > What if it was the IRQ debug bit that had changed? Oh good catch, I probably lost it in some rebase/version, it should be: + old_mode = dev_priv->psr.debug & I915_PSR_DEBUG_MODE_MASK; dev_priv->psr.debug = val; + intel_psr_irq_control(dev_priv, dev_priv->psr.debug); > > > + ret = intel_psr_fastset_force(dev_priv); > > > > - mutex_unlock(&dev_priv->psr.lock); > > return ret; > > } > >
On Tue, 2019-02-05 at 16:23 -0800, Souza, Jose wrote: > On Tue, 2019-02-05 at 15:50 -0800, Dhinakaran Pandiyan wrote: > > On Thu, 2019-01-31 at 17:59 -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) > > > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108341 > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > > > --- > > > 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 | 188 +++++++++++++++++----- > > > -- > > > -- > > > -- > > > 5 files changed, 119 insertions(+), 93 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c > > > b/drivers/gpu/drm/i915/i915_debugfs.c > > > index fa2c226fc779..766a5b4ad3d6 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_set_debugfs_mode(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 f11c66d172d3..baee581a0d5b 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 90ba5436370e..4c01decc30d3 100644 > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > @@ -2067,9 +2067,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_set_debugfs_mode(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..e970ffd7dd0d 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; > > > > Since you are removing .prepared, is there a need for > > psr_enable/psr_enable_locked separation? Same with disable and > > disable_locked? The whole thing was added before fastset to avoid > > modesets. > > > > psr_enable_locked() now will be called from psr_enable() and > psr_update() so we can't drop it, the same for psr_disable_locked() > it > will be called from psr_disable() and psr_update(). > > > > > > - 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,62 @@ 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); > > > } > > > > > > +#define PSR_MODE_STRING_GET(enabled, psr2_enabled) \ > > > + enabled ? (psr2_enabled ? "PSR2" : "PSR1") : "disabled" > > > + > > > +/** > > > + * 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; > > > + const char *old_mode, *new_mode; > > > + > > > + 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); > > > > This gets computed again in psr_enable_locked(). Move the > > assignment > > of > > dev_priv->psr.psr2_enabled outside of psr_enable_locked() ? > > > > > > > > Looking at the diff, this patch moved it down to > > psr_enable_locked(), > > why? > > I moved to make psr_enable_locked() be the one resposible to call the > necessary functions and change the internal state, otherwise we would > have the assignment of dev_priv->psr.psr2_enabled() in > intel_psr_enable() and intel_psr_update(). > In my opnion call intel_psr2_enabled() it from intel_psr_update() and > then in intel_psr_enable_locked() is a very small price that we pay > to > make code more simple. > > > > > > + > > > + if (psr->enabled == enable && psr2_enable == psr->psr2_enabled) > > > > nit: keeping the order consistent makes it easy to read IMO > > Okay > > > > > if (enable == psr->enabled && psr2_enable == psr->psr2_enabled) > > > + goto unlock; > > > + > > > + old_mode = PSR_MODE_STRING_GET(psr->enabled, psr- > > > > psr2_enabled); > > > > > > + new_mode = PSR_MODE_STRING_GET(enable, psr2_enable); > > > + DRM_DEBUG_KMS("Updating from %s to %s", old_mode, new_mode); > > > > Both intel_psr_{enable, disable}_locked() print what's happening, > > doesn't look like this adds a any new information. > > In the logs we have the information that PSR is being disabled but we > don't know the version but anyways if you want I can drop this debug > message. I see this in psr_disable_locked() DRM_DEBUG_KMS("Disabling PSR%s\n", dev_priv->psr.psr2_enabled ? "2" : "1"); > > > > > > + > > > + 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 +966,63 @@ 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)) { > > > + /* Mark mode as changed to trigger a pipe- > > > > update() */ > > > > > > + crtc_state->mode_changed = true; > > > > Add a flag to commit only if we found a crtc with eDP output? And > > perhaps, check for crtc_state->has_psr instead of eDP output? > > Huum we could add the crtc_state->has_psr check but maybe there will > be > a case where it is set to false and a new compute() will set it to > false? I can't think of a case, and whatever is responsible for changing crtc_state->has_psr would be doing a separate modeset iiuc > Anyways there is no down side in not checking it too. > > > > > > > > + break; > > > + } > > > + } > > > + > > > + err = drm_atomic_commit(state); > > > + > > > +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 false; > > > + 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_set_debugfs_mode(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 +1030,18 @@ 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); > > > + mutex_unlock(&dev_priv->psr.lock); > > > > > > - if (dev_priv->psr.prepared && enable) > > > - intel_psr_enable_locked(dev_priv, crtc_state); > > > + if (old_mode != mode) > > > > What if it was the IRQ debug bit that had changed? > > Oh good catch, I probably lost it in some rebase/version, it should > be: > > + old_mode = dev_priv->psr.debug & I915_PSR_DEBUG_MODE_MASK; > dev_priv->psr.debug = val; > + intel_psr_irq_control(dev_priv, dev_priv->psr.debug); > > > > > > + ret = intel_psr_fastset_force(dev_priv); > > > > > > - mutex_unlock(&dev_priv->psr.lock); > > > return ret; > > > } > > >
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index fa2c226fc779..766a5b4ad3d6 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_set_debugfs_mode(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 f11c66d172d3..baee581a0d5b 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 90ba5436370e..4c01decc30d3 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -2067,9 +2067,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_set_debugfs_mode(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..e970ffd7dd0d 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,62 @@ 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); } +#define PSR_MODE_STRING_GET(enabled, psr2_enabled) \ + enabled ? (psr2_enabled ? "PSR2" : "PSR1") : "disabled" + +/** + * 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; + const char *old_mode, *new_mode; + + 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 (psr->enabled == enable && psr2_enable == psr->psr2_enabled) + goto unlock; + + old_mode = PSR_MODE_STRING_GET(psr->enabled, psr->psr2_enabled); + new_mode = PSR_MODE_STRING_GET(enable, psr2_enable); + DRM_DEBUG_KMS("Updating from %s to %s", old_mode, new_mode); + + 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 +966,63 @@ 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)) { + /* Mark mode as changed to trigger a pipe->update() */ + crtc_state->mode_changed = true; + break; + } + } + + err = drm_atomic_commit(state); + +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 false; + 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_set_debugfs_mode(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 +1030,18 @@ 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); + mutex_unlock(&dev_priv->psr.lock); - if (dev_priv->psr.prepared && enable) - intel_psr_enable_locked(dev_priv, crtc_state); + if (old_mode != mode) + ret = intel_psr_fastset_force(dev_priv); - mutex_unlock(&dev_priv->psr.lock); return ret; }
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) Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108341 Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Signed-off-by: José Roberto de Souza <jose.souza@intel.com> --- 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 | 188 +++++++++++++++++----------- 5 files changed, 119 insertions(+), 93 deletions(-)