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

[Matt Roper]

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.


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.
>   *
>   * 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