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. Disabling DRRS from IGT is duplicating the code that already do that and also not validating the default code path. > > > > 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@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_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; > > > > } > > > >
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
