| Message ID | 20230906234732.3728630-7-matthew.d.roper@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Separate display workarounds from clock gating | expand |
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@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? > 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? 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 >
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@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. > > > 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. 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 > >
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@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@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
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)) 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) 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; - } }
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@intel.com> --- drivers/gpu/drm/i915/intel_clock_gating.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)