[7/9] drm/i915/psr: Rename PSR2 macros to better match meaning

Submitted by José Roberto de Souza on Nov. 27, 2018, 12:37 a.m.

Details

Message ID 20181127003710.18618-7-jose.souza@intel.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in DRI devel

Not browsing as part of any series.

Commit Message

José Roberto de Souza Nov. 27, 2018, 12:37 a.m.
The first 8 bits of PSR2_CTL have 2 fields to set frames count, the
first one is to set how many idle frames PSR2 HW needs to wait before
enter in deep sleep and the second one it is how many frames(it don't
need to be idle frames) PSR2 HW will wait before start the PSR
activation sequence.
The previous names was really misleading and caused wrong values being
set so better rename to make it clear.

Also taking the oportunity to improve those macros.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h  | 35 ++++++++++++++++----------------
 drivers/gpu/drm/i915/intel_psr.c |  7 ++++---
 2 files changed, 22 insertions(+), 20 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 47baf2fe8f71..73046bb9ec7c 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4203,23 +4203,24 @@  enum {
 #define   EDP_PSR_DEBUG_MASK_DISP_REG_WRITE    (1 << 16) /* Reserved in ICL+ */
 #define   EDP_PSR_DEBUG_EXIT_ON_PIXEL_UNDERRUN (1 << 15) /* SKL+ */
 
-#define EDP_PSR2_CTL			_MMIO(0x6f900)
-#define   EDP_PSR2_ENABLE		(1 << 31)
-#define   EDP_SU_TRACK_ENABLE		(1 << 30)
-#define   EDP_Y_COORDINATE_VALID	(1 << 26) /* GLK and CNL+ */
-#define   EDP_Y_COORDINATE_ENABLE	(1 << 25) /* GLK and CNL+ */
-#define   EDP_MAX_SU_DISABLE_TIME(t)	((t) << 20)
-#define   EDP_MAX_SU_DISABLE_TIME_MASK	(0x1f << 20)
-#define   EDP_PSR2_TP2_TIME_500us	(0 << 8)
-#define   EDP_PSR2_TP2_TIME_100us	(1 << 8)
-#define   EDP_PSR2_TP2_TIME_2500us	(2 << 8)
-#define   EDP_PSR2_TP2_TIME_50us	(3 << 8)
-#define   EDP_PSR2_TP2_TIME_MASK	(3 << 8)
-#define   EDP_PSR2_FRAME_BEFORE_SU_SHIFT 4
-#define   EDP_PSR2_FRAME_BEFORE_SU_MASK	(0xf << 4)
-#define   EDP_PSR2_FRAME_BEFORE_SU(a)	((a) << 4)
-#define   EDP_PSR2_IDLE_FRAME_MASK	0xf
-#define   EDP_PSR2_IDLE_FRAME_SHIFT	0
+#define EDP_PSR2_CTL					_MMIO(0x6f900)
+#define   EDP_PSR2_ENABLE				(1 << 31)
+#define   EDP_SU_TRACK_ENABLE				(1 << 30)
+#define   EDP_Y_COORDINATE_VALID			(1 << 26) /* GLK and CNL+ */
+#define   EDP_Y_COORDINATE_ENABLE			(1 << 25) /* GLK and CNL+ */
+#define   EDP_MAX_SU_DISABLE_TIME(t)			((t) << 20)
+#define   EDP_MAX_SU_DISABLE_TIME_MASK			(0x1f << 20)
+#define   EDP_PSR2_TP2_TIME_500us			(0 << 8)
+#define   EDP_PSR2_TP2_TIME_100us			(1 << 8)
+#define   EDP_PSR2_TP2_TIME_2500us			(2 << 8)
+#define   EDP_PSR2_TP2_TIME_50us			(3 << 8)
+#define   EDP_PSR2_TP2_TIME_MASK			(3 << 8)
+#define   EDP_PSR2_FRAMES_BEFORE_ACTIVATE_SHIFT		(4)
+#define   EDP_PSR2_FRAMES_BEFORE_ACTIVATE_MASK		(0xf << EDP_PSR2_FRAMES_BEFORE_ACTIVATE_SHIFT)
+#define   EDP_PSR2_FRAMES_BEFORE_ACTIVATE(n)		(((n) << EDP_PSR2_FRAMES_BEFORE_ACTIVATE_SHIFT) & EDP_PSR2_FRAMES_BEFORE_ACTIVATE_MASK)
+#define   EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP_MASK	(0xf)
+#define   EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP_SHIFT	(0)
+#define   EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP(n)		(((n) << EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP_SHIFT) & EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP_MASK)
 
 #define _PSR_EVENT_TRANS_A			0x60848
 #define _PSR_EVENT_TRANS_B			0x61848
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 9215c9052381..ba7bbe3f8df2 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -479,6 +479,7 @@  static void hsw_activate_psr1(struct intel_dp *intel_dp)
 static void hsw_activate_psr2(struct intel_dp *intel_dp)
 {
 	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
+	struct i915_psr *psr = &dev_priv->psr;
 	u32 val;
 
 	/* Let's use 6 as the minimum to cover all known cases including the
@@ -486,8 +487,8 @@  static void hsw_activate_psr2(struct intel_dp *intel_dp)
 	 */
 	int idle_frames = max(6, dev_priv->vbt.psr.idle_frames);
 
-	idle_frames = max(idle_frames, dev_priv->psr.sink_sync_latency + 1);
-	val = idle_frames << EDP_PSR2_IDLE_FRAME_SHIFT;
+	idle_frames = max(idle_frames, psr->sink_sync_latency + 1);
+	val = EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP(idle_frames);
 
 	/* FIXME: selective update is probably totally broken because it doesn't
 	 * mesh at all with our frontbuffer tracking. And the hw alone isn't
@@ -496,7 +497,7 @@  static void hsw_activate_psr2(struct intel_dp *intel_dp)
 	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
 		val |= EDP_Y_COORDINATE_ENABLE;
 
-	val |= EDP_PSR2_FRAME_BEFORE_SU(dev_priv->psr.sink_sync_latency + 1);
+	val |= EDP_PSR2_FRAMES_BEFORE_ACTIVATE(psr->sink_sync_latency + 1);
 
 	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us >= 0 &&
 	    dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 50)

Comments

On Mon, Nov 26, 2018 at 04:37:08PM -0800, José Roberto de Souza wrote:
> The first 8 bits of PSR2_CTL have 2 fields to set frames count, the
> first one is to set how many idle frames PSR2 HW needs to wait before
> enter in deep sleep and the second one it is how many frames(it don't
> need to be idle frames) PSR2 HW will wait before start the PSR
> activation sequence.
> The previous names was really misleading and caused wrong values being
> set so better rename to make it clear.

I honestly prefer the old names for 2 reasons:

- they are shorter
- they follow the exact name we have on spec

> 
> Also taking the oportunity to improve those macros.
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h  | 35 ++++++++++++++++----------------
>  drivers/gpu/drm/i915/intel_psr.c |  7 ++++---
>  2 files changed, 22 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 47baf2fe8f71..73046bb9ec7c 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4203,23 +4203,24 @@ enum {
>  #define   EDP_PSR_DEBUG_MASK_DISP_REG_WRITE    (1 << 16) /* Reserved in ICL+ */
>  #define   EDP_PSR_DEBUG_EXIT_ON_PIXEL_UNDERRUN (1 << 15) /* SKL+ */
>  
> -#define EDP_PSR2_CTL			_MMIO(0x6f900)
> -#define   EDP_PSR2_ENABLE		(1 << 31)
> -#define   EDP_SU_TRACK_ENABLE		(1 << 30)
> -#define   EDP_Y_COORDINATE_VALID	(1 << 26) /* GLK and CNL+ */
> -#define   EDP_Y_COORDINATE_ENABLE	(1 << 25) /* GLK and CNL+ */
> -#define   EDP_MAX_SU_DISABLE_TIME(t)	((t) << 20)
> -#define   EDP_MAX_SU_DISABLE_TIME_MASK	(0x1f << 20)
> -#define   EDP_PSR2_TP2_TIME_500us	(0 << 8)
> -#define   EDP_PSR2_TP2_TIME_100us	(1 << 8)
> -#define   EDP_PSR2_TP2_TIME_2500us	(2 << 8)
> -#define   EDP_PSR2_TP2_TIME_50us	(3 << 8)
> -#define   EDP_PSR2_TP2_TIME_MASK	(3 << 8)
> -#define   EDP_PSR2_FRAME_BEFORE_SU_SHIFT 4
> -#define   EDP_PSR2_FRAME_BEFORE_SU_MASK	(0xf << 4)
> -#define   EDP_PSR2_FRAME_BEFORE_SU(a)	((a) << 4)
> -#define   EDP_PSR2_IDLE_FRAME_MASK	0xf
> -#define   EDP_PSR2_IDLE_FRAME_SHIFT	0
> +#define EDP_PSR2_CTL					_MMIO(0x6f900)
> +#define   EDP_PSR2_ENABLE				(1 << 31)
> +#define   EDP_SU_TRACK_ENABLE				(1 << 30)
> +#define   EDP_Y_COORDINATE_VALID			(1 << 26) /* GLK and CNL+ */
> +#define   EDP_Y_COORDINATE_ENABLE			(1 << 25) /* GLK and CNL+ */
> +#define   EDP_MAX_SU_DISABLE_TIME(t)			((t) << 20)
> +#define   EDP_MAX_SU_DISABLE_TIME_MASK			(0x1f << 20)
> +#define   EDP_PSR2_TP2_TIME_500us			(0 << 8)
> +#define   EDP_PSR2_TP2_TIME_100us			(1 << 8)
> +#define   EDP_PSR2_TP2_TIME_2500us			(2 << 8)
> +#define   EDP_PSR2_TP2_TIME_50us			(3 << 8)
> +#define   EDP_PSR2_TP2_TIME_MASK			(3 << 8)
> +#define   EDP_PSR2_FRAMES_BEFORE_ACTIVATE_SHIFT		(4)
> +#define   EDP_PSR2_FRAMES_BEFORE_ACTIVATE_MASK		(0xf << EDP_PSR2_FRAMES_BEFORE_ACTIVATE_SHIFT)
> +#define   EDP_PSR2_FRAMES_BEFORE_ACTIVATE(n)		(((n) << EDP_PSR2_FRAMES_BEFORE_ACTIVATE_SHIFT) & EDP_PSR2_FRAMES_BEFORE_ACTIVATE_MASK)
> +#define   EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP_MASK	(0xf)
> +#define   EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP_SHIFT	(0)
> +#define   EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP(n)		(((n) << EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP_SHIFT) & EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP_MASK)
>  
>  #define _PSR_EVENT_TRANS_A			0x60848
>  #define _PSR_EVENT_TRANS_B			0x61848
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 9215c9052381..ba7bbe3f8df2 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -479,6 +479,7 @@ static void hsw_activate_psr1(struct intel_dp *intel_dp)
>  static void hsw_activate_psr2(struct intel_dp *intel_dp)
>  {
>  	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> +	struct i915_psr *psr = &dev_priv->psr;
>  	u32 val;
>  
>  	/* Let's use 6 as the minimum to cover all known cases including the
> @@ -486,8 +487,8 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
>  	 */
>  	int idle_frames = max(6, dev_priv->vbt.psr.idle_frames);
>  
> -	idle_frames = max(idle_frames, dev_priv->psr.sink_sync_latency + 1);
> -	val = idle_frames << EDP_PSR2_IDLE_FRAME_SHIFT;
> +	idle_frames = max(idle_frames, psr->sink_sync_latency + 1);
> +	val = EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP(idle_frames);
>  
>  	/* FIXME: selective update is probably totally broken because it doesn't
>  	 * mesh at all with our frontbuffer tracking. And the hw alone isn't
> @@ -496,7 +497,7 @@ static void hsw_activate_psr2(struct intel_dp *intel_dp)
>  	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
>  		val |= EDP_Y_COORDINATE_ENABLE;
>  
> -	val |= EDP_PSR2_FRAME_BEFORE_SU(dev_priv->psr.sink_sync_latency + 1);
> +	val |= EDP_PSR2_FRAMES_BEFORE_ACTIVATE(psr->sink_sync_latency + 1);
>  
>  	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us >= 0 &&
>  	    dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 50)
> -- 
> 2.19.2
>
On Thu, 2018-11-29 at 15:07 -0800, Rodrigo Vivi wrote:
> On Mon, Nov 26, 2018 at 04:37:08PM -0800, José Roberto de Souza
> wrote:
> > The first 8 bits of PSR2_CTL have 2 fields to set frames count, the
> > first one is to set how many idle frames PSR2 HW needs to wait
> > before
> > enter in deep sleep and the second one it is how many frames(it
> > don't
> > need to be idle frames) PSR2 HW will wait before start the PSR
> > activation sequence.
> > The previous names was really misleading and caused wrong values 
The idea was to setup a conservative configuration for PSR2 until we
were ready to enable the feature and some testing was done. Not sure
why you think the values are wrong.

> > being
> > set so better rename to make it clear.
> 
> I honestly prefer the old names for 2 reasons:
> 
> - they are shorter
> - they follow the exact name we have on spec
+1 for the above reason.

> 
> > 
> > Also taking the oportunity to improve those macros.
> > 
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h  | 35 ++++++++++++++++----------
> > ------
> >  drivers/gpu/drm/i915/intel_psr.c |  7 ++++---
> >  2 files changed, 22 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 47baf2fe8f71..73046bb9ec7c 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -4203,23 +4203,24 @@ enum {
> >  #define   EDP_PSR_DEBUG_MASK_DISP_REG_WRITE    (1 << 16) /*
> > Reserved in ICL+ */
> >  #define   EDP_PSR_DEBUG_EXIT_ON_PIXEL_UNDERRUN (1 << 15) /* SKL+
> > */
> >  
> > -#define EDP_PSR2_CTL			_MMIO(0x6f900)
> > -#define   EDP_PSR2_ENABLE		(1 << 31)
> > -#define   EDP_SU_TRACK_ENABLE		(1 << 30)
> > -#define   EDP_Y_COORDINATE_VALID	(1 << 26) /* GLK and CNL+ */
> > -#define   EDP_Y_COORDINATE_ENABLE	(1 << 25) /* GLK and CNL+ */
> > -#define   EDP_MAX_SU_DISABLE_TIME(t)	((t) << 20)
> > -#define   EDP_MAX_SU_DISABLE_TIME_MASK	(0x1f << 20)
> > -#define   EDP_PSR2_TP2_TIME_500us	(0 << 8)
> > -#define   EDP_PSR2_TP2_TIME_100us	(1 << 8)
> > -#define   EDP_PSR2_TP2_TIME_2500us	(2 << 8)
> > -#define   EDP_PSR2_TP2_TIME_50us	(3 << 8)
> > -#define   EDP_PSR2_TP2_TIME_MASK	(3 << 8)
> > -#define   EDP_PSR2_FRAME_BEFORE_SU_SHIFT 4
> > -#define   EDP_PSR2_FRAME_BEFORE_SU_MASK	(0xf << 4)
> > -#define   EDP_PSR2_FRAME_BEFORE_SU(a)	((a) << 4)
> > -#define   EDP_PSR2_IDLE_FRAME_MASK	0xf
> > -#define   EDP_PSR2_IDLE_FRAME_SHIFT	0
> > +#define EDP_PSR2_CTL					_MMIO(0
> > x6f900)
> > +#define   EDP_PSR2_ENABLE				(1 << 31)
> > +#define   EDP_SU_TRACK_ENABLE				(1 <<
> > 30)
> > +#define   EDP_Y_COORDINATE_VALID			(1 << 26) /*
> > GLK and CNL+ */
> > +#define   EDP_Y_COORDINATE_ENABLE			(1 << 25) /*
> > GLK and CNL+ */
> > +#define   EDP_MAX_SU_DISABLE_TIME(t)			((t) <<
> > 20)
> > +#define   EDP_MAX_SU_DISABLE_TIME_MASK			(0x1f
> > << 20)
> > +#define   EDP_PSR2_TP2_TIME_500us			(0 << 8)
> > +#define   EDP_PSR2_TP2_TIME_100us			(1 << 8)
> > +#define   EDP_PSR2_TP2_TIME_2500us			(2 << 8)
> > +#define   EDP_PSR2_TP2_TIME_50us			(3 << 8)
> > +#define   EDP_PSR2_TP2_TIME_MASK			(3 << 8)
> > +#define   EDP_PSR2_FRAMES_BEFORE_ACTIVATE_SHIFT		(4)
> > +#define   EDP_PSR2_FRAMES_BEFORE_ACTIVATE_MASK		(0xf <<
> > EDP_PSR2_FRAMES_BEFORE_ACTIVATE_SHIFT)
> > +#define   EDP_PSR2_FRAMES_BEFORE_ACTIVATE(n)		(((n)
> > << EDP_PSR2_FRAMES_BEFORE_ACTIVATE_SHIFT) &
> > EDP_PSR2_FRAMES_BEFORE_ACTIVATE_MASK)
> > +#define   EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP_MASK	(0xf)
> > +#define   EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP_SHIFT	(0)
> > +#define   EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP(n)		(((n)
> > << EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP_SHIFT) &
> > EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP_MASK)
> >  
> >  #define _PSR_EVENT_TRANS_A			0x60848
> >  #define _PSR_EVENT_TRANS_B			0x61848
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 9215c9052381..ba7bbe3f8df2 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -479,6 +479,7 @@ static void hsw_activate_psr1(struct intel_dp
> > *intel_dp)
> >  static void hsw_activate_psr2(struct intel_dp *intel_dp)
> >  {
> >  	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > +	struct i915_psr *psr = &dev_priv->psr;
> >  	u32 val;
> >  
> >  	/* Let's use 6 as the minimum to cover all known cases
> > including the
> > @@ -486,8 +487,8 @@ static void hsw_activate_psr2(struct intel_dp
> > *intel_dp)
> >  	 */
> >  	int idle_frames = max(6, dev_priv->vbt.psr.idle_frames);
> >  
> > -	idle_frames = max(idle_frames, dev_priv->psr.sink_sync_latency
> > + 1);
> > -	val = idle_frames << EDP_PSR2_IDLE_FRAME_SHIFT;
> > +	idle_frames = max(idle_frames, psr->sink_sync_latency + 1);
> > +	val = EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP(idle_frames);
> >  
> >  	/* FIXME: selective update is probably totally broken because
> > it doesn't
> >  	 * mesh at all with our frontbuffer tracking. And the hw alone
> > isn't
> > @@ -496,7 +497,7 @@ static void hsw_activate_psr2(struct intel_dp
> > *intel_dp)
> >  	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
> >  		val |= EDP_Y_COORDINATE_ENABLE;
> >  
> > -	val |= EDP_PSR2_FRAME_BEFORE_SU(dev_priv->psr.sink_sync_latency 
> > + 1);
> > +	val |= EDP_PSR2_FRAMES_BEFORE_ACTIVATE(psr->sink_sync_latency +
> > 1);
> >  
> >  	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us >= 0 &&
> >  	    dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 50)
> > -- 
> > 2.19.2
> >
On Thu, 2018-11-29 at 15:25 -0800, Dhinakaran Pandiyan wrote:
> On Thu, 2018-11-29 at 15:07 -0800, Rodrigo Vivi wrote:
> > On Mon, Nov 26, 2018 at 04:37:08PM -0800, José Roberto de Souza
> > wrote:
> > > The first 8 bits of PSR2_CTL have 2 fields to set frames count,
> > > the
> > > first one is to set how many idle frames PSR2 HW needs to wait
> > > before
> > > enter in deep sleep and the second one it is how many frames(it
> > > don't
> > > need to be idle frames) PSR2 HW will wait before start the PSR
> > > activation sequence.
> > > The previous names was really misleading and caused wrong values 
> The idea was to setup a conservative configuration for PSR2 until we
> were ready to enable the feature and some testing was done. Not sure
> why you think the values are wrong.
> 
> > > being
> > > set so better rename to make it clear.
> > 
> > I honestly prefer the old names for 2 reasons:
> > 
> > - they are shorter
> > - they follow the exact name we have on spec
> +1 for the above reason.

Okay droping this patch.

> 
> > > Also taking the oportunity to improve those macros.
> > > 
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_reg.h  | 35 ++++++++++++++++----------
> > > ------
> > >  drivers/gpu/drm/i915/intel_psr.c |  7 ++++---
> > >  2 files changed, 22 insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > b/drivers/gpu/drm/i915/i915_reg.h
> > > index 47baf2fe8f71..73046bb9ec7c 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -4203,23 +4203,24 @@ enum {
> > >  #define   EDP_PSR_DEBUG_MASK_DISP_REG_WRITE    (1 << 16) /*
> > > Reserved in ICL+ */
> > >  #define   EDP_PSR_DEBUG_EXIT_ON_PIXEL_UNDERRUN (1 << 15) /* SKL+
> > > */
> > >  
> > > -#define EDP_PSR2_CTL			_MMIO(0x6f900)
> > > -#define   EDP_PSR2_ENABLE		(1 << 31)
> > > -#define   EDP_SU_TRACK_ENABLE		(1 << 30)
> > > -#define   EDP_Y_COORDINATE_VALID	(1 << 26) /* GLK and CNL+ */
> > > -#define   EDP_Y_COORDINATE_ENABLE	(1 << 25) /* GLK and
> > > CNL+ */
> > > -#define   EDP_MAX_SU_DISABLE_TIME(t)	((t) << 20)
> > > -#define   EDP_MAX_SU_DISABLE_TIME_MASK	(0x1f << 20)
> > > -#define   EDP_PSR2_TP2_TIME_500us	(0 << 8)
> > > -#define   EDP_PSR2_TP2_TIME_100us	(1 << 8)
> > > -#define   EDP_PSR2_TP2_TIME_2500us	(2 << 8)
> > > -#define   EDP_PSR2_TP2_TIME_50us	(3 << 8)
> > > -#define   EDP_PSR2_TP2_TIME_MASK	(3 << 8)
> > > -#define   EDP_PSR2_FRAME_BEFORE_SU_SHIFT 4
> > > -#define   EDP_PSR2_FRAME_BEFORE_SU_MASK	(0xf << 4)
> > > -#define   EDP_PSR2_FRAME_BEFORE_SU(a)	((a) << 4)
> > > -#define   EDP_PSR2_IDLE_FRAME_MASK	0xf
> > > -#define   EDP_PSR2_IDLE_FRAME_SHIFT	0
> > > +#define EDP_PSR2_CTL					_MMIO(0
> > > x6f900)
> > > +#define   EDP_PSR2_ENABLE				(1 <<
> > > 31)
> > > +#define   EDP_SU_TRACK_ENABLE				(1 <<
> > > 30)
> > > +#define   EDP_Y_COORDINATE_VALID			(1 << 26) /*
> > > GLK and CNL+ */
> > > +#define   EDP_Y_COORDINATE_ENABLE			(1 <<
> > > 25) /*
> > > GLK and CNL+ */
> > > +#define   EDP_MAX_SU_DISABLE_TIME(t)			((t) <<
> > > 20)
> > > +#define   EDP_MAX_SU_DISABLE_TIME_MASK			(0x1f
> > > << 20)
> > > +#define   EDP_PSR2_TP2_TIME_500us			(0 <<
> > > 8)
> > > +#define   EDP_PSR2_TP2_TIME_100us			(1 <<
> > > 8)
> > > +#define   EDP_PSR2_TP2_TIME_2500us			(2 <<
> > > 8)
> > > +#define   EDP_PSR2_TP2_TIME_50us			(3 << 8)
> > > +#define   EDP_PSR2_TP2_TIME_MASK			(3 << 8)
> > > +#define   EDP_PSR2_FRAMES_BEFORE_ACTIVATE_SHIFT		(4)
> > > +#define   EDP_PSR2_FRAMES_BEFORE_ACTIVATE_MASK		(0xf <<
> > > EDP_PSR2_FRAMES_BEFORE_ACTIVATE_SHIFT)
> > > +#define   EDP_PSR2_FRAMES_BEFORE_ACTIVATE(n)		(((n)
> > > << EDP_PSR2_FRAMES_BEFORE_ACTIVATE_SHIFT) &
> > > EDP_PSR2_FRAMES_BEFORE_ACTIVATE_MASK)
> > > +#define   EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP_MASK	(0xf)
> > > +#define   EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP_SHIFT	(0)
> > > +#define   EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP(n)		(((n)
> > > << EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP_SHIFT) &
> > > EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP_MASK)
> > >  
> > >  #define _PSR_EVENT_TRANS_A			0x60848
> > >  #define _PSR_EVENT_TRANS_B			0x61848
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index 9215c9052381..ba7bbe3f8df2 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -479,6 +479,7 @@ static void hsw_activate_psr1(struct intel_dp
> > > *intel_dp)
> > >  static void hsw_activate_psr2(struct intel_dp *intel_dp)
> > >  {
> > >  	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > > +	struct i915_psr *psr = &dev_priv->psr;
> > >  	u32 val;
> > >  
> > >  	/* Let's use 6 as the minimum to cover all known cases
> > > including the
> > > @@ -486,8 +487,8 @@ static void hsw_activate_psr2(struct intel_dp
> > > *intel_dp)
> > >  	 */
> > >  	int idle_frames = max(6, dev_priv->vbt.psr.idle_frames);
> > >  
> > > -	idle_frames = max(idle_frames, dev_priv->psr.sink_sync_latency
> > > + 1);
> > > -	val = idle_frames << EDP_PSR2_IDLE_FRAME_SHIFT;
> > > +	idle_frames = max(idle_frames, psr->sink_sync_latency + 1);
> > > +	val = EDP_PSR2_IDLE_FRAMES_TO_DEEP_SLEEP(idle_frames);
> > >  
> > >  	/* FIXME: selective update is probably totally broken because
> > > it doesn't
> > >  	 * mesh at all with our frontbuffer tracking. And the hw alone
> > > isn't
> > > @@ -496,7 +497,7 @@ static void hsw_activate_psr2(struct intel_dp
> > > *intel_dp)
> > >  	if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
> > >  		val |= EDP_Y_COORDINATE_ENABLE;
> > >  
> > > -	val |= EDP_PSR2_FRAME_BEFORE_SU(dev_priv-
> > > >psr.sink_sync_latency 
> > > + 1);
> > > +	val |= EDP_PSR2_FRAMES_BEFORE_ACTIVATE(psr->sink_sync_latency +
> > > 1);
> > >  
> > >  	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time_us >= 0 &&
> > >  	    dev_priv->vbt.psr.tp2_tp3_wakeup_time_us <= 50)
> > > -- 
> > > 2.19.2
> > >