[RFC] drm/print: Introduce drm_line_printer
Jani Nikula
jani.nikula at linux.intel.com
Wed May 15 13:18:25 UTC 2024
On Wed, 15 May 2024, Michal Wajdeczko <michal.wajdeczko at intel.com> wrote:
> 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
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 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);
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
More information about the dri-devel
mailing list