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

["Souza, Jose" <jose.souza@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 13539 bytes --]

On Wed, 2019-02-06 at 18:56 -0800, Dhinakaran Pandiyan wrote:
> On Wed, 2019-02-06 at 13:18 -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)
> > 
> > v4: Adding back the missing call to
> > intel_psr_irq_control(Dhinakaran)
> 
> Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

pushed to dinq, thanks for the review.

> 
> > 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    | 182 ++++++++++++++++--------
> > ----
> >  5 files changed, 113 insertions(+), 93 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 0bd890c04fe4..20a49cc4a9a1 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_debug_set(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 a2293152cb6a..989d1ac72ec8 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 9ec3d00fbd19..3eba33a2f951 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -2077,9 +2077,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_debug_set(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..75c1a5deebf5 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,54 @@ 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);
> >  }
> >  
> > +/**
> > + * 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;
> > +
> > +	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 (enable == psr->enabled && psr2_enable == psr->psr2_enabled)
> > +		goto unlock;
> > +
> > +	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 +958,64 @@ 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) &&
> > +		    intel_crtc_state->has_psr) {
> > +			/* Mark mode as changed to trigger a pipe-
> > > update() */
> > +			crtc_state->mode_changed = true;
> > +			break;
> > +		}
> > +	}
> > +
> > +	err = drm_atomic_commit(state);
> >  
> > -	return false;
> > +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 err;
> >  }
> >  
> > -int intel_psr_set_debugfs_mode(struct drm_i915_private *dev_priv,
> > -			       struct drm_modeset_acquire_ctx *ctx,
> > -			       u64 val)
> > +int intel_psr_debug_set(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 +1023,19 @@ 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);
> >  
> > -	if (dev_priv->psr.prepared && enable)
> > -		intel_psr_enable_locked(dev_priv, crtc_state);
> > -
> >  	mutex_unlock(&dev_priv->psr.lock);
> > +
> > +	if (old_mode != mode)
> > +		ret = intel_psr_fastset_force(dev_priv);
> > +
> >  	return ret;
> >  }
> >  

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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