[PATCH v2 1/2] drm/xe: Separate 64K physical allocation for display

[Zbigniew Kempczyński]

On Tue, Jul 09, 2024 at 03:05:55PM -0400, Rodrigo Vivi wrote:
> On Tue, Jul 09, 2024 at 02:25:13PM +0200, Zbigniew Kempczyński wrote:
> > In case Tile4 + compression Battlemage requires physical 64K pages
> > for allocating display framebuffer. Add flag which distincts
> > buffer created for scanout from other buffers which don't need this
> > restriction.
> 
> I believe these 2 patches could be squashed together because this
> phrase here is the explanation for the next patch as well and
> that one just tell what it is doing without telling why.

It's not a problem to squash them. First patch just prepares the
code for the enablement of DISPLAY_NEED64K flag and I separated
them to make potential revert easier.

As you've noticed in your second comment (below) reuse NEED64K flag
is intrusive and requires adding more conditionals in the code for
already enabled platforms. I wanted to avoid this.

At the moment there's no BMG in CI but some IGT code (and UMD as well)
needs to be altered on bo creation for framebuffer (create ioctl() -
it now rejects non aligned to 64K buffers on BMG). I mean with use of
SCANOUT flag on BMG 4K buffer creation will return -EINVAL. Open
question is should we expose information about display requirement
to userspace somehow by some query that scanout buffer needs to be
aligned to 64K. From my perspective userspace can simply probe that
requirement - first trying to create 4K buffer with SCANOUT flag and
increase this to 64K if it fails.

--
Zbigniew

> 
> > 
> > Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
> > Cc: Matthew Auld <matthew.auld at intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > ---
> > v2: Distinct 64K for Battlemage only (Matt)
> > ---
> >  drivers/gpu/drm/xe/xe_bo.c           | 9 +++++++--
> >  drivers/gpu/drm/xe/xe_device_types.h | 1 +
> >  2 files changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
> > index 31192d983d9e..fbcf77698bf1 100644
> > --- a/drivers/gpu/drm/xe/xe_bo.c
> > +++ b/drivers/gpu/drm/xe/xe_bo.c
> > @@ -1984,9 +1984,13 @@ int xe_gem_create_ioctl(struct drm_device *dev, void *data,
> >  	if (args->flags & DRM_XE_GEM_CREATE_FLAG_DEFER_BACKING)
> >  		bo_flags |= XE_BO_FLAG_DEFER_BACKING;
> >  
> > -	if (args->flags & DRM_XE_GEM_CREATE_FLAG_SCANOUT)
> > +	if (args->flags & DRM_XE_GEM_CREATE_FLAG_SCANOUT) {
> >  		bo_flags |= XE_BO_FLAG_SCANOUT;
> >  
> > +		if (xe->info.vram_flags & XE_VRAM_FLAGS_DISPLAY_NEED64K)
> > +			bo_flags |= XE_BO_NEEDS_64K;
> > +	}
> > +
> >  	bo_flags |= args->placement << (ffs(XE_BO_FLAG_SYSTEM) - 1);
> >  
> >  	if (args->flags & DRM_XE_GEM_CREATE_FLAG_NEEDS_VISIBLE_VRAM) {
> > @@ -2315,8 +2319,9 @@ int xe_bo_dumb_create(struct drm_file *file_priv,
> >  	uint32_t handle;
> >  	int cpp = DIV_ROUND_UP(args->bpp, 8);
> >  	int err;
> > +	u8 flags_64k = XE_VRAM_FLAGS_NEED64K | XE_VRAM_FLAGS_DISPLAY_NEED64K;
> >  	u32 page_size = max_t(u32, PAGE_SIZE,
> > -		xe->info.vram_flags & XE_VRAM_FLAGS_NEED64K ? SZ_64K : SZ_4K);
> > +		xe->info.vram_flags & flags_64k ? SZ_64K : SZ_4K);
> >  
> >  	args->pitch = ALIGN(args->width * cpp, 64);
> >  	args->size = ALIGN(mul_u32_u32(args->pitch, args->height),
> > diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> > index f0cf9020e463..386faaffac53 100644
> > --- a/drivers/gpu/drm/xe/xe_device_types.h
> > +++ b/drivers/gpu/drm/xe/xe_device_types.h
> > @@ -46,6 +46,7 @@ struct xe_pat_ops;
> >  #define HAS_HECI_GSCFI(xe) ((xe)->info.has_heci_gscfi)
> >  
> >  #define XE_VRAM_FLAGS_NEED64K		BIT(0)
> > +#define XE_VRAM_FLAGS_DISPLAY_NEED64K	BIT(1)
> 
> I was wondering if we could simply reuse the existing flag, but probably looking
> at the next patch I believe that it could be intrusive and cause other issues.
> So, let's go with this.
> 
> >  
> >  #define XE_GT0		0
> >  #define XE_GT1		1
> > -- 
> > 2.34.1
> >