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

[Praveen Paneri]

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?

Thanks,
Praveen
>
>> +		I915_WRITE(FBC_YSTRIDE, FBC_STRIDE_OVERRIDE | cfb_stride);
>> +	}
>> +
>>   	dpfc_ctl = 0;
>>   	if (IS_IVYBRIDGE(dev_priv))
>>   		dpfc_ctl |= IVB_DPFC_CTL_PLANE(params->crtc.plane);
>> --
>> 1.9.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>