[Intel-xe] [PATCH] drm/xe: Move wa processing to gt_init

[Matthew Brost]

On Tue, Nov 21, 2023 at 08:48:47AM -0600, Lucas De Marchi wrote:
> On Tue, Nov 21, 2023 at 04:55:55PM +0530, Tejas Upadhyay wrote:
> > Currently WA processing for gt is happening in early init API.
> > It also includes processing of WAs which needs filter based on
> > hardware engine by looping over number of hardware engines in gt.
> > 
> > However at early init stage hardware engines are not associated
> > with gt in software which skips processing such WAs.
> > 
> > Example WAs which will be fixed to apply after this change,
> > "14011060649",
> > "16010515920",
> > "16021867713"
> > 
> > Fixes: 811a9b3d262d8 ("drm/xe: Introduce a new DRM driver for Intel GPUs")
> > Signed-off-by: Tejas Upadhyay <tejas.upadhyay at intel.com>
> > ---
> > drivers/gpu/drm/xe/xe_gt.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
> > index 0dddb751c6a4..3d0f7b6f952c 100644
> > --- a/drivers/gpu/drm/xe/xe_gt.c
> > +++ b/drivers/gpu/drm/xe/xe_gt.c
> > @@ -311,7 +311,6 @@ int xe_gt_init_early(struct xe_gt *gt)
> > 	if (err)
> > 		return err;
> > 
> > -	xe_wa_process_gt(gt);
> > 	xe_wa_process_oob(gt);
> > 	xe_tuning_process_gt(gt);
> > 
> > @@ -495,6 +494,7 @@ int xe_gt_init(struct xe_gt *gt)
> > 	if (err)
> > 		return err;
> > 
> > +	xe_wa_process_gt(gt);
> 
> the problem with that is that if we need to touch any register already
> touched until this point, the workarounds would not had been applied.
> Digging the git history, I had added this commit exactly to be able to
> initialize the gt workarounds as soon as possible:
> 
> 	4eb2e8d25ab4 ("drm/xe: Add xe_hw_engines_init_early()")
> 
> And then
> 
> 	816d37907ac6 ("drm/xe: Plug gt WAs to gt init/reset")
> 
> Note that these commits are in the xe branch, not drm-xe-next.
> So, it was:
> 
> 	xe_hw_engines_init_early(gt);

xe_hw_engines_init_early has be called after xe_uc_init_hwconfig after
the idea is gt->info.engine_mask would be set by reading the hwconfig
table from the GuC rather than from a hardcoded table in the driver.
Likewise for_each_hw_engine should only be called after
xe_hw_engines_init_early is called. With that, I think this patch is
correct. Does that make sense?

I suggested to Tejas that we add an assert to for_each_hw_engine to
enforce this is called after xe_hw_engines_init_early to catch issues
like this too.

> 	...
> 	xe_wa_process_gt();
> 
> That gt_fw_domain_init() seems a very poor place to initialize these
> things now. It started with commit b892e30c6cd7 ("drm/xe: Move load of
> minimal GuC and hwconfig read much earlier") and then people end up
> adding random stuff in that function :(
> 
> So I think we need to get back to the original idea of _early()
> initializing what's needed. Then we'd probably need a few cleanups in
> the initialization sequence so the function names at least partially
> matches what's being initialized.
> 

I agree on that the naming of all of this could be better...

Matt

> Lucas De Marchi
> 
> > 	xe_force_wake_init_engines(gt, gt_to_fw(gt));
> > 
> > 	err = all_fw_domain_init(gt);
> > -- 
> > 2.25.1
> >