Re: [PATCH v2 09/34] drm/xe: Make LRC W/A scratch buffer usage consistent

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

On Wed, Sep 24, 2025 at 01:01:15PM -0500, Lucas De Marchi wrote:
> On Tue, Sep 23, 2025 at 06:15:36PM -0700, Matthew Brost wrote:
> > The LRC W/A currently checks for LRC being iomem in some places, while
> > in others it checks if the scratch buffer is non-NULL. This
> > inconsistency causes issues with the VF post-migration recovery code,
> > which blindly passes in a scratch buffer.
> > 
> > This patch standardizes the check by consistently verifying whether the
> > LRC is iomem to determine if the scratch buffer should be used.
> > 
> > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > ---
> > drivers/gpu/drm/xe/xe_lrc.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_lrc.c b/drivers/gpu/drm/xe/xe_lrc.c
> > index 47e9df775072..aed0a3155d1f 100644
> > --- a/drivers/gpu/drm/xe/xe_lrc.c
> > +++ b/drivers/gpu/drm/xe/xe_lrc.c
> > @@ -1248,7 +1248,7 @@ static int setup_bo(struct bo_setup_state *state)
> > 
> > static void finish_bo(struct bo_setup_state *state)
> > {
> > -	if (!state->buffer)
> > +	if (!state->lrc->bo->vmap.is_iomem)
> 
> Please add the fixes as the first patches and find the commits they
> are fixing so they can propagate to drm-xe-fixes. With that, for a

VF migration doesn't work at all upstream, so I don't believe this
warrents a fixes tag.

> palliative fix:
> 
> Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com>
> 
> Long-term thoughts below:
> 
> The fact that we exposed xe_lrc_setup_wa_bb_with_scratch() to users
> outside xe_lrc seems completely wrong to me... it explains why
> indirect_ctx was completely forgotten.
> 

Let me add in indirect_ctx fixup in my next rev for completeness.

> And the ENOMEM handling is in a completely different place than the
> allocation which makes this a code smell.

I have a fix for that, we will needs fixes tag for this one.

> 
> I think this change is a palliative that we could accept, but going
> forward I think it needs to be better handled: we shouldn't be
> recalculating it on every lrc init, it should rather be handled like the
> default_lrc and just copied in place. Then on migration what really
> needs to be fixed up is the ggtt address, i.e. only these calls:
> 
> 	xe_lrc_write_ctx_reg(lrc,
> 			     CTX_CS_INDIRECT_CTX,
> 			     (xe_bo_ggtt_addr(lrc->bo) + state.offset) |
> 			     /* Size in CLs. */
> 			     (state.written * sizeof(u32) / 64));
> 
> 	and
> 
> 	xe_lrc_write_ctx_reg(lrc, CTX_BB_PER_CTX_PTR,
>                     xe_bo_ggtt_addr(lrc->bo) + state.offset + 1);
> 
> 
> without re-allocating and recalculating it over and over. I will take a
> look at doing this
> 

That is not a bad idea but also not huge deal with the way the code is
written as this isn't really a critical path anywhere it is called and
unlike default LRC which we need to save off for the GuC ADS / W/A
issued during driver load it is perfectly fine to regenerate on each LRC
init.

Matt

> Lucas De Marchi
> 
> > 		return;
> > 
> > 	xe_map_memcpy_to(gt_to_xe(state->lrc->gt), &state->lrc->bo->vmap,
> > -- 
> > 2.34.1
> >