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

["Vivi, Rodrigo" <rodrigo.vivi@intel.com>]

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@intel.com>
> Cc: Wayne Boyer <wayne.boyer@intel.com>
> 
> Signed-off-by: Jim Bride <jim.bride@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;

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

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