Re: [PATCH 2/3] drm/xe/tuning: Use proper register offset for GAMSTLB_CTRL

[Gustavo Sousa <gustavo.sousa@intel.com>]

Matt Roper <matthew.d.roper@intel.com> writes:

> On Mon, Apr 13, 2026 at 12:36:46PM -0300, Gustavo Sousa wrote:
>> Matt Roper <matthew.d.roper@intel.com> writes:
>> 
>> > From Xe2 onward (i.e., all platforms officially supported by the Xe
>> > driver), the GAMSTLB_CTRL register is located at offset 0x477C and
>> > represented by the macro "GAMSTLB_CTRL" in code.  However the register
>> > formerly resided at offset 0xCF4C on Xe1-era platforms, and we also have
>> > macro XEHP_GAMSTLB_CTRL that represents this old offset in the
>> > unofficial/developer-only Xe1 code.  When tuning for the register was
>> > added for Xe3p_LPG, the old Xe1-era macro was accidentally used instead
>> > of the proper macro for Xe2 and beyond, causing the tuning to not be
>> > applied properly.  Use the proper definition so that the correct offset
>> > is written to.
>> >
>> > Bspec: 59298
>> > Fixes: 377c89bfaa5d ("drm/xe/xe3p_lpg: Set STLB bank hash mode to 4KB")
>> > Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
>> > ---
>> >  drivers/gpu/drm/xe/xe_tuning.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/gpu/drm/xe/xe_tuning.c b/drivers/gpu/drm/xe/xe_tuning.c
>> > index ea48e2a60fcd..6fb8887d1482 100644
>> > --- a/drivers/gpu/drm/xe/xe_tuning.c
>> > +++ b/drivers/gpu/drm/xe/xe_tuning.c
>> > @@ -97,7 +97,7 @@ static const struct xe_rtp_entry_sr gt_tunings[] = {
>> >  	{ XE_RTP_NAME("Tuning: Set STLB Bank Hash Mode to 4KB"),
>> >  	  XE_RTP_RULES(GRAPHICS_VERSION_RANGE(3510, XE_RTP_END_VERSION_UNDEFINED),
>> >  		       IS_INTEGRATED),
>> > -	  XE_RTP_ACTIONS(FIELD_SET(XEHP_GAMSTLB_CTRL, BANK_HASH_MODE,
>> > +	  XE_RTP_ACTIONS(FIELD_SET(GAMSTLB_CTRL, BANK_HASH_MODE,
>> >  				   BANK_HASH_4KB_MODE))
>> 
>> Should we also consolidate the definitions in xe_gt_regs.h into a single
>> section as well?
>
> We've intentionally avoided doing that in Xe; when a register exists at
> different locations on different IP versions, we still maintain each
> definition at a properly offset-sorted location.  Making exceptions to
> the sorting rules got too chaotic on i915 so we decided early on not to
> allow that with Xe.

Ah, okay.  Makes sense.  On the other hand, we end up with a
fragmentation of where the bits are defined?  For example, if SOME_REG
changes the offset in a new hardware release, has new fields, but also
shares some fields with the previous version, I guess we would have the
field definitions in different places.

By the way, should we write down the rules for register headers for Xe
somewhere? (Unless it is already written somewhere and I did a poor job
searching for it.)

--
Gustavo Sousa

>
>
> Matt
>
>> 
>> In any case,
>> 
>> Reviewed-by: Gustavo Sousa <gustavo.sousa@intel.com>
>> 
>> >  	},
>> >  };
>> >
>> > -- 
>> > 2.53.0
>
> -- 
> Matt Roper
> Graphics Software Engineer
> Linux GPU Platform Enablement
> Intel Corporation