Re: [Intel-xe] [PATCH v2 02/14] drm/xe/uapi: Separate bo_create placement from flags

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

On Wed, Nov 29, 2023 at 11:36:03AM -0800, Welty, Brian wrote:
> 
> 
> On 11/22/2023 6:38 AM, Francois Dugast wrote:
> > From: Rodrigo Vivi <rodrigo.vivi@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@intel.com>
> > Signed-off-by: Francois Dugast <francois.dugast@intel.com>
> > Reviewed-by: Matthew Brost <matthew.brost@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 4305f5cbc2ab..bbce4cd80f7e 100644
> > --- a/drivers/gpu/drm/xe/xe_bo.c
> > +++ b/drivers/gpu/drm/xe/xe_bo.c
> > @@ -1799,19 +1799,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;
> 
> I think you are missing to test that args->placement is *only* a mask of
> regions.   Looks like you allow other bits to be included and those will
> get slammed into bo_flags below.

now that they are split I don't believe that they would get slammed below,
but anyway, good catch, you are absolutely right that we should check
for the real valid bits in a better way, like the one you proposed below.

> 
> Probably:
> 
>   if (XE_IOCTL_DBG(xe, (args->placement & ~xe->info.mem_region_mask) ||
> !args->placement))
>      return -EINVAL;

Thank you!

> 
> 
> -Brian
> 
> 
> > +
> >   	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))
> > @@ -1832,7 +1831,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 b0d13bc6d9cf..1bdd20d3c4a8 100644
> > --- a/include/uapi/drm/xe_drm.h
> > +++ b/include/uapi/drm/xe_drm.h
> > @@ -500,8 +500,11 @@ 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)
> > +	/** @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)
> >   /*
> >    * When using VRAM as a possible placement, ensure that the corresponding VRAM
> >    * allocation will always use the CPU accessible part of VRAM. This is important
> > @@ -517,7 +520,7 @@ struct drm_xe_gem_create {
> >    * display surfaces, therefore the kernel requires setting this flag for such
> >    * objects, otherwise an error is thrown on small-bar systems.
> >    */
> > -#define DRM_XE_GEM_CREATE_FLAG_NEEDS_VISIBLE_VRAM	(0x1 << 26)
> > +#define DRM_XE_GEM_CREATE_FLAG_NEEDS_VISIBLE_VRAM	(1 << 2)
> >   	/**
> >   	 * @flags: Flags, currently a mask of memory instances of where BO can
> >   	 * be placed
> > @@ -541,9 +544,6 @@ struct drm_xe_gem_create {
> >   	 */
> >   	__u32 handle;
> > -	/** @pad: MBZ */
> > -	__u32 pad;
> > -
> >   	/** @reserved: Reserved */
> >   	__u64 reserved[2];
> >   };