[PATCH v2 1/1] drm/xe/guc: Expose number of dss per group and helpers
Summers, Stuart
stuart.summers at intel.com
Tue Jan 30 15:57:53 UTC 2024
On Mon, 2024-01-29 at 16:03 -0500, Dong, Zhanjun wrote:
> Thanks for take time review it. Please see my comments below.
>
> Regards,
> Zhanjun
>
> On 2024-01-29 3:17 p.m., Summers, Stuart wrote:
> > On Fri, 2024-01-26 at 15:23 -0800, Zhanjun Dong wrote:
> > > Expose helper for dss per group of mcr, features like GuC
> > > register
> > > capture will need this info to prepare buffer required.
> >
> > I don't understand what this specifically has to do with GuC. Can
> > we
> > just list this as general functionality, not related to GuC (cover
> > letter) or MCR (function names, see below)?
>
> Yes, will move it from drm/xe/guc to drm/xe
>
> >
> > >
> > > Signed-off-by: Zhanjun Dong <zhanjun.dong at intel.com>
> > > ---
> > > drivers/gpu/drm/xe/xe_gt_mcr.c | 38
> > > ++++++++++++++++++++++++-
> > > drivers/gpu/drm/xe/xe_gt_mcr.h | 17 +++++++++++
> > > drivers/gpu/drm/xe/xe_gt_topology.c | 3 --
> > > drivers/gpu/drm/xe/xe_hw_engine_types.h | 3 ++
> > > 4 files changed, 57 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/xe/xe_gt_mcr.c
> > > b/drivers/gpu/drm/xe/xe_gt_mcr.c
> > > index 77925b35cf8d..f7002623f3b5 100644
> > > --- a/drivers/gpu/drm/xe/xe_gt_mcr.c
> > > +++ b/drivers/gpu/drm/xe/xe_gt_mcr.c
> > > @@ -291,11 +291,16 @@ static void init_steering_mslice(struct
> > > xe_gt
> > > *gt)
> > > gt->steering[LNCF].instance_target = 0; /*
> > > unused */
> > > }
> > >
> > > +int xe_gt_mcr_get_dss_per_group(struct xe_gt *gt)
> >
> > I don't understand why we're tying this name to MCR. DSS are
> > involved
> > in more than just MCR negotiation/configuration.
> >
> > I guess not a blocker, but I'd rather this just be something like:
> > xe_gt_get_dss_per_group()
> Sure, will be changed to this name
>
> >
> > > +{
> > > + return gt_to_xe(gt)->info.platform == XE_PVC ? 8 : 4;
> > > +}
> > > +
> > > 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_mcr_get_dss_per_group(gt);
> > >
> > > gt->steering[DSS].group_target = dss / dss_per_grp;
> > > gt->steering[DSS].instance_target = dss % dss_per_grp;
> > > @@ -683,3 +688,34 @@ void xe_gt_mcr_steering_dump(struct xe_gt
> > > *gt,
> > > struct drm_printer *p)
> > > }
> > > }
> > > }
> > > +
> > > +/**
> > > + * xe_gt_mcr_get_ss_steering - returns the group/instance
> > > steering
> > > for a SS
> > > + * @gt: GT structure
> > > + * @dss: DSS ID to obtain steering for
> > > + * @group: pointer to storage for steering group ID
> > > + * @instance: pointer to storage for steering instance ID
> > > + *
> > > + * Returns the steering IDs (via the @group and @instance
> > > parameters) that
> > > + * correspond to a specific subslice/DSS ID.
> > > + */
> > > +void xe_gt_mcr_get_ss_steering(struct xe_gt *gt, unsigned int
> > > dss,
> > > unsigned int *group,
> > > + unsigned int *instance)
> > > +{
> > > + int dss_per_grp = xe_gt_mcr_get_dss_per_group(gt);
> > > +
> > > + *group = dss / dss_per_grp;
> > > + *instance = dss % dss_per_grp;
> > > +}
> > > +
> > > +bool xe_gt_mcr_dss_has_subslice(struct xe_gt *gt, int slice, int
> > > subslice)
> > > +{
> > > + int dss_per_grp = xe_gt_mcr_get_dss_per_group(gt);
> > > + int index = slice * dss_per_grp + subslice;
> > > +
> > > + if (index >= XE_MAX_DSS_FUSE_BITS)
> >
> > Maybe interesting to print something here from a debugging
> > perspective,
> > especially on pre-silicon where maybe the fuse registers aren't set
> > up
> > correctly. Otherwise it's a bit of a goose chase trying to track
> > down
> > why the DSS count is 0, etc.
> Sure, will add debug print for this out of range case.
>
> >
> > > + return false;
> > > + else
> > > + return test_bit(index, gt->fuse_topo.g_dss_mask)
> > > ||
> > > + test_bit(index, gt->fuse_topo.c_dss_mask);
> > > +}
> > > diff --git a/drivers/gpu/drm/xe/xe_gt_mcr.h
> > > b/drivers/gpu/drm/xe/xe_gt_mcr.h
> > > index 27ca1bc880a0..1365b3e77226 100644
> > > --- a/drivers/gpu/drm/xe/xe_gt_mcr.h
> > > +++ b/drivers/gpu/drm/xe/xe_gt_mcr.h
> > > @@ -7,6 +7,7 @@
> > > #define _XE_GT_MCR_H_
> > >
> > > #include "regs/xe_reg_defs.h"
> > > +#include "xe_gt_types.h"
> > >
> > > struct drm_printer;
> > > struct xe_gt;
> > > @@ -25,5 +26,21 @@ void xe_gt_mcr_multicast_write(struct xe_gt
> > > *gt,
> > > struct xe_reg_mcr mcr_reg,
> > > u32 value);
> > >
> > > void xe_gt_mcr_steering_dump(struct xe_gt *gt, struct
> > > drm_printer
> > > *p);
> > > +int xe_gt_mcr_get_dss_per_group(struct xe_gt *gt);
> > > +void xe_gt_mcr_get_ss_steering(struct xe_gt *gt, unsigned int
> > > dss,
> > > + unsigned int *group, unsigned int
> > > *instance);
> > > +bool xe_gt_mcr_dss_has_subslice(struct xe_gt *gt, int slice, int
> > > subslice);
> > > +
> > > +#define _HAS_SS(ss_, gt_, group_, instance_)
> >
> > Why do we have the ss_ parameter?
> Oops, not used arg
> >
> > > xe_gt_mcr_dss_has_subslice(gt_, group_, instance_)
> >
> > Same comment as above, why is this tied naming-wise to "mcr"?
> >
> > > +
> > > +/*
> > > + * Loop over each subslice/DSS and determine the group and
> > > instance
> > > IDs that
> > > + * should be used to steer MCR accesses toward this DSS.
> > > + */
> > > +#define for_each_ss_steering(ss_, gt_, group_, instance_) \
> > > + for (ss_ = 0, xe_gt_mcr_get_ss_steering(gt_, 0, &group_,
> > > &instance_); \
> >
> > IMO it's confusing calling get_ss_steering twice here. I understand
> > the
> > macro piece, but it would be nice if we could reduce this to one
> > call.
> The 1st call is the one time execution before code block running;
> the 2nd xe_gt_mcr_get_ss_steering call will be run on every
> iteration,
> so this is by purpose and cann't be reduced.
Yeah I understand, just looks a little ugly IMO... maybe we can split
this into multiple macros for that purpose? Or not.. anyway no major
issue here.
>
> >
> > > + ss_ < XE_MAX_DSS_FUSE_BITS; \
> > > + ss_++, xe_gt_mcr_get_ss_steering(gt_, ss_, &group_,
> > > &instance_)) \
> > > + for_each_if(_HAS_SS(ss_, gt_, group_, instance_))
> > >
> > > #endif /* _XE_GT_MCR_H_ */
> > > diff --git a/drivers/gpu/drm/xe/xe_gt_topology.c
> > > b/drivers/gpu/drm/xe/xe_gt_topology.c
> > > index a8d7f272c30a..e973eeaac7f1 100644
> > > --- a/drivers/gpu/drm/xe/xe_gt_topology.c
> > > +++ b/drivers/gpu/drm/xe/xe_gt_topology.c
> > > @@ -11,9 +11,6 @@
> > > #include "xe_gt.h"
> > > #include "xe_mmio.h"
> > >
> > > -#define XE_MAX_DSS_FUSE_BITS (32 * XE_MAX_DSS_FUSE_REGS)
> > > -#define XE_MAX_EU_FUSE_BITS (32 * XE_MAX_EU_FUSE_REGS)
> > > -
> > > static void
> > > load_dss_mask(struct xe_gt *gt, xe_dss_mask_t mask, int
> > > numregs,
> > > ...)
> > > {
> > > diff --git a/drivers/gpu/drm/xe/xe_hw_engine_types.h
> > > b/drivers/gpu/drm/xe/xe_hw_engine_types.h
> > > index d7f828c76cc5..3e978a190041 100644
> > > --- a/drivers/gpu/drm/xe/xe_hw_engine_types.h
> > > +++ b/drivers/gpu/drm/xe/xe_hw_engine_types.h
> > > @@ -65,6 +65,9 @@ struct xe_bo;
> > > struct xe_execlist_port;
> > > struct xe_gt;
> > >
> > > +#define XE_MAX_DSS_FUSE_BITS (32 * XE_MAX_DSS_FUSE_REGS)
> > > +#define XE_MAX_EU_FUSE_BITS (32 * XE_MAX_EU_FUSE_REGS)
> >
> > Sorry maybe obvious, but why did you make this move?
> Before we expose the dss per group, the XE_MAX_DSS_FUSE_BITS only
> being
> referenced within xe_gt_topology.c. After this patch, these 2 macros
> will be used by for_each_ss_steering macro and thus being refrenced.
> Move it from the .c to xe_hw_engine_types.h to provide this
> definition.
Yeah makes sense and no problem.
Thanks,
Stuart
>
> >
> > Thanks,
> > Stuart
> >
> > > +
> > > /**
> > > * struct xe_hw_engine_class_intf - per hw engine class struct
> > > interface
> > > *
> >
More information about the Intel-xe
mailing list