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

[Rodrigo Vivi]

On Fri, May 5, 2017 at 2:02 PM, Jim Bride <jim.bride at 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 at intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
> Cc: Wayne Boyer <wayne.boyer at intel.com>
> Signed-off-by: Jim Bride <jim.bride at 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 at 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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br