Re: [PATCH v2 33/34] drm/xe/vf: Rebase CCS save/restore BB GGTT addresses

[Matthew Brost <matthew.brost@intel.com>]

On Wed, Sep 24, 2025 at 09:34:25AM +0530, K V P, Satyanarayana wrote:
> 
> 
> On 24-09-2025 06:46, Matthew Brost wrote:
> > Rebase the CCS save/restore BB's GGTT addresses during VF post-migration
> > recovery by setting the software ring tail to zero, the LRC ring head to
> > zero, and rewriting the jump-to-BB instructions.
> > 
> > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > ---
> >   drivers/gpu/drm/xe/xe_gt_sriov_vf.c  |  4 ++++
> >   drivers/gpu/drm/xe/xe_sriov_vf_ccs.c | 24 ++++++++++++++++++++++++
> >   drivers/gpu/drm/xe/xe_sriov_vf_ccs.h |  1 +
> >   3 files changed, 29 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_gt_sriov_vf.c b/drivers/gpu/drm/xe/xe_gt_sriov_vf.c
> > index 46d1d1fa91b2..d8d96dd76c73 100644
> > --- a/drivers/gpu/drm/xe/xe_gt_sriov_vf.c
> > +++ b/drivers/gpu/drm/xe/xe_gt_sriov_vf.c
> > @@ -34,6 +34,7 @@
> >   #include "xe_pm.h"
> >   #include "xe_sriov.h"
> >   #include "xe_sriov_vf.h"
> > +#include "xe_sriov_vf_ccs.h"
> >   #include "xe_tile_sriov_vf.h"
> >   #include "xe_tlb_inval.h"
> >   #include "xe_uc_fw.h"
> > @@ -1228,6 +1229,9 @@ static int vf_post_migration_fixups(struct xe_gt *gt)
> >   	if (err)
> >   		return err;
> > +	if (xe_gt_is_main_type(gt))
> > +		xe_sriov_vf_ccs_rebase(gt_to_xe(gt));
> > +
> >   	xe_gt_sriov_vf_default_lrcs_hwsp_rebase(gt);
> >   	err = xe_guc_contexts_hwsp_rebase(&gt->uc.guc, buf);
> >   	if (err)
> > diff --git a/drivers/gpu/drm/xe/xe_sriov_vf_ccs.c b/drivers/gpu/drm/xe/xe_sriov_vf_ccs.c
> > index 8dec616c37c9..bffad45e655f 100644
> > --- a/drivers/gpu/drm/xe/xe_sriov_vf_ccs.c
> > +++ b/drivers/gpu/drm/xe/xe_sriov_vf_ccs.c
> > @@ -175,6 +175,15 @@ static void ccs_rw_update_ring(struct xe_sriov_vf_ccs_ctx *ctx)
> >   	struct xe_lrc *lrc = xe_exec_queue_lrc(ctx->mig_q);
> >   	u32 dw[10], i = 0;
> > +	/*
> > +	 * XXX: Save/restore fixes — for some reason, the GuC only accepts the
> > +	 * save/restore context if the LRC head pointer is zero. This is evident
> > +	 * from repeated VF migrations failing when the LRC head pointer is
> > +	 * non-zero.
> > +	 */
> > +	lrc->ring.tail = 0;
> > +	xe_lrc_set_ring_head(lrc, 0);
> > +
> On lrc_init(), the ring may be pre-populated with some GPU instructions.
> Should not we keep them as it is? Making tail=0 will overwrite the
> pre-populated GPU instructions (May be some WAs??).
> -Satya.>   	dw[i++] = MI_ARB_ON_OFF | MI_ARB_ENABLE;

I’ve thought about that. W/A should be applied through a W/A BB
mechanism, not via the ring. If I recall correctly, the original code
for programming instructions in the ring came from Faith’s original Xe
PoC code from over four years ago, which we’ve never really revisited.

In my opinion, that code should be removed.

I could include the removal in this series, but I’d prefer not to expand
its scope. We can address it in a follow-up. I also don’t want this
series to grow in complexity by retaining what is almost certainly
useless code.

Matt

> >   	dw[i++] = MI_BATCH_BUFFER_START | XE_INSTR_NUM_DW(3);
> >   	dw[i++] = lower_32_bits(addr);
> > @@ -186,6 +195,21 @@ static void ccs_rw_update_ring(struct xe_sriov_vf_ccs_ctx *ctx)
> >   	xe_lrc_set_ring_tail(lrc, lrc->ring.tail);
> >   }
> > +void xe_sriov_vf_ccs_rebase(struct xe_device *xe)
> > +{
> > +	enum xe_sriov_vf_ccs_rw_ctxs ctx_id;
> > +
> > +	if (!IS_VF_CCS_READY(xe))
> > +		return;
> > +
> > +	for_each_ccs_rw_ctx(ctx_id) {
> > +		struct xe_sriov_vf_ccs_ctx *ctx =
> > +			&xe->sriov.vf.ccs.contexts[ctx_id];
> > +
> > +		ccs_rw_update_ring(ctx);
> > +	}
> > +}
> > +
> >   static int register_save_restore_context(struct xe_sriov_vf_ccs_ctx *ctx)
> >   {
> >   	int ctx_type;
> > diff --git a/drivers/gpu/drm/xe/xe_sriov_vf_ccs.h b/drivers/gpu/drm/xe/xe_sriov_vf_ccs.h
> > index 0745c0ff0228..f8ca6efce9ee 100644
> > --- a/drivers/gpu/drm/xe/xe_sriov_vf_ccs.h
> > +++ b/drivers/gpu/drm/xe/xe_sriov_vf_ccs.h
> > @@ -18,6 +18,7 @@ int xe_sriov_vf_ccs_init(struct xe_device *xe);
> >   int xe_sriov_vf_ccs_attach_bo(struct xe_bo *bo);
> >   int xe_sriov_vf_ccs_detach_bo(struct xe_bo *bo);
> >   int xe_sriov_vf_ccs_register_context(struct xe_device *xe);
> > +void xe_sriov_vf_ccs_rebase(struct xe_device *xe);
> >   void xe_sriov_vf_ccs_print(struct xe_device *xe, struct drm_printer *p);
> >   static inline bool xe_sriov_vf_ccs_ready(struct xe_device *xe)
>