[RFC] drm/print: Introduce drm_line_printer

[John Harrison]

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?

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

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.

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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20240523/1c33c0b2/attachment-0001.htm>