Re: [Intel-xe] [PATCH 03/15] drm/xe/xe2: Add MCR register steering for primary GT

[Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com>]

On 17.08.2023 07:47, Lucas De Marchi wrote:
> On Wed, Aug 16, 2023 at 03:27:54PM -0700, Matt Atwood wrote:
> > On Fri, Aug 11, 2023 at 09:06:06AM -0700, Lucas De Marchi wrote:
> > > From: Matt Roper <matthew.d.roper@intel.com>
> > > 
> > > Xe2 uses the same steering control register and steering semaphore
> > > register as MTL.  As with recent platforms, group/instance 0,0 is
> > > sufficient to target a non-terminated instance for most classes of MCR
> > > registers; the only types of ranges that need to consider platform
> > > fusing to find a non-terminated instance are SLICE/DSS ranges and a new
> > > SQIDI_PSMI type of range.
> > > 
> > > Note that the range of valid bits in XE2_NODE_ENABLE_MASK may be reduced
> > > for some Xe2 SKUs.  However the lowest bits are always valid and only
> > > the lowest instance is obtained via __ffs(), so there's no need to
> > > complicate the masking with extra platform/subplatform checks.
> > > 
> > > Also note that Wa_14017387313 suggests skipping MCR lock acquisition
> > > around GAM and GAMWKR registers to prevent MCR register accesses in an
> > > interrupt handler from deadlocking when the steering semaphore is
> > > already held outside the interrupt context.  At this time Xe never
> > > issues MCR accesses from within an interrupt handler so the workaround
> > > is not currently needed.
> > > 
> > > Bspec: 71185
> > > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > > ---
> > >  drivers/gpu/drm/xe/regs/xe_gt_regs.h |  1 +
> > >  drivers/gpu/drm/xe/xe_gt_mcr.c       | 50 +++++++++++++++++++++++++++-
> > >  drivers/gpu/drm/xe/xe_gt_types.h     |  1 +
> > >  3 files changed, 51 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/regs/xe_gt_regs.h b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> > > index ad7ae6a28caca..5bc6c7fa4759f 100644
> > > --- a/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> > > +++ b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> > > @@ -145,6 +145,7 @@
> > >  #define   EN_32B_ACCESS				REG_BIT(30)
> > > 
> > >  #define	MIRROR_FUSE3				XE_REG(0x9118)
> > > +#define   XE2_NODE_ENABLE_MASK			REG_GENMASK(31, 16)
> > >  #define   L3BANK_PAIR_COUNT			4
> > >  #define   L3BANK_MASK				REG_GENMASK(3, 0)
> > >  /* on Xe_HP the same fuses indicates mslices instead of L3 banks */
> > > diff --git a/drivers/gpu/drm/xe/xe_gt_mcr.c b/drivers/gpu/drm/xe/xe_gt_mcr.c
> > > index 9eb7a6a1348d6..ef9f4044ca1fd 100644
> > > --- a/drivers/gpu/drm/xe/xe_gt_mcr.c
> > > +++ b/drivers/gpu/drm/xe/xe_gt_mcr.c
> > > @@ -181,6 +181,39 @@ static const struct xe_mmio_range dg2_implicit_steering_table[] = {
> > >  	{},
> > >  };
> > > 
> > > +static const struct xe_mmio_range xe2lpg_dss_steering_table[] = {
> > > +	{ 0x005200, 0x0052FF },         /* SLICE */
> > > +	{ 0x005500, 0x007FFF },         /* SLICE */
> > > +	{ 0x008140, 0x00815F },         /* SLICE (0x8140-0x814F), DSS (0x8150-0x815F) */
> > > +	{ 0x0094D0, 0x00955F },         /* SLICE (0x94D0-0x951F), DSS (0x9520-0x955F) */
> > > +	{ 0x009680, 0x0096FF },         /* DSS */
> > > +	{ 0x00D800, 0x00D87F },         /* SLICE */
> > > +	{ 0x00DC00, 0x00DCFF },         /* SLICE */
> > > +	{ 0x00DE80, 0x00E8FF },         /* DSS (0xE000-0xE0FF reserved) */
> > > +	{ 0x00E980, 0x00E9FF },         /* SLICE */
> > > +	{ 0x013000, 0x0133FF },         /* DSS (0x13000-0x131FF), SLICE (0x13200-0x133FF) */
> > > +	{},
> > > +};
> > > +
> > > +static const struct xe_mmio_range xe2lpg_sqidi_psmi_steering_table[] = {
> > > +	{ 0x000B00, 0x000BFF },
> > > +	{ 0x001000, 0x001FFF },
> > > +	{},
> > > +};
> > > +
> > > +static const struct xe_mmio_range xe2lpg_instance0_steering_table[] = {
> > > +	{ 0x004000, 0x004AFF },         /* GAM, rsvd, GAMWKR */
> > 						^(4200->43FF reserved)
> 
> This is intended so we have simpler ranges to lookup on every mcr
> register access. Intention is to keep the table as small as possible
> without causing issues.
> 
> If it's reserved, there shouldn't be any access going on and we can
> coalesce the range with the next ranges. Explicitely leaving out the
> rsvd range would benefit debugging as it would automatically create a
> warning when accessing such a register, but I'm not sure it's worth it.
> Matt Roper, any thoughts?
> 
> > > +	{ 0x008700, 0x0087FF },         /* SQIDI, MEMPIPE */
> > 8700->87FF is SQIDI only, to include MEMPIPE entry extend to 887F.
> 
> ack
> 
> > > +	{ 0x00B000, 0x00B3FF },         /* NODE, L3BANK */
> > > +	{ 0x00C800, 0x00CFFF },         /* GAM */
> > > +	{ 0x00D880, 0x00D8FF },         /* NODE */
> > > +	{ 0x00DD00, 0x00DDFF },         /* MEMPIPE */
> > > +	{ 0x00E900, 0x00E97F },         /* MEMPIPE */
> > > +	{ 0x00EF00, 0x00F4FF },         /* GAM, GAMWKR */
> > GAM starts ate F000, EF00->EFFF is reserved.
> 
> same reason as above, we don't care about the reserved "holes"

I guess the above range was calculated without using project filter in
the Bspec. After applying the filter for LunarLake, the range turns out
to be

 {0x00F000, 0xFFFF}, /* GAM, GAMWKR */

Regards,
Bala

> 
> thanks
> Lucas De Marchi
> 
> > > +	{ 0x013400, 0x0135FF },         /* MEMPIPE */
> > > +	{},
> > > +};
> > > +
> > >  static void init_steering_l3bank(struct xe_gt *gt)
> > >  {
> > >  	if (GRAPHICS_VERx100(gt_to_xe(gt)) >= 1270) {
> > > @@ -265,6 +298,16 @@ static void init_steering_oaddrm(struct xe_gt *gt)
> > >  	gt->steering[DSS].instance_target = 0;		/* unused */
> > >  }
> > > 
> > > +static void init_steering_sqidi_psmi(struct xe_gt *gt)
> > > +{
> > > +	u32 mask = REG_FIELD_GET(XE2_NODE_ENABLE_MASK,
> > > +				 xe_mmio_read32(gt, MIRROR_FUSE3));
> > > +	u32 select = __ffs(mask);
> > > +
> > > +	gt->steering[SQIDI_PSMI].group_target = select >> 1;
> > > +	gt->steering[SQIDI_PSMI].instance_target = select & 0x1;
> > > +}
> > > +
> > >  static void init_steering_inst0(struct xe_gt *gt)
> > >  {
> > >  	gt->steering[DSS].group_target = 0;		/* unused */
> > > @@ -280,6 +323,7 @@ static const struct {
> > >  	[LNCF] =	{ "LNCF",	NULL }, /* initialized by mslice init */
> > >  	[DSS] =		{ "DSS",	init_steering_dss },
> > >  	[OADDRM] =	{ "OADDRM",	init_steering_oaddrm },
> > > +	[SQIDI_PSMI] =  { "SQIDI_PSMI", init_steering_sqidi_psmi },
> > >  	[INSTANCE0] =	{ "INSTANCE 0",	init_steering_inst0 },
> > >  	[IMPLICIT_STEERING] = { "IMPLICIT", NULL },
> > >  };
> > > @@ -298,7 +342,11 @@ void xe_gt_mcr_init(struct xe_gt *gt)
> > > 
> > >  		gt->steering[OADDRM].ranges = xelpmp_oaddrm_steering_table;
> > >  	} else {
> > > -		if (GRAPHICS_VERx100(xe) >= 1270) {
> > > +		if (GRAPHICS_VER(xe) >= 20) {
> > > +			gt->steering[DSS].ranges = xe2lpg_dss_steering_table;
> > > +			gt->steering[SQIDI_PSMI].ranges = xe2lpg_sqidi_psmi_steering_table;
> > > +			gt->steering[INSTANCE0].ranges = xe2lpg_instance0_steering_table;
> > > +		} else if (GRAPHICS_VERx100(xe) >= 1270) {
> > >  			gt->steering[INSTANCE0].ranges = xelpg_instance0_steering_table;
> > >  			gt->steering[L3BANK].ranges = xelpg_l3bank_steering_table;
> > >  			gt->steering[DSS].ranges = xelpg_dss_steering_table;
> > > diff --git a/drivers/gpu/drm/xe/xe_gt_types.h b/drivers/gpu/drm/xe/xe_gt_types.h
> > > index 48fd698ff62aa..d4310be3e1e7c 100644
> > > --- a/drivers/gpu/drm/xe/xe_gt_types.h
> > > +++ b/drivers/gpu/drm/xe/xe_gt_types.h
> > > @@ -55,6 +55,7 @@ enum xe_steering_type {
> > >  	LNCF,
> > >  	DSS,
> > >  	OADDRM,
> > > +	SQIDI_PSMI,
> > > 
> > >  	/*
> > >  	 * On some platforms there are multiple types of MCR registers that
> > > --
> > > 2.40.1
> > >