[PATCH v2 6/6] drm/xe: Add VM bind IOCTL error injection
Matthew Brost
matthew.brost at intel.com
Wed May 15 22:45:38 UTC 2024
On Wed, May 15, 2024 at 03:29:18PM -0600, Cavitt, Jonathan wrote:
> -----Original Message-----
> From: Intel-xe <intel-xe-bounces at lists.freedesktop.org> On Behalf Of Matthew Brost
> Sent: Tuesday, May 14, 2024 5:40 PM
> To: intel-xe at lists.freedesktop.org
> Cc: Brost, Matthew <matthew.brost at 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 at intel.com>
>
> I have a minor nit lower down, but nothing worth blocking over:
> Reviewed-by: Jonathan Cavitt <jonathan.cavitt at 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
> >
> >
More information about the Intel-xe
mailing list