[Intel-gfx] [PATCH 2/2] drm/i915/psr: Execute the default PSR code path when setting i915_edp_psr_debug
Souza, Jose
jose.souza at intel.com
Tue Dec 11 18:16:29 UTC 2018
On Tue, 2018-12-04 at 13:23 -0800, Dhinakaran Pandiyan wrote:
> On Tue, 2018-12-04 at 10:52 -0800, Souza, Jose wrote:
> > On Mon, 2018-12-03 at 18:58 -0800, Dhinakaran Pandiyan wrote:
> > > On Mon, 2018-12-03 at 17:54 -0800, Souza, Jose wrote:
> > > > On Mon, 2018-12-03 at 17:33 -0800, Dhinakaran Pandiyan wrote:
> > > > > On Thu, 2018-11-29 at 18:31 -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
> > > > > > is
> > > > > > executed in a regular modesets.
> > > > > >
> > > > > > So lets force a modeset in the PSR CRTC to trigger the
> > > > > > requested
> > > > > > PSR
> > > > > > state change and really stress the code path that matters
> > > > > > for
> > > > > > the
> > > > > > regular user.
> > > > > >
> > > > > > Also by doing this way it fixes the issue below, were DRRS
> > > > > > was
> > > > > > left
> > > > > > enabled together with PSR when enabling PSR from debugfs.
> > > > >
> > > > > While this patch does fix the issue, psr_compute_config() not
> > > > > checking
> > > > > crtc_state->has_drrs seems odd. We should change it to not
> > > > > set
> > > > > crtc_state->has_psr if crtc_state->has_drrs happens to be
> > > > > set.
> > > > > Or
> > > > > do
> > > > > it
> > > > > the other way around.
> > > >
> > > > psr_compute_config() is not called when enabling PSR from
> > > > debugfs,
> > > > this
> > >
> > > Right. My suggestion is to allow either ->has_drrs or ->has_psr
> > > being
> > > set (not both) in the kernel and disable DRRS in the IGT before
> > > starting the test.
> >
> > So in case were PSR is disabled by parameter and DRRS is supported
> > we
> > would not enable DRRS? Because has_psr is set even if PSR is
> > disabled.
> Set ->has_psr = true in psr_compute_config() only if the module
> parameter and debugfs mode allow it. That is how the code worked
> earlier. Given that this patch duplicates the atomic state and runs
> through all state checks, we can move back to the earlier way of
> completing all checks in psr_compute_config().
>
> > Disabling DRRS from IGT is duplicating the code that already do
> > that
> > and also not validating the default code path.
> Call drrs_compute_config() after psr_compute_config(), don't set
> has_drrs if has_psr is set.
What about add a flag to skip modeset so when running IGT tests we set
that flag and PSR mode will be changed in the next modeset, what is
already done after every write to i915_edp_psr_debug in IGT tests? This
way we remove the code duplication and only stress the default code
path.
Also plus the changes in has_drrs that you mentioned but in other
patch.
>
> > >
> > > > issue was reported when PSR1 was not enabled by default so DRRS
> > > > was
> > > > being enabled by default but not PSR and when PSR was enabled
> > > > from
> > > > debugfs both were kept enabled.
> > > >
> > > > In the first version of this patch I was just disabling DRRS
> > > > when
> > > > enabling PSR but Maarten suggested to not do so and instead
> > > > stress
> > > > the
> > > > default code path.
> > >
> > > This patch changes crtc_state->mode_changed to force a modeset,
> > > which
> > > means it is not exactly the same as the 'default' code path.
> >
> > Some drm and i915 code paths also set mode_changed and the result
> > of
> > this is exactly the same as do a full modeset.
> >
> > >
> > >
> > > > > > Bugzilla:
> > > > > > https://bugs.freedesktop.org/show_bug.cgi?id=108341
> > > > > > Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> > > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> > > > > > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > > > > > Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> > > > > > ---
> > > > > > drivers/gpu/drm/i915/i915_debugfs.c | 14 +---
> > > > > > drivers/gpu/drm/i915/i915_drv.h | 2 +-
> > > > > > drivers/gpu/drm/i915/intel_drv.h | 4 +-
> > > > > > drivers/gpu/drm/i915/intel_psr.c | 119 ++++++++++++----
> > > > > > ----
> > > > > > ----
> > > > > > ----
> > > > > > 4 files changed, 55 insertions(+), 84 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > > > > > b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > > > index 129b9a6f8309..bc32f683dc46 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > > > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > > > @@ -2775,7 +2775,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;
> > > > > > int ret;
> > > > > >
> > > > > > if (!CAN_PSR(dev_priv))
> > > > > > @@ -2785,18 +2784,7 @@ i915_edp_psr_debug_set(void *data,
> > > > > > u64
> > > > > > val)
> > > > > >
> > > > > > 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);
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > > > > b/drivers/gpu/drm/i915/i915_drv.h
> > > > > > index 43ac6873a2bb..93d8ca9c0375 100644
> > > > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > > > @@ -493,7 +493,7 @@ struct i915_psr {
> > > > > >
> > > > > > u32 debug;
> > > > > > bool sink_support;
> > > > > > - bool prepared, enabled;
> > > > > > + bool enabled;
> > > > > > struct intel_dp *dp;
> > > > > > bool active;
> > > > > > struct work_struct work;
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > > > index 86ff57738cd9..89d4eed0dd89 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > > > @@ -2056,9 +2056,7 @@ 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);
> > > > > > +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 827b8c31783d..305848cceda7 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > > > @@ -52,6 +52,7 @@
> > > > > > */
> > > > > >
> > > > > > #include <drm/drmP.h>
> > > > > > +#include <drm/drm_atomic_helper.h>
> > > > > >
> > > > > > #include "intel_drv.h"
> > > > > > #include "i915_drv.h"
> > > > > > @@ -711,10 +712,6 @@ 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");
> > > > > > - goto unlock;
> > > > > > - }
> > > > > >
> > > > > > if (!psr_global_enabled(dev_priv->psr.debug)) {
> > > > > > DRM_DEBUG_KMS("PSR disabled by flag\n");
> > > > > > @@ -723,7 +720,6 @@ void intel_psr_enable(struct intel_dp
> > > > > > *intel_dp,
> > > > > >
> > > > > > dev_priv->psr.psr2_enabled =
> > > > > > intel_psr2_enabled(dev_priv,
> > > > > > crtc_state);
> > > > > > dev_priv->psr.busy_frontbuffer_bits = 0;
> > > > > > - dev_priv->psr.prepared = true;
> > > > > > intel_psr_enable_locked(dev_priv, crtc_state);
> > > > > >
> > > > > > unlock:
> > > > > > @@ -807,14 +803,9 @@ 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);
> > > > > > }
> > > > > > @@ -883,36 +874,61 @@ 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_modeset_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_state *crtc_state;
> > > > > > + struct drm_crtc *crtc;
> > > > > > + int err, i;
> > > > > > +
> > > > > > + mutex_lock(&dev->mode_config.mutex);
> > > > > > + drm_modeset_acquire_init(&ctx, 0);
> > > > > > +
> > > > > > +modeset_lock_retry:
> > > > > > + err = drm_modeset_lock_all_ctx(dev, &ctx);
> > > > > > + if (err) {
> > > > > > + if (err == -EDEADLK) {
> > > > > > + err = drm_modeset_backoff(&ctx);
> > > > > > + if (!err)
> > > > > > + goto modeset_lock_retry;
> > > > > > + }
> > > > > >
> > > > > > - if (dev_priv->psr.psr2_enabled && mode ==
> > > > > > I915_PSR_DEBUG_FORCE_PSR1)
> > > > > > - return true;
> > > > > > + goto unlock;
> > > > > > + }
> > > > > >
> > > > > > - if (!dev_priv->psr.psr2_enabled && mode !=
> > > > > > I915_PSR_DEBUG_FORCE_PSR1)
> > > > > > - return true;
> > > > > > + state = drm_atomic_helper_duplicate_state(dev, &ctx);
> > > > > > + if (IS_ERR(state)) {
> > > > > > + err = PTR_ERR(state);
> > > > > > + goto unlock;
> > > > > > + }
> > > > > > +
> > > > > > + for_each_new_crtc_in_state(state, crtc, crtc_state, i)
> > > > > > {
> > > > > > + struct intel_crtc_state *pipe_state;
> > > > > > +
> > > > > > + pipe_state = to_intel_crtc_state(crtc_state);
> > > > > > + /* Mark mode as changed to force a modeset */
> > > > > > + if (pipe_state->has_psr)
> > > > > > + crtc_state->mode_changed = true;
> > > > > > + }
> > > > > > +
> > > > > > + err = drm_atomic_helper_commit_duplicated_state(state,
> > > > > > &ctx);
> > > > > >
> > > > > > - return false;
> > > > > > + drm_atomic_state_put(state);
> > > > > > +unlock:
> > > > > > + drm_modeset_drop_locks(&ctx);
> > > > > > + drm_modeset_acquire_fini(&ctx);
> > > > > > + mutex_unlock(&dev->mode_config.mutex);
> > > > > > +
> > > > > > + 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) {
> > > > > > @@ -920,49 +936,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_modeset_force(dev_priv);
> > > > > >
> > > > > > - mutex_unlock(&dev_priv->psr.lock);
> > > > > > return ret;
> > > > > > }
> > > > > >
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: This is a digitally signed message part
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20181211/e0ce29f9/attachment.sig>
More information about the Intel-gfx
mailing list