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

[Welty, Brian]

On 11/3/2023 7:34 AM, Francois Dugast wrote:
> From: Rodrigo Vivi <rodrigo.vivi at 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 at intel.com>
> Signed-off-by: Francois Dugast <francois.dugast at 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) {


>   				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;

-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