| Message ID | 1489561175-27470-1-git-send-email-praveen.paneri@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. Unless I'm missing some magic check somewhere that still prevents Y tiled + FBC. But then this patch wouldn't be sufficient either. > > Signed-off-by: Praveen Paneri <praveen.paneri@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. > > #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. > + 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? > + 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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
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@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@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx >
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@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.
Hi Praveen,
[auto build test ERROR on drm-intel/for-linux-next]
[also build test ERROR on v4.11-rc2 next-20170310]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Praveen-Paneri/drm-i915-Enable-FBC-for-non-X-tiled-FBs/20170318-084727
base: git://anongit.freedesktop.org/drm-intel for-linux-next
config: x86_64-randconfig-x003-201711 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All error/warnings (new ones prefixed by >>):
In file included from include/uapi/linux/stddef.h:1:0,
from include/linux/stddef.h:4,
from include/uapi/linux/posix_types.h:4,
from include/uapi/linux/types.h:13,
from include/linux/types.h:5,
from include/linux/async.h:15,
from drivers/gpu/drm/i915/intel_drv.h:28,
from drivers/gpu/drm/i915/intel_fbc.c:41:
drivers/gpu/drm/i915/intel_fbc.c: In function 'gen7_fbc_activate':
>> drivers/gpu/drm/i915/intel_fbc.c:305:33: error: 'cache' undeclared (first use in this function)
i915_gem_object_get_tiling(cache->vma->obj) != I915_TILING_X) {
^
include/linux/compiler.h:160:30: note: in definition of macro '__trace_if'
if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
^~~~
>> drivers/gpu/drm/i915/intel_fbc.c:304:2: note: in expansion of macro 'if'
if (INTEL_GEN(dev_priv) >= 9 &&
^~
drivers/gpu/drm/i915/intel_fbc.c:305:33: note: each undeclared identifier is reported only once for each function it appears in
i915_gem_object_get_tiling(cache->vma->obj) != I915_TILING_X) {
^
include/linux/compiler.h:160:30: note: in definition of macro '__trace_if'
if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
^~~~
>> drivers/gpu/drm/i915/intel_fbc.c:304:2: note: in expansion of macro 'if'
if (INTEL_GEN(dev_priv) >= 9 &&
^~
vim +/cache +305 drivers/gpu/drm/i915/intel_fbc.c
298 static void gen7_fbc_activate(struct drm_i915_private *dev_priv)
299 {
300 struct intel_fbc_reg_params *params = &dev_priv->fbc.params;
301 u32 dpfc_ctl;
302 int threshold = dev_priv->fbc.threshold;
303
> 304 if (INTEL_GEN(dev_priv) >= 9 &&
> 305 i915_gem_object_get_tiling(cache->vma->obj) != I915_TILING_X) {
306 struct intel_fbc_state_cache *cache = &dev_priv->fbc.state_cache;
307 int cfb_stride = DIV_ROUND_UP(cache->plane.src_w,
308 (32 * threshold)) * 8;
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
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) #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 && + 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; + 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);
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. Signed-off-by: Praveen Paneri <praveen.paneri@intel.com> --- drivers/gpu/drm/i915/i915_reg.h | 2 ++ drivers/gpu/drm/i915/intel_fbc.c | 8 ++++++++ 2 files changed, 10 insertions(+)