On Mon, Jun 05, 2017 at 05:23:17PM -0300, Paulo Zanoni wrote:
> Em Qui, 2017-06-01 às 21:02 +0300, ville.syrjala@xxxxxxxxxxxxxxx
> escreveu:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > The number of compressed segments has been available ever since
> > FBC2 was introduced in g4x, it just moved from the STATUS register
> > into STATUS2 on IVB.
> >
> > For FBC1 if we really wanted the number of compressed segments we'd
> > have to trawl through the tags, but in this case since the code just
> > uses the number of compressed segments as an indicator whether
> > compression has occurred we can just check the state of the
> > COMPRESSING and COMPRESSED bits. IIRC the hardware will try to
> > periodically recompress all uncompressed lines even if they haven't
> > changed and the COMPRESSED bit will be cleared while the compressor
> > is running, so just checking the COMPRESSED bit might not give us
> > the right answer. Hence it seems better to check for both
> > COMPRESSED and COMPRESSING as that should tell us that the
> > compressor is at least trying to do something.
> >
> > While at it move the IVB+ register define to the right place, unify
> > the naming convention of the compressed segment count masks, and
> > fix up the mask for g4x.
> >
> > Cc: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> > Cc: Gabriel Krisman Bertazi <krisman@xxxxxxxxxxxxxxx>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/i915_debugfs.c | 22 ++++++++++++++++------
> > drivers/gpu/drm/i915/i915_reg.h | 10 +++++-----
> > 2 files changed, 21 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 3b088685a553..126f1e8a9d0b 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -1670,12 +1670,22 @@ static int i915_fbc_status(struct seq_file
> > *m, void *unused)
> > seq_printf(m, "FBC disabled: %s\n",
> > dev_priv->fbc.no_fbc_reason);
> >
> > - if (intel_fbc_is_active(dev_priv) && INTEL_GEN(dev_priv) >=
> > 7) {
> > - uint32_t mask = INTEL_GEN(dev_priv) >= 8 ?
> > - BDW_FBC_COMPRESSION_MASK :
> > - IVB_FBC_COMPRESSION_MASK;
> > - seq_printf(m, "Compressing: %s\n",
> > - yesno(I915_READ(FBC_STATUS2) & mask));
> > + if (intel_fbc_is_active(dev_priv)) {
> > + u32 mask;
>
> Someone should sed our whole driver so it either fully uses u32 or
> uint32_t.
>
>
> > +
> > + if (INTEL_GEN(dev_priv) >= 8)
> > + mask = I915_READ(ILK_DPFC_STATUS2) &
> > BDW_DPFC_COMP_SEG_MASK;
> > + else if (INTEL_GEN(dev_priv) >= 7)
> > + mask = I915_READ(ILK_DPFC_STATUS2) &
> > IVB_DPFC_COMP_SEG_MASK;
> > + else if (INTEL_GEN(dev_priv) >= 5)
> > + mask = I915_READ(ILK_DPFC_STATUS) &
> > ILK_DPFC_COMP_SEG_MASK;
> > + else if (IS_G4X(dev_priv))
> > + mask = I915_READ(DPFC_STATUS) &
> > DPFC_COMP_SEG_MASK;
> > + else
> > + mask = I915_READ(FBC_STATUS) &
> > (FBC_STAT_COMPRESSING |
> > + FBC_STAT_COM
> > PRESSED);
> > +
> > + seq_printf(m, "Compressing: %s\n", yesno(mask));
> > }
> >
> > mutex_unlock(&dev_priv->fbc.lock);
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 231ee86625cd..23bdc079575c 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -2507,10 +2507,6 @@ enum skl_disp_power_wells {
> > #define FBC_FENCE_OFF _MMIO(0x3218) /* BSpec typo has
> > 321Bh */
> > #define FBC_TAG(i) _MMIO(0x3300 + (i) * 4)
> >
> > -#define FBC_STATUS2 _MMIO(0x43214)
> > -#define IVB_FBC_COMPRESSION_MASK 0x7ff
> > -#define BDW_FBC_COMPRESSION_MASK 0xfff
> > -
> > #define FBC_LL_SIZE (1536)
> >
> > #define FBC_LLC_READ_CTRL _MMIO(0x9044)
> > @@ -2539,7 +2535,7 @@ enum skl_disp_power_wells {
> > #define DPFC_INVAL_SEG_SHIFT (16)
> > #define DPFC_INVAL_SEG_MASK (0x07ff0000)
> > #define DPFC_COMP_SEG_SHIFT (0)
> > -#define DPFC_COMP_SEG_MASK (0x000003ff)
> > +#define DPFC_COMP_SEG_MASK (0x000007ff)
> > #define DPFC_STATUS2 _MMIO(0x3214)
> > #define DPFC_FENCE_YOFF _MMIO(0x3218)
> > #define DPFC_CHICKEN _MMIO(0x3224)
> > @@ -2553,6 +2549,10 @@ enum skl_disp_power_wells {
> > #define DPFC_RESERVED (0x1FFFFF00)
> > #define ILK_DPFC_RECOMP_CTL _MMIO(0x4320c)
> > #define ILK_DPFC_STATUS _MMIO(0x43210)
> > +#define ILK_DPFC_COMP_SEG_MASK 0x7ff
> > +#define ILK_DPFC_STATUS2 _MMIO(0x43214)
>
> I'm fine with using ILK names for something that appeared on ILK, but I
> can't find ILK_DPFC_STATUS2 on my ILK docs. It seems to me that STATUS2
> only appeared on IVB, and it's named FBC_STATUS2 there.
Hmm. Indeed it looks like it's not documented for ILK/SNB. But I
suspect it's actually there since g4x had it already. But I guess
I could rename it to IVB_FBC_STATUS2 or something like that just
to match the docs.
>
> If that's the case, ideally we should call it FBC_STATUS2, because
> that's the name. If you really insist, I could very reluctantly go with
> IVB_DPFC_STATUS2, although I really don't like the idea of going with a
> name that's nowhere in our docs. We should really only keep it as
> ILK_DPFC_STATUS2 if the register indeed was added on ILK (and in this
> case, please give me pointers to the appropriate doc).
>
> For the gen5+ part of the patch, if you rename the STATUS2 register,
> consider it Reviewed-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>.
>
> For the gen 2 - 4 part, I really didn't check the docs. But FBC is
> disabled by default on these platforms anyway and assumed to be broken,
> so: Acked-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>.
>
>
>
> > +#define IVB_DPFC_COMP_SEG_MASK 0x7ff
> > +#define BDW_DPFC_COMP_SEG_MASK 0xfff
> > #define ILK_DPFC_FENCE_YOFF _MMIO(0x43218)
> > #define ILK_DPFC_CHICKEN _MMIO(0x43224)
> > #define ILK_DPFC_DISABLE_DUMMY0 (1<<8)
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
