Re: [PATCH 2/2] drm/i915/psr: Execute the default PSR code path when setting i915_edp_psr_debug

[Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>]

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.


> 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.



> 
> > 
> > > 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_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;
> > >  }
> > >  

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx