diff mbox

[2/4] drm/i915/psr: Clean-up intel_enable_source_psr1()

Message ID 1494018126-30190-3-git-send-email-jim.bride@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

jim.bride@linux.intel.com May 5, 2017, 9:02 p.m. UTC
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.

v2: Do a field-wise init on EDP_PSR_MAX_SLEEP_TIME even though we
    always set it to the max value. (Rodrigo)

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@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  |  4 ++++
 drivers/gpu/drm/i915/intel_psr.c | 21 +++++++++++++++++++--
 2 files changed, 23 insertions(+), 2 deletions(-)

Comments

Rodrigo Vivi May 23, 2017, 7:07 p.m. UTC | #1
On Fri, May 5, 2017 at 2:02 PM, Jim Bride <jim.bride@linux.intel.com> 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.
>
> v2: Do a field-wise init on EDP_PSR_MAX_SLEEP_TIME even though we
>     always set it to the max value. (Rodrigo)
>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@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  |  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 ee8170c..3a63555 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3584,18 +3584,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 c3780d0..068c382 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;
>
> -       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. */

/* goes to next line

> +               val &= ~BDW_PSR_SINGLE_FRAME;
> +       } else {
> +               val &= ~EDP_PSR_LINK_STANDBY;
> +               val &= ~BDW_PSR_SINGLE_FRAME;

probably better to do this out of the if to avoid duplication. Comment above
already explain it well.

with these feel free to use:
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> +       }
> +
> +       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;
> --
> 2.7.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index ee8170c..3a63555 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3584,18 +3584,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 c3780d0..068c382 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;
 
-	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;