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

[Matt Roper]

On Thu, Mar 09, 2023 at 10:04:10AM -0800, Lucas De Marchi wrote:
> 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

My point is that with the current function signature (which only passes
a gt structure), it doesn't know what the context is.  It knows which
platform you're on, but it doesn't know which of that platform's
multiple slice types you're asking about.  On PVC it can infer that the
caller isn't asking about gslices since there's simply no geometry on
that platform, but it's still unclear whether they want the numbers
related to cslice, bslice, or mslice.  On DG2 or MTL the hardware
doesn't have bslices, but the function still can't determine whether the
caller was asking about gslices, cslices, or mslices without more
information.

Basically, if you want a single function for this, you'd probably need
to add an extra parameter that passes a slice type enum so that it knows
which way of chopping up the DSS pool is the one you're asking about.

> 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;

To be clear, the condition here is because the MCR rules are different
on PVC than they are on other platforms.  The size of slices stayed
exactly the same as they've always been (cslices are 8 DSS on all
platforms so far, gslices are 4 DSS on all platforms so far).
These could just be defines

        #define NUM_DSS_PER_GSLICE   4
        #define NUM_DSS_PER_CSLICE   8

The fact that steering uses cslices instead of gslices on PVC is
independent of which type of slice gets used for non-steering decisions
elsewhere in the driver.


Matt

> 	}
> 
> 	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

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