[Intel-gfx] [PATCH 03/11] drm/i915/psr: Share code between disable and exit

Souza, Jose jose.souza at intel.com
Mon Apr 2 21:15:14 UTC 2018


On Mon, 2018-04-02 at 11:11 -0700, Rodrigo Vivi wrote:
> On Fri, Mar 30, 2018 at 03:23:28PM -0700, José Roberto de Souza
> wrote:
> > The disable and exit sequence are very similar with a lot common
> > code between both, so here sharing the code.
> 
> I don't believe that we should do this.
> Disable as is has some extra wait/timeouts that could slow things
> down.
> 
> Or at least not do this while we don't have a proper robuts test in
> place
> and to test also performance impacts...

When the disabling argument is false it don't have the disable
wait/timeouts, I kept the same behavior to both just sharing the common
part.

> 
> > 
> > Signed-off-by: José Roberto de Souza <jose.souza at intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h  |  2 +-
> >  drivers/gpu/drm/i915/intel_psr.c | 84 ++++++++++++++++++--------
> > --------------
> >  2 files changed, 38 insertions(+), 48 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index a8d300280a2c..cb72ee27422f 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -617,7 +617,7 @@ struct i915_psr {
> >  	void (*enable_sink)(struct intel_dp *);
> >  	void (*activate)(struct intel_dp *);
> >  	void (*setup_vsc)(struct intel_dp *, const struct
> > intel_crtc_state *);
> > -	void (*exit)(struct intel_dp *intel_dp);
> > +	void (*exit)(struct intel_dp *intel_dp, bool disabling);
> >  };
> >  
> >  enum intel_pch {
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index bcaac9e69f8c..d3451afeb8bb 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -672,23 +672,9 @@ static void vlv_psr_disable(struct intel_dp
> > *intel_dp,
> >  	struct drm_device *dev = intel_dig_port->base.base.dev;
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> >  	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state-
> > >base.crtc);
> > -	uint32_t val;
> >  
> >  	if (dev_priv->psr.active) {
> > -		/* Put VLV PSR back to PSR_state 0 (disabled). */
> > -		if (intel_wait_for_register(dev_priv,
> > -					    VLV_PSRSTAT(crtc-
> > >pipe),
> > -					    VLV_EDP_PSR_IN_TRANS,
> > -					    0,
> > -					    1))
> > -			WARN(1, "PSR transition took longer than
> > expected\n");
> > -
> > -		val = I915_READ(VLV_PSRCTL(crtc->pipe));
> > -		val &= ~VLV_EDP_PSR_ACTIVE_ENTRY;
> > -		val &= ~VLV_EDP_PSR_ENABLE;
> > -		val &= ~VLV_EDP_PSR_MODE_MASK;
> > -		I915_WRITE(VLV_PSRCTL(crtc->pipe), val);
> > -
> > +		dev_priv->psr.exit(intel_dp, true);
> >  		dev_priv->psr.active = false;
> >  	} else {
> >  		WARN_ON(vlv_is_psr_active_on_pipe(dev, crtc-
> > >pipe));
> > @@ -703,31 +689,7 @@ static void hsw_psr_disable(struct intel_dp
> > *intel_dp,
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> >  
> >  	if (dev_priv->psr.active) {
> > -		i915_reg_t psr_status;
> > -		u32 psr_status_mask;
> > -
> > -		if (dev_priv->psr.psr2_enabled) {
> > -			psr_status = EDP_PSR2_STATUS;
> > -			psr_status_mask =
> > EDP_PSR2_STATUS_STATE_MASK;
> > -
> > -			I915_WRITE(EDP_PSR2_CTL,
> > -				   I915_READ(EDP_PSR2_CTL) &
> > -				   ~(EDP_PSR2_ENABLE |
> > EDP_SU_TRACK_ENABLE));
> > -
> > -		} else {
> > -			psr_status = EDP_PSR_STATUS;
> > -			psr_status_mask =
> > EDP_PSR_STATUS_STATE_MASK;
> > -
> > -			I915_WRITE(EDP_PSR_CTL,
> > -				   I915_READ(EDP_PSR_CTL) &
> > ~EDP_PSR_ENABLE);
> > -		}
> > -
> > -		/* Wait till PSR is idle */
> > -		if (intel_wait_for_register(dev_priv,
> > -					    psr_status,
> > psr_status_mask, 0,
> > -					    2000))
> > -			DRM_ERROR("Timed out waiting for PSR Idle
> > State\n");
> > -
> > +		dev_priv->psr.exit(intel_dp, true);
> >  		dev_priv->psr.active = false;
> >  	} else {
> >  		if (dev_priv->psr.psr2_enabled)
> > @@ -838,25 +800,41 @@ static void intel_psr_work(struct work_struct
> > *work)
> >  	mutex_unlock(&dev_priv->psr.lock);
> >  }
> >  
> > -static void hsw_psr_exit(struct intel_dp *intel_dp)
> > +static void hsw_psr_exit(struct intel_dp *intel_dp, bool
> > disabling)
> >  {
> >  	struct intel_digital_port *dig_port =
> > dp_to_dig_port(intel_dp);
> >  	struct drm_device *dev = dig_port->base.base.dev;
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > +	i915_reg_t psr_status;
> > +	u32 psr_status_mask;
> >  	u32 val;
> >  
> >  	if (dev_priv->psr.psr2_enabled) {
> > +		psr_status = EDP_PSR2_STATUS;
> > +		psr_status_mask = EDP_PSR2_STATUS_STATE_MASK;
> > +
> >  		val = I915_READ(EDP_PSR2_CTL);
> > -		WARN_ON(!(val & EDP_PSR2_ENABLE));
> > -		I915_WRITE(EDP_PSR2_CTL, val & ~EDP_PSR2_ENABLE);
> > +		WARN_ON(!disabling && !(val & EDP_PSR2_ENABLE));
> > +		val &= ~EDP_PSR2_ENABLE;
> > +		if (disabling)
> > +			val &= ~EDP_SU_TRACK_ENABLE;
> > +		I915_WRITE(EDP_PSR2_CTL, val);
> >  	} else {
> > +		psr_status = EDP_PSR_STATUS;
> > +		psr_status_mask = EDP_PSR_STATUS_STATE_MASK;
> > +
> >  		val = I915_READ(EDP_PSR_CTL);
> > -		WARN_ON(!(val & EDP_PSR_ENABLE));
> > +		WARN_ON(!disabling && !(val & EDP_PSR_ENABLE));
> >  		I915_WRITE(EDP_PSR_CTL, val & ~EDP_PSR_ENABLE);
> >  	}
> > +
> > +	/* When disabling wait till PSR is idle */
> > +	if (disabling && intel_wait_for_register(dev_priv,
> > psr_status,
> > +						 psr_status_mask,
> > 0, 2000))
> > +		DRM_ERROR("Timed out waiting for PSR Idle
> > State\n");
> >  }
> >  
> > -static void vlv_psr_exit(struct intel_dp *intel_dp)
> > +static void vlv_psr_exit(struct intel_dp *intel_dp, bool
> > disabling)
> >  {
> >  	struct intel_digital_port *dig_port =
> > dp_to_dig_port(intel_dp);
> >  	struct drm_device *dev = dig_port->base.base.dev;
> > @@ -865,10 +843,14 @@ static void vlv_psr_exit(struct intel_dp
> > *intel_dp)
> >  	enum pipe pipe = to_intel_crtc(crtc)->pipe;
> >  	u32 val;
> >  
> > +	if (disabling && intel_wait_for_register(dev_priv,
> > VLV_PSRSTAT(pipe),
> > +						 VLV_EDP_PSR_IN_TR
> > ANS, 0, 1))
> > +		DRM_WARN("PSR transition took longer than
> > expected\n");
> > +
> >  	val = I915_READ(VLV_PSRCTL(pipe));
> >  
> >  	/*
> > -	 * Here we do the transition drirectly from
> > +	 * Here we do the transition directly from
> >  	 * PSR_state 3 (active - no Remote Frame Buffer (RFB)
> > update) to
> >  	 * PSR_state 5 (exit).
> >  	 * PSR State 4 (active with single frame update) can be
> > skipped.
> > @@ -877,8 +859,16 @@ static void vlv_psr_exit(struct intel_dp
> > *intel_dp)
> >  	 * Now we are at Same state after vlv_psr_enable_source.
> >  	 */
> >  	val &= ~VLV_EDP_PSR_ACTIVE_ENTRY;
> > +	if (disabling) {
> > +		/* Put VLV PSR back to PSR_state 0 (disabled). */
> > +		val &= ~VLV_EDP_PSR_ENABLE;
> > +		val &= ~VLV_EDP_PSR_MODE_MASK;
> > +	}
> >  	I915_WRITE(VLV_PSRCTL(pipe), val);
> >  
> > +	if (disabling)
> > +		return;
> > +
> >  	/*
> >  	 * Send AUX wake up - Spec says after transitioning to PSR
> >  	 * active we have to send AUX wake up by writing 01h in
> > DPCD
> > @@ -898,7 +888,7 @@ static void intel_psr_exit(struct
> > drm_i915_private *dev_priv)
> >  	if (!dev_priv->psr.active)
> >  		return;
> >  
> > -	dev_priv->psr.exit(intel_dp);
> > +	dev_priv->psr.exit(intel_dp, false);
> >  	dev_priv->psr.active = false;
> >  }
> >  
> > -- 
> > 2.16.3
> > 


More information about the Intel-gfx mailing list