On Thu, 23 May 2024, John Harrison <john.c.harrison@xxxxxxxxx> 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@xxxxxxxxx>
>> To: dri-devel@xxxxxxxxxxxxxxxxxxxxx
>>
>>
>>
>> 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@xxxxxxxxx>
>> ---
>> 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
