Re: [PATCH 03/23] drm/xe/xe3p_lpm: Configure MAIN_GAMCTRL_QUEUE_SELECT

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

On 15.10.2025 09:03, Lucas De Marchi wrote:
> On Wed, Oct 15, 2025 at 12:06:55PM +0530, Vivekanandan, Balasubramani wrote:
> > On 14.10.2025 21:28, Lucas De Marchi wrote:
> > > On Tue, Oct 14, 2025 at 09:34:03AM -0700, Matt Roper wrote:
> > > > On Mon, Oct 13, 2025 at 08:24:35PM -0700, Lucas De Marchi wrote:
> > > > > From: Brian Welty <brian.welty@intel.com>
> > > > >
> > > > > Starting from Xe3p, there are two different copies of some of the GAM
> > > > > registers:  the traditional MCR variant at their old locations, and a
> > > > > new unicast copy known as "main_gamctrl."  The Xe driver doesn't use
> > > > > these registers directly, but we need to instruct the GuC on which set
> > > > > it should use.  Since the new, unicast registers are preferred (since
> > > > > they avoid the need for unnecessary MCR synchronization), set a new GuC
> > > > > feature flag, GUC_CTL_MAIN_GAMCTRL_QUEUES to convey this decision.  A
> > > > > new helper function, xe_guc_using_main_gamctrl_queues(), is added for
> > > > > use in the 3 independent places that need to handle configuration of the
> > > > > new reporting queues.
> > > > >
> > > > > The mmio write to enable the main gamctl is only done during the general
> > > > > GuC upload.  The gamctrl registers are not accessed by the GuC during
> > > > > hwconfig load.
> > > > >
> > > > > Last, the ADS blob for communicating the queue addresses contains both a
> > > > > DPA and GGTT offset. The GuC documentation states that DPA is now MBZ
> > > > > when using the MAIN_GAMCTRL queues.
> > > > >
> > > > > Signed-off-by: Brian Welty <brian.welty@intel.com>
> > > > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/xe/regs/xe_gt_regs.h |  3 +++
> > > > >  drivers/gpu/drm/xe/xe_gt.h           |  6 ++++++
> > > > >  drivers/gpu/drm/xe/xe_guc.c          | 27 +++++++++++++++++++++++++++
> > > > >  drivers/gpu/drm/xe/xe_guc.h          |  1 +
> > > > >  drivers/gpu/drm/xe/xe_guc_ads.c      |  6 +++++-
> > > > >  drivers/gpu/drm/xe/xe_guc_fwif.h     |  1 +
> > > > >  6 files changed, 43 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 51f2a03847f9d..47e13a3fb9072 100644
> > > > > --- a/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> > > > > +++ b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> > > > > @@ -545,6 +545,9 @@
> > > > >  #define SARB_CHICKEN1				XE_REG_MCR(0xe90c)
> > > > >  #define   COMP_CKN_IN				REG_GENMASK(30, 29)
> > > > >
> > > > > +#define MAIN_GAMCTRL_MODE			XE_REG(0xef00)
> > > > > +#define   MAIN_GAMCTRL_QUEUE_SELECT		REG_BIT(0)
> > > > > +
> > > > >  #define RCU_MODE				XE_REG(0x14800, XE_REG_OPTION_MASKED)
> > > > >  #define   RCU_MODE_FIXED_SLICE_CCS_MODE		REG_BIT(1)
> > > > >  #define   RCU_MODE_CCS_ENABLE			REG_BIT(0)
> > > > > diff --git a/drivers/gpu/drm/xe/xe_gt.h b/drivers/gpu/drm/xe/xe_gt.h
> > > > > index 5df2ffe3ff838..9d710049da455 100644
> > > > > --- a/drivers/gpu/drm/xe/xe_gt.h
> > > > > +++ b/drivers/gpu/drm/xe/xe_gt.h
> > > > > @@ -22,6 +22,12 @@
> > > > >
> > > > >  #define CCS_MASK(gt) (((gt)->info.engine_mask & XE_HW_ENGINE_CCS_MASK) >> XE_HW_ENGINE_CCS0)
> > > > >
> > > > > +#define GT_VER(gt) ({ \
> > > > > +	typeof(gt) gt_ = (gt); \
> > > > > +	struct xe_device *xe = gt_to_xe(gt_); \
> > > > > +	xe_gt_is_media_type(gt_) ? MEDIA_VER(xe) : GRAPHICS_VER(xe); \
> > > > > +})
> > > > > +
> > > > >  extern struct fault_attr gt_reset_failure;
> > > > >  static inline bool xe_fault_inject_gt_reset(void)
> > > > >  {
> > > > > diff --git a/drivers/gpu/drm/xe/xe_guc.c b/drivers/gpu/drm/xe/xe_guc.c
> > > > > index d94490979adc0..37e3735f34e63 100644
> > > > > --- a/drivers/gpu/drm/xe/xe_guc.c
> > > > > +++ b/drivers/gpu/drm/xe/xe_guc.c
> > > > > @@ -91,6 +91,9 @@ static u32 guc_ctl_feature_flags(struct xe_guc *guc)
> > > > >  	if (xe_configfs_get_psmi_enabled(to_pci_dev(xe->drm.dev)))
> > > > >  		flags |= GUC_CTL_ENABLE_PSMI_LOGGING;
> > > > >
> > > > > +	if (xe_guc_using_main_gamctrl_queues(guc))
> > > > > +		flags |= GUC_CTL_MAIN_GAMCTRL_QUEUES;
> > > > > +
> > > > >  	return flags;
> > > > >  }
> > > > >
> > > > > @@ -1255,8 +1258,13 @@ int xe_guc_min_load_for_hwconfig(struct xe_guc *guc)
> > > > >
> > > > >  int xe_guc_upload(struct xe_guc *guc)
> > > > >  {
> > > > > +	struct xe_gt *gt = guc_to_gt(guc);
> > > > > +
> > > > >  	xe_guc_ads_populate(&guc->ads);
> > > > >
> > > > > +	if (xe_guc_using_main_gamctrl_queues(guc))
> > > > > +		xe_mmio_write32(&gt->mmio, MAIN_GAMCTRL_MODE, MAIN_GAMCTRL_QUEUE_SELECT);
> > > > > +
> > > > >  	return __xe_guc_upload(guc);
> > > > >  }
> > > > >
> > > > > @@ -1657,6 +1665,25 @@ void xe_guc_declare_wedged(struct xe_guc *guc)
> > > > >  	xe_guc_submit_wedge(guc);
> > > > >  }
> > > > >
> > > > > +/**
> > > > > + * xe_guc_using_main_gamctrl_queues() - Detect which reporting queues to use.
> > > > > + * @guc: The GuC object
> > > > > + *
> > > > > + * For Xe3p and beyond, we want to program the hardware to use the
> > > > > + * "Main GAMCTRL queue" rather than the legacy queue before we upload
> > > > > + * the GuC firmware.  This will allow the GuC to use a new set of
> > > > > + * registers for pagefault handling and avoid some unnecessary
> > > > > + * complications with MCR register range handling.
> > > > > + *
> > > > > + * Return: true if can use new main gamctrl queues.
> > > > > + */
> > > > > +bool xe_guc_using_main_gamctrl_queues(struct xe_guc *guc)
> > > > > +{
> > > > > +	struct xe_gt *gt = guc_to_gt(guc);
> > > > > +
> > > > > +	return GT_VER(gt) >= 35;
> > > >
> > > > Revisiting the spec on this, I'm not sure whether using GT_VER() here is
> > > > actually the right thing to do.  As far as I can see, the media GT does
> > > > not actually have a "main gamctrl" register range at all (i.e., bspec
> > > > 76445 only lists MCR gamctrl ranges, and 0x38EF00-0x38EFFF falls within
> > > > a reserved/unused block).  That means that registers like
> > > 
> > > true
> > > 
> > > > MAIN_GAMCTRL_MODE that we're trying to write during xe_guc_upload above
> > > 
> > > We do have MAIN_GAMCTRL_MODE for xe3p_lpm in bspec 73540.
> > > The tagging is odd, but there is a reference to the register at
> > > 0x38EF00. It wouldn't make sense to have a 0x38xxxx register instance
> > > for primary/graphics GT.
> > > 
> > > So either
> > > 
> > > 1) we don't have it like you said
> > > 2) we have it and there's something missing in the MCR ranges
> > > 
> > > > don't actually exist for the media GT.  Furthermore, the tagging on the
> > > > register detail page 73540 also seems to imply that this only applies to
> > > > the primary/graphics GT.
> > > 
> > > so... in that case we'd need to check both the gt type and graphics ver,
> > > so we load GuC with those different flags. I will take a shot at that
> > > and check if anything breaks.
> > > 
> > > Bala, since you did the patch following this, do you remember of
> > > anything related to this?
> > 
> > There seems to be a conflict between the Bspec and the HSD. HSD
> > 16017994409 and the HAS claim the MainGAMCTRL is supported in Xe3p Media
> > IP whereas the Bspec page 73542 and the Steering table show the
> > MainGAMCTRL register range as not available for Xe3p Media.
> > 
> > For me it looks like a Bspec update issue. We have tested and verified
> > the current implementation which enables MainGAMCTRL for Media.
> > I would say lets keep the code as it is though it conflicts with the
> > spec and raise a Bspec issue to get it clarified.
> 
> I'm circling this back with HW and Spec people. I also did some changes
> local changes to test. Although since the old queues are still valid, I
> guess it will just pass, but without this optimization. On the other
> hand, I'd expect it to not work at all with the current code if it
> didn't apply to media.

There is an Bspec update to the steering table yesterday which now lists
the address range 0x38EF00 - 0x38EFFF as non-MCR. But still the Bspec
filtering is not correct. So the register range doesn't show up when
filtered for Xe3p Media.

Regards,
Bala

> 
> Another point is that we can delay this patch a little bit, since the
> next patch turns this off for media with the special bits set.
> 
> thanks
> Lucas De Marchi