On Thu, Dec 19, 2024 at 11:39:07AM -0800, Cavitt, Jonathan wrote:
> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of Ravi Kumar Vodapalli
> Sent: Thursday, December 19, 2024 9:37 AM
> To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: Vivekanandan, Balasubramani <balasubramani.vivekanandan@xxxxxxxxx>; Roper, Matthew D <matthew.d.roper@xxxxxxxxx>; De Marchi, Lucas <lucas.demarchi@xxxxxxxxx>; Sousa, Gustavo <gustavo.sousa@xxxxxxxxx>; Taylor, Clinton A <clinton.a.taylor@xxxxxxxxx>; Atwood, Matthew S <matthew.s.atwood@xxxxxxxxx>; Bhadane, Dnyaneshwar <dnyaneshwar.bhadane@xxxxxxxxx>; Kalvala, Haridhar <haridhar.kalvala@xxxxxxxxx>; Chauhan, Shekhar <shekhar.chauhan@xxxxxxxxx>
> Subject: [PATCH] drm/i915/display: Don't program DBUF_CTL tracker state service
> >
> > While display initialization along with MBUS credits programming
> > DBUF_CTL register is also programmed, as a part of it the tracker
> > state service field is also set to 0x8 value when default value is
> > other than 0x8 which are for platforms past display version 13.
> > For remaining platforms the default value is already 0x8 so don't
> > program them.
>
> This could use some rewording. Perhaps:
> """
> For platforms past display version 13, during display initialization along
> with MBUS credits and DBUF_CTL register programming, the tracker state
> service field is set to a value of 0x8, which is not the default value for
> these platforms. All other platforms use 0x8 as the default value and thus
> do not need to be overwritten.
> """
>
> Or maybe:
> """
> During display initialization and MBUS credits programming, the
> DBUF_CTL register is also programmed. Specifically, when
> programming DBUF_CTL, the tracker state service field is set to the
> value 0x8. However, this is already the default value for platforms
> using display versions 13 and prior, so we do not need to program
> the DBUF_CTL register on those platforms.
> """
I think these are still missing the point a bit. The key is that the
bspec asks us to program non-default values _only_ on certain platforms.
For all other platforms (both earlier and later), the expectation
is that the hardware's default settings (whatever they happen to be for
a given platform) are already correct and should not be adjusted.
The code here was originally written back during gen12 development
following the standard "assume driver changes will apply to all future
platforms too until we know otherwise" design, so the original test was
written as ">= 12." It looks like DG2 (one display version 13 platform)
still needed the programming, but ADL-P (the other display version 13
platform) did not. From version 14 onward, no platforms need it. So
the correct condition to match the hardware/bspec's rules would be:
if (DISPLAY_VER(display) == 12 || display->platform.dg2)
(I think on an earlier review of this I said it should be just
"DISPLAY_VER(display) == 12" but I overlooked that DG2 is indeed
included in the list of platforms on bspec page 49213).
Once later platforms like ADL-P, MTL, etc. rolled around, it turned out
that explicit programming was actually not expected to carry forward,
and we should have adjusted the condition to just 12+DG2 at that time,
but it was overlooked. That was technically a bug, but it turned out to
be harmless because the explicit value we were programming (8) happened
to match the new hardware defaults on display version 13. However we
shouldn't count on it staying harmless forever --- if the hardware
default ever changes to something else in the future, then it can cause
problems if we're still explicitly programming it to "8" by accident;
this patch is addressing that earlier oversight.
>
> >
> > Bspec: 49213
> > Signed-off-by: Ravi Kumar Vodapalli <ravi.kumar.vodapalli@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/display/intel_display_power.c | 5 +----
> > 1 file changed, 1 insertion(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
> > index 34465d56def0..98db721cba33 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> > @@ -1126,9 +1126,6 @@ static void gen12_dbuf_slices_config(struct intel_display *display)
> > {
> > enum dbuf_slice slice;
> >
> > - if (display->platform.alderlake_p)
> > - return;
> > -
>
> I take it we're removing this condition because we no longer expect this code to run
> on alderlake_p anyways?
There's a platform/version check at the callsite and another here inside
the function. We only need to do the check at one place or the other;
that's somewhat independent of fixing the check(s) themselves to match
the right platforms.
>
> > for_each_dbuf_slice(display, slice)
> > intel_de_rmw(display, DBUF_CTL_S(slice),
> > DBUF_TRACKER_STATE_SERVICE_MASK,
> > @@ -1681,7 +1678,7 @@ static void icl_display_core_init(struct intel_display *display,
> > /* 4. Enable CDCLK. */
> > intel_cdclk_init_hw(display);
> >
> > - if (DISPLAY_VER(display) >= 12)
> > + if (DISPLAY_VER(display) == 12)
>
> If I'm understanding the purpose of this patch correctly (which I'm probably not),
> shouldn't this be "if (DISPLAY_VER(display) > 13)"?
> We only want to overwrite the tracker state service field on platforms after display version 13,
> and it seems like gen12_dbuf_slices_config overwrites the tracker state service field.
No, this change here is [mostly] correct; explicit driver programming at
display init time should only happen on 12 and DG2. The DG2 condition
is missing (probably because I overlooked it and misspoke in an earlier
code review). For all other platforms (both earlier and later versions)
we should leave the MBUF_CTL registers at their hardware defaults and
not touch them here.
Also, don't confuse the code here (which adjusts the register at display
init time) with the other (re)programming of these values that happens
at runtime during modeset (adjusting based on how many pipes are
active). There are separate rules later in the bspec page and separate
code that handles that (correctly I believe); the patch here is just
addressing the specific stanza of the bspec page that says "The MBus
credits should be setup once with the following default values during
the display initialization," and that block only applies to the various
platforms that user display version 12, and then also to DG2.
Matt
>
> -Jonathan Cavitt
>
> > gen12_dbuf_slices_config(display);
> >
> > /* 5. Enable DBUF. */
> > --
> > 2.25.1
> >
> >
--
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation
