| Message ID | 1502207481-3302-1-git-send-email-jim.bride@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2017-08-08 at 08:51 -0700, Jim Bride wrote: > Bit 29 of SRD_CTL needs to have its value preserved, probably good to kind of quote spec somehow: "This field is used for hardware communication. Software must not change this field." > so right before we > write out the register we go ahead and read the register and preserve > the value of that bit before we write out the configured register value. > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Jani Nikula <jani.nikula@intel.com> > Signed-off-by: Jim Bride <jim.bride@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_reg.h | 1 + > drivers/gpu/drm/i915/intel_psr.c | 1 + > 2 files changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index b2546ad..ea8e421 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -3872,6 +3872,7 @@ enum { > #define EDP_PSR_CTL _MMIO(dev_priv->psr_mmio_base + 0) > #define EDP_PSR_ENABLE (1<<31) > #define BDW_PSR_SINGLE_FRAME (1<<30) > +#define EDP_PSR_RESTORE_PSR_ACTIVE_CTX (1<<29) /* SW can't modify */ - please use real tabs instead of spaces. - a MASK on the name is better since we are not using this define to set the bit, but to mask instead. > #define EDP_PSR_LINK_STANDBY (1<<27) > #define EDP_PSR_MIN_LINK_ENTRY_TIME_MASK (3<<25) > #define EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES (0<<25) > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > index 559f1ab..0d08efa 100644 > --- a/drivers/gpu/drm/i915/intel_psr.c > +++ b/drivers/gpu/drm/i915/intel_psr.c > @@ -315,6 +315,7 @@ static void intel_enable_source_psr1(struct intel_dp *intel_dp) > else > val |= EDP_PSR_TP1_TP2_SEL; > I wondered if we should add an extra comment here, but I believe it is not necessary if we have the "_MASK" on the bit name. > + val |= I915_READ(EDP_PSR_CTL) & EDP_PSR_RESTORE_PSR_ACTIVE_CTX; > I915_WRITE(EDP_PSR_CTL, val); > } >
On Tue, Aug 08, 2017 at 07:42:50PM +0000, Vivi, Rodrigo wrote: > On Tue, 2017-08-08 at 08:51 -0700, Jim Bride wrote: > > Bit 29 of SRD_CTL needs to have its value preserved, > > probably good to kind of quote spec somehow: > "This field is used for hardware communication. Software must not > change this field." Added "according to the B-Spec" after the word preserved. > > > so right before we > > write out the register we go ahead and read the register and preserve > > the value of that bit before we write out the configured register value. > > > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Jani Nikula <jani.nikula@intel.com> > > Signed-off-by: Jim Bride <jim.bride@linux.intel.com> > > --- > > drivers/gpu/drm/i915/i915_reg.h | 1 + > > drivers/gpu/drm/i915/intel_psr.c | 1 + > > 2 files changed, 2 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > index b2546ad..ea8e421 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -3872,6 +3872,7 @@ enum { > > #define EDP_PSR_CTL _MMIO(dev_priv->psr_mmio_base + 0) > > #define EDP_PSR_ENABLE (1<<31) > > #define BDW_PSR_SINGLE_FRAME (1<<30) > > +#define EDP_PSR_RESTORE_PSR_ACTIVE_CTX (1<<29) /* SW can't modify */ > > - please use real tabs instead of spaces. Not sure what happened there, but fixed. > > - a MASK on the name is better since we are not using this define to set > the bit, but to mask instead. Changed as per suggesation. > > #define EDP_PSR_LINK_STANDBY (1<<27) > > #define EDP_PSR_MIN_LINK_ENTRY_TIME_MASK (3<<25) > > #define EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES (0<<25) > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > > index 559f1ab..0d08efa 100644 > > --- a/drivers/gpu/drm/i915/intel_psr.c > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > @@ -315,6 +315,7 @@ static void intel_enable_source_psr1(struct intel_dp *intel_dp) > > else > > val |= EDP_PSR_TP1_TP2_SEL; > > > > I wondered if we should add an extra comment here, but I believe it is > not necessary if we have the "_MASK" on the bit name. I think it would be redundant with the comment in i915_reg.h, which I believe to be a better place for the note. In any event, a new version of the patch is coming with the above changes once I smoke-test everything again. Jim > > + val |= I915_READ(EDP_PSR_CTL) & EDP_PSR_RESTORE_PSR_ACTIVE_CTX; > > I915_WRITE(EDP_PSR_CTL, val); > > } > > >
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index b2546ad..ea8e421 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -3872,6 +3872,7 @@ enum { #define EDP_PSR_CTL _MMIO(dev_priv->psr_mmio_base + 0) #define EDP_PSR_ENABLE (1<<31) #define BDW_PSR_SINGLE_FRAME (1<<30) +#define EDP_PSR_RESTORE_PSR_ACTIVE_CTX (1<<29) /* SW can't modify */ #define EDP_PSR_LINK_STANDBY (1<<27) #define EDP_PSR_MIN_LINK_ENTRY_TIME_MASK (3<<25) #define EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES (0<<25) diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c index 559f1ab..0d08efa 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -315,6 +315,7 @@ static void intel_enable_source_psr1(struct intel_dp *intel_dp) else val |= EDP_PSR_TP1_TP2_SEL; + val |= I915_READ(EDP_PSR_CTL) & EDP_PSR_RESTORE_PSR_ACTIVE_CTX; I915_WRITE(EDP_PSR_CTL, val); }
Bit 29 of SRD_CTL needs to have its value preserved, so right before we write out the register we go ahead and read the register and preserve the value of that bit before we write out the configured register value. Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Jani Nikula <jani.nikula@intel.com> Signed-off-by: Jim Bride <jim.bride@linux.intel.com> --- drivers/gpu/drm/i915/i915_reg.h | 1 + drivers/gpu/drm/i915/intel_psr.c | 1 + 2 files changed, 2 insertions(+)