Re: [PATCH] drm/i915: Implement fbc_status "Compressing" info for all platforms

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

Em Qui, 2017-06-01 às 21:02 +0300, ville.syrjala@linux.intel.com
escreveu:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> 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@intel.com>
> Cc: Gabriel Krisman Bertazi <krisman@collabora.co.uk>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  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.

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@intel.com>.

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@intel.com>.



> +#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)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx