diff mbox series

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

Message ID 20240215164055.30585-2-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915: Use drm_printer more | expand

Commit Message

Ville Syrjälä Feb. 15, 2024, 4:40 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

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@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Rodrigo Vivi Feb. 22, 2024, 9:46 p.m. UTC | #1 
On Thu, Feb 15, 2024 at 06:40:44PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> 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@linux.intel.com>
> ---
>  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.

But anyway,

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

>  	else
>  		new_crtc_state->uapi.mode_changed = false;
>  
> -- 
> 2.43.0
>
Ville Syrjälä Feb. 23, 2024, 7:47 p.m. UTC | #2 
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@linux.intel.com>
> > 
> > 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@linux.intel.com>
> > ---
> >  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...
Jani Nikula Feb. 26, 2024, 2:57 p.m. UTC | #3 
On Fri, 23 Feb 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> 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@linux.intel.com>
>> > 
>> > 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@linux.intel.com>
>> > ---
>> >  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...
Andy Shevchenko Feb. 26, 2024, 3:10 p.m. UTC | #4 
On Mon, Feb 26, 2024 at 04:57:58PM +0200, Jani Nikula wrote:
> On Fri, 23 Feb 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> > On Thu, Feb 22, 2024 at 04:46:12PM -0500, Rodrigo Vivi wrote:

...

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

It will become something like %{name[:subextensions]}, where subextensions
is what we now have with different letters/numbers after %pX (X is a letter
which you proposed to have written as name AFAIU).

> 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.
Jani Nikula Feb. 26, 2024, 3:35 p.m. UTC | #5 
On Mon, 26 Feb 2024, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> On Mon, Feb 26, 2024 at 04:57:58PM +0200, Jani Nikula wrote:
>> On Fri, 23 Feb 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
>> > On Thu, Feb 22, 2024 at 04:46:12PM -0500, Rodrigo Vivi wrote:
>
> ...
>
>> > 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}.
>
> It will become something like %{name[:subextensions]}, where subextensions
> is what we now have with different letters/numbers after %pX (X is a letter
> which you proposed to have written as name AFAIU).

Thanks, I appreciate it, a lot!

But could you perhaps try to go with just clean %{name} only instead of
adding [:subextensions] right away, please?

I presume the suggestion comes from an implementation detail, and I
guess it would be handy to reuse the current implementation for
subextension.

For example, %pb -> %{bitmap} and %pbl -> %{bitmap:l}. But really I
think the better option would be for the latter to become, say,
%{bitmap-list}. The goal here is to make them easy to remember and
understand, without resorting to looking up the documentation!


BR,
Jani.

>
>> 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.
Andy Shevchenko Feb. 26, 2024, 4:30 p.m. UTC | #6 
On Mon, Feb 26, 2024 at 05:35:51PM +0200, Jani Nikula wrote:
> On Mon, 26 Feb 2024, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > On Mon, Feb 26, 2024 at 04:57:58PM +0200, Jani Nikula wrote:
> >> On Fri, 23 Feb 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> >> > On Thu, Feb 22, 2024 at 04:46:12PM -0500, Rodrigo Vivi wrote:

...

> >> > 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}.
> >
> > It will become something like %{name[:subextensions]}, where subextensions
> > is what we now have with different letters/numbers after %pX (X is a letter
> > which you proposed to have written as name AFAIU).
> 
> Thanks, I appreciate it, a lot!

Oh, I meant "can" rather than "will".

> But could you perhaps try to go with just clean %{name} only instead of
> adding [:subextensions] right away, please?
> 
> I presume the suggestion comes from an implementation detail, and I
> guess it would be handy to reuse the current implementation for
> subextension.
> 
> For example, %pb -> %{bitmap} and %pbl -> %{bitmap:l}. But really I
> think the better option would be for the latter to become, say,
> %{bitmap-list}. The goal here is to make them easy to remember and
> understand, without resorting to looking up the documentation!

Okay, so it seems you have something in mind, perhaps you can submit a draft
of the list of those "names"?

> >> 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.
Ville Syrjälä Feb. 26, 2024, 4:35 p.m. UTC | #7 
On Mon, Feb 26, 2024 at 05:35:51PM +0200, Jani Nikula wrote:
> On Mon, 26 Feb 2024, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > On Mon, Feb 26, 2024 at 04:57:58PM +0200, Jani Nikula wrote:
> >> On Fri, 23 Feb 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> >> > On Thu, Feb 22, 2024 at 04:46:12PM -0500, Rodrigo Vivi wrote:
> >
> > ...
> >
> >> > 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}.
> >
> > It will become something like %{name[:subextensions]}, where subextensions
> > is what we now have with different letters/numbers after %pX (X is a letter
> > which you proposed to have written as name AFAIU).
> 
> Thanks, I appreciate it, a lot!
> 
> But could you perhaps try to go with just clean %{name} only instead of
> adding [:subextensions] right away, please?
> 
> I presume the suggestion comes from an implementation detail, and I
> guess it would be handy to reuse the current implementation for
> subextension.
> 
> For example, %pb -> %{bitmap} and %pbl -> %{bitmap:l}. But really I
> think the better option would be for the latter to become, say,
> %{bitmap-list}. The goal here is to make them easy to remember and
> understand, without resorting to looking up the documentation!

I was also wondering if we should have some kind of namespace
thing in there. Eg. instead of %{drm_crtc} it could be something
like %{drm:crtc}. Then it would be clear which subsystem (when that
makes sense) "owns" that particular format. But I suppose using
the C foo_ namespacing rule would also work since it should already
be a thing for exported symbols anyway.
Rasmus Villemoes Feb. 27, 2024, 9:38 a.m. UTC | #8 
On 26/02/2024 15.57, Jani Nikula wrote:

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

Sorry to spoil the fun, but that's a non-starter.

foo.c: In function ‘foo’:
foo.c:5:24: warning: unknown conversion type character ‘{’ in format
[-Wformat=]
    5 |         printf("Hello %{function} World\n", &foo);
      |                        ^

You can't start accepting stuff that -Wformat will warn about. We're not
going to start building with Wno-format.

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

If people can be trusted to write callbacks with the proper semantics
for snprintf [1], we could do a generic

typedef char * (*printf_callback)(char *buf, char *end, void *ctx);

struct printf_ext {
  printf_callback cb;
  void *ctx;
};

#define PRINTF_EXT(callback, context) &(struct printf_ext){ .cb =
callback, .ctx = context }

// in drm-land

char* my_drm_gizmo_formatter(char *buf, char *end, void *ctx)
{
  struct drm_gizmo *dg = ctx;
  ....
  return buf;
}
#define pX_gizmo(dg) PRINTF_EXT(my_drm_gizmo_formatter, dg)

   printk("error: gizmo %pX in wrong state!\n", pX_gizmo(dg));

Then vsprintf.c doesn't need to know anything about any particular
subsystem. And if a subsystem breaks snprintf semantics, they get to
keep the pieces. With a little more macro magic, one might even be able
to throw in some type safety checks.

Rasmus

[1] You can't sleep, you can't allocate memory, you probably can't even
take any raw spinlocks, you must attempt to do the full formatting so
you can tell how much room would be needed, but you must of course not
write anything beyond end. Calling vsnprintf() to format various integer
members is probably ok, but recursively using %pX to print full
subobjects is likely a bad idea.
Ville Syrjälä Feb. 27, 2024, 6:32 p.m. UTC | #9 
On Tue, Feb 27, 2024 at 10:38:10AM +0100, Rasmus Villemoes wrote:
> On 26/02/2024 15.57, Jani Nikula wrote:
> 
> > 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}.
> 
> Sorry to spoil the fun, but that's a non-starter.
> 
> foo.c: In function ‘foo’:
> foo.c:5:24: warning: unknown conversion type character ‘{’ in format
> [-Wformat=]
>     5 |         printf("Hello %{function} World\n", &foo);
>       |                        ^
> 
> You can't start accepting stuff that -Wformat will warn about. We're not
> going to start building with Wno-format.

Are there any sensible looking characters we could use for
this? Ideally I'd like to have something to bracket the
outsides, and perhaps a namespace separator in the middle.

Or are we really forced into having essentially a random set
of characters following just a %p/etc.?

> 
> > 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.
> 
> If people can be trusted to write callbacks with the proper semantics
> for snprintf [1], we could do a generic

Yeah, I was at some point thinking that having a version of
register_printf_function() for printk() might be nice. The dangers
being that we get conflicts between subsystems (*), or that it gets
totally out of hand, or as you point out below people will start
to do questionable things in there.

(*) My earlier "include a subsystem namespace in the format" 
idea was basically how I was thinking of avoiding conflicts.

> 
> typedef char * (*printf_callback)(char *buf, char *end, void *ctx);
> 
> struct printf_ext {
>   printf_callback cb;
>   void *ctx;
> };
> 
> #define PRINTF_EXT(callback, context) &(struct printf_ext){ .cb =
> callback, .ctx = context }
> 
> // in drm-land
> 
> char* my_drm_gizmo_formatter(char *buf, char *end, void *ctx)
> {
>   struct drm_gizmo *dg = ctx;
>   ....
>   return buf;
> }
> #define pX_gizmo(dg) PRINTF_EXT(my_drm_gizmo_formatter, dg)
> 
>    printk("error: gizmo %pX in wrong state!\n", pX_gizmo(dg));
> 
> Then vsprintf.c doesn't need to know anything about any particular
> subsystem. And if a subsystem breaks snprintf semantics, they get to
> keep the pieces. With a little more macro magic, one might even be able
> to throw in some type safety checks.
> 
> Rasmus
> 
> [1] You can't sleep, you can't allocate memory, you probably can't even
> take any raw spinlocks, you must attempt to do the full formatting so
> you can tell how much room would be needed, but you must of course not
> write anything beyond end. Calling vsnprintf() to format various integer
> members is probably ok, but recursively using %pX to print full
> subobjects is likely a bad idea.
Rasmus Villemoes Feb. 28, 2024, 8:32 a.m. UTC | #10 
On 27/02/2024 19.32, Ville Syrjälä wrote:
> On Tue, Feb 27, 2024 at 10:38:10AM +0100, Rasmus Villemoes wrote:
>> On 26/02/2024 15.57, Jani Nikula wrote:
>>
>>> 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}.
>>
>> Sorry to spoil the fun, but that's a non-starter.
>>
>> foo.c: In function ‘foo’:
>> foo.c:5:24: warning: unknown conversion type character ‘{’ in format
>> [-Wformat=]
>>     5 |         printf("Hello %{function} World\n", &foo);
>>       |                        ^
>>
>> You can't start accepting stuff that -Wformat will warn about. We're not
>> going to start building with Wno-format.
> 
> Are there any sensible looking characters we could use for
> this? Ideally I'd like to have something to bracket the
> outsides, and perhaps a namespace separator in the middle.
> 
> Or are we really forced into having essentially a random set
> of characters following just a %p/etc.?

You can't put stuff after % that is not in the C standard (or POSIX) -
not until you teach all supported compilers a way to register your own
printf specifier and the semantics of the expected varargs. And the only
format specifier that will accept a random pointer is %p.

Now, as for what we put after %p, the reason we've ended up with the
"random collection of letters" is (probably, I wasn't around when this
was introduced) that you can very reasonably have a format string with
%p followed by some punctuation where you mean for that punctuation to
be output as-is (as a normal printf() implementation would), whereas it
would be weird to write %pR" and expect some output like 0x1234fedcR .
Hence the heuristic was that one could allow any alphanumerics to modify
how that %p should be handled, and in the format string parser simply
skip over those alphanumerics - all without making the compiler angry.

So the problem with introducing %p{some-thing} is that somebody could
already have that %p (possibly with some existing alphanumeric
extension(s)) followed by an opening curly brace, with the latter
expected to be a literal thing. Same for any other punctuation
character. You could probably mostly grep and see if any exist, but
there might be format strings broken across two lines using implicit
string concatenation that won't be found, as well as even more creative
things.

That leaves something like %pX{}, i.e. where some new letter is
designated to indicate "hey, I want something much more readable and
please interpret what's inside {}". That's doable, and then you could
put mostly anything (except } and %) inside there. The format parsing
would just need to be taught that X is special and skip to the }, not
just alphanumerics.

>>> 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.
>>
>> If people can be trusted to write callbacks with the proper semantics
>> for snprintf [1], we could do a generic
> 
> Yeah, I was at some point thinking that having a version of
> register_printf_function() for printk() might be nice. The dangers
> being that we get conflicts between subsystems (*), or that it gets
> totally out of hand, or as you point out below people will start
> to do questionable things in there.
> 
> (*) My earlier "include a subsystem namespace in the format" 
> idea was basically how I was thinking of avoiding conflicts.

So if we really want to go down this road, I think it should be
something like %pX{drm:whatever}, with core printf just looking up the
token "drm" in a run-time list of registered callbacks (because I don't
want vsprintf.c filled with random subsystems' formatting code), and
that single callback would then be handed a pointer to the rest of the
format string (i.e. the "whatever}..."), the pointer argument from the
varargs, and the buf,end pair. But then we're back to trusting that
callback (which might of course multiplex based on the "whatever" part)
to behave correctly. And then we might as well avoid the string parsing
and just do the "callback + pointer" in one struct (passed as pointer to
compound literal), which also avoids the tricky "what to do about module
unload versus unregistering of callbacks" etc.

Rasmus
Petr Mladek Feb. 28, 2024, 9:55 a.m. UTC | #11 
On Wed 2024-02-28 09:32:37, Rasmus Villemoes wrote:
> On 27/02/2024 19.32, Ville Syrjälä wrote:
> > On Tue, Feb 27, 2024 at 10:38:10AM +0100, Rasmus Villemoes wrote:
> >> On 26/02/2024 15.57, Jani Nikula wrote:
> So if we really want to go down this road, I think it should be
> something like %pX{drm:whatever}, with core printf just looking up the
> token "drm" in a run-time list of registered callbacks (because I don't
> want vsprintf.c filled with random subsystems' formatting code), and
> that single callback would then be handed a pointer to the rest of the
> format string (i.e. the "whatever}..."), the pointer argument from the
> varargs, and the buf,end pair. But then we're back to trusting that
> callback (which might of course multiplex based on the "whatever" part)
> to behave correctly. And then we might as well avoid the string parsing
> and just do the "callback + pointer" in one struct (passed as pointer to
> compound literal), which also avoids the tricky "what to do about module
> unload versus unregistering of callbacks" etc.

Mathew Wilcox had the idea to introduce a structure:

	struct printf_state {
	       char *buf;
	       char *end;
	       void *ptr;
	};

Where *ptr would point to some data which should be printed,
same as wee pass to the %pbla now.

Then allow to implement:

	char *my_func(struct printf_state *ps, void *ptr);

and use it as:

	printk("%pX(%p)\n", my_func, ptr);


One problem here is type checking of the data passed via *ptr
but we already have the same problem now.

Another problem is how to make sure that the function is safe.
A solution might be to implement an API for appending characters,
strings, numbers, ... Similar to seq_buf() API.

AFAIK, the result was to actually use the existing seq_buf API
to format the string. AFAIK, it motivated:

   + commit 96928d9032a7c34f1 ("seq_buf: Add seq_buf_do_printk() helper")

and probably also

   + commit d0ed46b60396cfa7 ("tracing: Move readpos from seq_buf to trace_seq")

and also this one is pretty useful:

   + commit dcc4e5728eeaeda8 ("seq_buf: Introduce DECLARE_SEQ_BUF and
     seq_buf_str()")

And it motivated:

   + commit dcc4e5728eeaeda84878ca0 ("seq_buf: Introduce
     DECLARE_SEQ_BUF and seq_buf_str()")


Best Regards,
Petr
diff mbox series

Patch

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);
 	else
 		new_crtc_state->uapi.mode_changed = false;