Re: [PATCH v2 1/1] lib/vsprintf: Add support for printing V4L2 and DRM fourccs

[Sakari Ailus]

Hi Rasmus,

Thanks for the comments.

On Fri, Apr 03, 2020 at 02:10:53PM +0200, Rasmus Villemoes wrote:
> On 03/04/2020 11.11, Sakari Ailus wrote:
> > Add a printk modifier %ppf (for pixel format) for printing V4L2 and DRM
> > pixel formats denoted by 4ccs. The 4cc encoding is the same for both so
> > the same implementation can be used.
> 
> This seems quite niche to me, I'm not sure that belongs in vsprintf.c.
> What's wrong with having a
> 
> char *fourcc_string(char *buf, u32 x)
> 
> that formats x into buf and returns buf, so it can be used in a
> 
> char buf[8];
> pr_debug("bla: %s\n", fourcc_string(buf, x))

I guess that could be one option. But changing the implementation could
require changing the size of all those buffers.

We had this approach, too:

<URL:https://lore.kernel.org/linux-media/20190916100433.24367-1-hverkuil-cisco@xxxxxxxxx/>

Let's see if we'll get more comments on this.

> 
> Or, for that matter, since it's for debugging, why not just print x with
> 0x%08x?

People generally prefer readable output that they can understand. The codes
are currently being printed in characters, and that's how they are defined
in kernel headers, too. Therefore the hexadecimal values are of secondary
importance (although they could be printed too, as apparently a similar
function in DRM does).

> 
> At the very least, the "case '4'" in pointer() should be guarded by
> appropriate CONFIG_*.
> 
> Good that Documentation/ gets updated, but test_printf needs updating as
> well.

Agreed.

> 
> 
> > Suggested-by: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>
> > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > ---
> > since v1:
> > 
> > - Improve documentation (add -BE suffix, refer to "FourCC".
> > 
> > - Use '%p4cc' conversion specifier instead of '%ppf'.
> 
> Cute. Remember to update the commit log (which still says %ppf).

I will.

> 
> > - Fix 31st bit handling in printing FourCC codes.
> > 
> > - Use string() correctly, to allow e.g. proper field width handling.
> > 
> > - Remove loop, use put_unaligned_le32() instead.
> > 
> >  Documentation/core-api/printk-formats.rst | 12 +++++++++++
> >  lib/vsprintf.c                            | 25 +++++++++++++++++++++++
> >  2 files changed, 37 insertions(+)
> > 
> > diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
> > index 8ebe46b1af39..550568520ab6 100644
> > --- a/Documentation/core-api/printk-formats.rst
> > +++ b/Documentation/core-api/printk-formats.rst
> > @@ -545,6 +545,18 @@ For printing netdev_features_t.
> >  
> >  Passed by reference.
> >  
> > +V4L2 and DRM FourCC code (pixel format)
> > +---------------------------------------
> > +
> > +::
> > +
> > +	%p4cc
> > +
> > +Print a FourCC code used by V4L2 or DRM. The "-BE" suffix is added on big endian
> > +formats.
> > +
> > +Passed by reference.
> 
> Maybe it's obvious to anyone in that business, but perhaps make it more
> clear the 4cc is stored in a u32 (and not, e.g., a __le32 or some other
> integer), that obviously matters when the code treats the pointer as a u32*.

The established practice is to use u32 (as this is really no hardware
involved) but I guess it'd be good to document that here, too.

> > +
> > +	put_unaligned_le32(*fourcc & ~BIT(31), s);
> > +
> > +	if (*fourcc & BIT(31))
> > +		strscpy(s + sizeof(*fourcc), FOURCC_STRING_BE,
> > +			sizeof(FOURCC_STRING_BE));
> 
> put_unaligned_le32(0x0045422d, s + 4) probably generates smaller code,
> and is more in line with building the first part of the string with
> put_unaligned_le32().

Uh. The fourcc code is made of printable characters (apart from the 31st
bit) so it can be printed, but I wouldn't use that here. "-BE" is just a
string and not related to 4ccs.

-- 
Regards,

Sakari Ailus