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

["Souza, Jose" <jose.souza@intel.com>]

On Thu, 2023-08-03 at 18:25 +0000, Matthew Brost wrote:
> 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.

Can't find any.

Upstream i915 don't have anything handling 56bits canonical address, Agama version has ppgtt_msb in the intel_device_info.
I don't think we need to store it in xe_graphics_desc...

> 
> Also it should be 56, right?

yeah missed that, will send another version after get feedback from the first comment.

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