Re: [PATCH 01/12] drm/i915: Indicate which pipe failed the fastset check overall

[Ville Syrjälä]

On Thu, Feb 22, 2024 at 04:46:12PM -0500, Rodrigo Vivi wrote:
> On Thu, Feb 15, 2024 at 06:40:44PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > 
> > intel_crtc_check_fastset() is done per-pipe, so it would be nice
> > to know which pipe it was that failed its checkup.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 00ac65a14029..a7f487f5c2b2 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -5562,14 +5562,16 @@ static int intel_modeset_checks(struct intel_atomic_state *state)
> >  static void intel_crtc_check_fastset(const struct intel_crtc_state *old_crtc_state,
> >  				     struct intel_crtc_state *new_crtc_state)
> >  {
> > -	struct drm_i915_private *i915 = to_i915(old_crtc_state->uapi.crtc->dev);
> > +	struct intel_crtc *crtc = to_intel_crtc(new_crtc_state->uapi.crtc);
> > +	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> >  
> >  	/* only allow LRR when the timings stay within the VRR range */
> >  	if (old_crtc_state->vrr.in_range != new_crtc_state->vrr.in_range)
> >  		new_crtc_state->update_lrr = false;
> >  
> >  	if (!intel_pipe_config_compare(old_crtc_state, new_crtc_state, true))
> > -		drm_dbg_kms(&i915->drm, "fastset requirement not met, forcing full modeset\n");
> > +		drm_dbg_kms(&i915->drm, "[CRTC:%d:%s] fastset requirement not met, forcing full modeset\n",
> > +			    crtc->base.base.id, crtc->base.name);
> 
> looking to other patches in this same series, I wonder
> if we shouldn't benefit of a crct_dbg(crtc, "message") that would print
> [CRTC:%d:%s] underneath.

There has been some discussion on this topic recently, but
I don't think that particular approach is good because:
a) it only works when you need to talk about one partiuclar
   object and often we need to talk about more than one
b) different debug function for every little thing is just ugly,
   and we'd probably end up with dozens of differently named
   variants which takes up too many slots in my brain's pattern
   matcher

I think Jani proposed that drm_dbg_kms() could take different kinds
of objects as its first parameter to do this, but I don't like that
either because of point a).

One idea that was floating about was that each object would embed
a .debug_string or somesuch thing which would include the preferred
formatting. With that you could prints with just a simple %s. The
downside is that when you then read the format string you have no
idea what kind of thing each %s refers to unless you also parse
the full argument list to figure out which is which.

And one basic idea I was mulling over at some point was simply
to define DRM_CRTC_FMT/ARGS/etc. macros and use those. But that
makes the format string super ugly and hard to read.


I think the proper solution would be to have actually
sensible conversion specifiers in the format string.
So instead of %<set of random characters> we'd have something
more like %{drm_crtc} (or whatever color you want to throw
on that particular bikeshed).

Adding vsprintf.c folks to cc in case they have some ideas...

-- 
Ville Syrjälä
Intel