[Intel-xe] [RFC PATCH] drm/xe/uAPi: Clarify the meaning of min_page_size and MIN_ALIGNMENT

[Francois Dugast]

On Mon, Oct 16, 2023 at 01:03:07PM +0200, Thomas Hellström wrote:
> Hi,
> 
> On 10/13/23 22:16, Rodrigo Vivi wrote:
> > On Fri, Oct 13, 2023 at 12:22:26PM +0200, Thomas Hellström wrote:
> > > In particular, don't *enforce* alignment of buffer object sizes to
> > > MIN_ALIGNMENT, as enforcing that for imported dma-bufs would not
> > > really make sense.
> > > 
> > > Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> > > Cc: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > > Cc: Matthew Brost <matthew.brost at intel.com>
> > > Cc: Francois Dugast <francois.dugast at intel.com>
> > > Signed-off-by: Thomas Hellström <thomas.hellstrom at linux.intel.com>
> > > ---
> > >   include/uapi/drm/xe_drm.h | 17 ++++++++++++-----
> > >   1 file changed, 12 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> > > index d48d8e3c898c..5895c95879d1 100644
> > > --- a/include/uapi/drm/xe_drm.h
> > > +++ b/include/uapi/drm/xe_drm.h
> > > @@ -167,11 +167,10 @@ struct drm_xe_query_mem_region {
> > >   	 *
> > >   	 * When the kernel allocates memory for this region, the
> > >   	 * underlying pages will be at least @min_page_size in size.
> > > -	 *
> > > -	 * Important note: When userspace allocates a GTT address which
> > > -	 * can point to memory allocated from this region, it must also
> > > -	 * respect this minimum alignment. This is enforced by the
> > > -	 * kernel.
> > This old statement seems bold on the kernel is enforcing it. If we are
> > but intend to not enforce anymore we should also change the code in
> > the same patch, no?!
> > Also by the commit message I had the impression that this was removing
> > the enforcement, and not just the documentation about it.
> 
> This RFC was more intended to trigger a discussion on how to handle this.
> I'll actually bring back the above somewhat reworded and also rework the
> commit message a bit to reflect that this is an RFC for an uAPI change under
> discussion.
> 
> > 
> > > +	 * Buffer objects with an allowable placement in this region may
> > > +	 * be created with a smaller size, but for increased performance,
> > > +	 * user-space drivers should align sizes of such buffer objects
> > > +	 * to this value whenever possible.
> > >   	 */
> > >   	__u32 min_page_size;
> > >   	/**
> > > @@ -252,6 +251,14 @@ struct drm_xe_query_config {
> > >   #define XE_QUERY_CONFIG_REV_AND_DEVICE_ID	0
> > >   #define XE_QUERY_CONFIG_FLAGS			1
> > >   	#define XE_QUERY_CONFIG_FLAGS_HAS_VRAM		(0x1 << 0)
> > > +/*
> > > + * XE_QUERY_CONFIG_MIN_ALIGNMENT - GPU VM bind address - and size alignment.
> > > + * GPU virtual address mappings of buffer objects must have their VM addr and
> > > + * range aligned to this value, with the exception of buffer objects with
> > > + * allowable placement in system memory only. If this alignment results in
> > > + * a mapping beyond the end of the buffer object, accesses beyond the
> > > + * end of the buffer object have undefined reslut.
> > s/reslut/result.
> > 
> > Should we make this into a kernel doc?
> 
> It looks like all other defines of constants in this file doesn't use
> kerneldoc. If we want to do that, It looks like we need to break this out to
> a top-level DOC: section, similar to what's done for 
> XE_RESET_FAILED_UEVENT.
> 
> /Thomas
> 
> 

Hi Thomas,

A proposal to make constants visible in kernel doc is part of the
uAPI alignment series take 2:
https://patchwork.freedesktop.org/patch/562078/?series=124977&rev=1

Francois