Re: [Intel-xe] [PATCH v2 37/50] drm/xe/uapi: Convert tile_mask to a pt_placement_hint

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

On Tue, Nov 07, 2023 at 04:17:51PM -0800, Welty, Brian wrote:
> 
> On 11/3/2023 7:34 AM, Francois Dugast wrote:
> > From: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > 
> > The previous tile_mask was also an optional hint, and only used
> > for the page-table tree placement. However, it was so tied
> > with the tile concept itself. Let's clarify things up and make
> > this generic enough. So accept any valid memory region mask.
> > It could even be a direct near_mem_region gotten from the engine_info.
> > pt stands for page table.
> > 
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: Francois Dugast <francois.dugast@intel.com>
> > ---
> >   drivers/gpu/drm/xe/xe_vm.c | 14 ++++++++++----
> >   include/uapi/drm/xe_drm.h  | 16 +++++++++++++---
> >   2 files changed, 23 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> > index 5538b0ed81e8..c7eb8d43bf33 100644
> > --- a/drivers/gpu/drm/xe/xe_vm.c
> > +++ b/drivers/gpu/drm/xe/xe_vm.c
> > @@ -3007,11 +3007,16 @@ int xe_vm_bind_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
> >   			goto release_vm_lock;
> >   		}
> > -		if (bind_ops[i].tile_mask) {
> > +		if (bind_ops[i].pt_placement_hint) {
> >   			u64 valid_tiles = BIT(xe->info.tile_count) - 1;
> > +			/*
> > +			 * System memory is currently ignored from this hint,
> > +			 * which gets entirely converted to a tile_mask
> > +			 */
> > +			u8 system_memory = 0x1;
> > -			if (XE_IOCTL_DBG(xe, bind_ops[i].tile_mask &
> > -					 ~valid_tiles)) {
> > +			if (XE_IOCTL_DBG(xe, bind_ops[i].pt_placement_hint &
> > +					 ~valid_tiles & ~system_memory)) {
> 
> But valid_tiles is not correct bitmask to use here as that is tiles, not a
> mask of VRAM regions...
> If pt_placement_hint is actually memory regions, I think you simply want:
> 	if (XE_IOCTL_DBG(xe, bind_ops[i].pt_placement_hint &
> ~xe->info.mem_region_mask) {

yes, this is a better check indeed. let's change.

> 
> 
> >   				err = -EINVAL;
> >   				goto release_vm_lock;
> >   			}
> > @@ -3088,7 +3093,8 @@ int xe_vm_bind_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
> >   		u32 op = bind_ops[i].op;
> >   		u32 flags = bind_ops[i].flags;
> >   		u64 obj_offset = bind_ops[i].obj_offset;
> > -		u8 tile_mask = bind_ops[i].tile_mask;
> > +		/* Remove the system memory bit when converting to tiles */
> > +		u8 tile_mask = bind_ops[i].pt_placement_hint & ~0x1;
> 
> Same as above...  conversion doesn't look right.
> If you want a tile_mask, don't you need to shift the pt_placement_hint ?
> So you want to convert XE_PL_VRAM0 and XE_PL_VRAM1 to a bitmask of tiles?
> I think you need:
>    tile_mask = bind_ops[i].pt_placement_hint  >> 1;

good catch, so we don't need to change the rest of the internal code.

> 
> -Brian
> 
> 
> >   		u32 prefetch_region = bind_ops[i].prefetch_mem_region_instance;
> >   		ops[i] = vm_bind_ioctl_ops_create(vm, bos[i], obj_offset,
> > diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> > index 35ce3605fc0b..2d0fb4386a69 100644
> > --- a/include/uapi/drm/xe_drm.h
> > +++ b/include/uapi/drm/xe_drm.h
> > @@ -850,10 +850,20 @@ struct drm_xe_vm_bind_op {
> >   	__u64 addr;
> >   	/**
> > -	 * @tile_mask: Mask for which tiles to create binds for, 0 == All tiles,
> > -	 * only applies to creating new VMAs
> > +	 * @pt_placement_hint: An optional memory_region bit-mask hint, which
> > +	 * only applies when creating new VMAs. Default value '0' is the
> > +	 * recommended value.
> > +	 *
> > +	 * It hints the optimal placement for the page-table tree for this VMA.
> > +	 * For instance, when userspace is using engines living in a secondary
> > +	 * tile with allocated BOs near those engines, that same
> > +	 * @near_mem_region could be used in this hint field.
> > +	 *
> > +	 * Since it is a hint, the Xe kernel driver is free to ignore this mask
> > +	 * and choose the best location for the page-table, taking into
> > +	 * consideration the running hardware and runtime constrains.
> >   	 */
> > -	__u64 tile_mask;
> > +	__u64 pt_placement_hint;
> >   #define DRM_XE_VM_BIND_OP_MAP		0x0
> >   #define DRM_XE_VM_BIND_OP_UNMAP		0x1