[Intel-xe] [PATCH 3/3] drm/xe: NULL binding implementation

[Niranjana Vishwanathapura]

On Thu, Jun 15, 2023 at 06:01:15PM +0000, Matthew Brost wrote:
>On Tue, Jun 13, 2023 at 07:49:07PM -0700, Niranjana Vishwanathapura wrote:
>> On Wed, Jun 07, 2023 at 10:03:33PM -0700, Matthew Brost wrote:
>> > Add uAPI and implementation for NULL bindings. A NULL binding is defined
>> > as writes dropped and read zero. A single bit in the uAPI has been added
>> > which results in a single bit in the PTEs being set.
>> >
>> > NULL bindings are indended to be used to implement VK sparse bindings,
>> > in particular residencyNonResidentStrict property.
>> >
>> > Suggested-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
>> > Signed-off-by: Matthew Brost <matthew.brost at intel.com>
>> > ---
>> > drivers/gpu/drm/xe/xe_bo.h           |  1 +
>> > drivers/gpu/drm/xe/xe_exec.c         |  2 +
>> > drivers/gpu/drm/xe/xe_gt_pagefault.c |  4 +-
>> > drivers/gpu/drm/xe/xe_pt.c           | 71 +++++++++++++++------
>> > drivers/gpu/drm/xe/xe_vm.c           | 95 ++++++++++++++++++----------
>> > drivers/gpu/drm/xe/xe_vm.h           | 12 +++-
>> > drivers/gpu/drm/xe/xe_vm_types.h     |  1 +
>> > include/uapi/drm/xe_drm.h            |  8 +++
>> > 8 files changed, 138 insertions(+), 56 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/xe/xe_bo.h b/drivers/gpu/drm/xe/xe_bo.h
>> > index dd3d448fee0b..3a148cc6e811 100644
>> > --- a/drivers/gpu/drm/xe/xe_bo.h
>> > +++ b/drivers/gpu/drm/xe/xe_bo.h
>> > @@ -61,6 +61,7 @@
>> > #define XE_PPGTT_PTE_LM			BIT_ULL(11)
>> > #define XE_PDE_64K			BIT_ULL(6)
>> > #define XE_PTE_PS64			BIT_ULL(8)
>> > +#define XE_PTE_NULL			BIT_ULL(9)
>> >
>> > #define XE_PAGE_PRESENT			BIT_ULL(0)
>> > #define XE_PAGE_RW			BIT_ULL(1)
>> > diff --git a/drivers/gpu/drm/xe/xe_exec.c b/drivers/gpu/drm/xe/xe_exec.c
>> > index e44076ee2e11..4f7694a29348 100644
>> > --- a/drivers/gpu/drm/xe/xe_exec.c
>> > +++ b/drivers/gpu/drm/xe/xe_exec.c
>> > @@ -120,6 +120,8 @@ static int xe_exec_begin(struct xe_engine *e, struct ww_acquire_ctx *ww,
>> > 	 * to a location where the GPU can access it).
>> > 	 */
>> > 	list_for_each_entry(vma, &vm->rebind_list, rebind_link) {
>> > +		XE_WARN_ON(xe_vma_is_null(vma));
>> > +
>>
>> Also return error here?
>>
>
>No, just make clear NULL bindings should never be rebound.
>

But in such case, this function further down will reference vma->bo which
will be NULL and hence runs into NULL pointer exception?

>> > 		if (xe_vma_is_userptr(vma))
>> > 			continue;
>> >

<snip>

>> > @@ -3389,6 +3411,7 @@ int xe_vm_invalidate_vma(struct xe_vma *vma)
>> > 	int ret;
>> >
>> > 	XE_BUG_ON(!xe_vm_in_fault_mode(vma->vm));
>> > +	XE_WARN_ON(xe_vma_is_null(vma));
>>
>> return error?
>>
>
>No, again just make it clear you can't invalidate a NULL binding.
>

Same here.

Niranjana

>> > 	trace_xe_vma_usm_invalidate(vma);
>> >
>> > 	/* Check that we don't race with page-table updates */