Re: [PATCH v3 4/8] drm/print: Add drm_printf_indent()

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Oct 26, 2017 at 08:51:57PM +0200, Noralf Trønnes wrote:
> 
> Den 26.10.2017 19.49, skrev Ville Syrjälä:
> > On Thu, Oct 26, 2017 at 06:57:27PM +0200, Noralf Trønnes wrote:
> >> Add drm_printf_indent() that adds tab indentation according to argument.
> >>
> >> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> >> ---
> >>   drivers/gpu/drm/drm_print.c | 21 +++++++++++++++++++++
> >>   include/drm/drm_print.h     |  2 ++
> >>   2 files changed, 23 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_print.c b/drivers/gpu/drm/drm_print.c
> >> index 0b3bf476dc4b..dac3ee98b30f 100644
> >> --- a/drivers/gpu/drm/drm_print.c
> >> +++ b/drivers/gpu/drm/drm_print.c
> >> @@ -64,6 +64,27 @@ void drm_printf(struct drm_printer *p, const char *f, ...)
> >>   }
> >>   EXPORT_SYMBOL(drm_printf);
> >>   
> >> +/**
> >> + * drm_printf_indent - print to a &drm_printer stream with indentation
> >> + * @p: the &drm_printer
> >> + * @i: indentation
> >> + * @f: format string
> >> + */
> >> +void drm_printf_indent(struct drm_printer *p, unsigned int i, const char *f, ...)
> >> +{
> >> +	struct va_format vaf;
> >> +	va_list args;
> >> +
> >> +	drm_printf(p, "%.*s", i, "\t\t\t\t\t\t\t\t\t\t");
> >> +
> >> +	va_start(args, f);
> >> +	vaf.fmt = f;
> >> +	vaf.va = &args;
> >> +	p->printfn(p, &vaf);
> >> +	va_end(args);
> >> +}
> >> +EXPORT_SYMBOL(drm_printf_indent);
> > The double printf() is rather unfortunate. Sadly I don't think there's
> > any proper way to manipulate a va_list to avoid that.
> >
> > Hmm. Would it work if we simply make it a macro? Eg.
> >
> > #define drm_printf_indent(printer, indent, fmt, ...) \
> > 	drm_printf((printer), "%.*s" fmt, (indent), "\t\t\t", ##__VA_ARGS__)
> 
> The macro worked fine and it looks like a better solution to me.

Only downside is that it bloats every format string. But as long
as there aren't a huge number of useless callers we should be fine.

> 
> > The "\t\t\t..." thing is also rather off putting, but I guess it's
> > the best we can do if we want to keep it to one printf(). And maybe we
> > should have a check in there to make sure we have enought tabs in the
> > string to satisfy the indent level, or we just clamp the indent level
> > silently to something reasonable?
> 
> I put 10 tabs in my suggestion, which should be enough and I think it's
> OK to just silently fail to do more. If 10 isn't enough it's easy to add
> more for the developer that hits the limit.

10 is probably even overkill. I'd say if you have to go past four or so
then there's alredy a bigger problem in the code ;)

> 
> Noralf.
> 
> > Oh, seeing the \t now reminds me that tabs won't necesarily get printed
> > out properly. At least I've seen fbcon just printing some weird blobs
> > instead of tabs. Not sure if it's just a matter of having a crappy font
> > or what. That said, the state dump code is using tabs already, so I guess
> > this wouldn't make it worse at least.
> >
> >> +
> >>   #define DRM_PRINTK_FMT "[" DRM_NAME ":%s]%s %pV"
> >>   
> >>   void drm_dev_printk(const struct device *dev, const char *level,
> >> diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h
> >> index 7b9c86a6ca3e..73dcd16eca49 100644
> >> --- a/include/drm/drm_print.h
> >> +++ b/include/drm/drm_print.h
> >> @@ -79,6 +79,8 @@ void __drm_printfn_debug(struct drm_printer *p, struct va_format *vaf);
> >>   
> >>   __printf(2, 3)
> >>   void drm_printf(struct drm_printer *p, const char *f, ...);
> >> +__printf(3, 4)
> >> +void drm_printf_indent(struct drm_printer *p, unsigned int i, const char *f, ...);
> >>   
> >>   
> >>   /**
> >> -- 
> >> 2.14.2

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel