subject:"\[Intel\-gfx\] \[PATCH\] drm\/i915\/psr\: Clean\-up intel_enable_source

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

2017-04-03 Thread Jim Bride

On Mon, Apr 03, 2017 at 05:42:39PM +, 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 
> > Cc: Wayne Boyer 
> > 
> > Signed-off-by: Jim Bride 
> > ---
> >  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;
> 
___
Intel-gfx

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

2017-04-03 Thread Vivi, Rodrigo

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 
> Cc: Wayne Boyer 
> 
> Signed-off-by: Jim Bride 
> ---
>  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

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

2017-04-03 Thread Jim Bride

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 
Cc: Wayne Boyer 

Signed-off-by: Jim Bride 
---
 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.
+*/
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;
-- 
2.7.4

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

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

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

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

3 matches

Site Navigation

Mail list logo

Footer information