[Intel-xe] [PATCH 04/19] drm/xe: Add helper to get dss per group

Thu Mar 9 18:04:10 UTC 2023

On Thu, Mar 09, 2023 at 08:06:06AM -0800, Matt Roper wrote:
>On Wed, Mar 08, 2023 at 08:51:07PM -0800, Lucas De Marchi wrote:
>> On Wed, Mar 08, 2023 at 02:43:55PM -0800, Matt Roper wrote:
>> > On Wed, Mar 08, 2023 at 02:20:17PM -0800, Lucas De Marchi wrote:
>> > > On Wed, Mar 08, 2023 at 02:16:48PM -0800, Lucas De Marchi wrote:
>> > > > On Wed, Mar 08, 2023 at 01:53:38PM -0800, Matt Roper wrote:
>> > > > > On Tue, Mar 07, 2023 at 01:24:31AM -0800, Lucas De Marchi wrote:
>> > > > > > This is already used in intel_gt_mcr and will be used in workarounds.
>> > > > > > Add helpers so the platform-dependency is in a single place.
>> > > > > >
>> > > > > > Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
>> > > > > > ---
>> > > > > > drivers/gpu/drm/xe/xe_gt_mcr.c      | 2 +-
>> > > > > > drivers/gpu/drm/xe/xe_gt_topology.c | 6 ++++++
>> > > > > > drivers/gpu/drm/xe/xe_gt_topology.h | 3 +++
>> > > > > > 3 files changed, 10 insertions(+), 1 deletion(-)
>> > > > > >
>> > > > > > diff --git a/drivers/gpu/drm/xe/xe_gt_mcr.c b/drivers/gpu/drm/xe/xe_gt_mcr.c
>> > > > > > index a42061c4b9d2..694a79b40ec0 100644
>> > > > > > --- a/drivers/gpu/drm/xe/xe_gt_mcr.c
>> > > > > > +++ b/drivers/gpu/drm/xe/xe_gt_mcr.c
>> > > > > > @@ -223,7 +223,7 @@ static void init_steering_dss(struct xe_gt *gt)
>> > > > > > {
>> > > > > > 	unsigned int dss = min(xe_dss_mask_group_ffs(gt->fuse_topo.g_dss_mask, 0, 0),
>> > > > > > 			       xe_dss_mask_group_ffs(gt->fuse_topo.c_dss_mask, 0, 0));
>> > > > > > -	unsigned int dss_per_grp = gt_to_xe(gt)->info.platform == XE_PVC ? 8 : 4;
>> > > > > > +	unsigned int dss_per_grp = xe_gt_topology_get_dss_per_group(gt);
>> > > > > >
>> > > > > > 	gt->steering[DSS].group_target = dss / dss_per_grp;
>> > > > > > 	gt->steering[DSS].instance_target = dss % dss_per_grp;
>> > > > > > diff --git a/drivers/gpu/drm/xe/xe_gt_topology.c b/drivers/gpu/drm/xe/xe_gt_topology.c
>> > > > > > index 967f2349c67a..9fe06a46a400 100644
>> > > > > > --- a/drivers/gpu/drm/xe/xe_gt_topology.c
>> > > > > > +++ b/drivers/gpu/drm/xe/xe_gt_topology.c
>> > > > > > @@ -103,6 +103,12 @@ xe_gt_topology_dump(struct xe_gt *gt, struct drm_printer *p)
>> > > > > >
>> > > > > > }
>> > > > > >
>> > > > > > +unsigned int
>> > > > > > +xe_gt_topology_get_dss_per_group(struct xe_gt *gt)
>> > > > >
>> > > > > When this shows up as a general interface, it leaves me wondering what a
>> > > > > "group" is.  The rules for DSS steering happen to use gslices on Xe_HP
>> > > > > platforms and cslices on Xe_HPC platforms, but that's just the
>> > > > > platform-specific rules of steering and doesn't automatically translate
>> > > > > into which type of slice would be appropriate to use as a grouping
>> > > > > elsewhere in the driver.
>> > > >
>> > > > humn... maybe I myself didn't understand the group meaning here.
>> > > > is group a generic term for cslice/gslice?
>> > >
>> > > btw the term "group" is already used in e.g. xe_dss_mask_group_ffs()
>> > > exported in drivers/gpu/drm/xe/xe_gt_topology.h
>> >
>> > Right, but that's just working on an arbitrary grouping of bits (of any
>> > size).  The definition of "group" in that case is explicitly provided by
>> > the caller via parameters; the function itself doesn't decide what a
>> > group is.
>>
>>
>> so.. do you think documentation for what this is should be provided and
>> be sufficient? Otherwise, if you prefer not to expose this, how should
>> we implement xe_rtp_match_first_gslice_fused_off()? Just hardcode it as
>> 4?  I was trying to avoid the word "slice" so I used the word group as
>
>Hardcoding as 4 sounds fine to me since that's true for all current
>platforms and at least the next few upcoming platforms as well.
>
>> it is used around this code. Maybe I shouldn't.
>>
>> What about the following?
>>
>>  /**
>>   * xe_gt_topology_get_dss_per_slice - Get number of dss per slice
>
>"Slice" is also ambiguous since we have gslices, cslices, mslices, and
>bslices these days that are all related to different subsets of the DSS
>pool and that can all potentially be used in different areas of the
>driver.  If we're making a function specifically for gslices, then it
>should state that; if we're making a general function that can be used
>with anything, then it needs to take parameters that describe what type
>of DSS grouping we want to use.
>
>Unfortunately the bspec still uses the term "slice" in a couple places
>without specifying which of the types of slices it's dealing with, so
>you have to read between the lines a bit.  I remember that leading to a
>lot of confusion during early xehpsdv bringup where different sets of
>hardware architects were arguing about "slices" and didn't realize that
>some of them were talking about gslices while others were talking about
>cslices.

I think the 4 hardcoded is worse than slice being an abstraction for
gslice/cslice/mslice/bslice depending on the context. The reason to have
just one xe_gt_topology_get_dss_per_slice would be the one below

>
>
>Matt
>
>>   * @gt: ...
>>   *
>>   * Each slice in the GT contains up to a fixed number of DSS. This
>>   * can be used when the ``g_dss_mask`` / ``c_dss_mask`` saved needs
>>   * to be interpreted on a per-slice basis.

^ here. Because for the current platforms they are the same.
we expose g_dss_mask and c_dss_mask at the platform info level, but we
don't expose how they can be grouped. This leads to additional platform
checks in different places rather than the one place we can have a
better understanding, the xe_gt_topology.c file.

If I hardcode the 4 in that function. We will still have:

	drivers/gpu/drm/xe/xe_gt_mcr.c:
	init_steering_dss() {
		unsigned int dss_per_grp = gt_to_xe(gt)->info.platform == XE_PVC ? 8 : 4;
	}

	drivers/gpu/drm/xe/xe_rtp.c:
	xe_rtp_match_first_gslice_fused_off() {
		unsigned int dss_per_gslice = 4;
	}

None of them seem to be in the right place (xe_gt_topology.c, for me).
Particularly the one inside xe_rtp.c seems totally out of place.

That said, I can disagree and commit here. Hardcode it for now as 4
and later we figure out if we want an abstraction in xe_gt_topology.c

Lucas De Marchi

>>   *
>>   * Returns: Number of DSS per slice to use with [gc]_dss_mask
>>   */
>>
>>
>> thanks
>> Lucas De Marchi
>
>-- 
>Matt Roper
>Graphics Software Engineer
>Linux GPU Platform Enablement
>Intel Corporation