[PATCH v3 1/4] drm/xe/bmg: Update Wa_14022085890
Summers, Stuart
stuart.summers at intel.com
Wed Jun 11 23:31:35 UTC 2025
On Wed, 2025-06-11 at 18:02 -0500, Lucas De Marchi wrote:
> On Fri, Jun 06, 2025 at 07:09:39PM -0500, Lucas De Marchi wrote:
> > On Wed, Jun 04, 2025 at 12:06:52AM +0000, Stuart Summers wrote:
> > > On Tue, 2025-06-03 at 13:31 -0700, Belgaumkar, Vinay wrote:
> > > >
> > > > On 6/3/2025 12:31 PM, Summers, Stuart wrote:
> > > > > On Tue, 2025-06-03 at 11:41 -0700, Belgaumkar, Vinay wrote:
> > > > > > On 6/3/2025 10:36 AM, Summers, Stuart wrote:
> > > > > > > On Mon, 2025-06-02 at 16:44 -0700, Vinay Belgaumkar
> > > > > > > wrote:
> > > > > > > > Set GT min frequency to 1200Mhz once driver load is
> > > > > > > > complete.
> > > > > > > >
> > > > > > > > v2: Review comments (Rodrigo)
> > > > > > > > v3: Apply Wa earlier so user_req_min is not clobbered.
> > > > > > > >
> > > > > > > > Cc: Matt Roper <matthew.d.roper at intel.com>
> > > > > > > > Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > > > > > > > Signed-off-by: Vinay Belgaumkar
> > > > > > > > <vinay.belgaumkar at intel.com>
> > > > > > > > ---
> > > > > > > > drivers/gpu/drm/xe/xe_guc_pc.c | 5 +++++
> > > > > > > > drivers/gpu/drm/xe/xe_wa_oob.rules | 1 +
> > > > > > > > 2 files changed, 6 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c
> > > > > > > > b/drivers/gpu/drm/xe/xe_guc_pc.c
> > > > > > > > index 18c623992035..cb0563494fcc 100644
> > > > > > > > --- a/drivers/gpu/drm/xe/xe_guc_pc.c
> > > > > > > > +++ b/drivers/gpu/drm/xe/xe_guc_pc.c
> > > > > > > > @@ -51,6 +51,7 @@
> > > > > > > >
> > > > > > > > #define LNL_MERT_FREQ_CAP 800
> > > > > > > > #define BMG_MERT_FREQ_CAP 2133
> > > > > > > > +#define BMG_MIN_FREQ 1200
> > > > > > > >
> > > > > > > > #define SLPC_RESET_TIMEOUT_MS 5 /* roughly 5ms, but
> > > > > > > > no
> > > > > > > > need for
> > > > > > > > precision */
> > > > > > > > #define SLPC_RESET_EXTENDED_TIMEOUT_MS 1000 /* To be
> > > > > > > > used
> > > > > > > > only
> > > > > > > > at
> > > > > > > > pc_start */
> > > > > > > > @@ -843,6 +844,9 @@ static int
> > > > > > > > pc_adjust_freq_bounds(struct
> > > > > > > > xe_guc_pc
> > > > > > > > *pc)
> > > > > > > > if (pc_get_min_freq(pc) > pc->rp0_freq)
> > > > > > > > ret = pc_set_min_freq(pc, pc-
> > > > > > > > >rp0_freq);
> > > > > > > >
> > > > > > > > + if (XE_WA(pc_to_gt(pc), 14022085890))
> > > > > > > > + ret = pc_set_min_freq(pc,
> > > > > > > > max(BMG_MIN_FREQ,
> > > > > > > > pc_get_min_freq(pc)));
> > > > > > > Do we want this above the get_min_freq() call above?
> > > > > > > Seems like
> > > > > > > that
> > > > > > > would further limit which is what we'd want in that case
> > > > > > > right?
> > > > > > No, this is during SLPC init, we don't need to read the min
> > > > > > freq
> > > > > > at
> > > > > > this
> > > > > > point. We will adjust it later in the flow if there was a
> > > > > > user
> > > > > > request.
> > > >
> > > > This ensures min frequency is set to 1200. That is required by
> > > > the
> > > > WA.
> > > > If there was no previous user request (like before a GT reset),
> > > > the
> > > > min
> > > > will be set to 1200 instead of the usual RPe (which is around
> > > > 500).
> > >
> > > Ok makes sense to me. I did confirm the workaround frequency
> > > called out
> > > in bspec matches here:
> > > Reviewed-by: Stuart Summers <stuart.summers at intel.com>
> >
> > but why do we do 2 rmw instead of 1? Something like this
> >
> > u32 min_freq;
> > ...
> >
> > /*
> > * Same thing happens for Server platforms where min is
> > listed as
> > * RPMax
> > */
> > min_freq = min(pc_get_min_freq(pc), pc->rp0_freq);
> >
> > /* But shouldn't go to less than BMG_MIN_FREQ on affected
> > platforms */
> > if (XE_WA(pc_to_gt(pc), 14022085890))
> > min_freq = max(BMG_MIN_FREQ, min_freq);
> >
> > ret = pc_set_min_freq(pc, min_freq);
>
> humn... but that would mean we would always have a call to
> pc_set_min_freq(), even if not needed, i.e.
> pc_get_min_freq() <= pc->rp0_freq and for platforms affected by
> 14022085890, pc_get_min_freq() >= BMG_MIN_FREQ.
>
> still trying to figure out one thing though: in BMG this will apply
> only
> to the main gt. media gt will not set it and since pc is per gt, it's
> not the correct thing to do.
>
> Matt Roper, what about add the diff below? Untested, will do later
> today.
>
> > diff --git a/drivers/gpu/drm/xe/xe_guc_pc.c
> > b/drivers/gpu/drm/xe/xe_guc_pc.c
> > index 3861e947387fd..04e112c751096 100644
> > --- a/drivers/gpu/drm/xe/xe_guc_pc.c
> > +++ b/drivers/gpu/drm/xe/xe_guc_pc.c
> > @@ -818,6 +818,7 @@ void xe_guc_pc_init_early(struct xe_guc_pc *pc)
> >
> > static int pc_adjust_freq_bounds(struct xe_guc_pc *pc)
> > {
> > + struct xe_tile *tile = gt_to_tile(pc_to_gt(pc));
> > int ret;
> >
> > lockdep_assert_held(&pc->freq_lock);
> > @@ -844,7 +845,7 @@ static int pc_adjust_freq_bounds(struct
> > xe_guc_pc *pc)
> > if (pc_get_min_freq(pc) > pc->rp0_freq)
> > ret = pc_set_min_freq(pc, pc->rp0_freq);
> >
> > - if (XE_WA(pc_to_gt(pc), 14022085890))
> > + if (XE_WA(tile->primary_gt, 14022085890))
I didn't see anything in bspec that explicitly called out media vs non-
media. Where are you getting that here?
Thanks,
Stuart
> > ret = pc_set_min_freq(pc, max(BMG_MIN_FREQ,
> > pc_get_min_freq(pc)));
> >
> > out:
> > diff --git a/drivers/gpu/drm/xe/xe_wa_oob.rules
> > b/drivers/gpu/drm/xe/xe_wa_oob.rules
> > index bda025ab900da..38b589fe9198d 100644
> > --- a/drivers/gpu/drm/xe/xe_wa_oob.rules
> > +++ b/drivers/gpu/drm/xe/xe_wa_oob.rules
> > @@ -59,4 +59,7 @@ no_media_l3 MEDIA_VERSION(3000)
> > MEDIA_VERSION_RANGE(1301, 3000)
> > 16026508708 GRAPHICS_VERSION_RANGE(1200, 3001)
> > MEDIA_VERSION_RANGE(1300, 3000)
> > +
> > +# SoC workaround - should apply to all platforms using this
> > +# graphics version in tile->primary_gt
> > 14022085890 GRAPHICS_VERSION(2001)
>
> Lucas De Marchi
More information about the Intel-xe
mailing list