| Message ID | 1483812724-6503-1-git-send-email-vathsala.nagaraju@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Jan 07, 2017 at 11:42:04PM +0530, vathsala nagaraju wrote: > As per bpsec, CHICKEN_TRANS_EDP bit 12 ,15 > must be programmed. > Enable bit 12 for programmable header packet. > Enable bit 15 for Y cordinate support. > > v2: (Rodrigo) > - move CHICKEN_TRANS_EDP bit set logic right after setup_vsc > > v3:(Rodrigo) > - initialize chicken_trans to CHICKEN_TRANS_BIT12 instead of 0 > > v4:(Rodrigo) > - initialize chicken_trans=0,add chicken_trans=CHICKEN_TRANS_BIT12 Weird. Just scope the variable properly, use the correct type. > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Cc: Jim Bride <jim.bride@linux.intel.com> > Signed-off-by: vathsala nagaraju <vathsala.nagaraju@intel.com> > Signed-off-by: Patil Deepti <deepti.patil@intel.com> > --- > drivers/gpu/drm/i915/i915_reg.h | 7 +++++++ > drivers/gpu/drm/i915/intel_psr.c | 7 +++++++ > 2 files changed, 14 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 7830e6e..5ca506a 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -6449,6 +6449,13 @@ enum { > #define BDW_DPRS_MASK_VBLANK_SRD (1 << 0) > #define CHICKEN_PIPESL_1(pipe) _MMIO_PIPE(pipe, _CHICKEN_PIPESL_1_A, _CHICKEN_PIPESL_1_B) > > +#define CHICKEN_TRANS_A 0x420c0 > +#define CHICKEN_TRANS_B 0x420c4 > +#define CHICKEN_TRANS(trans) _MMIO_TRANS(trans, CHICKEN_TRANS_A, CHICKEN_TRANS_B) > +#define TRANS_EDP 3 > +#define CHICKEN_TRANS_BIT12 (1<<12) > +#define CHICKEN_TRANS_BIT15 (1<<15) Useless naming. Either given them proper names or don't. > if (!HAS_PSR(dev_priv)) { u32 chicken; > DRM_DEBUG_KMS("PSR not supported on this platform\n"); > @@ -505,6 +506,12 @@ void intel_psr_enable(struct intel_dp *intel_dp) > dev_priv->psr.psr2_support = false; > else > skl_psr_setup_su_vsc(intel_dp); > + /* Set CHICKEN_TRANS_BIT12 for programable header */ > + chicken_trans = CHICKEN_TRANS_BIT12; We can see you are setting CHICKEN_TRANS_BIT12, so don't bother repeating that. What programmable header? Why is this in a chicken bit, what is the bspec reference, all of those would be useful to answer. > + /* Set CHICKEN_TRANS_BIT15 if Y coordinate is supported */ > + if (dev_priv->psr.y_cord_support) > + chicken_trans |= CHICKEN_TRANS_BIT15; Again. Tell us why, we can read the code. Are the names meaningful? More meaningful than chicken |= BIT(15); ? > + I915_WRITE(CHICKEN_TRANS(TRANS_EDP), chicken_trans); > } else { > /* set up vsc header for psr1 */ > hsw_psr_setup_vsc(intel_dp); > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Sunday 08 January 2017 01:14 AM, Chris Wilson wrote: > On Sat, Jan 07, 2017 at 11:42:04PM +0530, vathsala nagaraju wrote: >> As per bpsec, CHICKEN_TRANS_EDP bit 12 ,15 >> must be programmed. >> Enable bit 12 for programmable header packet. >> Enable bit 15 for Y cordinate support. >> >> v2: (Rodrigo) >> - move CHICKEN_TRANS_EDP bit set logic right after setup_vsc >> >> v3:(Rodrigo) >> - initialize chicken_trans to CHICKEN_TRANS_BIT12 instead of 0 >> >> v4:(Rodrigo) >> - initialize chicken_trans=0,add chicken_trans=CHICKEN_TRANS_BIT12 > Weird. Just scope the variable properly, use the correct type. > >> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> >> Cc: Jim Bride <jim.bride@linux.intel.com> >> Signed-off-by: vathsala nagaraju <vathsala.nagaraju@intel.com> >> Signed-off-by: Patil Deepti <deepti.patil@intel.com> >> --- >> drivers/gpu/drm/i915/i915_reg.h | 7 +++++++ >> drivers/gpu/drm/i915/intel_psr.c | 7 +++++++ >> 2 files changed, 14 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >> index 7830e6e..5ca506a 100644 >> --- a/drivers/gpu/drm/i915/i915_reg.h >> +++ b/drivers/gpu/drm/i915/i915_reg.h >> @@ -6449,6 +6449,13 @@ enum { >> #define BDW_DPRS_MASK_VBLANK_SRD (1 << 0) >> #define CHICKEN_PIPESL_1(pipe) _MMIO_PIPE(pipe, _CHICKEN_PIPESL_1_A, _CHICKEN_PIPESL_1_B) >> >> +#define CHICKEN_TRANS_A 0x420c0 >> +#define CHICKEN_TRANS_B 0x420c4 >> +#define CHICKEN_TRANS(trans) _MMIO_TRANS(trans, CHICKEN_TRANS_A, CHICKEN_TRANS_B) >> +#define TRANS_EDP 3 >> +#define CHICKEN_TRANS_BIT12 (1<<12) >> +#define CHICKEN_TRANS_BIT15 (1<<15) > Useless naming. Either given them proper names or don't. > >> if (!HAS_PSR(dev_priv)) { > u32 chicken; > >> DRM_DEBUG_KMS("PSR not supported on this platform\n"); >> @@ -505,6 +506,12 @@ void intel_psr_enable(struct intel_dp *intel_dp) >> dev_priv->psr.psr2_support = false; >> else >> skl_psr_setup_su_vsc(intel_dp); >> + /* Set CHICKEN_TRANS_BIT12 for programable header */ >> + chicken_trans = CHICKEN_TRANS_BIT12; > We can see you are setting CHICKEN_TRANS_BIT12, so don't bother > repeating that. What programmable header? Why is this in a chicken bit, > what is the bspec reference, all of those would be useful to answer. Thanks for the review. In bspec, it's part of psr2 enable sequence. "Program Transcoder EDP VSC DIP header with a valid setting for PSR2 and Set CHICKEN_TRANS_EDP(0x420cc) bit 12 for programmable header packet" Will remove the comment. > >> + /* Set CHICKEN_TRANS_BIT15 if Y coordinate is supported */ >> + if (dev_priv->psr.y_cord_support) >> + chicken_trans |= CHICKEN_TRANS_BIT15; > Again. Tell us why, we can read the code. Are the names meaningful? More > meaningful than chicken |= BIT(15); ? In bspec, for register CHICKEN_TRANS, bit field name for 12 and 15 are spare 12 and spare 15. Named CHICKEN_TRANS_BIT15 instead of spare 15. Will remove the comment and change to BIT(12) and BIT(15) > >> + I915_WRITE(CHICKEN_TRANS(TRANS_EDP), chicken_trans); >> } else { >> /* set up vsc header for psr1 */ >> hsw_psr_setup_vsc(intel_dp); >> -- >> 1.9.1 >> >> _______________________________________________ >> 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 7830e6e..5ca506a 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -6449,6 +6449,13 @@ enum { #define BDW_DPRS_MASK_VBLANK_SRD (1 << 0) #define CHICKEN_PIPESL_1(pipe) _MMIO_PIPE(pipe, _CHICKEN_PIPESL_1_A, _CHICKEN_PIPESL_1_B) +#define CHICKEN_TRANS_A 0x420c0 +#define CHICKEN_TRANS_B 0x420c4 +#define CHICKEN_TRANS(trans) _MMIO_TRANS(trans, CHICKEN_TRANS_A, CHICKEN_TRANS_B) +#define TRANS_EDP 3 +#define CHICKEN_TRANS_BIT12 (1<<12) +#define CHICKEN_TRANS_BIT15 (1<<15) + #define DISP_ARB_CTL _MMIO(0x45000) #define DISP_FBC_MEMORY_WAKE (1<<31) #define DISP_TILE_SURFACE_SWIZZLING (1<<13) diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c index b28891b..1e5dd8f 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -475,6 +475,7 @@ void intel_psr_enable(struct intel_dp *intel_dp) struct drm_device *dev = intel_dig_port->base.base.dev; struct drm_i915_private *dev_priv = to_i915(dev); struct intel_crtc *crtc = to_intel_crtc(intel_dig_port->base.base.crtc); + uint32_t chicken_trans = 0; if (!HAS_PSR(dev_priv)) { DRM_DEBUG_KMS("PSR not supported on this platform\n"); @@ -505,6 +506,12 @@ void intel_psr_enable(struct intel_dp *intel_dp) dev_priv->psr.psr2_support = false; else skl_psr_setup_su_vsc(intel_dp); + /* Set CHICKEN_TRANS_BIT12 for programable header */ + chicken_trans = CHICKEN_TRANS_BIT12; + /* Set CHICKEN_TRANS_BIT15 if Y coordinate is supported */ + if (dev_priv->psr.y_cord_support) + chicken_trans |= CHICKEN_TRANS_BIT15; + I915_WRITE(CHICKEN_TRANS(TRANS_EDP), chicken_trans); } else { /* set up vsc header for psr1 */ hsw_psr_setup_vsc(intel_dp);