Re: [RFC] drm/print: Introduce drm_line_printer

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 15 May 2024, Michal Wajdeczko <michal.wajdeczko@intel.com> wrote:
> On 15.05.2024 11:56, Jani Nikula wrote:
>> On Tue, 14 May 2024, Michal Wajdeczko <michal.wajdeczko@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

Because of __builtin_return_address(0). But currently
__drm_printfn_dbg() doesn't use that anyway, because it would already be
off...

Not really a meaningful argument against this patch, to be fair.

>> 
>> 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@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);

The cast is actually not required.

BR,
Jani.


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

-- 
Jani Nikula, Intel