Re: [PATCH 06/12] drm/i915: Convert intel_dpll_dump_hw_state() to drm_printer

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

On Thu, Feb 22, 2024 at 04:54:07PM -0500, Rodrigo Vivi wrote:
> On Thu, Feb 15, 2024 at 06:40:49PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Utilize drm_printer in pipe_config_pll_mismatch() to avoid
> > a bit of code duplication.
> > 
> > To achieve this we need to plumb the printer all way to the
> > dpll_mgr .dump_hw_state() functions. Those are also used by
> > intel_crtc_state_dump() which needs to be adjusted as well.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  .../drm/i915/display/intel_crtc_state_dump.c  |   5 +-
> >  drivers/gpu/drm/i915/display/intel_display.c  |  27 ++---
> >  drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 105 ++++++++----------
> >  drivers/gpu/drm/i915/display/intel_dpll_mgr.h |   2 +
> >  4 files changed, 67 insertions(+), 72 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
> > index 4bcf446c75f4..59d2b3d39951 100644
> > --- a/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
> > +++ b/drivers/gpu/drm/i915/display/intel_crtc_state_dump.c
> > @@ -205,9 +205,12 @@ void intel_crtc_state_dump(const struct intel_crtc_state *pipe_config,
> >  	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> >  	const struct intel_plane_state *plane_state;
> >  	struct intel_plane *plane;
> > +	struct drm_printer p;
> >  	char buf[64];
> >  	int i;
> >  
> > +	p = drm_dbg_printer(&i915->drm, DRM_UT_KMS, NULL);
> > +
> >  	drm_dbg_kms(&i915->drm, "[CRTC:%d:%s] enable: %s [%s]\n",
> >  		    crtc->base.base.id, crtc->base.name,
> >  		    str_yes_no(pipe_config->hw.enable), context);
> > @@ -356,7 +359,7 @@ void intel_crtc_state_dump(const struct intel_crtc_state *pipe_config,
> >  		    pipe_config->ips_enabled, pipe_config->double_wide,
> >  		    pipe_config->has_drrs);
> >  
> > -	intel_dpll_dump_hw_state(i915, &pipe_config->dpll_hw_state);
> > +	intel_dpll_dump_hw_state(i915, &p, &pipe_config->dpll_hw_state);
> >  
> >  	if (IS_CHERRYVIEW(i915))
> >  		drm_dbg_kms(&i915->drm,
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index e5010049d52e..d7f39ad84138 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -4927,26 +4927,27 @@ pipe_config_pll_mismatch(bool fastset,
> >  			 const struct intel_dpll_hw_state *b)
> >  {
> >  	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> > +	struct drm_printer p;
> >  
> >  	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);
> > +		p = drm_dbg_printer(&i915->drm, DRM_UT_KMS, NULL);
> > +
> > +		drm_printf(&p, "[CRTC:%d:%s] fastset requirement not met in %s\n",
> > +			   crtc->base.base.id, crtc->base.name, name);
> >  	} 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);
> > +		p = drm_err_printer(&i915->drm, NULL);
> > +
> > +		drm_printf(&p, "[CRTC:%d:%s] mismatch in %s\n",
> > +			   crtc->base.base.id, crtc->base.name, name);
> >  	}
> > +
> > +	drm_dbg_kms(&i915->drm, "expected:\n");
> > +	intel_dpll_dump_hw_state(i915, &p, a);
> > +	drm_dbg_kms(&i915->drm, "found:\n");
> > +	intel_dpll_dump_hw_state(i915, &p, b);
> 
> forgot to convert here?

Looks like that part ended up in the last patch. Probably
a rebase fail on my part. I'll shuffle it back over here.

-- 
Ville Syrjälä
Intel