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

["K V P, Satyanarayana" <satyanarayana.k.v.p@intel.com>]

On 24-09-2025 12:02, Matthew Brost wrote:
> 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
LGTM.
Reviewed-by: Satyanarayana K V P <satyanarayana.k.v.p@intel.com>>
>>>    	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)
>>