[RFC] drm/print: Introduce drm_line_printer

[Michal Wajdeczko]

On 15.05.2024 11:56, Jani Nikula wrote:
> On Tue, 14 May 2024, Michal Wajdeczko <michal.wajdeczko at intel.com> wrote:
>> 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.
> 
> Interesting. Nothing another level of abstraction can't solve. ;)
> 
> Except maybe it'll make adding function names to debug printers harder.

but why? primary printer should work in the same way with or without
this line printer

> 
> No strong feelings either way about it, I'll let others chime in.

forgot to mention that this new printer was aimed to simplify the manual
and error prone work done as part of [1] but I'm afraid there is little
traction to have any kind of generic solution at all, since it is
considered as 'over engineering', even at the cost of trashing other
printers that don't need extra robustness

[1] https://patchwork.freedesktop.org/patch/593223/?series=133349&rev=1

> 
> A few comments inline.
> 
> 
> BR,
> Jani.
> 
>>
>> 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);
> 
> Subtle. Might warrant adding a union in struct drm_printer for clarity.

good idea

> 
>> +	struct drm_printer *dp = p->arg;
>> +
>> +	drm_printf(dp, "%u: %pV", line, vaf);
>> +}
>> +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)
> 
> Just p is customary for the drm_printer. "dp" gives me too much Display
> Port vibes.

ha, it was 'p' initially, but then it was still asymmetric compared to
__drm_printfn_line where 'p' was actually referring to the line printer

maybe best option would be to don't have local vars at all:

drm_printf((struct drm_printer *)p->arg, "%u: %pV", p->line, vaf);

> 
>> +{
>> +	struct drm_printer lp = {
>> +		.printfn = __drm_printfn_line,
>> +		.arg = dp,
>> +	};
>> +	return lp;
>> +}
>> +
>>  /*
>>   * struct device based logging
>>   *
>