Re: [PATCH v3 1/2] drm/xe/vf: Refactor CCS save/restore to use default migration context

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

On Wed, 2025-08-06 at 09:20 -0700, Matthew Brost wrote:
> On Wed, Aug 06, 2025 at 10:08:01AM -0600, Summers, Stuart wrote:
> > On Wed, 2025-08-06 at 13:59 +0530, Satyanarayana K V P wrote:
> > > Previously, CCS save/restore operations created separate
> > > migration
> > > contexts with new VM memory allocations, resulting in significant
> > > overhead.
> > > 
> > > This commit eliminates redundant context creation reusing the
> > > default
> > > migration context by registering new execution queues for CCS
> > > save
> > > and
> > > restore on the existing migrate VM.
> > > 
> > > Signed-off-by: Satyanarayana K V P
> > > <satyanarayana.k.v.p@intel.com>
> > > Suggested-by: Matthew Brost <matthew.brost@intel.com>
> > > Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> > > 
> > > ---
> > > V2 -> V3:
> > > - Fixed review comments (Matthew Brost).
> > > 
> > > V1 -> V2:
> > > - Fixed kernel-doc issues reported by patchworks.
> > > ---
> > >  drivers/gpu/drm/xe/xe_migrate.c            | 18 ++++----
> > >  drivers/gpu/drm/xe/xe_migrate.h            |  2 +-
> > >  drivers/gpu/drm/xe/xe_pm.c                 |  3 ++
> > >  drivers/gpu/drm/xe/xe_sriov_vf_ccs.c       | 49 ++++++++++++----
> > > ----
> > > --
> > >  drivers/gpu/drm/xe/xe_sriov_vf_ccs_types.h |  6 +--
> > >  5 files changed, 43 insertions(+), 35 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_migrate.c
> > > b/drivers/gpu/drm/xe/xe_migrate.c
> > > index 3a276e2348a2..7d3c7c4e2185 100644
> > > --- a/drivers/gpu/drm/xe/xe_migrate.c
> > > +++ b/drivers/gpu/drm/xe/xe_migrate.c
> > > @@ -960,14 +960,14 @@ struct xe_lrc *xe_migrate_lrc(struct
> > > xe_migrate
> > > *migrate)
> > >         return migrate->q->lrc[0];
> > >  }
> > >  
> > > -static int emit_flush_invalidate(struct xe_migrate *m, u32 *dw,
> > > int
> > > i,
> > > +static int emit_flush_invalidate(struct xe_exec_queue *q, u32
> > > *dw,
> > > int i,
> > >                                  u32 flags)
> > >  {
> > >         dw[i++] = MI_FLUSH_DW | MI_INVALIDATE_TLB |
> > > MI_FLUSH_DW_OP_STOREDW |
> > >                   MI_FLUSH_IMM_DW | flags;
> > > -       dw[i++] =
> > > lower_32_bits(xe_lrc_start_seqno_ggtt_addr(xe_migrate_lrc(m))) |
> > > +       dw[i++] = lower_32_bits(xe_lrc_start_seqno_ggtt_addr(q-
> > > > lrc[0])) |
> > >                   MI_FLUSH_DW_USE_GTT;
> > > -       dw[i++] =
> > > upper_32_bits(xe_lrc_start_seqno_ggtt_addr(xe_migrate_lrc(m)));
> > > +       dw[i++] = upper_32_bits(xe_lrc_start_seqno_ggtt_addr(q-
> > > > lrc[0]));
> > >         dw[i++] = MI_NOOP;
> > >         dw[i++] = MI_NOOP;
> > >  
> > > @@ -976,7 +976,8 @@ static int emit_flush_invalidate(struct
> > > xe_migrate *m, u32 *dw, int i,
> > >  
> > >  /**
> > >   * xe_migrate_ccs_rw_copy() - Copy content of TTM resources.
> > > - * @m: The migration context.
> > > + * @tile: Tile whose migration context to be used.
> > > + * @q : Execution to be used along with migration context.
> > >   * @src_bo: The buffer object @src is currently bound to.
> > >   * @read_write : Creates BB commands for CCS read/write.
> > >   *
> > > @@ -987,7 +988,7 @@ static int emit_flush_invalidate(struct
> > > xe_migrate *m, u32 *dw, int i,
> > >   *
> > >   * Return: 0 if successful, negative error code on failure.
> > >   */
> > > -int xe_migrate_ccs_rw_copy(struct xe_migrate *m,
> > > +int xe_migrate_ccs_rw_copy(struct xe_tile *tile, struct
> > > xe_exec_queue *q,
> > >                            struct xe_bo *src_bo,
> > >                            enum xe_sriov_vf_ccs_rw_ctxs
> > > read_write)
> > >  
> > > @@ -995,7 +996,8 @@ int xe_migrate_ccs_rw_copy(struct xe_migrate
> > > *m,
> > >         bool src_is_pltt = read_write ==
> > > XE_SRIOV_VF_CCS_READ_CTX;
> > >         bool dst_is_pltt = read_write ==
> > > XE_SRIOV_VF_CCS_WRITE_CTX;
> > >         struct ttm_resource *src = src_bo->ttm.resource;
> > > -       struct xe_gt *gt = m->tile->primary_gt;
> > > +       struct xe_migrate *m = tile->migrate;
> > > +       struct xe_gt *gt = tile->primary_gt;
> > >         u32 batch_size, batch_size_allocated;
> > >         struct xe_device *xe = gt_to_xe(gt);
> > >         struct xe_res_cursor src_it, ccs_it;
> > > @@ -1078,11 +1080,11 @@ int xe_migrate_ccs_rw_copy(struct
> > > xe_migrate
> > > *m,
> > >  
> > >                 emit_pte(m, bb, ccs_pt, false, false, &ccs_it,
> > > ccs_size, src);
> > >  
> > > -               bb->len = emit_flush_invalidate(m, bb->cs, bb-
> > > >len,
> > > flush_flags);
> > > +               bb->len = emit_flush_invalidate(q, bb->cs, bb-
> > > >len,
> > > flush_flags);
> > >                 flush_flags = xe_migrate_ccs_copy(m, bb,
> > > src_L0_ofs,
> > > src_is_pltt,
> > >                                                   src_L0_ofs,
> > > dst_is_pltt,
> > >                                                   src_L0,
> > > ccs_ofs,
> > > true);
> > > -               bb->len = emit_flush_invalidate(m, bb->cs, bb-
> > > >len,
> > > flush_flags);
> > > +               bb->len = emit_flush_invalidate(q, bb->cs, bb-
> > > >len,
> > > flush_flags);
> > >  
> > >                 size -= src_L0;
> > >         }
> > > diff --git a/drivers/gpu/drm/xe/xe_migrate.h
> > > b/drivers/gpu/drm/xe/xe_migrate.h
> > > index e81ea6b27fb5..9e20da6d58c2 100644
> > > --- a/drivers/gpu/drm/xe/xe_migrate.h
> > > +++ b/drivers/gpu/drm/xe/xe_migrate.h
> > > @@ -124,7 +124,7 @@ struct dma_fence *xe_migrate_copy(struct
> > > xe_migrate *m,
> > >                                   struct ttm_resource *dst,
> > >                                   bool copy_only_ccs);
> > >  
> > > -int xe_migrate_ccs_rw_copy(struct xe_migrate *m,
> > > +int xe_migrate_ccs_rw_copy(struct xe_tile *tile, struct
> > > xe_exec_queue *q,
> > >                            struct xe_bo *src_bo,
> > >                            enum xe_sriov_vf_ccs_rw_ctxs
> > > read_write);
> > >  
> > > diff --git a/drivers/gpu/drm/xe/xe_pm.c
> > > b/drivers/gpu/drm/xe/xe_pm.c
> > > index 44aaf154ddf7..5e8126ca8e27 100644
> > > --- a/drivers/gpu/drm/xe/xe_pm.c
> > > +++ b/drivers/gpu/drm/xe/xe_pm.c
> > > @@ -209,6 +209,9 @@ int xe_pm_resume(struct xe_device *xe)
> > >  
> > >         xe_pxp_pm_resume(xe->pxp);
> > >  
> > > +       if (IS_SRIOV_VF(xe))
> > > +               xe_sriov_vf_ccs_register_context(xe);
> > > +
> > >         drm_dbg(&xe->drm, "Device resumed\n");
> > >         return 0;
> > >  err:
> > > diff --git a/drivers/gpu/drm/xe/xe_sriov_vf_ccs.c
> > > b/drivers/gpu/drm/xe/xe_sriov_vf_ccs.c
> > > index f0ca2a9b2bb7..a87f39eae4dc 100644
> > > --- a/drivers/gpu/drm/xe/xe_sriov_vf_ccs.c
> > > +++ b/drivers/gpu/drm/xe/xe_sriov_vf_ccs.c
> > > @@ -8,6 +8,7 @@
> > >  #include "xe_bb.h"
> > >  #include "xe_bo.h"
> > >  #include "xe_device.h"
> > > +#include "xe_exec_queue.h"
> > >  #include "xe_exec_queue_types.h"
> > >  #include "xe_guc_submit.h"
> > >  #include "xe_lrc.h"
> > > @@ -168,7 +169,7 @@ static int alloc_bb_pool(struct xe_tile
> > > *tile,
> > > struct xe_tile_vf_ccs *ctx)
> > >  
> > >  static void ccs_rw_update_ring(struct xe_tile_vf_ccs *ctx)
> > >  {
> > > -       struct xe_lrc *lrc = xe_migrate_lrc(ctx->migrate);
> > > +       struct xe_lrc *lrc = ctx->q->lrc[0];
> > >         u64 addr = xe_sa_manager_gpu_addr(ctx->mem.ccs_bb_pool);
> > >         u32 dw[10], i = 0;
> > >  
> > > @@ -183,13 +184,12 @@ static void ccs_rw_update_ring(struct
> > > xe_tile_vf_ccs *ctx)
> > >         xe_lrc_set_ring_tail(lrc, lrc->ring.tail);
> > >  }
> > >  
> > > -static int register_save_restore_context(struct xe_migrate *m,
> > > -                                        enum
> > > xe_sriov_vf_ccs_rw_ctxs
> > > ctx_id)
> > > +static int register_save_restore_context(struct xe_tile_vf_ccs
> > > *ctx)
> > >  {
> > >         int err = -EINVAL;
> > >         int ctx_type;
> > >  
> > > -       switch (ctx_id) {
> > > +       switch (ctx->ctx_id) {
> > >         case XE_SRIOV_VF_CCS_READ_CTX:
> > >                 ctx_type = GUC_CONTEXT_COMPRESSION_SAVE;
> > >                 break;
> > > @@ -200,7 +200,7 @@ static int
> > > register_save_restore_context(struct
> > > xe_migrate *m,
> > >                 return err;
> > >         }
> > >  
> > > -       xe_guc_register_exec_queue(xe_migrate_exec_queue(m),
> > > ctx_type);
> > > +       xe_guc_register_exec_queue(ctx->q, ctx_type);
> > >         return 0;
> > >  }
> > >  
> > > @@ -225,7 +225,7 @@ int xe_sriov_vf_ccs_register_context(struct
> > > xe_device *xe)
> > >  
> > >         for_each_ccs_rw_ctx(ctx_id) {
> > >                 ctx = &tile->sriov.vf.ccs[ctx_id];
> > > -               err = register_save_restore_context(ctx->migrate,
> > > ctx_id);
> > > +               err = register_save_restore_context(ctx);
> > >                 if (err)
> > >                         return err;
> > >         }
> > > @@ -236,13 +236,14 @@ int xe_sriov_vf_ccs_register_context(struct
> > > xe_device *xe)
> > >  static void xe_sriov_vf_ccs_fini(void *arg)
> > >  {
> > >         struct xe_tile_vf_ccs *ctx = arg;
> > > -       struct xe_lrc *lrc = xe_migrate_lrc(ctx->migrate);
> > > +       struct xe_lrc *lrc = ctx->q->lrc[0];
> > >  
> > >         /*
> > >          * Make TAIL = HEAD in the ring so that no issues are
> > > seen if
> > > Guc
> > >          * submits this context to HW on VF pause after unbinding
> > > device.
> > >          */
> > >         xe_lrc_set_ring_tail(lrc, xe_lrc_ring_head(lrc));
> > > +       xe_exec_queue_put(ctx->q);
> > >  }
> > >  
> > >  /**
> > > @@ -258,8 +259,9 @@ int xe_sriov_vf_ccs_init(struct xe_device
> > > *xe)
> > >  {
> > >         struct xe_tile *tile = xe_device_get_root_tile(xe);
> > >         enum xe_sriov_vf_ccs_rw_ctxs ctx_id;
> > > -       struct xe_migrate *migrate;
> > >         struct xe_tile_vf_ccs *ctx;
> > > +       struct xe_exec_queue *q;
> > > +       u32 flags;
> > >         int err;
> > >  
> > >         xe_assert(xe, IS_SRIOV_VF(xe));
> > > @@ -270,27 +272,25 @@ int xe_sriov_vf_ccs_init(struct xe_device
> > > *xe)
> > >                 ctx = &tile->sriov.vf.ccs[ctx_id];
> > >                 ctx->ctx_id = ctx_id;
> > >  
> > > -               migrate = xe_migrate_alloc(tile);
> > > -               if (!migrate) {
> > > -                       err = -ENOMEM;
> > > +               flags = EXEC_QUEUE_FLAG_KERNEL |
> > > +                       EXEC_QUEUE_FLAG_PERMANENT |
> > > +                       EXEC_QUEUE_FLAG_MIGRATE;
> > 
> > When USM is supported, we also set EXEC_QUEUE_FLAG_HIGH_PRIORITY
> > for
> > the migration context. Is there a reason not to do that here?
> > 
> 
> I don't think EXEC_QUEUE_FLAG_HIGH_PRIORITY applies here as this is a
> special queue that is only run upon VF save / restore, so I don't
> think
> it would be subject to normal scheduling polices and also it should
> have
> exclusive execution when it runs, at least within a VF.

Thanks Matt yeah makes sense to me too.

Reviewed-by: Stuart Summers <stuart.summers@intel.com>

> 
> > Otherwise the change makes sense to me. We still have a unique
> > queue
> > here, just not the VM creation per VF which I agree seems like a
> > good
> > idea.
> > 
> 
> I agree this patch LGTM:
> Reviewed-by: Matthew Brost <matthew.brost@intel.com>
> 
> > Thanks,
> > Stuart
> > 
> > > +               q = xe_exec_queue_create_bind(xe, tile, flags,
> > > 0);
> > > +               if (IS_ERR(q)) {
> > > +                       err = PTR_ERR(q);
> > >                         goto err_ret;
> > >                 }
> > > -
> > > -               err = xe_migrate_init(migrate);
> > > -               if (err)
> > > -                       goto err_ret;
> > > -
> > > -               ctx->migrate = migrate;
> > > +               ctx->q = q;
> > >  
> > >                 err = alloc_bb_pool(tile, ctx);
> > >                 if (err)
> > > -                       goto err_ret;
> > > +                       goto err_free_queue;
> > >  
> > >                 ccs_rw_update_ring(ctx);
> > >  
> > > -               err = register_save_restore_context(ctx->migrate,
> > > ctx_id);
> > > +               err = register_save_restore_context(ctx);
> > >                 if (err)
> > > -                       goto err_ret;
> > > +                       goto err_free_queue;
> > >  
> > >                 err = devm_add_action_or_reset(xe->drm.dev,
> > >                                               
> > > xe_sriov_vf_ccs_fini,
> > > @@ -301,6 +301,9 @@ int xe_sriov_vf_ccs_init(struct xe_device
> > > *xe)
> > >  
> > >         return 0;
> > >  
> > > +err_free_queue:
> > > +       xe_exec_queue_put(q);
> > > +
> > >  err_ret:
> > >         return err;
> > >  }
> > > @@ -319,7 +322,7 @@ int xe_sriov_vf_ccs_attach_bo(struct xe_bo
> > > *bo)
> > >  {
> > >         struct xe_device *xe = xe_bo_device(bo);
> > >         enum xe_sriov_vf_ccs_rw_ctxs ctx_id;
> > > -       struct xe_migrate *migrate;
> > > +       struct xe_tile_vf_ccs *ctx;
> > >         struct xe_tile *tile;
> > >         struct xe_bb *bb;
> > >         int err = 0;
> > > @@ -334,8 +337,8 @@ int xe_sriov_vf_ccs_attach_bo(struct xe_bo
> > > *bo)
> > >                 /* bb should be NULL here. Assert if not NULL */
> > >                 xe_assert(xe, !bb);
> > >  
> > > -               migrate = tile->sriov.vf.ccs[ctx_id].migrate;
> > > -               err = xe_migrate_ccs_rw_copy(migrate, bo,
> > > ctx_id);
> > > +               ctx = &tile->sriov.vf.ccs[ctx_id];
> > > +               err = xe_migrate_ccs_rw_copy(tile, ctx->q, bo,
> > > ctx_id);
> > >         }
> > >         return err;
> > >  }
> > > diff --git a/drivers/gpu/drm/xe/xe_sriov_vf_ccs_types.h
> > > b/drivers/gpu/drm/xe/xe_sriov_vf_ccs_types.h
> > > index e240f3fd18af..1add0541aed8 100644
> > > --- a/drivers/gpu/drm/xe/xe_sriov_vf_ccs_types.h
> > > +++ b/drivers/gpu/drm/xe/xe_sriov_vf_ccs_types.h
> > > @@ -41,11 +41,11 @@ struct xe_sa_manager;
> > >  struct xe_tile_vf_ccs {
> > >         /** @id: Id to which context it belongs to */
> > >         enum xe_sriov_vf_ccs_rw_ctxs ctx_id;
> > > -       /** @migrate: Migration helper for save/restore of CCS
> > > data
> > > */
> > > -       struct xe_migrate *migrate;
> > > +       /** @q: exec queues used for migration */
> > > +       struct xe_exec_queue *q;
> > >  
> > >         struct {
> > > -               /** @ccs_rw_bb_pool: Pool from which batch
> > > buffers
> > > are allocated. */
> > > +               /** @ccs_bb_pool: Pool from which batch buffers
> > > are
> > > allocated. */
> > >                 struct xe_sa_manager *ccs_bb_pool;
> > >         } mem;
> > >  };
> >