Re: [PATCH 4/5] drm/i915: Add PLL .compare_hw_state() vfunc

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Feb 14, 2024 at 03:50:49PM +0200, Jani Nikula wrote:
> On Fri, 09 Feb 2024, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Chunk up the humenguous dpll_hw_state comparison check into per-platform
> > variants, implemented in the dpll_mgr. This is step one in allowing
> > each platform (or perhaps even PLL) type to have a custom hw state
> > structure instead of having to smash it all into one.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c  | 78 ++++++++-------
> >  drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 95 +++++++++++++++++++
> >  drivers/gpu/drm/i915/display/intel_dpll_mgr.h |  3 +
> >  3 files changed, 141 insertions(+), 35 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 1d381fa96c84..66ee6749fdae 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -4907,6 +4907,36 @@ pipe_config_mismatch(bool fastset, const struct intel_crtc *crtc,
> >  	va_end(args);
> >  }
> >  
> > +static void
> > +pipe_config_pll_mismatch(bool fastset,
> > +			 const struct intel_crtc *crtc,
> > +			 const char *name,
> > +			 const struct intel_dpll_hw_state *a,
> > +			 const struct intel_dpll_hw_state *b)
> > +{
> > +	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> > +
> > +	if (fastset) {
> > +		if (!drm_debug_enabled(DRM_UT_KMS))
> > +			return;
> > +
> > +		drm_dbg_kms(&i915->drm,
> > +			    "[CRTC:%d:%s] fastset requirement not met in %s\n",
> > +			    crtc->base.base.id, crtc->base.name, name);
> > +		drm_dbg_kms(&i915->drm, "expected:\n");
> > +		intel_dpll_dump_hw_state(i915, a);
> > +		drm_dbg_kms(&i915->drm, "found:\n");
> > +		intel_dpll_dump_hw_state(i915, b);
> > +	} else {
> > +		drm_err(&i915->drm, "[CRTC:%d:%s] mismatch in %s buffer\n",
> > +			crtc->base.base.id, crtc->base.name, name);
> > +		drm_err(&i915->drm, "expected:\n");
> > +		intel_dpll_dump_hw_state(i915, a);
> > +		drm_err(&i915->drm, "found:\n");
> > +		intel_dpll_dump_hw_state(i915, b);
> > +	}
> > +}
> 
> As follow-up, would be great to see this move towards drm_printer based
> approach, similar to pipe_config_dp_vsc_sdp_mismatch(). Reduces
> duplication.

Why did we convert just that single thing and not everything?

-- 
Ville Syrjälä
Intel