Re: [PATCH 1/5] drm/xe: Drop __engine_mask

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

On Mon, Apr 29, 2024 at 04:03:25PM -0500, Lucas De Marchi wrote:
> On Mon, Apr 29, 2024 at 08:36:04PM GMT, Matthew Brost wrote:
> > On Mon, Apr 29, 2024 at 04:29:34PM -0400, Rodrigo Vivi wrote:
> > > On Thu, Apr 25, 2024 at 11:24:06AM -0700, Lucas De Marchi wrote:
> > > > Not really used, it's just a copy of engine_mask, which already reads
> > > > the fuses to mark engines as available/not-available.
> > > 
> > > I got confused trying to understand why this ever existed.
> > > It indeed doesn't make much sense on today's code.
> > > 
> > 
> > That was my doing with the intent that we never use the engine mask
> > until after hwconfig load as the engine mask would be read from that. Or
> > alternatively in SRIOV via CTB (or MMIO) relays.
> > 
> > i.e. it was fake way to enforce correct load ordering.
> 
> but we never used it from hwconfig (instead we kept the fake copy here)
> and we never will as it doesn't have the info we need.
> 
> Instead we just went out of our way and started using __engine_mask
> instead where needed. See e.g.  drivers/gpu/drm/xe/xe_migrate.c. I fail to
> see how exactly this enforces the correct load ordering. At most it
> would return the wrong value and we would silently ignore it (e.g. if we
> had a call like xe_hw_engine_mask_per_class() before that point since it
> uses gt->info.engine_mask).
> 
> For SR-IOV, nothing overrides engine_mask anywhere. Not even if I look
> at the wip branch for VF.  So, I'd say that if VF needs it, then please
> add the prep together with patches making use of that because the way it
> is is now it's too fragile.
> 

They should read the fuses via a GuC relay.

> > 
> > Assuming fuses are read after this removed LoC, the load ordering is
> > correct.
> 
> fuses are read much much earlier than that, in xe_pci.c. Why would it be
> wrong?
> 

I was thinking the fuse read required hwconfig step (also brings up
CTBs) but now that I think about this, I believe the fuses can be read
via a MMIO relay so the current location probably works. Double check
with Michal Winarski on that.

Anyways don't let anything I say hold this up.

Matt

> Lucas De Marchi
> 
> > 
> > Matt
> > 
> > > >
> > > > While at it, use XE_HW_ENGINE_BCS_MASK to span all copy engines.
> > > >
> > > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > > 
> > > 
> > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > 
> > > 
> > > > ---
> > > >  drivers/gpu/drm/xe/xe_gt.c       | 3 ---
> > > >  drivers/gpu/drm/xe/xe_gt_types.h | 6 ------
> > > >  drivers/gpu/drm/xe/xe_migrate.c  | 3 +--
> > > >  drivers/gpu/drm/xe/xe_pci.c      | 6 +++---
> > > >  4 files changed, 4 insertions(+), 14 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
> > > > index e922e77f5010..00a22cf2f5b5 100644
> > > > --- a/drivers/gpu/drm/xe/xe_gt.c
> > > > +++ b/drivers/gpu/drm/xe/xe_gt.c
> > > > @@ -515,9 +515,6 @@ int xe_gt_init_hwconfig(struct xe_gt *gt)
> > > >  	if (err)
> > > >  		goto out_fw;
> > > >
> > > > -	/* XXX: Fake that we pull the engine mask from hwconfig blob */
> > > > -	gt->info.engine_mask = gt->info.__engine_mask;
> > > > -
> > > >  out_fw:
> > > >  	xe_force_wake_put(gt_to_fw(gt), XE_FW_GT);
> > > >  out:
> > > > diff --git a/drivers/gpu/drm/xe/xe_gt_types.h b/drivers/gpu/drm/xe/xe_gt_types.h
> > > > index cfdc761ff7f4..72568414fb7d 100644
> > > > --- a/drivers/gpu/drm/xe/xe_gt_types.h
> > > > +++ b/drivers/gpu/drm/xe/xe_gt_types.h
> > > > @@ -116,12 +116,6 @@ struct xe_gt {
> > > >  		u32 reference_clock;
> > > >  		/** @info.engine_mask: mask of engines present on GT */
> > > >  		u64 engine_mask;
> > > > -		/**
> > > > -		 * @info.__engine_mask: mask of engines present on GT read from
> > > > -		 * xe_pci.c, used to fake reading the engine_mask from the
> > > > -		 * hwconfig blob.
> > > > -		 */
> > > > -		u64 __engine_mask;
> > > >  		/** @info.gmdid: raw GMD_ID value from hardware */
> > > >  		u32 gmdid;
> > > >  	} info;
> > > > diff --git a/drivers/gpu/drm/xe/xe_migrate.c b/drivers/gpu/drm/xe/xe_migrate.c
> > > > index 9f6e9b7f11c8..59a3f24d31e6 100644
> > > > --- a/drivers/gpu/drm/xe/xe_migrate.c
> > > > +++ b/drivers/gpu/drm/xe/xe_migrate.c
> > > > @@ -936,8 +936,7 @@ static bool has_service_copy_support(struct xe_gt *gt)
> > > >  	 * all of the actual service copy engines (BCS1-BCS8) have been fused
> > > >  	 * off.
> > > >  	 */
> > > > -	return gt->info.__engine_mask & GENMASK(XE_HW_ENGINE_BCS8,
> > > > -						XE_HW_ENGINE_BCS1);
> > > > +	return gt->info.engine_mask & XE_HW_ENGINE_BCS_MASK;
> > > >  }
> > > >
> > > >  static u32 emit_clear_cmd_len(struct xe_gt *gt)
> > > > diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
> > > > index a0cf5dd803c2..6b2086ea24ab 100644
> > > > --- a/drivers/gpu/drm/xe/xe_pci.c
> > > > +++ b/drivers/gpu/drm/xe/xe_pci.c
> > > > @@ -656,9 +656,9 @@ static int xe_info_init(struct xe_device *xe,
> > > >  		gt = tile->primary_gt;
> > > >  		gt->info.id = xe->info.gt_count++;
> > > >  		gt->info.type = XE_GT_TYPE_MAIN;
> > > > -		gt->info.__engine_mask = graphics_desc->hw_engine_mask;
> > > > +		gt->info.engine_mask = graphics_desc->hw_engine_mask;
> > > >  		if (MEDIA_VER(xe) < 13 && media_desc)
> > > > -			gt->info.__engine_mask |= media_desc->hw_engine_mask;
> > > > +			gt->info.engine_mask |= media_desc->hw_engine_mask;
> > > >
> > > >  		if (MEDIA_VER(xe) < 13 || !media_desc)
> > > >  			continue;
> > > > @@ -673,7 +673,7 @@ static int xe_info_init(struct xe_device *xe,
> > > >
> > > >  		gt = tile->media_gt;
> > > >  		gt->info.type = XE_GT_TYPE_MEDIA;
> > > > -		gt->info.__engine_mask = media_desc->hw_engine_mask;
> > > > +		gt->info.engine_mask = media_desc->hw_engine_mask;
> > > >  		gt->mmio.adj_offset = MEDIA_GT_GSI_OFFSET;
> > > >  		gt->mmio.adj_limit = MEDIA_GT_GSI_LENGTH;
> > > >
> > > > --
> > > > 2.43.0
> > > >