Re: [PATCH v3 09/25] drm/xe: Add struct xe_pt_job_ops

["Summers, Stuart" <stuart.summers@intel.com>]

On Fri, 2026-02-27 at 17:34 -0800, Matthew Brost wrote:
> Add struct xe_pt_job_ops, a dynamically refcounted object that
> contains
> the information required to issue a CPU bind via a job after the
> initial
> bind IOCTL returns.
> 
> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_migrate.c  |  10 +--
>  drivers/gpu/drm/xe/xe_pt.c       | 132 +++++++++++++++++++++++++++--
> --
>  drivers/gpu/drm/xe/xe_pt.h       |   4 +
>  drivers/gpu/drm/xe/xe_pt_types.h |  27 +++++--
>  drivers/gpu/drm/xe/xe_vm.c       |  10 +--
>  5 files changed, 149 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_migrate.c
> b/drivers/gpu/drm/xe/xe_migrate.c
> index 69e6e3135ec6..cd6802642ef3 100644
> --- a/drivers/gpu/drm/xe/xe_migrate.c
> +++ b/drivers/gpu/drm/xe/xe_migrate.c
> @@ -1771,7 +1771,7 @@ xe_migrate_update_pgtables_cpu(struct
> xe_migrate *m,
>         }
>  
>         xe_migrate_update_pgtables_cpu_execute(vm, m->tile, ops,
> -                                              pt_update_ops->ops,
> +                                              pt_update_ops-
> >pt_job_ops->ops,
>                                                pt_update_ops-
> >num_ops);
>  
>         return dma_fence_get_stub();
> @@ -1798,7 +1798,7 @@ __xe_migrate_update_pgtables(struct xe_migrate
> *m,
>         bool usm = is_migrate && xe->info.has_usm;
>  
>         for (i = 0; i < pt_update_ops->num_ops; ++i) {
> -               struct xe_vm_pgtable_update_op *pt_op =
> &pt_update_ops->ops[i];
> +               struct xe_vm_pgtable_update_op *pt_op =
> &pt_update_ops->pt_job_ops->ops[i];
>                 struct xe_vm_pgtable_update *updates = pt_op-
> >entries;
>  
>                 num_updates += pt_op->num_entries;
> @@ -1867,7 +1867,7 @@ __xe_migrate_update_pgtables(struct xe_migrate
> *m,
>  
>                         for (; i < pt_update_ops->num_ops; ++i) {
>                                 struct xe_vm_pgtable_update_op *pt_op
> =
> -                                       &pt_update_ops->ops[i];
> +                                       &pt_update_ops->pt_job_ops-
> >ops[i];
>                                 struct xe_vm_pgtable_update *updates
> = pt_op->entries;
>  
>                                 for (; j < pt_op->num_entries; ++j,
> ++current_update, ++idx) {
> @@ -1904,7 +1904,7 @@ __xe_migrate_update_pgtables(struct xe_migrate
> *m,
>                         (page_ofs / sizeof(u64)) * XE_PAGE_SIZE;
>                 for (i = 0; i < pt_update_ops->num_ops; ++i) {
>                         struct xe_vm_pgtable_update_op *pt_op =
> -                               &pt_update_ops->ops[i];
> +                               &pt_update_ops->pt_job_ops->ops[i];
>                         struct xe_vm_pgtable_update *updates = pt_op-
> >entries;
>  
>                         for (j = 0; j < pt_op->num_entries; ++j) {
> @@ -1922,7 +1922,7 @@ __xe_migrate_update_pgtables(struct xe_migrate
> *m,
>  
>                 for (i = 0; i < pt_update_ops->num_ops; ++i) {
>                         struct xe_vm_pgtable_update_op *pt_op =
> -                               &pt_update_ops->ops[i];
> +                               &pt_update_ops->pt_job_ops->ops[i];
>                         struct xe_vm_pgtable_update *updates = pt_op-
> >entries;
>  
>                         for (j = 0; j < pt_op->num_entries; ++j)
> diff --git a/drivers/gpu/drm/xe/xe_pt.c b/drivers/gpu/drm/xe/xe_pt.c
> index 1f24eff75185..6b56e62a35c1 100644
> --- a/drivers/gpu/drm/xe/xe_pt.c
> +++ b/drivers/gpu/drm/xe/xe_pt.c
> @@ -204,7 +204,9 @@ unsigned int xe_pt_shift(unsigned int level)
>   * and finally frees @pt. TODO: Can we remove the @flags argument?
>   */
>  void xe_pt_destroy(struct xe_pt *pt, u32 flags, struct llist_head
> *deferred)
> +

Extra line..

>  {
> +       bool added = false;
>         int i;
>  
>         if (!pt)
> @@ -212,7 +214,20 @@ void xe_pt_destroy(struct xe_pt *pt, u32 flags,
> struct llist_head *deferred)
>  
>         XE_WARN_ON(!list_empty(&pt->bo->ttm.base.gpuva.list));
>         xe_bo_unpin(pt->bo);
> -       xe_bo_put_deferred(pt->bo, deferred, NULL);
> +       xe_bo_put_deferred(pt->bo, deferred, &added);
> +       if (added) {
> +               xe_assert(pt->bo->vm->xe, !kref_read(&pt->bo-
> >ttm.base.refcount));
> +
> +               /*
> +                * We need the VM present until the BO is destroyed
> as it shares
> +                * a dma-resv and BO destroy is async. Reinit BO
> refcount so
> +                * xe_bo_put_async can be used when the PT job ops
> refcount goes
> +                * to zero.
> +                */
> +               xe_vm_get(pt->bo->vm);
> +               pt->bo->flags |= XE_BO_FLAG_PUT_VM_ASYNC;
> +               kref_init(&pt->bo->ttm.base.refcount);
> +       }
>  
>         if (pt->level > 0 && pt->num_live) {
>                 struct xe_pt_dir *pt_dir = as_xe_pt_dir(pt);
> @@ -1884,13 +1899,13 @@ xe_pt_commit_prepare_unbind(struct xe_vma
> *vma,
>  static struct xe_vm_pgtable_update_op *
>  to_pt_op(struct xe_vm_pgtable_update_ops *pt_update_ops, u32 op_idx)
>  {
> -       return &pt_update_ops->ops[op_idx];
> +       return &pt_update_ops->pt_job_ops->ops[op_idx];
>  }
>  
>  static u32
>  get_current_op(struct xe_vm_pgtable_update_ops *pt_update_ops)
>  {
> -       return pt_update_ops->current_op;
> +       return pt_update_ops->pt_job_ops->current_op;
>  }
>  
>  static struct xe_vm_pgtable_update_op *
> @@ -1902,7 +1917,7 @@ to_current_pt_op(struct
> xe_vm_pgtable_update_ops *pt_update_ops)
>  static void
>  incr_current_op(struct xe_vm_pgtable_update_ops *pt_update_ops)
>  {
> -       ++pt_update_ops->current_op;
> +       ++pt_update_ops->pt_job_ops->current_op;
>  }
>  
>  static void
> @@ -2264,7 +2279,6 @@ static int op_prepare(struct xe_vm *vm,
>  static void
>  xe_pt_update_ops_init(struct xe_vm_pgtable_update_ops
> *pt_update_ops)
>  {
> -       init_llist_head(&pt_update_ops->deferred);
>         pt_update_ops->start = ~0x0ull;
>         pt_update_ops->last = 0x0ull;
>         xe_page_reclaim_list_init(&pt_update_ops->prl);
> @@ -2612,7 +2626,8 @@ xe_pt_update_ops_run(struct xe_tile *tile,
> struct xe_vma_ops *vops)
>                         to_pt_op(pt_update_ops, i);
>  
>                 xe_pt_commit(pt_op->vma, pt_op->entries,
> -                            pt_op->num_entries, &pt_update_ops-
> >deferred);
> +                            pt_op->num_entries,
> +                            &pt_update_ops->pt_job_ops->deferred);
>                 pt_op->vma = NULL;      /* skip in
> xe_pt_update_ops_abort */
>         }
>  
> @@ -2700,19 +2715,8 @@ void xe_pt_update_ops_fini(struct xe_tile
> *tile, struct xe_vma_ops *vops)
>  {
>         struct xe_vm_pgtable_update_ops *pt_update_ops =
>                 &vops->pt_update_ops[tile->id];
> -       int i;
>  
>         xe_page_reclaim_entries_put(pt_update_ops->prl.entries);
> -
> -       lockdep_assert_held(&vops->vm->lock);
> -       xe_vm_assert_held(vops->vm);
> -
> -       for (i = 0; i < pt_update_ops->current_op; ++i) {
> -               struct xe_vm_pgtable_update_op *pt_op =
> &pt_update_ops->ops[i];
> -
> -               xe_pt_free_bind(pt_op->entries, pt_op->num_entries);
> -       }
> -       xe_bo_put_commit(&vops->pt_update_ops[tile->id].deferred);
>  }
>  
>  /**
> @@ -2749,3 +2753,97 @@ void xe_pt_update_ops_abort(struct xe_tile
> *tile, struct xe_vma_ops *vops)
>  
>         xe_pt_update_ops_fini(tile, vops);
>  }
> +
> +/**
> + * xe_pt_job_ops_alloc() - Allocate PT job ops
> + * @num_ops: Number of VM PT update ops
> + *
> + * Allocate PT job ops and internal array of VM PT update ops.
> + *
> + * Return: Pointer to PT job ops or NULL
> + */
> +struct xe_pt_job_ops *xe_pt_job_ops_alloc(u32 num_ops)
> +{
> +       struct xe_pt_job_ops *pt_job_ops;
> +
> +       pt_job_ops = kmalloc(sizeof(*pt_job_ops), GFP_KERNEL);
> +       if (!pt_job_ops)
> +               return NULL;
> +
> +       pt_job_ops->ops = kvmalloc_array(num_ops, sizeof(*pt_job_ops-
> >ops),
> +                                        GFP_KERNEL);
> +       if (!pt_job_ops->ops) {
> +               kvfree(pt_job_ops);

This should be kfree right?

Thanks,
Stuart

> +               return NULL;
> +       }
> +
> +       pt_job_ops->current_op = 0;
> +       kref_init(&pt_job_ops->refcount);
> +       init_llist_head(&pt_job_ops->deferred);
> +
> +       return pt_job_ops;
> +}
> +
> +/**
> + * xe_pt_job_ops_get() - Get PT job ops
> + * @pt_job_ops: PT job ops to get
> + *
> + * Take a reference to PT job ops
> + *
> + * Return: Pointer to PT job ops or NULL
> + */
> +struct xe_pt_job_ops *xe_pt_job_ops_get(struct xe_pt_job_ops
> *pt_job_ops)
> +{
> +       if (pt_job_ops)
> +               kref_get(&pt_job_ops->refcount);
> +
> +       return pt_job_ops;
> +}
> +
> +static void xe_pt_update_ops_free(struct xe_vm_pgtable_update_op
> *pt_op,
> +                                 u32 num_ops)
> +{
> +       u32 i;
> +
> +       for (i = 0; i < num_ops; ++i, ++pt_op)
> +               xe_pt_free_bind(pt_op->entries, pt_op->num_entries);
> +}
> +
> +static void xe_pt_job_ops_destroy(struct kref *ref)
> +{
> +       struct xe_pt_job_ops *pt_job_ops =
> +               container_of(ref, struct xe_pt_job_ops, refcount);
> +       struct llist_node *freed;
> +       struct xe_bo *bo, *next;
> +
> +       xe_pt_update_ops_free(pt_job_ops->ops,
> +                             pt_job_ops->current_op);
> +
> +       freed = llist_del_all(&pt_job_ops->deferred);
> +       if (freed) {
> +               llist_for_each_entry_safe(bo, next, freed, freed)
> +                       /*
> +                        * If called from run_job, we are in the dma-
> fencing
> +                        * path and cannot take dma-resv locks so use
> an async
> +                        * put.
> +                        */
> +                       xe_bo_put_async(bo);
> +       }
> +
> +       kvfree(pt_job_ops->ops);
> +       kfree(pt_job_ops);
> +}
> +
> +/**
> + * xe_pt_job_ops_put() - Put PT job ops
> + * @pt_job_ops: PT job ops to put
> + *
> + * Drop a reference to PT job ops
> + */
> +void xe_pt_job_ops_put(struct xe_pt_job_ops *pt_job_ops)
> +{
> +       if (!pt_job_ops)
> +               return;
> +
> +       kref_put(&pt_job_ops->refcount, xe_pt_job_ops_destroy);
> +}
> diff --git a/drivers/gpu/drm/xe/xe_pt.h b/drivers/gpu/drm/xe/xe_pt.h
> index 4daeebaab5a1..5faddb8e700c 100644
> --- a/drivers/gpu/drm/xe/xe_pt.h
> +++ b/drivers/gpu/drm/xe/xe_pt.h
> @@ -49,4 +49,8 @@ bool xe_pt_zap_ptes(struct xe_tile *tile, struct
> xe_vma *vma);
>  bool xe_pt_zap_ptes_range(struct xe_tile *tile, struct xe_vm *vm,
>                           struct xe_svm_range *range);
>  
> +struct xe_pt_job_ops *xe_pt_job_ops_alloc(u32 num_ops);
> +struct xe_pt_job_ops *xe_pt_job_ops_get(struct xe_pt_job_ops
> *pt_job_ops);
> +void xe_pt_job_ops_put(struct xe_pt_job_ops *pt_job_ops);
> +
>  #endif
> diff --git a/drivers/gpu/drm/xe/xe_pt_types.h
> b/drivers/gpu/drm/xe/xe_pt_types.h
> index 84b51d3762a4..92d50573ed1d 100644
> --- a/drivers/gpu/drm/xe/xe_pt_types.h
> +++ b/drivers/gpu/drm/xe/xe_pt_types.h
> @@ -91,12 +91,29 @@ struct xe_vm_pgtable_update_op {
>         bool rebind;
>  };
>  
> +/**
> + * struct xe_pt_job_ops - Page-table update operations (dynamically
> allocated)
> + *
> + * This is the portion of &struct xe_vma_ops and
> + * &struct xe_vm_pgtable_update_ops that is dynamically allocated,
> as it
> + * must remain valid until the associated bind job completes. A
> reference
> + * count controls its lifetime.
> + */
> +struct xe_pt_job_ops {
> +       /** @current_op: current page-table update operation */
> +       u32 current_op;
> +       /** @refcount: reference count */
> +       struct kref refcount;
> +       /** @deferred: list of deferred PT entries to destroy */
> +       struct llist_head deferred;
> +       /** @ops: page-table update operations */
> +       struct xe_vm_pgtable_update_op *ops;
> +};
> +
>  /** struct xe_vm_pgtable_update_ops: page table update operations */
>  struct xe_vm_pgtable_update_ops {
> -       /** @ops: operations */
> -       struct xe_vm_pgtable_update_op *ops;
> -       /** @deferred: deferred list to destroy PT entries */
> -       struct llist_head deferred;
> +       /** @pt_job_ops: PT update operations dynamic allocation*/
> +       struct xe_pt_job_ops *pt_job_ops;
>         /** @q: exec queue for PT operations */
>         struct xe_exec_queue *q;
>         /** @prl: embedded page reclaim list */
> @@ -107,8 +124,6 @@ struct xe_vm_pgtable_update_ops {
>         u64 last;
>         /** @num_ops: number of operations */
>         u32 num_ops;
> -       /** @current_op: current operations */
> -       u32 current_op;
>         /** @needs_svm_lock: Needs SVM lock */
>         bool needs_svm_lock;
>         /** @needs_invalidation: Needs invalidation */
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index 548b0769b3ef..3e2d2191b78c 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -585,11 +585,9 @@ static int xe_vma_ops_alloc(struct xe_vma_ops
> *vops, bool array_of_binds)
>                 if (!vops->pt_update_ops[i].num_ops)
>                         continue;
>  
> -               vops->pt_update_ops[i].ops =
> -                       kmalloc_objs(*vops->pt_update_ops[i].ops,
> -                                    vops->pt_update_ops[i].num_ops,
> -                                    GFP_KERNEL | __GFP_RETRY_MAYFAIL
> | __GFP_NOWARN);
> -               if (!vops->pt_update_ops[i].ops)
> +               vops->pt_update_ops[i].pt_job_ops =
> +                       xe_pt_job_ops_alloc(vops-
> >pt_update_ops[i].num_ops);
> +               if (!vops->pt_update_ops[i].pt_job_ops)
>                         return array_of_binds ? -ENOBUFS : -ENOMEM;
>         }
>  
> @@ -625,7 +623,7 @@ static void xe_vma_ops_fini(struct xe_vma_ops
> *vops)
>         xe_vma_svm_prefetch_ops_fini(vops);
>  
>         for (i = 0; i < XE_MAX_TILES_PER_DEVICE; ++i)
> -               kfree(vops->pt_update_ops[i].ops);
> +               xe_pt_job_ops_put(vops->pt_update_ops[i].pt_job_ops);
>  }
>  
>  static void xe_vma_ops_incr_pt_update_ops(struct xe_vma_ops *vops,
> u8 tile_mask, int inc_val)