[Intel-gfx] [PATCH] drm/i915/psr: Clean-up intel_enable_source_psr1()

[Jim Bride]

On Mon, Apr 03, 2017 at 05:42:39PM +0000, Vivi, Rodrigo wrote:
> On Mon, 2017-04-03 at 10:07 -0700, Jim Bride wrote:
> > On SKL+ there is a bit in SRD_CTL that software is not supposed to
> > modify, but we currently clobber that bit when we enable PSR.  In
> > order to preserve the value of that bit, go ahead and read SRD_CTL and
> > do a field-wise setting of the various bits that we need to initialize
> > before writing the register back out.  Additionally, go ahead and
> > explicitly disable single-frame update since we aren't currently
> > supporting it.
> > 
> > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > Cc: Wayne Boyer <wayne.boyer at intel.com>
> > 
> > Signed-off-by: Jim Bride <jim.bride at linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h  |  3 +++
> >  drivers/gpu/drm/i915/intel_psr.c | 23 +++++++++++++++++++++--
> >  2 files changed, 24 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 11b12f4..54d39e4 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -3590,14 +3590,17 @@ enum {
> >  #define   EDP_PSR_SKIP_AUX_EXIT			(1<<12)
> >  #define   EDP_PSR_TP1_TP2_SEL			(0<<11)
> >  #define   EDP_PSR_TP1_TP3_SEL			(1<<11)
> > +#define   EDP_PSR_TP2_TP3_TIME_MASK             (3<<8)
> >  #define   EDP_PSR_TP2_TP3_TIME_500us		(0<<8)
> >  #define   EDP_PSR_TP2_TP3_TIME_100us		(1<<8)
> >  #define   EDP_PSR_TP2_TP3_TIME_2500us		(2<<8)
> >  #define   EDP_PSR_TP2_TP3_TIME_0us		(3<<8)
> > +#define   EDP_PSR_TP1_TIME_MASK                 (0x3<<4)
> >  #define   EDP_PSR_TP1_TIME_500us		(0<<4)
> >  #define   EDP_PSR_TP1_TIME_100us		(1<<4)
> >  #define   EDP_PSR_TP1_TIME_2500us		(2<<4)
> >  #define   EDP_PSR_TP1_TIME_0us			(3<<4)
> > +#define   EDP_PSR_IDLE_FRAME_MASK               (0xf<<0)
> >  #define   EDP_PSR_IDLE_FRAME_SHIFT		0
> >  
> >  #define EDP_PSR_AUX_CTL				_MMIO(dev_priv->psr_mmio_base + 0x10)
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > index c3780d0..a050859 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -280,17 +280,34 @@ static void intel_enable_source_psr1(struct intel_dp *intel_dp)
> >  	 * with the 5 or 6 idle patterns.
> >  	 */
> >  	uint32_t idle_frames = max(6, dev_priv->vbt.psr.idle_frames);
> > -	uint32_t val = EDP_PSR_ENABLE;
> > +	uint32_t val = I915_READ(EDP_PSR_CTL);
> >  
> > +	val |= EDP_PSR_ENABLE;
> > +
> > +	/* We always set the max sleep time to the maximum value, so
> > +	 * no need to zero out the field first.
> > +	 */
> 
> I believe it is better to zero out instead of adding a comment.
> So we could play with max_sleep_time if needed.
> 
> Otherwise we shouldn't allow the flexible value here so we should create
> a define EDP_PSR_MAX_SLEEP_TIME (0x1f << 20)
> and here do a val |= EDP_PSR_MAX_SLEEP_TIME;

That's fair.  I'll wait a bit in case there's further comments, and then
spin a new version without said comment and with zeroing out the field.

Jim


> >  	val |= max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT;
> > +
> > +	val &= ~EDP_PSR_IDLE_FRAME_MASK;
> >  	val |= idle_frames << EDP_PSR_IDLE_FRAME_SHIFT;
> >  
> > +	val &= ~EDP_PSR_MIN_LINK_ENTRY_TIME_MASK;
> >  	if (IS_HASWELL(dev_priv))
> >  		val |= EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES;
> >  
> > -	if (dev_priv->psr.link_standby)
> > +	if (dev_priv->psr.link_standby) {
> >  		val |= EDP_PSR_LINK_STANDBY;
> >  
> > +		/* SFU should only be enabled with link standby, but for
> > +		 * now we do not support it. */
> > +		val &= ~BDW_PSR_SINGLE_FRAME;
> > +	} else {
> > +		val &= ~EDP_PSR_LINK_STANDBY;
> > +		val &= ~BDW_PSR_SINGLE_FRAME;
> > +	}
> > +
> > +	val &= ~EDP_PSR_TP1_TIME_MASK;
> >  	if (dev_priv->vbt.psr.tp1_wakeup_time > 5)
> >  		val |= EDP_PSR_TP1_TIME_2500us;
> >  	else if (dev_priv->vbt.psr.tp1_wakeup_time > 1)
> > @@ -300,6 +317,7 @@ static void intel_enable_source_psr1(struct intel_dp *intel_dp)
> >  	else
> >  		val |= EDP_PSR_TP1_TIME_0us;
> >  
> > +	val &= ~EDP_PSR_TP2_TP3_TIME_MASK;
> >  	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 5)
> >  		val |= EDP_PSR_TP2_TP3_TIME_2500us;
> >  	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 1)
> > @@ -309,6 +327,7 @@ static void intel_enable_source_psr1(struct intel_dp *intel_dp)
> >  	else
> >  		val |= EDP_PSR_TP2_TP3_TIME_0us;
> >  
> > +	val &= ~EDP_PSR_TP1_TP3_SEL;
> >  	if (intel_dp_source_supports_hbr2(intel_dp) &&
> >  	    drm_dp_tps3_supported(intel_dp->dpcd))
> >  		val |= EDP_PSR_TP1_TP3_SEL;
>