[RFC] drm/print: Introduce drm_line_printer

[Jani Nikula]

On Thu, 23 May 2024, John Harrison <john.c.harrison at intel.com> wrote:
> On 5/23/2024 16:54, Daniele Ceraolo Spurio wrote:
>> -------- Forwarded Message --------
>> Subject: 	[RFC] drm/print: Introduce drm_line_printer
>> Date: 	Tue, 14 May 2024 16:56:31 +0200
>> From: 	Michal Wajdeczko <michal.wajdeczko at intel.com>
>> To: 	dri-devel at lists.freedesktop.org
>>
>>
>>
>> This drm printer wrapper can be used to increase the robustness of
>> the captured output generated by any other drm_printer to make sure
>> we didn't lost any intermediate lines of the output by adding line
>> numbers to each output line. Helpful for capturing some crash data.
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
>> ---
>> drivers/gpu/drm/drm_print.c | 9 +++++++++
>> include/drm/drm_print.h | 37 +++++++++++++++++++++++++++++++++++++
>> 2 files changed, 46 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
>> index cf2efb44722c..d6fb50d3407a 100644
>> --- a/drivers/gpu/drm/drm_print.c
>> +++ b/drivers/gpu/drm/drm_print.c
>> @@ -214,6 +214,15 @@ void __drm_printfn_err(struct drm_printer *p, 
>> struct va_format *vaf)
>> }
>> EXPORT_SYMBOL(__drm_printfn_err);
>> +void __drm_printfn_line(struct drm_printer *p, struct va_format *vaf)
>> +{
>> + unsigned int line = (uintptr_t)(++p->prefix);
> The prefix field is officially supposed to be a const char *. There is 
> no documentation to say that this is intended to be used as a private 
> data field by random printer wrappers. So overloading it like this feels 
> very hacky and dangerous. Also, you are mixing types - uintptr_t then 
> uint. So an arch with 64-bit pointers but only 32-bit ints would hit a 
> truncated compiler warning?

I already commented on abusing the type in another reply. I think making
prefix a union would dodge that issue. Otherwise, I think each printer
can pretty much use the arg and prefix members as they see fit. It's not
mucking the printer being wrapped.

>
>> + struct drm_printer *dp = p->arg;
>> +
>> + drm_printf(dp, "%u: %pV", line, vaf);
> This is insufficient. As previously commented, there needs to be a 
> global counter as well as a local line counter. The global count must be 
> global to at least whatever entity is generating a specific set of 
> prints. Being global to a higher level, e.g. kernel global, is fine. But 
> without that, two concurrent dumps that get interleaved can be 
> impossible to separate resulting in a useless bug report/log.
>
> The prefix field could potentially be split into a 16:16 global:local 
> index with the global master just being a static u16 inside that 
> function. With the first print call to a given drm_printer object being 
> defined by the global value being zero. And it then sets the global 
> value to the next increment skipping over zero on a 16-bit wrap around. 
> But see above about prefix not being intended for such purposes. So now 
> you are just piling hacks upon hacks.

To tackle that issue, I think I'd add a "unique id" kind of parameter to
drm_line_printer(). If the user needs more uniqueness, they can maintain
it locally (in a data structure or static or whatever) and id++
it. Could even skip it in printing if set to 0 if it's not likely the
caller needs it. This dodges the issue of having to store global stuff
in the printer, and keeps output lean when not needed.

For example:

        static int id;
	struct drm_printer dp = drm_err_printer(drm, "crash");
	struct drm_printer lp = drm_line_printer(&dp, ++id);

> Plus it would be much nicer output to have the ability to put an 
> arbitrary prefix in front of the G.L number, as per the original 
> implementation. The whole point of this is to aid identification of 
> otherwise uniform data such as hexdumps. So anything that makes it less 
> clear is bad.

The prefix in the printer being wrapped is intact, so you could add it
there. In the above example, it's "crash".

BR,
Jani.




>
> John.
>
>
>> +}
>> +EXPORT_SYMBOL(__drm_printfn_line);
>> +
>> /**
>> * drm_puts - print a const string to a &drm_printer stream
>> * @p: the &drm printer
>> diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
>> index 089950ad8681..58cc73c53853 100644
>> --- a/include/drm/drm_print.h
>> +++ b/include/drm/drm_print.h
>> @@ -186,6 +186,7 @@ void __drm_puts_seq_file(struct drm_printer *p, 
>> const char *str);
>> void __drm_printfn_info(struct drm_printer *p, struct va_format *vaf);
>> void __drm_printfn_dbg(struct drm_printer *p, struct va_format *vaf);
>> void __drm_printfn_err(struct drm_printer *p, struct va_format *vaf);
>> +void __drm_printfn_line(struct drm_printer *p, struct va_format *vaf);
>> __printf(2, 3)
>> void drm_printf(struct drm_printer *p, const char *f, ...);
>> @@ -357,6 +358,42 @@ static inline struct drm_printer 
>> drm_err_printer(struct drm_device *drm,
>> return p;
>> }
>> +/**
>> + * drm_line_printer - construct a &drm_printer that prefixes outputs 
>> with line numbers
>> + * @dp: the &struct drm_printer which actually generates the output
>> + *
>> + * This printer can be used to increase the robustness of the 
>> captured output
>> + * to make sure we didn't lost any intermediate lines of the output. 
>> Helpful
>> + * while capturing some crash data.
>> + *
>> + * For example::
>> + *
>> + * void crash_dump(struct drm_device *drm)
>> + * {
>> + * struct drm_printer dp = drm_err_printer(drm, "crash");
>> + * struct drm_printer lp = drm_line_printer(&dp);
>> + *
>> + * drm_printf(&lp, "foo");
>> + * drm_printf(&lp, "bar");
>> + * }
>> + *
>> + * Above code will print into the dmesg something like::
>> + *
>> + * [ ] 0000:00:00.0: [drm] *ERROR* crash 1: foo
>> + * [ ] 0000:00:00.0: [drm] *ERROR* crash 2: bar
>> + *
>> + * RETURNS:
>> + * The &drm_printer object
>> + */
>> +static inline struct drm_printer drm_line_printer(struct drm_printer *dp)
>> +{
>> + struct drm_printer lp = {
>> + .printfn = __drm_printfn_line,
>> + .arg = dp,
>> + };
>> + return lp;
>> +}
>> +
>> /*
>> * struct device based logging
>> *
>> -- 
>> 2.43.0
>>

-- 
Jani Nikula, Intel