[Intel-xe] [RFC v1 15/17] drm/xe/uapi: Separate bo_create placement from flags

[Francois Dugast]

On Wed, Oct 11, 2023 at 04:14:26PM +0200, Souza, Jose wrote:
> On Wed, 2023-10-11 at 13:59 +0000, Francois Dugast wrote:
> > From: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > 
> > Although the flags are about the creation, the memory placement
> > of the BO deserves a proper dedicated field in the uapi.
> > 
> > Besides getting more clear, it also allows to remove the
> > 'magic' shifts from the flags that was a concern during the
> > uapi reviews.
> > 
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_bo.c | 15 +++++++--------
> >  include/uapi/drm/xe_drm.h  | 12 ++++++------
> >  2 files changed, 13 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
> > index 4f0cd65c8522..c2cc859b0921 100644
> > --- a/drivers/gpu/drm/xe/xe_bo.c
> > +++ b/drivers/gpu/drm/xe/xe_bo.c
> > @@ -1768,19 +1768,18 @@ int xe_gem_create_ioctl(struct drm_device *dev, void *data,
> >  	u32 handle;
> >  	int err;
> >  
> > -	if (XE_IOCTL_DBG(xe, args->extensions) || XE_IOCTL_DBG(xe, args->pad) ||
> > +	if (XE_IOCTL_DBG(xe, args->extensions) ||
> >  	    XE_IOCTL_DBG(xe, args->reserved[0] || args->reserved[1]))
> >  		return -EINVAL;
> >  
> > +	/* at least one valid memory placement must be specified */
> > +	if (XE_IOCTL_DBG(xe, !(args->placement & xe->info.mem_region_mask)))
> > +		return -EINVAL;
> > +
> >  	if (XE_IOCTL_DBG(xe, args->flags &
> >  			 ~(DRM_XE_GEM_CREATE_FLAG_DEFER_BACKING |
> >  			   DRM_XE_GEM_CREATE_FLAG_SCANOUT |
> > -			   DRM_XE_GEM_CREATE_FLAG_NEEDS_VISIBLE_VRAM |
> > -			   xe->info.mem_region_mask)))
> > -		return -EINVAL;
> > -
> > -	/* at least one memory type must be specified */
> > -	if (XE_IOCTL_DBG(xe, !(args->flags & xe->info.mem_region_mask)))
> > +			   DRM_XE_GEM_CREATE_FLAG_NEEDS_VISIBLE_VRAM)))
> >  		return -EINVAL;
> >  
> >  	if (XE_IOCTL_DBG(xe, args->handle))
> > @@ -1801,7 +1800,7 @@ int xe_gem_create_ioctl(struct drm_device *dev, void *data,
> >  	if (args->flags & DRM_XE_GEM_CREATE_FLAG_SCANOUT)
> >  		bo_flags |= XE_BO_SCANOUT_BIT;
> >  
> > -	bo_flags |= args->flags << (ffs(XE_BO_CREATE_SYSTEM_BIT) - 1);
> > +	bo_flags |= args->placement << (ffs(XE_BO_CREATE_SYSTEM_BIT) - 1);
> >  
> >  	if (args->flags & DRM_XE_GEM_CREATE_FLAG_NEEDS_VISIBLE_VRAM) {
> >  		if (XE_IOCTL_DBG(xe, !(bo_flags & XE_BO_CREATE_VRAM_MASK)))
> > diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> > index df0561fc6c37..97e5f8f4aca3 100644
> > --- a/include/uapi/drm/xe_drm.h
> > +++ b/include/uapi/drm/xe_drm.h
> > @@ -607,9 +607,12 @@ struct drm_xe_gem_create {
> >  	 */
> >  	__u64 size;
> >  
> > -#define DRM_XE_GEM_CREATE_FLAG_DEFER_BACKING		(0x1 << 24)
> > -#define DRM_XE_GEM_CREATE_FLAG_SCANOUT			(0x1 << 25)
> > -#define DRM_XE_GEM_CREATE_FLAG_NEEDS_VISIBLE_VRAM	(0x1 << 26)
> > +	/** @placement: A mask of memory instances of where BO can be placed. */
> > +	__u32 placement;
> > +
> > +#define DRM_XE_GEM_CREATE_FLAG_DEFER_BACKING		(1 << 0)
> > +#define DRM_XE_GEM_CREATE_FLAG_SCANOUT			(1 << 1)
> > +#define DRM_XE_GEM_CREATE_FLAG_NEEDS_VISIBLE_VRAM	(1 << 2)
> 
> #define DRM_XE_GEM_CREATE_PLACEMENT_FLAG_DEFER_BACKING...
> 
> also this 3 flags should be define above the __u32 placement; like all other flags in xe_drm.h
> 

At first I also read it like you did because placement and the new constant
values come next to each other in the diff but actually those constants
are meant to be used as flags, not as placement.

This means the naming DRM_XE_GEM_CREATE_FLAG_* is consistent and the define
location is correct, it follows the same rules as for other defines:

    #define ...
    #define ...
    /** @flags ...
    __u32 flags;

Francois

> 
> 
> >  	/**
> >  	 * @flags: Flags, currently a mask of memory instances of where BO can
> >  	 * be placed
> > @@ -633,9 +636,6 @@ struct drm_xe_gem_create {
> >  	 */
> >  	__u32 handle;
> >  
> > -	/** @pad: MBZ */
> > -	__u32 pad;
> > -
> >  	/** @reserved: Reserved */
> >  	__u64 reserved[2];
> >  };
>