| Message ID | 1502389833-32621-1-git-send-email-praveen.paneri@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Em Sex, 2017-08-11 às 00:00 +0530, Praveen Paneri escreveu: > When FBC is enabled for linear, legacy Y-tiled and Yf-tiled > surfaces on gen9, the cfb stride must be programmed by SW as > > cfb_stride = ceiling[(at least plane width in pixels)/ > (32 * compression limit factor)] * 8 > > v2: Minor fix for a build error > v3: Fixed subject, register name and platform check (Ville) > v4: Added WA details in comment (Paulo) > v5: > - Read modified reg write to preserve other bit values (Paulo) > - Store modified stride value in reg_params (Paulo) > - Keep GLK out of the WA (Paulo) > v6: > - added additional field in reg_params for gen9_wa_cfb_stride > (Paulo) > - Used appropriate bit mask while writing the register (Paulo) > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Signed-off-by: Praveen Paneri <praveen.paneri@intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/i915_reg.h | 4 ++++ > drivers/gpu/drm/i915/intel_fbc.c | 27 +++++++++++++++++++++++++++ > 3 files changed, 32 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > b/drivers/gpu/drm/i915/i915_drv.h > index 907603c..1d40a7c 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1106,6 +1106,7 @@ struct intel_fbc { > } fb; > > int cfb_size; > + unsigned int gen9_wa_cfb_stride; > } params; > > struct intel_fbc_work { > diff --git a/drivers/gpu/drm/i915/i915_reg.h > b/drivers/gpu/drm/i915/i915_reg.h > index 56df86e..51cab2f 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -6801,6 +6801,10 @@ enum { > #define GLK_CL1_PWR_DOWN (1 << 11) > #define GLK_CL0_PWR_DOWN (1 << 10) > > +#define CHICKEN_MISC_4 _MMIO(0x4208c) > +#define FBC_STRIDE_OVERRIDE (1<<13) See the new comment at the top of this file. (1 << 13) is preferred. > +#define FBC_STRIDE_MASK 0x1FFF > + > #define _CHICKEN_PIPESL_1_A 0x420b0 > #define _CHICKEN_PIPESL_1_B 0x420b4 > #define HSW_FBCQ_DIS (1 << 22) > diff --git a/drivers/gpu/drm/i915/intel_fbc.c > b/drivers/gpu/drm/i915/intel_fbc.c > index 860b8c2..21f6b33 100644 > --- a/drivers/gpu/drm/i915/intel_fbc.c > +++ b/drivers/gpu/drm/i915/intel_fbc.c > @@ -291,6 +291,21 @@ static void gen7_fbc_activate(struct > drm_i915_private *dev_priv) > u32 dpfc_ctl; > int threshold = dev_priv->fbc.threshold; > > + /* Display WA #0529: skl, kbl, bxt but not for glk*/ Don't need to list excluded platforms. Also, missing space before the asterisk. > + if (IS_GEN9(dev_priv) && !IS_GEMINILAKE(dev_priv)) { > + u32 chicken_misc4 = I915_READ(CHICKEN_MISC_4); In our driver, variables with the name of register usually contain its address. We generally use "val" for values. > + > + if (i915_gem_object_get_tiling(params->vma->obj) > + != I915_TILING_X) { > + I915_WRITE(CHICKEN_MISC_4, chicken_misc4 | > + FBC_STRIDE_OVERRIDE | > + (params->gen9_wa_cfb_stride & > FBC_STRIDE_MASK)); We're not masking the old value before overwriting it, eventually everything is going to be 0xFFF if we keep changing the stride without disabling it. > + } else { > + I915_WRITE(CHICKEN_MISC_4, chicken_misc4 & > + ~(FBC_STRIDE_OVERRIDE | > FBC_STRIDE_MASK)); > + } > + } > + Major issue with spaces instead of tabs in the whole chunk above. > dpfc_ctl = 0; > if (IS_IVYBRIDGE(dev_priv)) > dpfc_ctl |= IVB_DPFC_CTL_PLANE(params->crtc.plane); > @@ -865,6 +880,7 @@ static void intel_fbc_get_reg_params(struct > intel_crtc *crtc, > struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); > struct intel_fbc *fbc = &dev_priv->fbc; > struct intel_fbc_state_cache *cache = &fbc->state_cache; > + int threshold = dev_priv->fbc.threshold; > > /* Since all our fields are integer types, use memset here > so the > * comparison function can rely on memcmp because the > padding will be > @@ -880,6 +896,17 @@ static void intel_fbc_get_reg_params(struct > intel_crtc *crtc, > params->fb.format = cache->fb.format; > params->fb.stride = cache->fb.stride; > > + /* Display WA #0529: skl, kbl, bxt but not for glk*/ > + if (IS_GEN9(dev_priv) && !IS_GEMINILAKE(dev_priv)) { > + /* calculate cfb stride for y-tiled mode */ > + if (i915_gem_object_get_tiling(params->vma->obj) > + != I915_TILING_X) { We don't need the check here, it's already done in the other part of the code. Anyway, to save time on yet another iteration of this patch I went ahead and fixed the styling issues and the masking problem and merged the patch. Thanks for the patch! > + int cfb_stride = DIV_ROUND_UP(cache- > >plane.src_w, > + (32 * threshold)) * 8; > + params->gen9_wa_cfb_stride = cfb_stride; > + } > + } > + > params->cfb_size = intel_fbc_calculate_cfb_size(dev_priv, > cache); > } >
Hi Paulo, Thank you for the review and fix-ups :) regards, Praveen On Saturday 26 August 2017 06:19 AM, Paulo Zanoni wrote: > Em Sex, 2017-08-11 às 00:00 +0530, Praveen Paneri escreveu: >> When FBC is enabled for linear, legacy Y-tiled and Yf-tiled >> surfaces on gen9, the cfb stride must be programmed by SW as >> >> cfb_stride = ceiling[(at least plane width in pixels)/ >> (32 * compression limit factor)] * 8 >> >> v2: Minor fix for a build error >> v3: Fixed subject, register name and platform check (Ville) >> v4: Added WA details in comment (Paulo) >> v5: >> - Read modified reg write to preserve other bit values (Paulo) >> - Store modified stride value in reg_params (Paulo) >> - Keep GLK out of the WA (Paulo) >> v6: >> - added additional field in reg_params for gen9_wa_cfb_stride >> (Paulo) >> - Used appropriate bit mask while writing the register (Paulo) >> >> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> >> Signed-off-by: Praveen Paneri <praveen.paneri@intel.com> >> --- >> drivers/gpu/drm/i915/i915_drv.h | 1 + >> drivers/gpu/drm/i915/i915_reg.h | 4 ++++ >> drivers/gpu/drm/i915/intel_fbc.c | 27 +++++++++++++++++++++++++++ >> 3 files changed, 32 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h >> b/drivers/gpu/drm/i915/i915_drv.h >> index 907603c..1d40a7c 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -1106,6 +1106,7 @@ struct intel_fbc { >> } fb; >> >> int cfb_size; >> + unsigned int gen9_wa_cfb_stride; >> } params; >> >> struct intel_fbc_work { >> diff --git a/drivers/gpu/drm/i915/i915_reg.h >> b/drivers/gpu/drm/i915/i915_reg.h >> index 56df86e..51cab2f 100644 >> --- a/drivers/gpu/drm/i915/i915_reg.h >> +++ b/drivers/gpu/drm/i915/i915_reg.h >> @@ -6801,6 +6801,10 @@ enum { >> #define GLK_CL1_PWR_DOWN (1 << 11) >> #define GLK_CL0_PWR_DOWN (1 << 10) >> >> +#define CHICKEN_MISC_4 _MMIO(0x4208c) >> +#define FBC_STRIDE_OVERRIDE (1<<13) > > See the new comment at the top of this file. (1 << 13) is preferred. > > >> +#define FBC_STRIDE_MASK 0x1FFF >> + >> #define _CHICKEN_PIPESL_1_A 0x420b0 >> #define _CHICKEN_PIPESL_1_B 0x420b4 >> #define HSW_FBCQ_DIS (1 << 22) >> diff --git a/drivers/gpu/drm/i915/intel_fbc.c >> b/drivers/gpu/drm/i915/intel_fbc.c >> index 860b8c2..21f6b33 100644 >> --- a/drivers/gpu/drm/i915/intel_fbc.c >> +++ b/drivers/gpu/drm/i915/intel_fbc.c >> @@ -291,6 +291,21 @@ static void gen7_fbc_activate(struct >> drm_i915_private *dev_priv) >> u32 dpfc_ctl; >> int threshold = dev_priv->fbc.threshold; >> >> + /* Display WA #0529: skl, kbl, bxt but not for glk*/ > > Don't need to list excluded platforms. Also, missing space before the > asterisk. > > >> + if (IS_GEN9(dev_priv) && !IS_GEMINILAKE(dev_priv)) { >> + u32 chicken_misc4 = I915_READ(CHICKEN_MISC_4); > > In our driver, variables with the name of register usually contain its > address. We generally use "val" for values. > > >> + >> + if (i915_gem_object_get_tiling(params->vma->obj) >> + != I915_TILING_X) { >> + I915_WRITE(CHICKEN_MISC_4, chicken_misc4 | >> + FBC_STRIDE_OVERRIDE | >> + (params->gen9_wa_cfb_stride & >> FBC_STRIDE_MASK)); > > We're not masking the old value before overwriting it, eventually > everything is going to be 0xFFF if we keep changing the stride without > disabling it. > > >> + } else { >> + I915_WRITE(CHICKEN_MISC_4, chicken_misc4 & >> + ~(FBC_STRIDE_OVERRIDE | >> FBC_STRIDE_MASK)); >> + } >> + } >> + > > Major issue with spaces instead of tabs in the whole chunk above. > > >> dpfc_ctl = 0; >> if (IS_IVYBRIDGE(dev_priv)) >> dpfc_ctl |= IVB_DPFC_CTL_PLANE(params->crtc.plane); >> @@ -865,6 +880,7 @@ static void intel_fbc_get_reg_params(struct >> intel_crtc *crtc, >> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); >> struct intel_fbc *fbc = &dev_priv->fbc; >> struct intel_fbc_state_cache *cache = &fbc->state_cache; >> + int threshold = dev_priv->fbc.threshold; >> >> /* Since all our fields are integer types, use memset here >> so the >> * comparison function can rely on memcmp because the >> padding will be >> @@ -880,6 +896,17 @@ static void intel_fbc_get_reg_params(struct >> intel_crtc *crtc, >> params->fb.format = cache->fb.format; >> params->fb.stride = cache->fb.stride; >> >> + /* Display WA #0529: skl, kbl, bxt but not for glk*/ >> + if (IS_GEN9(dev_priv) && !IS_GEMINILAKE(dev_priv)) { >> + /* calculate cfb stride for y-tiled mode */ >> + if (i915_gem_object_get_tiling(params->vma->obj) >> + != I915_TILING_X) { > > We don't need the check here, it's already done in the other part of > the code. > > Anyway, to save time on yet another iteration of this patch I went > ahead and fixed the styling issues and the masking problem and merged > the patch. Thanks for the patch! > >> + int cfb_stride = DIV_ROUND_UP(cache- >>> plane.src_w, >> + (32 * threshold)) * 8; >> + params->gen9_wa_cfb_stride = cfb_stride; >> + } >> + } >> + >> params->cfb_size = intel_fbc_calculate_cfb_size(dev_priv, >> cache); >> } >>
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 907603c..1d40a7c 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1106,6 +1106,7 @@ struct intel_fbc { } fb; int cfb_size; + unsigned int gen9_wa_cfb_stride; } params; struct intel_fbc_work { diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 56df86e..51cab2f 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -6801,6 +6801,10 @@ enum { #define GLK_CL1_PWR_DOWN (1 << 11) #define GLK_CL0_PWR_DOWN (1 << 10) +#define CHICKEN_MISC_4 _MMIO(0x4208c) +#define FBC_STRIDE_OVERRIDE (1<<13) +#define FBC_STRIDE_MASK 0x1FFF + #define _CHICKEN_PIPESL_1_A 0x420b0 #define _CHICKEN_PIPESL_1_B 0x420b4 #define HSW_FBCQ_DIS (1 << 22) diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c index 860b8c2..21f6b33 100644 --- a/drivers/gpu/drm/i915/intel_fbc.c +++ b/drivers/gpu/drm/i915/intel_fbc.c @@ -291,6 +291,21 @@ static void gen7_fbc_activate(struct drm_i915_private *dev_priv) u32 dpfc_ctl; int threshold = dev_priv->fbc.threshold; + /* Display WA #0529: skl, kbl, bxt but not for glk*/ + if (IS_GEN9(dev_priv) && !IS_GEMINILAKE(dev_priv)) { + u32 chicken_misc4 = I915_READ(CHICKEN_MISC_4); + + if (i915_gem_object_get_tiling(params->vma->obj) + != I915_TILING_X) { + I915_WRITE(CHICKEN_MISC_4, chicken_misc4 | + FBC_STRIDE_OVERRIDE | + (params->gen9_wa_cfb_stride & FBC_STRIDE_MASK)); + } else { + I915_WRITE(CHICKEN_MISC_4, chicken_misc4 & + ~(FBC_STRIDE_OVERRIDE | FBC_STRIDE_MASK)); + } + } + dpfc_ctl = 0; if (IS_IVYBRIDGE(dev_priv)) dpfc_ctl |= IVB_DPFC_CTL_PLANE(params->crtc.plane); @@ -865,6 +880,7 @@ static void intel_fbc_get_reg_params(struct intel_crtc *crtc, struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); struct intel_fbc *fbc = &dev_priv->fbc; struct intel_fbc_state_cache *cache = &fbc->state_cache; + int threshold = dev_priv->fbc.threshold; /* Since all our fields are integer types, use memset here so the * comparison function can rely on memcmp because the padding will be @@ -880,6 +896,17 @@ static void intel_fbc_get_reg_params(struct intel_crtc *crtc, params->fb.format = cache->fb.format; params->fb.stride = cache->fb.stride; + /* Display WA #0529: skl, kbl, bxt but not for glk*/ + if (IS_GEN9(dev_priv) && !IS_GEMINILAKE(dev_priv)) { + /* calculate cfb stride for y-tiled mode */ + if (i915_gem_object_get_tiling(params->vma->obj) + != I915_TILING_X) { + int cfb_stride = DIV_ROUND_UP(cache->plane.src_w, + (32 * threshold)) * 8; + params->gen9_wa_cfb_stride = cfb_stride; + } + } + params->cfb_size = intel_fbc_calculate_cfb_size(dev_priv, cache); }
When FBC is enabled for linear, legacy Y-tiled and Yf-tiled surfaces on gen9, the cfb stride must be programmed by SW as cfb_stride = ceiling[(at least plane width in pixels)/ (32 * compression limit factor)] * 8 v2: Minor fix for a build error v3: Fixed subject, register name and platform check (Ville) v4: Added WA details in comment (Paulo) v5: - Read modified reg write to preserve other bit values (Paulo) - Store modified stride value in reg_params (Paulo) - Keep GLK out of the WA (Paulo) v6: - added additional field in reg_params for gen9_wa_cfb_stride (Paulo) - Used appropriate bit mask while writing the register (Paulo) Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> Signed-off-by: Praveen Paneri <praveen.paneri@intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/i915_reg.h | 4 ++++ drivers/gpu/drm/i915/intel_fbc.c | 27 +++++++++++++++++++++++++++ 3 files changed, 32 insertions(+)