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