Quoting Cavitt, Jonathan (2024-12-19 16:39:07-03:00)
>-----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
Ravi, it would be better if new revisions of the patch have version
specifiers in the subject (you can use git format-patch -v for that) and
the commit message had the change log of what changed across versions of
the patch.
>>
>> 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.
>"""
Note that DBUF_TRACKER_STATE_SERVICE is set only during initialization.
Looking at the git history, we see that it was done for TGL because the
field didn't actually have 0x8 as default value, so the driver had to
take care of it. In that regard, a reference to 359d0eff8409
("drm/i915/display: Program DBUF_CTL tracker state service") could be
made in the commit message to provide this historical context.
According to the BSpec, programming that field is only applicable to
display version 12. The most compeling reason why we should not program
that field for other display versions that is not part of the official
programming sequence in BSpec, and doing so could affect future
platforms if the default is changed for some reason.
The changes look good to me. With the commit message updated to reflect
the two paragraphs above,
Reviewed-by: Gustavo Sousa <gustavo.sousa@xxxxxxxxx>
>
>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.
>"""
>
>>
>> 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?
>
>> 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.
It is the opposite: we only want to program DBUF_TRACKER_STATE_SERVICE
for display version 12.
--
Gustavo Sousa
