[PATCH] drm/i915/display: Don't program DBUF_CTL tracker state service

[Cavitt, Jonathan]

-----Original Message-----
From: Intel-gfx <intel-gfx-bounces at lists.freedesktop.org> On Behalf Of Ravi Kumar Vodapalli
Sent: Thursday, December 19, 2024 9:37 AM
To: intel-gfx at lists.freedesktop.org
Cc: Vivekanandan, Balasubramani <balasubramani.vivekanandan at intel.com>; Roper, Matthew D <matthew.d.roper at intel.com>; De Marchi, Lucas <lucas.demarchi at intel.com>; Sousa, Gustavo <gustavo.sousa at intel.com>; Taylor, Clinton A <clinton.a.taylor at intel.com>; Atwood, Matthew S <matthew.s.atwood at intel.com>; Bhadane, Dnyaneshwar <dnyaneshwar.bhadane at intel.com>; Kalvala, Haridhar <haridhar.kalvala at intel.com>; Chauhan, Shekhar <shekhar.chauhan at intel.com>
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.
"""

> 
> Bspec: 49213
> Signed-off-by: Ravi Kumar Vodapalli <ravi.kumar.vodapalli at intel.com>
> ---
>  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.

-Jonathan Cavitt

>  		gen12_dbuf_slices_config(display);
>  
>  	/* 5. Enable DBUF. */
> -- 
> 2.25.1
> 
>