[Intel-xe] [PATCH v2 27/30] drm/xe: Allow GT looping and lookup on standalone media

[Matt Roper]

On Fri, May 26, 2023 at 03:37:07PM -0700, Lucas De Marchi wrote:
> On Fri, May 19, 2023 at 04:18:24PM -0700, Matt Roper wrote:
> > Allow xe_device_get_gt() and for_each_gt() to operate as expected on
> > platforms with standalone media.
> > 
> > FIXME: We need to figure out a consistent ID scheme for GTs.  At the
> > moment our platforms either have multi-tile (i.e., PVC) or standalone
> > media (MTL) but not both.  If a future platform supports both of these
> > capabilities at the same time, how will we number the GTs of the
> > platform?   primary-primary-media-media?  primary-media-primary-media?
> > For that matter should we even still be exposing the concept of 'GT' to
> > userspace or should that switch to tile instead (and keep the hardware's
> > separation of render and media an internal implementation detail like it
> > is on i915)?  If we only expose tiles to userspace and not GTs, then we
> > may not even need per-GT ID numbers anymore.
> 
> is this something to leave on the commit or something to solve now?
> I think userspace may care about what tile the GT is located, but not
> much about the order... I think just making it a depth-first on each
> tile would be ok and we can have a tile as a gt attribute.
> 
> > 
> > Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
> > ---
> > drivers/gpu/drm/xe/xe_device.h | 27 +++++++++++++++++++++------
> > 1 file changed, 21 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_device.h b/drivers/gpu/drm/xe/xe_device.h
> > index 156c62ac0381..e0085fcd3f47 100644
> > --- a/drivers/gpu/drm/xe/xe_device.h
> > +++ b/drivers/gpu/drm/xe/xe_device.h
> > @@ -55,16 +55,27 @@ static inline struct xe_tile *xe_device_get_root_tile(struct xe_device *xe)
> > 
> > static inline struct xe_gt *xe_device_get_gt(struct xe_device *xe, u8 gt_id)
> > {
> > +	struct xe_tile *root_tile = xe_device_get_root_tile(xe);
> > 	struct xe_gt *gt;
> > 
> > -	XE_BUG_ON(gt_id > XE_MAX_TILES_PER_DEVICE);
> > +	if (drm_WARN_ON(&xe->drm, gt_id > XE_MAX_TILES_PER_DEVICE))
> 
> this condition is odd.
> 
> > +		return root_tile->primary_gt;
> > 
> > -	gt = xe->tiles[gt_id].primary_gt;
> > -	if (drm_WARN_ON(&xe->drm, !gt))
> > +	/*
> > +	 * FIXME: This only works for now because multi-tile and standalone
> > +	 * media are mutually exclusive on the platforms we have today.
> > +	 */
> > +	if (MEDIA_VER(xe) >= 13) {
> > +		gt = gt_id ? root_tile->media_gt : root_tile->primary_gt;
> > +	} else {
> > +		gt = xe->tiles[gt_id].primary_gt;
> > +	}
> 
> I would expect something like:
> 
> 	for_each_tile(...)
> 		for_each_gt(...)
> 			if (gt->id == gt_id)
> 				return gt_id
> 
> which could be optimized by reserving the gt numbers
> with XE_MAX_TILES_PER_DEVICE
> 
> #define XE_MAX_GTS_PER_TILE 2
> 
> 	tile = gt_id / XE_MAX_GTS_PER_TILE;
> 	gt = gt_id % XE_MAX_GTS_PER_TILE;
> 	return !gt ? xe->tiles[tile].primary_gt : xe->tiles[tile].media_gt;

That seems more natural, but it does cause an immediate uapi change that
I'm not sure userspace can cope with without changes on their side
(i.e., the GT IDs for PVC would become 0 and 2 instead of 0 and 1).
With this initial series I was hoping to keep things working as-is for
now while we figure out the bigger question of what we want our UAPI to
look like.  From an IOCTL point of view, it seems like userspace only
ever cares about tiles rather than GTs, although from a sysfs/debugfs
point of view there are some places where exposing both concepts is
important (e.g., GT-level power management).  Maybe GTs don't even
really need user-visible IDs as long as we come up with sensible
sysfs/debugfs interfaces?

Whatever we decide, I think there's definitely a bunch of places in the
uapi that will need to be cleaned up to work with GTs and/or tiles
appropriately.  But that might be best left to a follow-up series
specifically focused on whatever UAPI changes we settle on?

> 
> but hey... why are we not keeping a gt[] to make things simpler instead
> of calling them explicitly primary_gt/media_gt?

I used gt[] in an early version, but switched to explicit primary /
media pointers because it made GTs and tiles more distinct and made it
harder to accidentally use GT indices where tile indices should have
been used or vice-versa.


Matt

> 
> Lucas De Marchi
> 
> > +
> > +	if (!gt)
> > 		return NULL;
> > 
> > -	XE_BUG_ON(gt->info.id != gt_id);
> > -	XE_BUG_ON(gt->info.type == XE_GT_TYPE_UNINITIALIZED);
> > +	drm_WARN_ON(&xe->drm, gt->info.id != gt_id);
> > +	drm_WARN_ON(&xe->drm, gt->info.type == XE_GT_TYPE_UNINITIALIZED);
> > 
> > 	return gt;
> > }
> > @@ -98,8 +109,12 @@ static inline void xe_device_guc_submission_disable(struct xe_device *xe)
> > 	for ((id__) = 0; (id__) < (xe__)->info.tile_count; (id__++)) \
> > 		for_each_if ((tile__) = &(xe__)->tiles[(id__)])
> > 
> > +/*
> > + * FIXME: This only works for now since multi-tile and standalone media
> > + * happen to be mutually exclusive.  Future platforms may change this...
> > + */
> > #define for_each_gt(gt__, xe__, id__) \
> > -	for ((id__) = 0; (id__) < (xe__)->info.tile_count; (id__++)) \
> > +	for ((id__) = 0; (id__) < (xe__)->info.tile_count + (MEDIA_VER(xe__) >= 13 ? 1 : 0); (id__++)) \
> > 		for_each_if ((gt__) = xe_device_get_gt((xe__), (id__)))
> > 
> > static inline struct xe_force_wake * gt_to_fw(struct xe_gt *gt)
> > -- 
> > 2.40.0
> > 

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation