Re: [PATCH] drm/i915/cnl: WaFbcSkipSegments

[Paulo Zanoni <paulo.r.zanoni@intel.com>]

Em Sex, 2017-09-01 às 14:31 -0700, Rodrigo Vivi escreveu:
> On Fri, Sep 1, 2017 at 2:04 PM, Paulo Zanoni <paulo.r.zanoni@intel.co
> m> wrote:
> > Em Ter, 2017-08-29 às 16:08 -0700, Rodrigo Vivi escreveu:
> > > Skip compressing 1 segment at the end of the frame,
> > > avoid a pixel count mismatch nuke event when last active
> > > pixel and dummy pixel has same color for Odd Plane
> > > Width / Height.
> > > 
> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_reg.h | 2 ++
> > >  drivers/gpu/drm/i915/intel_pm.c | 4 ++++
> > >  2 files changed, 6 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > b/drivers/gpu/drm/i915/i915_reg.h
> > > index e2908ae34004..0072ef79bf34 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -2939,6 +2939,8 @@ enum i915_power_well_id {
> > >  #define ILK_DPFC_CHICKEN     _MMIO(0x43224)
> > >  #define   ILK_DPFC_DISABLE_DUMMY0 (1<<8)
> > >  #define   ILK_DPFC_NUKE_ON_ANY_MODIFICATION  (1<<23)
> > > +#define   CNL_SKIP_SEG_EN    (1<<12)
> > > +#define   CNL_SKIP_SEG_COUNT (1<<10)
> > 
> > #define CNL_SKIP_SEG_COUNT_MASK (3 << 10)
> > #define CNL_SKIP_SEG_COUNT(x) ((x) << 10)
> 
> By the definition we should add the SHIFT then as well?
> Although we are not going to read ever?!

I don't see the point in adding it if we're not using it. Most of the
times where we have the function-like macro we don't need _SHIFT, but
_MASK is needed due to the RMW.

> 
> > 
> > Also, since you're going to submit v2 anyway, I wouldn't complain
> > if
> > you drive-by fixed the definitions of ILK_DPFC_CHICKEN bits to use
> > spaces as documented in the beginning of the file. If we fix every
> > time
> > we touch a reg, we may at some point be consistent :).
> 
> Last patch I tried to fix the style on the same patch Jani nacked it.
> So I wonder if we need a bit patch to fix all the style entirely.

I just gave a suggestion, fixing the spacing is not required in this
patch.

> 
> > 
> > 
> > >  #define ILK_FBC_RT_BASE              _MMIO(0x2128)
> > >  #define   ILK_FBC_RT_VALID   (1<<0)
> > >  #define   SNB_FBC_FRONT_BUFFER       (1<<1)
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > b/drivers/gpu/drm/i915/intel_pm.c
> > > index d5ff0b9f999f..acf793256507 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -8283,6 +8283,10 @@ static void
> > > cannonlake_init_clock_gating(struct drm_i915_private *dev_priv)
> > >               I915_WRITE(SLICE_UNIT_LEVEL_CLKGATE,
> > >                          I915_READ(SLICE_UNIT_LEVEL_CLKGATE) |
> > >                          SARBUNIT_CLKGATE_DIS);
> > > +
> > > +     /* WaFbcSkipSegments:cnl */
> > 
> > Since this is also documented in BSpec but it doesn't include the
> > WA
> > name, can you please also document that this is WA #1133 in this
> > comment? I'd do something like:
> > 
> > /* Display WA #1133: WaFbcSkipSegments:cnl,glk. */
> 
> good idea!
> 
> > 
> > Please notice that we also need to apply this WA for GLK.
> 
> hm... indeed...
> > 
> > 
> > > +     I915_WRITE(ILK_DPFC_CHICKEN, I915_READ(ILK_DPFC_CHICKEN) |
> > > +                CNL_SKIP_SEG_EN | CNL_SKIP_SEG_COUNT);
> > 
> > Need to clear bit 11 too.
> 
> ouch! yeap!
> 
> Thanks,
> Rodrigo.
> 
> > 
> > val = READ();
> > val &= ~CNL_SKIP_SEG_COUNT_MASK;
> > val |= CNL_SKIP_SEG_EN | CNL_SKIP_SEG_COUNT(1);
> > 
> > >  }
> > > 
> > >  static void kabylake_init_clock_gating(struct drm_i915_private
> > > *dev_priv)
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx