Re: [Intel-xe] [PATCH v4] drm/xe: Make explicit that exec uAPI expects canonical addresses

[Matthew Brost <matthew.brost@intel.com>]

On Thu, Aug 03, 2023 at 08:30:16AM -0700, José Roberto de Souza wrote:
> The batch buffer address in exec uAPI is used when emitting
> MI_BATCH_BUFFER_START that expect canonical addresses in future
> platforms, for current ones the bits above 57 for PVC and 47 for
> other platforms are ignored.
> 
> So the safest approach is to require canonical address for all
> platforms supported by Xe to avoid uAPI breaks.
> 
> v2:
> - fix check for non parallel engines
> 
> v3:
> - fix style
> 
> v4:
> - replace XE_IOCTL_ERR by XE_IOCTL_DBG
> 
> BSpec: 60223 59475 45718
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_device.c |  7 +++++++
>  drivers/gpu/drm/xe/xe_device.h |  2 ++
>  drivers/gpu/drm/xe/xe_exec.c   | 16 ++++++++++++++++
>  include/uapi/drm/xe_drm.h      |  4 ++--
>  4 files changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index 766df07de979c..9a4995304328e 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
> @@ -500,3 +500,10 @@ void xe_device_mem_access_put(struct xe_device *xe)
>  
>  	XE_WARN_ON(ref < 0);
>  }
> +
> +u64 xe_device_canonical_addr(struct xe_device *xe, u64 address)
> +{
> +	const int high_address_bit = xe->info.dma_mask_size > 47 ? 57 : 47;

Do we have defines for this somewhere in Xe? We probably do.

Also it should be 56, right?

Matt

> +
> +	return sign_extend64(address, high_address_bit);
> +}
> diff --git a/drivers/gpu/drm/xe/xe_device.h b/drivers/gpu/drm/xe/xe_device.h
> index 71582094834c6..a061b348f25e2 100644
> --- a/drivers/gpu/drm/xe/xe_device.h
> +++ b/drivers/gpu/drm/xe/xe_device.h
> @@ -143,6 +143,8 @@ void xe_device_mem_access_put(struct xe_device *xe);
>  void xe_device_assert_mem_access(struct xe_device *xe);
>  bool xe_device_mem_access_ongoing(struct xe_device *xe);
>  
> +u64 xe_device_canonical_addr(struct xe_device *xe, u64 address);
> +
>  static inline bool xe_device_in_fault_mode(struct xe_device *xe)
>  {
>  	return xe->usm.num_vm_in_fault_mode != 0;
> diff --git a/drivers/gpu/drm/xe/xe_exec.c b/drivers/gpu/drm/xe/xe_exec.c
> index 8a5b614df0900..4f651a042e560 100644
> --- a/drivers/gpu/drm/xe/xe_exec.c
> +++ b/drivers/gpu/drm/xe/xe_exec.c
> @@ -229,6 +229,22 @@ int xe_exec_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
>  			err = -EFAULT;
>  			goto err_syncs;
>  		}
> +
> +		for (i = 0; i < q->width; i++) {
> +			const u64 canonical_addr = xe_device_canonical_addr(xe, addresses[i]);
> +
> +			if (XE_IOCTL_DBG(xe, addresses[i] != canonical_addr)) {
> +				err = -EINVAL;
> +				goto err_syncs;
> +			}
> +		}
> +	} else {
> +		const u64 canonical_addr = xe_device_canonical_addr(xe, args->address);
> +
> +		if (XE_IOCTL_DBG(xe, args->address != canonical_addr)) {
> +			err = -EINVAL;
> +			goto err_syncs;
> +		}
>  	}
>  
>  	/*
> diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> index 86f16d50e9ccc..477072fd6c207 100644
> --- a/include/uapi/drm/xe_drm.h
> +++ b/include/uapi/drm/xe_drm.h
> @@ -865,8 +865,8 @@ struct drm_xe_exec {
>  	__u64 syncs;
>  
>  	/**
> -	 * @address: address of batch buffer if num_batch_buffer == 1 or an
> -	 * array of batch buffer addresses
> +	 * @address: canonical address of batch buffer if num_batch_buffer == 1
> +	 * or an array of batch buffer canonical addresses
>  	 */
>  	__u64 address;
>  
> -- 
> 2.41.0
>