[Intel-gfx] [PATCH] drm/i915: Enable FBC for non X-tiled FBs

[Ville Syrjälä]

On Thu, Mar 16, 2017 at 05:07:26PM +0530, Praveen Paneri wrote:
> Hi Ville,
> 
> On Wednesday 15 March 2017 03:53 PM, Ville Syrjälä wrote:
> > On Wed, Mar 15, 2017 at 12:29:35PM +0530, Praveen Paneri wrote:
> >> FBC is only enabled for X-tiled framebuffers but there are
> >> quite a few cases where we tend to use Y-tiled framebuffers.
> >> So enabling it for non X-tiled framebuffers.
> >
> > This patch doesn't actuall enable anything. In fact to me it looks like
> > we're already allowing Y tiling + FBC, so the commit message/subject should
> > likely say that you're fixing something.
> >
> I agree! I can rename the patch as
> Fix FBC Stride programming for Y-tiled FBs
> > Unless I'm missing some magic check somewhere that still prevents
> > Y tiled + FBC. But then this patch wouldn't be sufficient either.
> We had a check to activate FBC only for X-tiled buffers but that has 
> been removed by this patch.
> https://patchwork.kernel.org/patch/9546759/
> 
> With this change, we don't know what would happen in case of Y-tiled 
> buffer as it is suggested that Stride for Y-tiled buffers must be 
> programmed by SW (Probably a HW bug/limitation).
> 
> >
> >>
> >> Signed-off-by: Praveen Paneri <praveen.paneri at intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/i915_reg.h  | 2 ++
> >>   drivers/gpu/drm/i915/intel_fbc.c | 8 ++++++++
> >>   2 files changed, 10 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> >> index cc843f9..9d7a376 100644
> >> --- a/drivers/gpu/drm/i915/i915_reg.h
> >> +++ b/drivers/gpu/drm/i915/i915_reg.h
> >> @@ -2506,6 +2506,8 @@ enum skl_disp_power_wells {
> >>   #define  BDW_FBC_COMPRESSION_MASK	0xfff
> >>
> >>   #define FBC_LL_SIZE		(1536)
> >> +#define FBC_YSTRIDE		_MMIO(0x4208c)
> >> +#define   FBC_STRIDE_OVERRIDE	(1<<13)
> >
> > These defines shouldn't be in the middle of the FBC1 register defines.
> > The register should also be called CHICKEN_MISC_1, so the defines
> > should live next to the other CHICKEN_MISC register defines.
> Will fix this
> >
> >>
> >>   #define FBC_LLC_READ_CTRL	_MMIO(0x9044)
> >>   #define   FBC_LLC_FULLY_OPEN	(1<<30)
> >> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> >> index 17d418b..0ac9889 100644
> >> --- a/drivers/gpu/drm/i915/intel_fbc.c
> >> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> >> @@ -301,6 +301,14 @@ static void gen7_fbc_activate(struct drm_i915_private *dev_priv)
> >>   	u32 dpfc_ctl;
> >>   	int threshold = dev_priv->fbc.threshold;
> >>
> >> +	if (INTEL_GEN(dev_priv) >= 9 &&
> >
> > AFAICS IS_GEN9() would be better.
> Yes!
> >
> >> +	    i915_gem_object_get_tiling(cache->vma->obj) != I915_TILING_X) {
> >> +		struct intel_fbc_state_cache *cache = &dev_priv->fbc.state_cache;
> >> +		int cfb_stride = DIV_ROUND_UP(cache->plane.src_w,
> >> +					      (32 * threshold)) * 8;
> >
> > Are we taking this compressed stride alignment requirement into account
> > when we allocate the cfb?
> cfb size is calculated as (line * cache->fb.stride) but cfb is allocated 
> as per the original FB size (threshold = 1) where stride would be higher 
> than what is calculated here. Would we still need to take care of the 
> alignment?

I don't know. The cfb allocation code is too hard to follow so I've
decided not to even try.

-- 
Ville Syrjälä
Intel OTC