On Mon, Apr 03, 2017 at 05:42:39PM +0000, 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 <rodrigo.vivi@xxxxxxxxx>
> > Cc: Wayne Boyer <wayne.boyer@xxxxxxxxx>
> >
> > Signed-off-by: Jim Bride <jim.bride@xxxxxxxxxxxxxxx>
> > ---
> > 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 mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
