| Message ID | 1499811596-14634-2-git-send-email-jim.bride@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tuesday, July 11, 2017 3:19:53 PM PDT 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 which bit is that? > 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. I see a caller to intel_psr_single_frame_update() > > v2: * Do a field-wise init on EDP_PSR_MAX_SLEEP_TIME even though we > always set it to the max value. (Rodrigo) > * Rebase > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Cc: Wayne Boyer <wayne.boyer@intel.com> > Cc: Jani Nikula <jani.nikula@intel.com> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@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 c712d01..3e62429 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -3789,18 +3789,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 559f1ab..132987b 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; EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES is (0<<25) We can just remove the two lines above, I think it's just misleading to do a nop after checking for Haswell. Curious how the entry time is determined, the rest seem to come from VBT. > > - 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;
Quoting Dhinakaran Pandiyan (2017-07-12 09:47:25) > On Tuesday, July 11, 2017 3:19:53 PM PDT 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 which bit is that? I think we would all be happier with keeping the explicit construction (and a smaller patch) if we used val |= I915_READ(EDP_PSR_CTL) & EDP_PSR_CTL_RSVD_MASK; Better name than reserved most welcome, since reserved often means "do not set; reserved for past/future use" rather than must not modify but keep. So EDP_PSR_CTL_KEEP_MASK ? -Chris
On Wed, 12 Jul 2017, Chris Wilson <chris@chris-wilson.co.uk> wrote: > Quoting Dhinakaran Pandiyan (2017-07-12 09:47:25) >> On Tuesday, July 11, 2017 3:19:53 PM PDT 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 which bit is that? > > I think we would all be happier with keeping the explicit construction > (and a smaller patch) if we used > > val |= I915_READ(EDP_PSR_CTL) & EDP_PSR_CTL_RSVD_MASK; Agreed. Avoid read-modify-write as much as possible. BR, Jani.
On Fri, Jul 14, 2017 at 12:34:28PM +0300, Jani Nikula wrote: > On Wed, 12 Jul 2017, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > Quoting Dhinakaran Pandiyan (2017-07-12 09:47:25) > >> On Tuesday, July 11, 2017 3:19:53 PM PDT 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 which bit is that? Bit 29 (Context restore to PSR Active) in SRD_CTL. I'll add it to the commit message. It's worth noting that the bit is not technically reserved, but rather that SW is not allowed to change it. > > > > I think we would all be happier with keeping the explicit construction > > (and a smaller patch) if we used > > > > val |= I915_READ(EDP_PSR_CTL) & EDP_PSR_CTL_RSVD_MASK; > > Agreed. Avoid read-modify-write as much as possible. I can do this if everyone thinks it's the thing to do, but it does open us up to a similar class of bug (B-Spec restricting mods to a bit / bit range after initial support for a platform was added) in the future. IMHO the code as written is safer. Jim > > BR, > Jani. > > > -- > Jani Nikula, Intel Open Source Technology Center > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, 03 Aug 2017, Jim Bride <jim.bride@linux.intel.com> wrote: > On Fri, Jul 14, 2017 at 12:34:28PM +0300, Jani Nikula wrote: >> On Wed, 12 Jul 2017, Chris Wilson <chris@chris-wilson.co.uk> wrote: >> > Quoting Dhinakaran Pandiyan (2017-07-12 09:47:25) >> >> On Tuesday, July 11, 2017 3:19:53 PM PDT 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 which bit is that? > > Bit 29 (Context restore to PSR Active) in SRD_CTL. I'll add it to the > commit message. It's worth noting that the bit is not technically > reserved, but rather that SW is not allowed to change it. > >> > >> > I think we would all be happier with keeping the explicit construction >> > (and a smaller patch) if we used >> > >> > val |= I915_READ(EDP_PSR_CTL) & EDP_PSR_CTL_RSVD_MASK; >> >> Agreed. Avoid read-modify-write as much as possible. > > I can do this if everyone thinks it's the thing to do, but it > does open us up to a similar class of bug (B-Spec restricting mods > to a bit / bit range after initial support for a platform was added) > in the future. IMHO the code as written is safer. Chris' suggestion preserves the restricted bits that must remain the same, while initializing everything else. Instead of only changing the bits we must change, only preserve the bits we must not change. Sorry if I wasn't clear with the "as much as possible" part there. Preserving the restricted bits is a functional change, and the subject of this patch does not reflect that. When I look at the logs, I pretty much expect clean up commits to be non-functional. There are some areas where I'd look the other way, but PSR is something where we must carefully split up the patches and write the commit messages diligently, because I know we will be spending time debugging this code and reading the logs. BR, Jani. > > Jim > > >> >> BR, >> Jani. >> >> >> -- >> Jani Nikula, Intel Open Source Technology Center >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Aug 04, 2017 at 10:29:33AM +0300, Jani Nikula wrote: > On Thu, 03 Aug 2017, Jim Bride <jim.bride@linux.intel.com> wrote: > > On Fri, Jul 14, 2017 at 12:34:28PM +0300, Jani Nikula wrote: > >> On Wed, 12 Jul 2017, Chris Wilson <chris@chris-wilson.co.uk> wrote: > >> > Quoting Dhinakaran Pandiyan (2017-07-12 09:47:25) > >> >> On Tuesday, July 11, 2017 3:19:53 PM PDT 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 which bit is that? > > > > Bit 29 (Context restore to PSR Active) in SRD_CTL. I'll add it to the > > commit message. It's worth noting that the bit is not technically > > reserved, but rather that SW is not allowed to change it. > > > >> > > >> > I think we would all be happier with keeping the explicit construction > >> > (and a smaller patch) if we used > >> > > >> > val |= I915_READ(EDP_PSR_CTL) & EDP_PSR_CTL_RSVD_MASK; > >> > >> Agreed. Avoid read-modify-write as much as possible. > > > > I can do this if everyone thinks it's the thing to do, but it > > does open us up to a similar class of bug (B-Spec restricting mods > > to a bit / bit range after initial support for a platform was added) > > in the future. IMHO the code as written is safer. > > Chris' suggestion preserves the restricted bits that must remain the > same, while initializing everything else. Instead of only changing the > bits we must change, only preserve the bits we must not change. Sorry if > I wasn't clear with the "as much as possible" part there. I think I followed you. What I was trying to highlight is that the patch as written doesn't touch anything other than what we explicitly need to initialize. While Chris' suggestion is much more terse, it leaves us open to another bit being flagged out as 'software shouldn't change' and we'd have a similar bug again. The patch as written doesn't expose us to that situation. I'm happy to go with Chris' suggestion if everyone still thinks it's the right thing, but I wanted to highlight that it's not entirely equivalent to what was in the original patch and in my opinion it's less safe than the original patch. > Preserving the restricted bits is a functional change, and the subject > of this patch does not reflect that. When I look at the logs, I pretty > much expect clean up commits to be non-functional. There are some areas > where I'd look the other way, but PSR is something where we must > carefully split up the patches and write the commit messages diligently, > because I know we will be spending time debugging this code and reading > the logs. I will remove the word 'clean-up' and reword the subject, independent of what we decide relative to the two approaches described above. The body of the commit message (IMHO) does a good job (and I'll add the specific bit in SRD_CTL to the body also) of describing the functional changes that the patch makes. Jim > BR, > Jani. > > > > > > > Jim > > > > > >> > >> BR, > >> Jani. > >> > >> > >> -- > >> Jani Nikula, Intel Open Source Technology Center > >> _______________________________________________ > >> Intel-gfx mailing list > >> Intel-gfx@lists.freedesktop.org > >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Jani Nikula, Intel Open Source Technology Center
On Mon, Aug 07, 2017 at 08:55:00AM -0700, Jim Bride wrote: > On Fri, Aug 04, 2017 at 10:29:33AM +0300, Jani Nikula wrote: > > On Thu, 03 Aug 2017, Jim Bride <jim.bride@linux.intel.com> wrote: > > > On Fri, Jul 14, 2017 at 12:34:28PM +0300, Jani Nikula wrote: > > >> On Wed, 12 Jul 2017, Chris Wilson <chris@chris-wilson.co.uk> wrote: > > >> > Quoting Dhinakaran Pandiyan (2017-07-12 09:47:25) > > >> >> On Tuesday, July 11, 2017 3:19:53 PM PDT 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 which bit is that? > > > > > > Bit 29 (Context restore to PSR Active) in SRD_CTL. I'll add it to the > > > commit message. It's worth noting that the bit is not technically > > > reserved, but rather that SW is not allowed to change it. > > > > > >> > > > >> > I think we would all be happier with keeping the explicit construction > > >> > (and a smaller patch) if we used > > >> > > > >> > val |= I915_READ(EDP_PSR_CTL) & EDP_PSR_CTL_RSVD_MASK; > > >> > > >> Agreed. Avoid read-modify-write as much as possible. > > > > > > I can do this if everyone thinks it's the thing to do, but it > > > does open us up to a similar class of bug (B-Spec restricting mods > > > to a bit / bit range after initial support for a platform was added) > > > in the future. IMHO the code as written is safer. > > > > Chris' suggestion preserves the restricted bits that must remain the > > same, while initializing everything else. Instead of only changing the > > bits we must change, only preserve the bits we must not change. Sorry if > > I wasn't clear with the "as much as possible" part there. > > I think I followed you. What I was trying to highlight is that the > patch as written doesn't touch anything other than what we explicitly > need to initialize. While Chris' suggestion is much more terse, it > leaves us open to another bit being flagged out as 'software > shouldn't change' and we'd have a similar bug again. The patch as > written doesn't expose us to that situation. I'm happy to go with > Chris' suggestion if everyone still thinks it's the right thing, but > I wanted to highlight that it's not entirely equivalent to what was > in the original patch and in my opinion it's less safe than the > original patch. Ok, folks think brevity wins out here, so I'm going to go ahead and spin a different, stand-alone patch following Chris' suggestion. Please disregard this one. Jim > > Preserving the restricted bits is a functional change, and the subject > > of this patch does not reflect that. When I look at the logs, I pretty > > much expect clean up commits to be non-functional. There are some areas > > where I'd look the other way, but PSR is something where we must > > carefully split up the patches and write the commit messages diligently, > > because I know we will be spending time debugging this code and reading > > the logs. > > I will remove the word 'clean-up' and reword the subject, independent > of what we decide relative to the two approaches described above. > The body of the commit message (IMHO) does a good job (and I'll add > the specific bit in SRD_CTL to the body also) of describing the > functional changes that the patch makes. > > Jim > > > BR, > > Jani. > > > > > > > > > > > > Jim > > > > > > > > >> > > >> BR, > > >> Jani. > > >> > > >> > > >> -- > > >> Jani Nikula, Intel Open Source Technology Center > > >> _______________________________________________ > > >> Intel-gfx mailing list > > >> Intel-gfx@lists.freedesktop.org > > >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > > Jani Nikula, Intel Open Source Technology Center > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index c712d01..3e62429 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -3789,18 +3789,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 559f1ab..132987b 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;
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) * Rebase Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> Cc: Wayne Boyer <wayne.boyer@intel.com> Cc: Jani Nikula <jani.nikula@intel.com> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@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(-)