On 12/20/2024 2:49 AM, Cavitt, Jonathan
wrote:
-----Original Message-----From: Roper, Matthew D <matthew.d.roper@xxxxxxxxx> Sent: Thursday, December 19, 2024 12:50 PM To: Cavitt, Jonathan <jonathan.cavitt@xxxxxxxxx> Cc: Vodapalli, Ravi Kumar <ravi.kumar.vodapalli@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; Vivekanandan, Balasubramani <balasubramani.vivekanandan@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: Re: [PATCH] drm/i915/display: Don't program DBUF_CTL tracker state service 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 serviceWhile 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)
Here since DG2 check is also coming into picture which is display version 13 we have to roll back
to previous code, because the higher display versions than 12 should be handled in gen12_dbuf_slices_config() function and implement check for dg2 in that function
if (DISPLAY_VER(display) >= 12) gen12_dbuf_slices_config(display);
Ravi Kumar V
(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.Thank you to both @Roper, Matthew D and @Sousa, Gustavo for your input here. Perhaps this would be a better revision to the commit message in that case? """ Only the following platforms mandate updating DBUF_TRACKER_STATE_SERVICE during display initialization: 1. All platforms that use display version 12 2. DG2 All other platforms should use their default values unless stated otherwise, so update the display initialization code to reflect this requirement. """ -Jonathan CavittMatt-Jonathan Cavittgen12_dbuf_slices_config(display); /* 5. Enable DBUF. */ -- 2.25.1-- Matt Roper Graphics Software Engineer Linux GPU Platform Enablement Intel Corporation
