[Intel-gfx] [PATCH 1/4] drm/i915: Stop forcing clock gating init for future platforms

[Lucas De Marchi]

On Fri, Sep 08, 2023 at 02:50:55PM -0700, Matt Roper wrote:
>On Fri, Sep 08, 2023 at 04:45:33PM -0500, Lucas De Marchi wrote:
>> On Wed, Sep 06, 2023 at 04:47:34PM -0700, Matt Roper wrote:
>> > In the early days of i915, pretty much every platform needed to
>> > initialize _something_ in the clock gating init functions.  In some
>> > cases the items initialized were inside the GT (and really should have
>> > been initialized through the GT workaround infrastructure instead).
>> > In other cases they were display programming (sometimes not even related
>> > to "clock gating" at all!) which probably needs to move inside the
>> > display-specific code.  The number of initialization tasks that are
>> > truly "clock gating" and don't fall within the GT or display domains is
>> > relatively limited.  Let's stop forcing future platforms to always
>> > define a clock gating init hook.
>> >
>> > Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
>> > ---
>> > drivers/gpu/drm/i915/intel_clock_gating.c | 10 +++-------
>> > 1 file changed, 3 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_clock_gating.c b/drivers/gpu/drm/i915/intel_clock_gating.c
>> > index c66eb6abd4a2..1f2e2d7087cb 100644
>> > --- a/drivers/gpu/drm/i915/intel_clock_gating.c
>> > +++ b/drivers/gpu/drm/i915/intel_clock_gating.c
>> > @@ -835,9 +835,7 @@ CG_FUNCS(nop);
>> >  */
>> > void intel_clock_gating_hooks_init(struct drm_i915_private *i915)
>> > {
>> > -	if (IS_METEORLAKE(i915))
>> > -		i915->clock_gating_funcs = &nop_clock_gating_funcs;
>> > -	else if (IS_PONTEVECCHIO(i915))
>> > +	if (IS_PONTEVECCHIO(i915))
>>
>> shouldn't we use the normal "last platforms first" and just add a check
>> here for GRAPHICS/DISPLAY version >= x?
>
>That's basically what we're doing here.  PVC is the latest/newest
>platform that needs any clock gating handling.

kind of.... we usually have checks for >= in the middle of if/else
ladder, which would short circuit it. Anyway, since all the checks here
are for == version, I guess this is good enough.
>
>>
>> > 		i915->clock_gating_funcs = &pvc_clock_gating_funcs;
>> > 	else if (IS_DG2(i915))
>> > 		i915->clock_gating_funcs = &dg2_clock_gating_funcs;
>> > @@ -845,7 +843,7 @@ void intel_clock_gating_hooks_init(struct drm_i915_private *i915)
>> > 		i915->clock_gating_funcs = &xehpsdv_clock_gating_funcs;
>> > 	else if (IS_ALDERLAKE_P(i915))
>> > 		i915->clock_gating_funcs = &adlp_clock_gating_funcs;
>> > -	else if (GRAPHICS_VER(i915) == 12)
>> > +	else if (DISPLAY_VER(i915) == 12)
>>
>> why changing this while the others still check for graphics version?
>
>If this used graphics version, it would pick up stuff like MTL now that
>they don't show up at the top of the ladder.  DISPLAY_VER matches the
>platforms that we actually mean to match, especially since the
>programming inside this specific branch is display-only programming.

ok


Reviewed-by: Lucas De Marchi <lucas.demarchi at intel.com>

thanks
Lucas De Marchi

>
>
>Matt
>
>>
>> Lucas De Marchi
>>
>> > 		i915->clock_gating_funcs = &gen12lp_clock_gating_funcs;
>> > 	else if (GRAPHICS_VER(i915) == 11)
>> > 		i915->clock_gating_funcs = &icl_clock_gating_funcs;
>> > @@ -885,8 +883,6 @@ void intel_clock_gating_hooks_init(struct drm_i915_private *i915)
>> > 		i915->clock_gating_funcs = &i85x_clock_gating_funcs;
>> > 	else if (GRAPHICS_VER(i915) == 2)
>> > 		i915->clock_gating_funcs = &i830_clock_gating_funcs;
>> > -	else {
>> > -		MISSING_CASE(INTEL_DEVID(i915));
>> > +	else
>> > 		i915->clock_gating_funcs = &nop_clock_gating_funcs;
>> > -	}
>> > }
>> > --
>> > 2.41.0
>> >
>
>-- 
>Matt Roper
>Graphics Software Engineer
>Linux GPU Platform Enablement
>Intel Corporation