Re: [PATCH v3 2/3] drm/xe: Expose display alignment requirement

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Fri, Jul 12, 2024 at 11:00:13AM +0200, Zbigniew Kempczyński wrote:
> On Thu, Jul 11, 2024 at 05:44:24PM -0400, Rodrigo Vivi wrote:
> > 
> > On Thu, Jul 11, 2024 at 11:09:24AM +0200, Zbigniew Kempczyński wrote:
> > 
> > Please use prefix 'drm/xe/uapi:' on the commit subject for anything
> > touching uapi.
> > 
> > And also documentation please.
> 
> Ok, I'm going to retag this to drm/xe/uapi.

Thanks. And sorry for taking so long to answer here.

> 
> Hmm, I've added this similar to MIN_ALIGNMENT (see on the bottom of the
> patch). Do you want to elaborate this more? I thought xe_drm.h is
> appropriate place to document this flag.

Yeap, I believe it would be good to elaborate it more. Specially because
the impact on the media side for instance.

> 
> > 
> > But more then that, we need to engage first with userspace and get
> > them to implement this.
> 
> I'm aware that BMG display complicated things a bit. As it supports
> 4K on vram I think switching vram_flags to XE_VRAM_FLAGS_NEED64K will
> work, but we loose 4K granularity on vram so imo there's a waste
> of memory for smaller allocations.
> 
> > 
> > > Scanout buffer on Battlemage requires allocation in 64K contigues
> > > pages to support Tile4 + compression what differs from normal bo
> > > requirements. Expose display alignment configuration to userspace
> > > to ensure it will properly align requested memory.
> > 
> > We should probably consider something that aligns better with
> > the default alignment that comes from the min_page_size
> > from struct drm_xe_mem_region.
> 
> At the moment I see following options:
> 
> 1. change xe_graphics_desc bmg to:
>    .vram_flags = XE_VRAM_FLAGS_NEED64K
>    at cost of loosing 4K granularity on vram

yeap, this is bad... basically expanding the media limitation to every umd :/

> 
> 2. add new flag as I proposed; we may burn out one reserved u64 (u32)
>    and expose min_display_page_size. But I'm not sure should we follow
>    this way as display is not taken from system memory but vram
>    (this field will be inappropriate set for system memory).
> 
> 3. add new flag as I proposed, and expose this in via config query
>    (current proposal).
> 
> 4. any other ideas?

no other idea to be honest. Let's ask UMD folks' opinions on this as well.

> 
> > 
> > BTW: This will be a big impact in Media because right now they are
> > having to use SCANOUT flag for every buffer since they have no
> > ways to know upfront what buffers they will send to display.
> 
> If media will use buffer which is Tile4 + compressed I think they
> will have no choice and they will need to allocate buffers in 64K contigues
> chunks somehow.

indeed. but let's get their ack here at least.

> 
> At the moment I've mitigated this issue seen in kms_ccs for bmg
> in series: https://patchwork.freedesktop.org/series/135994/
> but this is not temporary and not fully correct solution until kernel
> will switch to 64K physical alignment for scanout buffers.
> 
> --
> Zbigniew
> 
> > 
> > > 
> > > Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
> > > Cc: Matthew Auld <matthew.auld@intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > ---
> > >  drivers/gpu/drm/xe/xe_query.c | 4 +++-
> > >  include/uapi/drm/xe_drm.h     | 3 +++
> > >  2 files changed, 6 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_query.c b/drivers/gpu/drm/xe/xe_query.c
> > > index 4e01df6b1b7a..c32894f3eea1 100644
> > > --- a/drivers/gpu/drm/xe/xe_query.c
> > > +++ b/drivers/gpu/drm/xe/xe_query.c
> > > @@ -313,7 +313,7 @@ static int query_mem_regions(struct xe_device *xe,
> > >  
> > >  static int query_config(struct xe_device *xe, struct drm_xe_device_query *query)
> > >  {
> > > -	const u32 num_params = DRM_XE_QUERY_CONFIG_MAX_EXEC_QUEUE_PRIORITY + 1;
> > > +	const u32 num_params = DRM_XE_QUERY_CONFIG_DISPLAY_ALIGNMENT + 1;
> > >  	size_t size =
> > >  		sizeof(struct drm_xe_query_config) + num_params * sizeof(u64);
> > >  	struct drm_xe_query_config __user *query_ptr =
> > > @@ -342,6 +342,8 @@ static int query_config(struct xe_device *xe, struct drm_xe_device_query *query)
> > >  	config->info[DRM_XE_QUERY_CONFIG_VA_BITS] = xe->info.va_bits;
> > >  	config->info[DRM_XE_QUERY_CONFIG_MAX_EXEC_QUEUE_PRIORITY] =
> > >  		xe_exec_queue_device_get_max_priority(xe);
> > > +	config->info[DRM_XE_QUERY_CONFIG_DISPLAY_ALIGNMENT] =
> > > +		xe->info.vram_flags & XE_VRAM_FLAGS_DISPLAY_NEED64K ? SZ_64K : SZ_4K;
> > >  
> > >  	if (copy_to_user(query_ptr, config, size)) {
> > >  		kfree(config);
> > > diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> > > index 19619d4952a8..c7a930dbf58c 100644
> > > --- a/include/uapi/drm/xe_drm.h
> > > +++ b/include/uapi/drm/xe_drm.h
> > > @@ -398,6 +398,8 @@ struct drm_xe_query_mem_regions {
> > >   *  - %DRM_XE_QUERY_CONFIG_VA_BITS - Maximum bits of a virtual address
> > >   *  - %DRM_XE_QUERY_CONFIG_MAX_EXEC_QUEUE_PRIORITY - Value of the highest
> > >   *    available exec queue priority
> > > + *  - %DRM_XE_QUERY_CONFIG_DISPLAY_ALIGNMENT - Alignment of contigous physical
> > > + *    memory allocation required by the display, typically SZ_4K or SZ_64K
> > >   */
> > >  struct drm_xe_query_config {
> > >  	/** @num_params: number of parameters returned in info */
> > > @@ -412,6 +414,7 @@ struct drm_xe_query_config {
> > >  #define DRM_XE_QUERY_CONFIG_MIN_ALIGNMENT		2
> > >  #define DRM_XE_QUERY_CONFIG_VA_BITS			3
> > >  #define DRM_XE_QUERY_CONFIG_MAX_EXEC_QUEUE_PRIORITY	4
> > > +#define DRM_XE_QUERY_CONFIG_DISPLAY_ALIGNMENT		5
> > >  	/** @info: array of elements containing the config info */
> > >  	__u64 info[];
> > >  };
> > > -- 
> > > 2.34.1
> > >