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

[Jani Nikula]

On Fri, 23 Feb 2024, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> wrote:
> 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

Agreed. I dislike the approach in i915 gem and xe drivers. There are
just too many logging variants now, and as the gates are open, more are
coming. Please let's not go there with display.

> 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).

Fair, but arguably that's not as bad, as you'd then have the "main"
object you're logging with, the info for that comes for free, and you
can add the additional stuff for the other objects manually. But indeed
only solves part of the problem.

> 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.

Also, that would have to be a fixed string, initialized at object
creation. Can't have a function returning the string, because it gets
complicated with C.

> 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.

Agreed.

> 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).

Personally I suck at remembering even the standard printf conversion
specifiers, let alone all the kernel extensions. I basically have to
look them up every time. I'd really love some %{name} format for named
pointer things. And indeed preferrably without the %p. Just %{name}.

And then we could discuss adding support for drm specific things. I
guess one downside is that the functions to do this would have to be in
vsprintf.c instead of drm. Unless we add some code in drm for this
that's always built-in.

BR,
Jani.


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

-- 
Jani Nikula, Intel