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