Re: [PATCH v2 6/6] drm/xe: Add VM bind IOCTL error injection

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

On Wed, May 15, 2024 at 03:29:18PM -0600, Cavitt, Jonathan wrote:
> -----Original Message-----
> From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of Matthew Brost
> Sent: Tuesday, May 14, 2024 5:40 PM
> To: intel-xe@lists.freedesktop.org
> Cc: Brost, Matthew <matthew.brost@intel.com>
> Subject: [PATCH v2 6/6] drm/xe: Add VM bind IOCTL error injection
> > 
> > Add VM bind IOCTL error injection which steals MSB of the bind flags
> > field which if set injects errors at various points in the VM bind
> > IOCTL. Intended to validate error paths. Enabled by CONFIG_DRM_XE_DEBUG.
> > 
> > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> 
> I have a minor nit lower down, but nothing worth blocking over:
> Reviewed-by: Jonathan Cavitt <jonathan.cavitt@intel.com>
> 
> > ---
> >  drivers/gpu/drm/xe/xe_device_types.h | 12 ++++++++++++
> >  drivers/gpu/drm/xe/xe_pt.c           | 12 ++++++++++++
> >  drivers/gpu/drm/xe/xe_vm.c           | 24 ++++++++++++++++++++++++
> >  drivers/gpu/drm/xe/xe_vm_types.h     | 14 ++++++++++++++
> >  4 files changed, 62 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> > index 0af739981ebf..6c8c55e9addd 100644
> > --- a/drivers/gpu/drm/xe/xe_device_types.h
> > +++ b/drivers/gpu/drm/xe/xe_device_types.h
> > @@ -22,6 +22,10 @@
> >  #include "xe_sriov_types.h"
> >  #include "xe_step_types.h"
> >  
> > +#if IS_ENABLED(CONFIG_DRM_XE_DEBUG)
> > +#define TEST_VM_OPS_ERROR
> > +#endif
> > +
> >  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
> >  #include "soc/intel_pch.h"
> >  #include "intel_display_core.h"
> > @@ -471,6 +475,14 @@ struct xe_device {
> >  		int mode;
> >  	} wedged;
> >  
> > +#ifdef TEST_VM_OPS_ERROR
> > +	/**
> > +	 * @vm_inject_error_position: inject errors at different places in VM
> > +	 * bind IOCTL based on this value
> > +	 */
> > +	u8 vm_inject_error_position;
> > +#endif
> > +
> >  	/* private: */
> >  
> >  #if IS_ENABLED(CONFIG_DRM_XE_DISPLAY)
> > diff --git a/drivers/gpu/drm/xe/xe_pt.c b/drivers/gpu/drm/xe/xe_pt.c
> > index 7f792b7feb06..0907aeaa25ed 100644
> > --- a/drivers/gpu/drm/xe/xe_pt.c
> > +++ b/drivers/gpu/drm/xe/xe_pt.c
> > @@ -1865,6 +1865,12 @@ int xe_pt_update_ops_prepare(struct xe_tile *tile, struct xe_vma_ops *vops)
> >  	xe_tile_assert(tile, pt_update_ops->current_op <=
> >  		       pt_update_ops->num_ops);
> >  
> > +#ifdef TEST_VM_OPS_ERROR
> > +	if (vops->inject_error &&
> > +	    vops->vm->xe->vm_inject_error_position == FORCE_OP_ERROR_PREPARE)
> > +		return -ENOSPC;
> > +#endif
> > +
> >  	return 0;
> >  }
> >  
> > @@ -2005,6 +2011,12 @@ xe_pt_update_ops_run(struct xe_tile *tile, struct xe_vma_ops *vops)
> >  		return dma_fence_get_stub();
> >  	}
> >  
> > +#ifdef TEST_VM_OPS_ERROR
> > +	if (vops->inject_error &&
> > +	    vm->xe->vm_inject_error_position == FORCE_OP_ERROR_RUN)
> > +		return ERR_PTR(-ENOSPC);
> > +#endif
> > +
> >  	if (pt_update_ops->needs_invalidation) {
> >  		ifence = kzalloc(sizeof(*ifence), GFP_KERNEL);
> >  		if (!ifence) {
> > diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> > index 7fa533902bda..2bac8539d024 100644
> > --- a/drivers/gpu/drm/xe/xe_vm.c
> > +++ b/drivers/gpu/drm/xe/xe_vm.c
> > @@ -2467,6 +2467,12 @@ static int vm_bind_ioctl_ops_lock_and_prep(struct drm_exec *exec,
> >  			return err;
> >  	}
> >  
> > +#ifdef TEST_VM_OPS_ERROR
> > +	if (vops->inject_error &&
> > +	    vm->xe->vm_inject_error_position == FORCE_OP_ERROR_LOCK)
> > +		return -ENOSPC;
> > +#endif
> > +
> >  	return 0;
> >  }
> >  
> > @@ -2703,11 +2709,20 @@ static int vm_bind_ioctl_ops_execute(struct xe_vm *vm,
> >  	return err;
> >  }
> >  
> > +#ifdef TEST_VM_OPS_ERROR
> > +#define SUPPORTED_FLAGS	\
> > +	(FORCE_OP_ERROR | \
> > +	 DRM_XE_VM_BIND_FLAG_READONLY | \
> > +	 DRM_XE_VM_BIND_FLAG_IMMEDIATE | \
> > +	 DRM_XE_VM_BIND_FLAG_NULL | \
> > +	 DRM_XE_VM_BIND_FLAG_DUMPABLE) 
> > +#else
> >  #define SUPPORTED_FLAGS	\
> >  	(DRM_XE_VM_BIND_FLAG_READONLY | \
> >  	 DRM_XE_VM_BIND_FLAG_IMMEDIATE | \
> >  	 DRM_XE_VM_BIND_FLAG_NULL | \
> >  	 DRM_XE_VM_BIND_FLAG_DUMPABLE)
> > +#endif
> 
> 
> I'm aware this is basically the only correct way of doing this,
> but the fact that both instances of SUPPORTED_FLAGS are
> the same save for the presence or absence of
> FORCE_OP_ERROR rubs me the wrong way.
> 
> I know we can't just "append" to SUPPORTED_FLAGS, but
> maybe we can use a stub?  Something like:
> 
> #define SUPPORTED_FLAGS_STUB  \
> 	(DRM_XE_VM_BIND_FLAG_READONLY | \
>  	 DRM_XE_VM_BIND_FLAG_IMMEDIATE | \
>  	 DRM_XE_VM_BIND_FLAG_NULL | \
>  	 DRM_XE_VM_BIND_FLAG_DUMPABLE)
> #ifdef TEST_VM_OPS_ERROR
> #define SUPPORTED_FLAGS  \
> 	(SUPPORTED_FLAGS_STUB)
> #else
> #define SUPPORTED_FLAGS  \
> 	(SUPPORTED_FLAGS_STUB |\
> 	 FORCE_OP_ERROR)
> #endif
> 
> Feel free to disregard this, it's just a suggestion.

That's a good suggestion. Will change.

Matt

> -Jonathan Cavitt
> 
> 
> >  #define XE_64K_PAGE_MASK 0xffffull
> >  #define ALL_DRM_XE_SYNCS_FLAGS (DRM_XE_SYNCS_FLAG_WAIT_FOR_OP)
> >  
> > @@ -3055,6 +3070,15 @@ int xe_vm_bind_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
> >  		err = vm_bind_ioctl_ops_parse(vm, ops[i], &vops);
> >  		if (err)
> >  			goto unwind_ops;
> > +
> > +#ifdef TEST_VM_OPS_ERROR
> > +		if (flags & FORCE_OP_ERROR) {
> > +			vops.inject_error = true;
> > +			vm->xe->vm_inject_error_position =
> > +				(vm->xe->vm_inject_error_position + 1) %
> > +				FORCE_OP_ERROR_COUNT;
> > +		}
> > +#endif
> >  	}
> >  
> >  	/* Nothing to do */
> > diff --git a/drivers/gpu/drm/xe/xe_vm_types.h b/drivers/gpu/drm/xe/xe_vm_types.h
> > index 39bfa259c7be..2ba7d9ad5ba6 100644
> > --- a/drivers/gpu/drm/xe/xe_vm_types.h
> > +++ b/drivers/gpu/drm/xe/xe_vm_types.h
> > @@ -23,6 +23,16 @@ struct xe_user_fence;
> >  struct xe_vm;
> >  struct xe_vm_pgtable_update_op;
> >  
> > +#if IS_ENABLED(CONFIG_DRM_XE_DEBUG)
> > +#define TEST_VM_OPS_ERROR
> > +#define FORCE_OP_ERROR	BIT(31)
> > +
> > +#define FORCE_OP_ERROR_LOCK	0
> > +#define FORCE_OP_ERROR_PREPARE	1
> > +#define FORCE_OP_ERROR_RUN	2
> > +#define FORCE_OP_ERROR_COUNT	3
> > +#endif
> > +
> >  #define XE_VMA_READ_ONLY	DRM_GPUVA_USERBITS
> >  #define XE_VMA_DESTROYED	(DRM_GPUVA_USERBITS << 1)
> >  #define XE_VMA_ATOMIC_PTE_BIT	(DRM_GPUVA_USERBITS << 2)
> > @@ -361,6 +371,10 @@ struct xe_vma_ops {
> >  	u32 num_syncs;
> >  	/** @pt_update_ops: page table update operations */
> >  	struct xe_vm_pgtable_update_ops pt_update_ops[XE_MAX_TILES_PER_DEVICE];
> > +#ifdef TEST_VM_OPS_ERROR
> > +	/** @inject_error: inject error to test error handling */
> > +	bool inject_error;
> > +#endif
> >  };
> >  
> >  #endif
> > -- 
> > 2.34.1
> > 
> >