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

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

On Tuesday, July 11, 2017 3:19:53 PM PDT 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 which bit is that?

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

I see a caller to intel_psr_single_frame_update() 

> 
> v2: * Do a field-wise init on EDP_PSR_MAX_SLEEP_TIME even though we
>       always set it to the max value. (Rodrigo)
>     * Rebase
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Wayne Boyer <wayne.boyer@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Jim Bride <jim.bride@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h  |  4 ++++
>  drivers/gpu/drm/i915/intel_psr.c | 21 +++++++++++++++++++--
>  2 files changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h index c712d01..3e62429 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3789,18 +3789,22 @@ enum {
>  #define   EDP_PSR_MIN_LINK_ENTRY_TIME_4_LINES	(1<<25)
>  #define   EDP_PSR_MIN_LINK_ENTRY_TIME_2_LINES	(2<<25)
>  #define   EDP_PSR_MIN_LINK_ENTRY_TIME_0_LINES	(3<<25)
> +#define   EDP_PSR_MAX_SLEEP_TIME_MASK           (0x1f<<20)
>  #define   EDP_PSR_MAX_SLEEP_TIME_SHIFT		20
>  #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 559f1ab..132987b 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -280,17 +280,32 @@ 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;
> +
> +	val &= ~EDP_PSR_MAX_SLEEP_TIME_MASK;
>  	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;

EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES is  (0<<25) 
We can just remove the two lines above, I think it's just misleading to do a 
nop after checking for Haswell.

Curious how the entry time is determined, the rest seem to come from VBT.

> 
> -	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 +315,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 +325,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