[PATCH v3] drm/xe/vm: Add a helper xe_vm_range_tilemask_tlb_invalidation()
Summers, Stuart
stuart.summers at intel.com
Wed Jun 11 22:38:06 UTC 2025
On Wed, 2025-06-11 at 13:38 -0700, Matthew Brost wrote:
> On Wed, Jun 11, 2025 at 01:14:35PM -0600, Summers, Stuart wrote:
> > On Wed, 2025-06-11 at 12:09 -0700, Matthew Brost wrote:
> > > On Wed, Jun 11, 2025 at 12:27:09PM -0600, Summers, Stuart wrote:
> > > > On Wed, 2025-06-11 at 11:25 -0700, Matthew Brost wrote:
> > > > > On Wed, Jun 11, 2025 at 11:56:25AM -0600, Summers, Stuart
> > > > > wrote:
> > > > > > On Mon, 2025-06-09 at 09:46 +0530, Himal Prasad Ghimiray
> > > > > > wrote:
> > > > > > > Introduce xe_vm_range_tilemask_tlb_invalidation(), which
> > > > > > > issues a
> > > > > > > TLB
> > > > > > > invalidation for a specified address range across GTs
> > > > > > > indicated
> > > > > > > by a
> > > > > > > tilemask.
> > > > > > >
> > > > > > > v2 (Matthew Brost)
> > > > > > > - Move WARN_ON_ONCE to svm caller
> > > > > > > - Remove xe_gt_tlb_invalidation_vma
> > > > > > > - s/XE_WARN_ON/WARN_ON_ONCE
> > > > > > >
> > > > > > > v3
> > > > > > > - Rebase
> > > > > > >
> > > > > > > Suggested-by: Matthew Brost <matthew.brost at intel.com>
> > > > > > > Signed-off-by: Himal Prasad Ghimiray
> > > > > > > <himal.prasad.ghimiray at intel.com>
> > > > > > > Reviewed-by: Matthew Brost <matthew.brost at intel.com>
> > > > > > > ---
> > > > > > > drivers/gpu/drm/xe/xe_gt_tlb_invalidation.c | 24 -----
> > > > > > > drivers/gpu/drm/xe/xe_gt_tlb_invalidation.h | 3 -
> > > > > > > drivers/gpu/drm/xe/xe_svm.c | 43 +-----
> > > > > > > --
> > > > > > > drivers/gpu/drm/xe/xe_vm.c | 103
> > > > > > > +++++++++++++-
> > > > > > > ----
> > > > > > > --
> > > > > > > drivers/gpu/drm/xe/xe_vm.h | 3 +
> > > > > > > 5 files changed, 75 insertions(+), 101 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/gpu/drm/xe/xe_gt_tlb_invalidation.c
> > > > > > > b/drivers/gpu/drm/xe/xe_gt_tlb_invalidation.c
> > > > > > > index 084cbdeba8ea..0111ee59b81d 100644
> > > > > > > --- a/drivers/gpu/drm/xe/xe_gt_tlb_invalidation.c
> > > > > > > +++ b/drivers/gpu/drm/xe/xe_gt_tlb_invalidation.c
> > > > > > > @@ -440,30 +440,6 @@ void
> > > > > > > xe_gt_tlb_invalidation_vm(struct
> > > > > > > xe_gt
> > > > > > > *gt,
> > > > > > > struct xe_vm *vm)
> > > > > > > xe_gt_tlb_invalidation_fence_wait(&fence);
> > > > > > > }
> > > > > > >
> > > > > > > -/**
> > > > > > > - * xe_gt_tlb_invalidation_vma - Issue a TLB invalidation
> > > > > > > on
> > > > > > > this
> > > > > > > GT
> > > > > > > for a VMA
> > > > > > > - * @gt: GT structure
> > > > > > > - * @fence: invalidation fence which will be signal on
> > > > > > > TLB
> > > > > > > invalidation
> > > > > > > - * completion, can be NULL
> > > > > > > - * @vma: VMA to invalidate
> > > > > > > - *
> > > > > > > - * Issue a range based TLB invalidation if supported, if
> > > > > > > not
> > > > > > > fallback to a full
> > > > > > > - * TLB invalidation. Completion of TLB is asynchronous
> > > > > > > and
> > > > > > > caller
> > > > > > > can use
> > > > > > > - * the invalidation fence to wait for completion.
> > > > > > > - *
> > > > > > > - * Return: Negative error code on error, 0 on success
> > > > > > > - */
> > > > > > > -int xe_gt_tlb_invalidation_vma(struct xe_gt *gt,
> > > > > > > - struct
> > > > > > > xe_gt_tlb_invalidation_fence
> > > > > > > *fence,
> > > > > > > - struct xe_vma *vma)
> > > > > > > -{
> > > > > > > - xe_gt_assert(gt, vma);
> > > > > > > -
> > > > > > > - return xe_gt_tlb_invalidation_range(gt, fence,
> > > > > > > xe_vma_start(vma),
> > > > > > > -
> > > > > > > xe_vma_end(vma),
> > > > > > > -
> > > > > > > xe_vma_vm(vma)-
> > > > > > > > usm.asid);
> > > > > > > -}
> > > > > > > -
> > > > > > > /**
> > > > > > > * xe_guc_tlb_invalidation_done_handler - TLB
> > > > > > > invalidation
> > > > > > > done
> > > > > > > handler
> > > > > > > * @guc: guc
> > > > > > > diff --git a/drivers/gpu/drm/xe/xe_gt_tlb_invalidation.h
> > > > > > > b/drivers/gpu/drm/xe/xe_gt_tlb_invalidation.h
> > > > > > > index abe9b03d543e..31072dbcad8e 100644
> > > > > > > --- a/drivers/gpu/drm/xe/xe_gt_tlb_invalidation.h
> > > > > > > +++ b/drivers/gpu/drm/xe/xe_gt_tlb_invalidation.h
> > > > > > > @@ -19,9 +19,6 @@ int
> > > > > > > xe_gt_tlb_invalidation_init_early(struct
> > > > > > > xe_gt
> > > > > > > *gt);
> > > > > > >
> > > > > > > void xe_gt_tlb_invalidation_reset(struct xe_gt *gt);
> > > > > > > int xe_gt_tlb_invalidation_ggtt(struct xe_gt *gt);
> > > > > > > -int xe_gt_tlb_invalidation_vma(struct xe_gt *gt,
> > > > > > > - struct
> > > > > > > xe_gt_tlb_invalidation_fence
> > > > > > > *fence,
> > > > > > > - struct xe_vma *vma);
> > > > > > > void xe_gt_tlb_invalidation_vm(struct xe_gt *gt, struct
> > > > > > > xe_vm
> > > > > > > *vm);
> > > > > > > int xe_gt_tlb_invalidation_range(struct xe_gt *gt,
> > > > > > > struct
> > > > > > > xe_gt_tlb_invalidation_fence
> > > > > > > *fence,
> > > > > > > diff --git a/drivers/gpu/drm/xe/xe_svm.c
> > > > > > > b/drivers/gpu/drm/xe/xe_svm.c
> > > > > > > index 83c63fd7b481..3fd2d63c73a2 100644
> > > > > > > --- a/drivers/gpu/drm/xe/xe_svm.c
> > > > > > > +++ b/drivers/gpu/drm/xe/xe_svm.c
> > > > > > > @@ -167,14 +167,9 @@ static void xe_svm_invalidate(struct
> > > > > > > drm_gpusvm
> > > > > > > *gpusvm,
> > > > > > > {
> > > > > > > struct xe_vm *vm = gpusvm_to_vm(gpusvm);
> > > > > > > struct xe_device *xe = vm->xe;
> > > > > > > - struct xe_tile *tile;
> > > > > > > struct drm_gpusvm_range *r, *first;
> > > > > > > - struct xe_gt_tlb_invalidation_fence
> > > > > > > - fence[XE_MAX_TILES_PER_DEVICE *
> > > > > > > XE_MAX_GT_PER_TILE];
> > > > > > > u64 adj_start = mmu_range->start, adj_end =
> > > > > > > mmu_range-
> > > > > > > > end;
> > > > > > > u8 tile_mask = 0;
> > > > > > > - u8 id;
> > > > > > > - u32 fence_id = 0;
> > > > > > > long err;
> > > > > > >
> > > > > > > xe_svm_assert_in_notifier(vm);
> > > > > > > @@ -220,42 +215,8 @@ static void xe_svm_invalidate(struct
> > > > > > > drm_gpusvm
> > > > > > > *gpusvm,
> > > > > > >
> > > > > > > xe_device_wmb(xe);
> > > > > > >
> > > > > > > - for_each_tile(tile, xe, id) {
> > > > > > > - if (tile_mask & BIT(id)) {
> > > > > > > - int err;
> > > > > > > -
> > > > > > > -
> > > > > > > xe_gt_tlb_invalidation_fence_init(
> > > > > > > tile-
> > > > > > > > primary_gt,
> > > > > > > -
> > > > > > > &fence[fence_id], true);
> > > > > > > -
> > > > > > > - err =
> > > > > > > xe_gt_tlb_invalidation_range(tile-
> > > > > > > > primary_gt,
> > > > > > > -
> > > > > > >
> > > > > > > &fence[fence_id],
> > > > > > > -
> > > > > > >
> > > > > > > adj_start,
> > > > > > > -
> > > > > > >
> > > > > > > adj_end,
> > > > > > > -
> > > > > > >
> > > > > > > vm-
> > > > > > > > usm.asid);
> > > > > > > - if (WARN_ON_ONCE(err < 0))
> > > > > > > - goto wait;
> > > > > > > - ++fence_id;
> > > > > > > -
> > > > > > > - if (!tile->media_gt)
> > > > > > > - continue;
> > > > > > > -
> > > > > > > -
> > > > > > > xe_gt_tlb_invalidation_fence_init(
> > > > > > > tile-
> > > > > > > > media_gt,
> > > > > > > -
> > > > > > > &fence[fence_id], true);
> > > > > > > -
> > > > > > > - err =
> > > > > > > xe_gt_tlb_invalidation_range(tile-
> > > > > > > > media_gt,
> > > > > > > -
> > > > > > >
> > > > > > > &fence[fence_id],
> > > > > > > -
> > > > > > >
> > > > > > > adj_start,
> > > > > > > -
> > > > > > >
> > > > > > > adj_end,
> > > > > > > -
> > > > > > >
> > > > > > > vm-
> > > > > > > > usm.asid);
> > > > > > > - if (WARN_ON_ONCE(err < 0))
> > > > > > > - goto wait;
> > > > > > > - ++fence_id;
> > > > > > > - }
> > > > > > > - }
> > > > > > > -
> > > > > > > -wait:
> > > > > > > - for (id = 0; id < fence_id; ++id)
> > > > > > > -
> > > > > > > xe_gt_tlb_invalidation_fence_wait(&fence[i
> > > > > > > d]);
> > > > > > > + err = xe_vm_range_tilemask_tlb_invalidation(vm,
> > > > > > > adj_start,
> > > > > > > adj_end, tile_mask);
> > > > > > > + WARN_ON_ONCE(err);
> > > > > > >
> > > > > > > range_notifier_event_end:
> > > > > > > r = first;
> > > > > > > diff --git a/drivers/gpu/drm/xe/xe_vm.c
> > > > > > > b/drivers/gpu/drm/xe/xe_vm.c
> > > > > > > index 18f967ce1f1a..a0c70698265a 100644
> > > > > > > --- a/drivers/gpu/drm/xe/xe_vm.c
> > > > > > > +++ b/drivers/gpu/drm/xe/xe_vm.c
> > > > > > > @@ -3842,6 +3842,68 @@ void xe_vm_unlock(struct xe_vm
> > > > > > > *vm)
> > > > > > > dma_resv_unlock(xe_vm_resv(vm));
> > > > > > > }
> > > > > > >
> > > > > > > +/**
> > > > > > > + * xe_vm_range_tilemask_tlb_invalidation - Issue a TLB
> > > > > > > invalidation
> > > > > > > on this tilemask for an
> > > > > > > + * address range
> > > > > > > + * @vm: The VM
> > > > > > > + * @start: start address
> > > > > > > + * @end: end address
> > > > > > > + * @tile_mask: mask for which gt's issue tlb
> > > > > > > invalidation
> > > > > > > + *
> > > > > > > + * Issue a range based TLB invalidation for gt's in
> > > > > > > tilemask
> > > > > > > + *
> > > > > > > + * Returns 0 for success, negative error code otherwise.
> > > > > > > + */
> > > > > > > +int xe_vm_range_tilemask_tlb_invalidation(struct xe_vm
> > > > > > > *vm,
> > > > > > > u64
> > > > > > > start,
> > > > > > > + u64 end, u8
> > > > > > > tile_mask)
> > > > > > > +{
> > > > > > > + struct xe_gt_tlb_invalidation_fence
> > > > > > > fence[XE_MAX_TILES_PER_DEVICE * XE_MAX_GT_PER_TILE];
> > > > > > > + struct xe_tile *tile;
> > > > > > > + u32 fence_id = 0;
> > > > > > > + u8 id;
> > > > > > > + int err;
> > > > > > > +
> > > > > > > + if (!tile_mask)
> > > > > > > + return 0;
> > > > > > > +
> > > > > > > + for_each_tile(tile, vm->xe, id) {
> > > > > > > + if (tile_mask & BIT(id)) {
> > > > > > > + xe_gt_tlb_invalidation_fence_init
> > > > > > > (til
> > > > > > > e-
> > > > > > > > primary_gt,
> > > > > > > +
> > > > > > > &fence[fence_id], true);
> > > > > > > +
> > > > > > > + err =
> > > > > > > xe_gt_tlb_invalidation_range(tile-
> > > > > > > > primary_gt,
> > > > > > > +
> > > > > > >
> > > > > > > &fence[fence_id],
> > > > > > > +
> > > > > > >
> > > > > > > start,
> > > > > > > +
> > > > > > >
> > > > > > > end,
> > > > > > > +
> > > > > > >
> > > > > > > vm-
> > > > > > > > usm.asid);
> > > > > >
> > > > > > Since we're already doing the refactoring of the svm and
> > > > > > non-
> > > > > > svm
> > > > > > cases
> > > > > > here, can you also refactor this so we have something like:
> > > > > > invalidate_tiles(tile_mask)
> > > > > >
> > > > > > void invalidate_tiles(tile_mask)
> > > > > > {
> > > > > > for_each_tile(tile, tile_mask)
> > > > > > invalidate_tile(tile)
> > > > > > }
> > > > > >
> > > > > > void invalidate_tile(tile)
> > > > > > {
> > > > > > fence[i] = invalidate_gt(primary)
> > > > > > fence[i++] = invalidate_gt(media)
> > > > > >
> > > > > > for_each_i(i)
> > > > > > wait_for_invalidation(i)
> > > > >
> > > > > You'd lose a level pipeline with this suggestion. The can
> > > > > write
> > > > > is
> > > > > quite
> > > > > intentional - issue all TLB invalidations to hardware, then
> > > > > wait
> > > > > on
> > > > > all
> > > > > invalidtaions. What you suggest would issue TLB invalidations
> > > > > to
> > > > > each
> > > > > tile and wait invalidations from that tile - if you have
> > > > > multiple
> > > > > tiles
> > > > > you delay issuing the invalidations until the prior tile
> > > > > completes
> > > > > its
> > > > > invalidations. This could be cleaned up with helpers but
> > > > > regardless
> > > > > we
> > > > > want the pipeling in place.
> > > >
> > > > Ok makes sense. Then we can just pull the
> > > > wait_for_invalidation()
> > > > out
> > > > to the invalidate_tiles() piece:
> > > >
> > > > void invalidate_tiles(tile_mask)
> > > > for_each_tile(tile, tile_mask)
> > > > fence[i++] = invalidate_tile(tile)
> > > >
> > > > for_each_i(i)
> > > > wait_for_invalidation(i)
> > > >
> > >
> > > That still doesn't work as each tile can generate multiple fences
> > > if
> > > it
> > > has a media GT. Yes, we could have helper which takes a fence
> > > index
> > > by
> > > reference by ROI on this low IMO - this is not some crazy a
> > > couple
> > > 100
> > > line function which is hard to follow.
> >
> > Perhaps, but we should also be in the habbit of coding functions
> > that
> > try to perform just a single operation as much as possible. This is
>
> Sure, hence the helper here which reduces the LoC. Another helper
> which
> is used in exactly one spot, touches stack variables from another
> function is kinda ick but that is just an opinion.
>
> > what prevents the bloat down the road. I also don't think the
> > suggestion I had here was intrusive enough to warrant strong
> > pushback.
> >
>
> Well to be fair, both of your suggestions do not really work - that
> the
> is the main issue at hand.
>
Also feeling a little under the weather here so please bear with me.
Let me send an actual code example of what I was thinking rather than
pseudocode. Hopefully that will clear things up. Not trying to block
the review. I'll respond back this afternoon if I'm not able to get
this out.
Thanks,
Stuart
>
> Matt
>
> > Thanks,
> > Stuart
> >
> > >
> > > Matt
> > >
> > > >
> > > > Thanks,
> > > > Stuart
> > > >
> > > > >
> > > > > Matt
> > > > >
> > > > > > }
> > > > > >
> > > > > > invalidate_gt(gt_type)
> > > > > > {
> > > > > > invalidation_fence_init(gt_type)
> > > > > > fence = invalidation_fence_range(gt_type)
> > > > > >
> > > > > > return fence
> > > > > > }
> > > > > >
> > > > > > Thanks,
> > > > > > Stuart
> > > > > >
> > > > > > > + if (err)
> > > > > > > + goto wait;
> > > > > > > + ++fence_id;
> > > > > > > +
> > > > > > > + if (!tile->media_gt)
> > > > > > > + continue;
> > > > > > > +
> > > > > > > + xe_gt_tlb_invalidation_fence_init
> > > > > > > (til
> > > > > > > e-
> > > > > > > > media_gt,
> > > > > > > +
> > > > > > > &fence[fence_id], true);
> > > > > > > +
> > > > > > > + err =
> > > > > > > xe_gt_tlb_invalidation_range(tile-
> > > > > > > > media_gt,
> > > > > > > +
> > > > > > >
> > > > > > > &fence[fence_id],
> > > > > > > +
> > > > > > >
> > > > > > > start,
> > > > > > > +
> > > > > > >
> > > > > > > end,
> > > > > > > +
> > > > > > >
> > > > > > > vm-
> > > > > > > > usm.asid);
> > > > > > > + if (err)
> > > > > > > + goto wait;
> > > > > > > + ++fence_id;
> > > > > > > + }
> > > > > > > + }
> > > > > > > +
> > > > > > > +wait:
> > > > > > > + for (id = 0; id < fence_id; ++id)
> > > > > > > + xe_gt_tlb_invalidation_fence_wait(&fence[
> > > > > > > id])
> > > > > > > ;
> > > > > > > +
> > > > > > > + return err;
> > > > > > > +}
> > > > > > > +
> > > > > > > /**
> > > > > > > * xe_vm_invalidate_vma - invalidate GPU mappings for
> > > > > > > VMA
> > > > > > > without a
> > > > > > > lock
> > > > > > > * @vma: VMA to invalidate
> > > > > > > @@ -3857,11 +3919,9 @@ int xe_vm_invalidate_vma(struct
> > > > > > > xe_vma
> > > > > > > *vma)
> > > > > > > struct xe_device *xe = xe_vma_vm(vma)->xe;
> > > > > > > struct xe_vm *vm = xe_vma_vm(vma);
> > > > > > > struct xe_tile *tile;
> > > > > > > - struct xe_gt_tlb_invalidation_fence
> > > > > > > - fence[XE_MAX_TILES_PER_DEVICE *
> > > > > > > XE_MAX_GT_PER_TILE];
> > > > > > > - u8 id;
> > > > > > > - u32 fence_id = 0;
> > > > > > > + u8 tile_mask = 0;
> > > > > > > int ret = 0;
> > > > > > > + u8 id;
> > > > > > >
> > > > > > > xe_assert(xe, !xe_vma_is_null(vma));
> > > > > > > xe_assert(xe, !xe_vma_is_cpu_addr_mirror(vma));
> > > > > > > @@ -3892,37 +3952,14 @@ int xe_vm_invalidate_vma(struct
> > > > > > > xe_vma
> > > > > > > *vma)
> > > > > > > }
> > > > > > > }
> > > > > > >
> > > > > > > - for_each_tile(tile, xe, id) {
> > > > > > > - if (xe_pt_zap_ptes(tile, vma)) {
> > > > > > > - xe_device_wmb(xe);
> > > > > > > -
> > > > > > > xe_gt_tlb_invalidation_fence_init(
> > > > > > > tile-
> > > > > > > > primary_gt,
> > > > > > > -
> > > > > > > &fence[fence_id],
> > > > > > > -
> > > > > > > true);
> > > > > > > -
> > > > > > > - ret =
> > > > > > > xe_gt_tlb_invalidation_vma(tile-
> > > > > > > > primary_gt,
> > > > > > > -
> > > > > > > &fence[fence_id], vma);
> > > > > > > - if (ret)
> > > > > > > - goto wait;
> > > > > > > - ++fence_id;
> > > > > > > -
> > > > > > > - if (!tile->media_gt)
> > > > > > > - continue;
> > > > > > > -
> > > > > > > -
> > > > > > > xe_gt_tlb_invalidation_fence_init(
> > > > > > > tile-
> > > > > > > > media_gt,
> > > > > > > -
> > > > > > > &fence[fence_id],
> > > > > > > -
> > > > > > > true);
> > > > > > > + for_each_tile(tile, xe, id)
> > > > > > > + if (xe_pt_zap_ptes(tile, vma))
> > > > > > > + tile_mask |= BIT(id);
> > > > > > >
> > > > > > > - ret =
> > > > > > > xe_gt_tlb_invalidation_vma(tile-
> > > > > > > > media_gt,
> > > > > > > -
> > > > > > > &fence[fence_id], vma);
> > > > > > > - if (ret)
> > > > > > > - goto wait;
> > > > > > > - ++fence_id;
> > > > > > > - }
> > > > > > > - }
> > > > > > > + xe_device_wmb(xe);
> > > > > > >
> > > > > > > -wait:
> > > > > > > - for (id = 0; id < fence_id; ++id)
> > > > > > > -
> > > > > > > xe_gt_tlb_invalidation_fence_wait(&fence[i
> > > > > > > d]);
> > > > > > > + ret =
> > > > > > > xe_vm_range_tilemask_tlb_invalidation(xe_vma_vm(vma),
> > > > > > > xe_vma_start(vma),
> > > > > > > +
> > > > > > > xe_vma_end(vma),
> > > > > > > tile_mask);
> > > > > > >
> > > > > > > /* WRITE_ONCE pair with READ_ONCE in
> > > > > > > xe_gt_pagefault.c */
> > > > > > > WRITE_ONCE(vma->tile_invalidated, vma-
> > > > > > > >tile_mask);
> > > > > > > diff --git a/drivers/gpu/drm/xe/xe_vm.h
> > > > > > > b/drivers/gpu/drm/xe/xe_vm.h
> > > > > > > index 88d6c0ef89c7..acd3fd6c605b 100644
> > > > > > > --- a/drivers/gpu/drm/xe/xe_vm.h
> > > > > > > +++ b/drivers/gpu/drm/xe/xe_vm.h
> > > > > > > @@ -228,6 +228,9 @@ struct dma_fence
> > > > > > > *xe_vm_range_rebind(struct
> > > > > > > xe_vm
> > > > > > > *vm,
> > > > > > > struct dma_fence *xe_vm_range_unbind(struct xe_vm *vm,
> > > > > > > struct xe_svm_range
> > > > > > > *range);
> > > > > > >
> > > > > > > +int xe_vm_range_tilemask_tlb_invalidation(struct xe_vm
> > > > > > > *vm,
> > > > > > > u64
> > > > > > > start,
> > > > > > > + u64 end, u8
> > > > > > > tile_mask);
> > > > > > > +
> > > > > > > int xe_vm_invalidate_vma(struct xe_vma *vma);
> > > > > > >
> > > > > > > int xe_vm_validate_protected(struct xe_vm *vm);
> > > > > >
> > > >
> >
More information about the Intel-xe
mailing list