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.lankho...@linux.intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandi...@intel.com>
> Cc: Rodrigo Vivi <rodrigo.v...@intel.com>
> Signed-off-by: José Roberto de Souza <jose.so...@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;
> }
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
