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

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

[Sakari Ailus]

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.

Suggested-by: Mauro Carvalho Chehab <mchehab@kernel.org>
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
since v1:

- Improve documentation (add -BE suffix, refer to "FourCC".

- Use '%p4cc' conversion specifier instead of '%ppf'.

- 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.
+
 Thanks
 ======
 
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 7c488a1ce318..93eea6a320da 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1721,6 +1721,28 @@ char *netdev_bits(char *buf, char *end, const void *addr,
 	return special_hex_number(buf, end, num, size);
 }
 
+static noinline_for_stack
+char *fourcc_string(char *buf, char *end, const u32 *fourcc,
+		    struct printf_spec spec, const char *fmt)
+{
+#define FOURCC_STRING_BE	"-BE"
+	char s[sizeof(*fourcc) + sizeof(FOURCC_STRING_BE)] = { 0 };
+
+	if (check_pointer(&buf, end, fourcc, spec))
+		return buf;
+
+	if (fmt[1] != 'c' || fmt[2] != 'c')
+		return error_string(buf, end, "(%p4?)", spec);
+
+	put_unaligned_le32(*fourcc & ~BIT(31), s);
+
+	if (*fourcc & BIT(31))
+		strscpy(s + sizeof(*fourcc), FOURCC_STRING_BE,
+			sizeof(FOURCC_STRING_BE));
+
+	return string(buf, end, s, spec);
+}
+
 static noinline_for_stack
 char *address_val(char *buf, char *end, const void *addr,
 		  struct printf_spec spec, const char *fmt)
@@ -2131,6 +2153,7 @@ char *fwnode_string(char *buf, char *end, struct fwnode_handle *fwnode,
  *       correctness of the format string and va_list arguments.
  * - 'K' For a kernel pointer that should be hidden from unprivileged users
  * - 'NF' For a netdev_features_t
+ * - '4cc' V4L2 or DRM FourCC code, with "-BE" suffix on big endian formats.
  * - 'h[CDN]' For a variable-length buffer, it prints it as a hex string with
  *            a certain separator (' ' by default):
  *              C colon
@@ -2223,6 +2246,8 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
 		return restricted_pointer(buf, end, ptr, spec);
 	case 'N':
 		return netdev_bits(buf, end, ptr, spec, fmt);
+	case '4':
+		return fourcc_string(buf, end, ptr, spec, fmt);
 	case 'a':
 		return address_val(buf, end, ptr, spec, fmt);
 	case 'd':
-- 
2.20.1

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

[Andy Shevchenko]

On Fri, Apr 03, 2020 at 12:11:56PM +0300, 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.

...

> +static noinline_for_stack
> +char *fourcc_string(char *buf, char *end, const u32 *fourcc,
> +		    struct printf_spec spec, const char *fmt)
> +{

> +#define FOURCC_STRING_BE	"-BE"
> +	char s[sizeof(*fourcc) + sizeof(FOURCC_STRING_BE)] = { 0 };

I guess it makes it too complicated.

	char s[8];

> +	if (check_pointer(&buf, end, fourcc, spec))
> +		return buf;
> +
> +	if (fmt[1] != 'c' || fmt[2] != 'c')
> +		return error_string(buf, end, "(%p4?)", spec);
> +

> +	put_unaligned_le32(*fourcc & ~BIT(31), s);

Can you elaborate what the difference in output with this bit set over cleared?
I.o.w. why don't we need to put it as BE and for LE case addd "-LE"?

> +	if (*fourcc & BIT(31))
> +		strscpy(s + sizeof(*fourcc), FOURCC_STRING_BE,
> +			sizeof(FOURCC_STRING_BE));

We know the size, and we may put '\0' as well
	if (*fourcc & BIT(31))
		strscpy(&s[4], "-BE", sizeof("-BE"));
	else
		strscpy(&s[4], "", sizeof(""));


> +	return string(buf, end, s, spec);
> +}

-- 
With Best Regards,
Andy Shevchenko

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

[Sakari Ailus]

Hi Andy,

Thanks for the comments.

On Fri, Apr 03, 2020 at 12:31:03PM +0300, Andy Shevchenko wrote:
> On Fri, Apr 03, 2020 at 12:11:56PM +0300, 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.
> 
> ...
> 
> > +static noinline_for_stack
> > +char *fourcc_string(char *buf, char *end, const u32 *fourcc,
> > +		    struct printf_spec spec, const char *fmt)
> > +{
> 
> > +#define FOURCC_STRING_BE	"-BE"
> > +	char s[sizeof(*fourcc) + sizeof(FOURCC_STRING_BE)] = { 0 };
> 
> I guess it makes it too complicated.

The above also clearly binds the size to the data that is expected to
contain there. I'd prefer keeping it as-is. And yes, 8 would be correct,
too.

> 
> 	char s[8];
> 
> > +	if (check_pointer(&buf, end, fourcc, spec))
> > +		return buf;
> > +
> > +	if (fmt[1] != 'c' || fmt[2] != 'c')
> > +		return error_string(buf, end, "(%p4?)", spec);
> > +
> 
> > +	put_unaligned_le32(*fourcc & ~BIT(31), s);
> 
> Can you elaborate what the difference in output with this bit set over cleared?
> I.o.w. why don't we need to put it as BE and for LE case addd "-LE"?

The established practice is that big endian formats have "-BE" suffix
whereas the little endian ones have nothing. (At least when it comes to
V4L2.)

> 
> > +	if (*fourcc & BIT(31))
> > +		strscpy(s + sizeof(*fourcc), FOURCC_STRING_BE,
> > +			sizeof(FOURCC_STRING_BE));
> 
> We know the size, and we may put '\0' as well
> 	if (*fourcc & BIT(31))
> 		strscpy(&s[4], "-BE", sizeof("-BE"));
> 	else
> 		strscpy(&s[4], "", sizeof(""));

The rest of the struct memory has already been set to zero in variable
declaration.

-- 
Kind regards,

Sakari Ailus

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

[Andy Shevchenko]

On Fri, Apr 03, 2020 at 12:39:16PM +0300, Sakari Ailus wrote:
> On Fri, Apr 03, 2020 at 12:31:03PM +0300, Andy Shevchenko wrote:
> > On Fri, Apr 03, 2020 at 12:11:56PM +0300, 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.
> > 
> > ...
> > 
> > > +static noinline_for_stack
> > > +char *fourcc_string(char *buf, char *end, const u32 *fourcc,
> > > +		    struct printf_spec spec, const char *fmt)
> > > +{
> > 
> > > +#define FOURCC_STRING_BE	"-BE"
> > > +	char s[sizeof(*fourcc) + sizeof(FOURCC_STRING_BE)] = { 0 };
> > 
> > I guess it makes it too complicated.
> 
> The above also clearly binds the size to the data that is expected to
> contain there. I'd prefer keeping it as-is. And yes, 8 would be correct,
> too.

OK.

> > 	char s[8];
> > 
> > > +	if (check_pointer(&buf, end, fourcc, spec))
> > > +		return buf;
> > > +
> > > +	if (fmt[1] != 'c' || fmt[2] != 'c')
> > > +		return error_string(buf, end, "(%p4?)", spec);
> > > +
> > 
> > > +	put_unaligned_le32(*fourcc & ~BIT(31), s);
> > 
> > Can you elaborate what the difference in output with this bit set over cleared?
> > I.o.w. why don't we need to put it as BE and for LE case addd "-LE"?
> 
> The established practice is that big endian formats have "-BE" suffix
> whereas the little endian ones have nothing. (At least when it comes to
> V4L2.)

What I meant by the first part of the question is ordering of the characters.
That ordering of characters is not related to that flag, correct? So, bit
actually defines the endianess of the data in the certain fourcc.

Probably you need to put a comment to explain this.

> > > +	if (*fourcc & BIT(31))
> > > +		strscpy(s + sizeof(*fourcc), FOURCC_STRING_BE,
> > > +			sizeof(FOURCC_STRING_BE));
> > 
> > We know the size, and we may put '\0' as well
> > 	if (*fourcc & BIT(31))
> > 		strscpy(&s[4], "-BE", sizeof("-BE"));
> > 	else
> > 		strscpy(&s[4], "", sizeof(""));
> 
> The rest of the struct memory has already been set to zero in variable
> declaration.

Which is bogus in my opinion. strscpy() or direct '\0' termination will put it
more explicit.

-- 
With Best Regards,
Andy Shevchenko

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

[Sakari Ailus]

Hi Andy,

On Fri, Apr 03, 2020 at 12:54:41PM +0300, Andy Shevchenko wrote:
> On Fri, Apr 03, 2020 at 12:39:16PM +0300, Sakari Ailus wrote:
> > On Fri, Apr 03, 2020 at 12:31:03PM +0300, Andy Shevchenko wrote:
> > > On Fri, Apr 03, 2020 at 12:11:56PM +0300, 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.
> > > 
> > > ...
> > > 
> > > > +static noinline_for_stack
> > > > +char *fourcc_string(char *buf, char *end, const u32 *fourcc,
> > > > +		    struct printf_spec spec, const char *fmt)
> > > > +{
> > > 
> > > > +#define FOURCC_STRING_BE	"-BE"
> > > > +	char s[sizeof(*fourcc) + sizeof(FOURCC_STRING_BE)] = { 0 };
> > > 
> > > I guess it makes it too complicated.
> > 
> > The above also clearly binds the size to the data that is expected to
> > contain there. I'd prefer keeping it as-is. And yes, 8 would be correct,
> > too.
> 
> OK.
> 
> > > 	char s[8];
> > > 
> > > > +	if (check_pointer(&buf, end, fourcc, spec))
> > > > +		return buf;
> > > > +
> > > > +	if (fmt[1] != 'c' || fmt[2] != 'c')
> > > > +		return error_string(buf, end, "(%p4?)", spec);
> > > > +
> > > 
> > > > +	put_unaligned_le32(*fourcc & ~BIT(31), s);
> > > 
> > > Can you elaborate what the difference in output with this bit set over cleared?
> > > I.o.w. why don't we need to put it as BE and for LE case addd "-LE"?
> > 
> > The established practice is that big endian formats have "-BE" suffix
> > whereas the little endian ones have nothing. (At least when it comes to
> > V4L2.)
> 
> What I meant by the first part of the question is ordering of the characters.
> That ordering of characters is not related to that flag, correct? So, bit
> actually defines the endianess of the data in the certain fourcc.
> 
> Probably you need to put a comment to explain this.

How about:

The 31st bit defines the endianness of the data, so save its printing to
the big endian suffix.

> 
> > > > +	if (*fourcc & BIT(31))
> > > > +		strscpy(s + sizeof(*fourcc), FOURCC_STRING_BE,
> > > > +			sizeof(FOURCC_STRING_BE));
> > > 
> > > We know the size, and we may put '\0' as well
> > > 	if (*fourcc & BIT(31))
> > > 		strscpy(&s[4], "-BE", sizeof("-BE"));
> > > 	else
> > > 		strscpy(&s[4], "", sizeof(""));
> > 
> > The rest of the struct memory has already been set to zero in variable
> > declaration.
> 
> Which is bogus in my opinion. strscpy() or direct '\0' termination will put it
> more explicit.

There's no need to assign nul a simple character using strscpy(). In that
case I'd just do

	s[sizeof(*fourcc)] = '\0';

and remove the initial assignment to zero.

-- 
Regards,

Sakari Ailus

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

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Fri, Apr 03, 2020 at 12:11:56PM +0300, 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.
> 
> Suggested-by: Mauro Carvalho Chehab <mchehab@kernel.org>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> since v1:
> 
> - Improve documentation (add -BE suffix, refer to "FourCC".
> 
> - Use '%p4cc' conversion specifier instead of '%ppf'.
> 
> - 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.
> +
>  Thanks
>  ======
>  
> diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> index 7c488a1ce318..93eea6a320da 100644
> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1721,6 +1721,28 @@ char *netdev_bits(char *buf, char *end, const void *addr,
>  	return special_hex_number(buf, end, num, size);
>  }
>  
> +static noinline_for_stack
> +char *fourcc_string(char *buf, char *end, const u32 *fourcc,
> +		    struct printf_spec spec, const char *fmt)
> +{
> +#define FOURCC_STRING_BE	"-BE"
> +	char s[sizeof(*fourcc) + sizeof(FOURCC_STRING_BE)] = { 0 };
> +
> +	if (check_pointer(&buf, end, fourcc, spec))
> +		return buf;
> +
> +	if (fmt[1] != 'c' || fmt[2] != 'c')
> +		return error_string(buf, end, "(%p4?)", spec);
> +
> +	put_unaligned_le32(*fourcc & ~BIT(31), s);
> +
> +	if (*fourcc & BIT(31))
> +		strscpy(s + sizeof(*fourcc), FOURCC_STRING_BE,
> +			sizeof(FOURCC_STRING_BE));
> +
> +	return string(buf, end, s, spec);

Taking V4L2_PIX_FMT_Y16_BE as an example, this will print 'Y16 -BE'
(without quotes). There are other 4CCs that contain spaces and would
suffer from a similar issue. Even in little-endian format, it would
result in additional spaces in the output string. Is this what we want ?
Should the caller always enclose the 4CC in quotes or brackets for
clarity ? Or should still be done here ?

> +}
> +
>  static noinline_for_stack
>  char *address_val(char *buf, char *end, const void *addr,
>  		  struct printf_spec spec, const char *fmt)
> @@ -2131,6 +2153,7 @@ char *fwnode_string(char *buf, char *end, struct fwnode_handle *fwnode,
>   *       correctness of the format string and va_list arguments.
>   * - 'K' For a kernel pointer that should be hidden from unprivileged users
>   * - 'NF' For a netdev_features_t
> + * - '4cc' V4L2 or DRM FourCC code, with "-BE" suffix on big endian formats.
>   * - 'h[CDN]' For a variable-length buffer, it prints it as a hex string with
>   *            a certain separator (' ' by default):
>   *              C colon
> @@ -2223,6 +2246,8 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,
>  		return restricted_pointer(buf, end, ptr, spec);
>  	case 'N':
>  		return netdev_bits(buf, end, ptr, spec, fmt);
> +	case '4':
> +		return fourcc_string(buf, end, ptr, spec, fmt);
>  	case 'a':
>  		return address_val(buf, end, ptr, spec, fmt);
>  	case 'd':

-- 
Regards,

Laurent Pinchart

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

[Sakari Ailus]

Hi Laurent,

Thanks for the comments.

On Fri, Apr 03, 2020 at 01:24:49PM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Fri, Apr 03, 2020 at 12:11:56PM +0300, 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.
> > 
> > Suggested-by: Mauro Carvalho Chehab <mchehab@kernel.org>
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> > since v1:
> > 
> > - Improve documentation (add -BE suffix, refer to "FourCC".
> > 
> > - Use '%p4cc' conversion specifier instead of '%ppf'.
> > 
> > - 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.
> > +
> >  Thanks
> >  ======
> >  
> > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> > index 7c488a1ce318..93eea6a320da 100644
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -1721,6 +1721,28 @@ char *netdev_bits(char *buf, char *end, const void *addr,
> >  	return special_hex_number(buf, end, num, size);
> >  }
> >  
> > +static noinline_for_stack
> > +char *fourcc_string(char *buf, char *end, const u32 *fourcc,
> > +		    struct printf_spec spec, const char *fmt)
> > +{
> > +#define FOURCC_STRING_BE	"-BE"
> > +	char s[sizeof(*fourcc) + sizeof(FOURCC_STRING_BE)] = { 0 };
> > +
> > +	if (check_pointer(&buf, end, fourcc, spec))
> > +		return buf;
> > +
> > +	if (fmt[1] != 'c' || fmt[2] != 'c')
> > +		return error_string(buf, end, "(%p4?)", spec);
> > +
> > +	put_unaligned_le32(*fourcc & ~BIT(31), s);
> > +
> > +	if (*fourcc & BIT(31))
> > +		strscpy(s + sizeof(*fourcc), FOURCC_STRING_BE,
> > +			sizeof(FOURCC_STRING_BE));
> > +
> > +	return string(buf, end, s, spec);
> 
> Taking V4L2_PIX_FMT_Y16_BE as an example, this will print 'Y16 -BE'
> (without quotes). There are other 4CCs that contain spaces and would
> suffer from a similar issue. Even in little-endian format, it would
> result in additional spaces in the output string. Is this what we want ?
> Should the caller always enclose the 4CC in quotes or brackets for
> clarity ? Or should still be done here ?

Good question. Space is indeed a valid character in a 4cc code.

If I omit one or more spaces, it will no longer be a 4cc, but a 3cc or even
a 2cc. Jokes aside, there are probably fair arguments both ways.

I presume there's no 4cc code where the location of a space would make a
difference but all of the spaces are trailing spaces.

It's also worth noting that the formats printed are mostly for debugging
purpose and thus even getting a hypothetical case wrong is not a grave
issue. This would also support just printing them as-is though.

I'm leaning slightly towards omitting any spaces if the code has them. This
is something that couldn't be done by using a macro...

-- 
Regards,

Sakari Ailus

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

[Mauro Carvalho Chehab]

Em Fri, 3 Apr 2020 13:47:02 +0300
Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:

> > > +static noinline_for_stack
> > > +char *fourcc_string(char *buf, char *end, const u32 *fourcc,
> > > +		    struct printf_spec spec, const char *fmt)
> > > +{
> > > +#define FOURCC_STRING_BE	"-BE"
> > > +	char s[sizeof(*fourcc) + sizeof(FOURCC_STRING_BE)] = { 0 };
> > > +
> > > +	if (check_pointer(&buf, end, fourcc, spec))
> > > +		return buf;
> > > +
> > > +	if (fmt[1] != 'c' || fmt[2] != 'c')
> > > +		return error_string(buf, end, "(%p4?)", spec);
> > > +
> > > +	put_unaligned_le32(*fourcc & ~BIT(31), s);
> > > +
> > > +	if (*fourcc & BIT(31))
> > > +		strscpy(s + sizeof(*fourcc), FOURCC_STRING_BE,
> > > +			sizeof(FOURCC_STRING_BE));
> > > +
> > > +	return string(buf, end, s, spec);  
> > 
> > Taking V4L2_PIX_FMT_Y16_BE as an example, this will print 'Y16 -BE'
> > (without quotes). There are other 4CCs that contain spaces and would
> > suffer from a similar issue. Even in little-endian format, it would
> > result in additional spaces in the output string. Is this what we want ?
> > Should the caller always enclose the 4CC in quotes or brackets for
> > clarity ? Or should still be done here ?  
> 
> Good question. Space is indeed a valid character in a 4cc code.
> 
> If I omit one or more spaces, it will no longer be a 4cc, but a 3cc or even
> a 2cc. Jokes aside, there are probably fair arguments both ways.
> 
> I presume there's no 4cc code where the location of a space would make a
> difference but all of the spaces are trailing spaces.

Yes. I guess it doesn't make any sense to allow a 4cc code with an
space before or in the middle.

Btw, on a quick search at the Internet for non-Linux definitions,
a Fourcc code "Y8  " is actually shown at the lists as just "Y8", 
e. g. removing the leading spaces:

	https://www.fourcc.org/codecs.php
	http://abcavi.kibi.ru/fourcc.php
	https://softron.zendesk.com/hc/en-us/articles/207695697-List-of-FourCC-codes-for-video-codecs
	https://www.free-codecs.com/guides/guides.php?f=fourcc

One interesting detail there is that some tables show some codes 
like "BGR(15)". While I'm not sure how this is encoded, I suspect
that the fourcc is actually "BGR\x15".

We don't do that on V4L, nor we have plans to do so. Not sure if
DRM would accept something like that. Of so, then the logic should
have some special handler if the code is below 32.	

> It's also worth noting that the formats printed are mostly for debugging
> purpose and thus even getting a hypothetical case wrong is not a grave
> issue. This would also support just printing them as-is though.
> 
> I'm leaning slightly towards omitting any spaces if the code has them. 

I would just remove trailing spaces, and then use a loop from the end
to remove trailing spaces (and optionally handle codes ending with a
value below 32, if are there any such case with DRM fourcc codes).

On the other hand, I don't mind if you prefer to use just one for()
loop and just trip any spaces inside it.

> This is something that couldn't be done by using a macro...

Well, I suspect that it might be possible to write a macro
for doing that too, for example using preprocessor concatenation
logic that could produce the same results. If you do something 
like that, however, I suspect that te macro would face some 
portability issues, as, as far as I know, not all C compilers
would handle string concatenation the same way.

Thanks,
Mauro

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

[Andy Shevchenko]

On Fri, Apr 03, 2020 at 01:19:26PM +0200, Mauro Carvalho Chehab wrote:
> Em Fri, 3 Apr 2020 13:47:02 +0300
> Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> 
> > > > +static noinline_for_stack
> > > > +char *fourcc_string(char *buf, char *end, const u32 *fourcc,
> > > > +		    struct printf_spec spec, const char *fmt)
> > > > +{
> > > > +#define FOURCC_STRING_BE	"-BE"
> > > > +	char s[sizeof(*fourcc) + sizeof(FOURCC_STRING_BE)] = { 0 };
> > > > +
> > > > +	if (check_pointer(&buf, end, fourcc, spec))
> > > > +		return buf;
> > > > +
> > > > +	if (fmt[1] != 'c' || fmt[2] != 'c')
> > > > +		return error_string(buf, end, "(%p4?)", spec);
> > > > +
> > > > +	put_unaligned_le32(*fourcc & ~BIT(31), s);
> > > > +
> > > > +	if (*fourcc & BIT(31))
> > > > +		strscpy(s + sizeof(*fourcc), FOURCC_STRING_BE,
> > > > +			sizeof(FOURCC_STRING_BE));
> > > > +
> > > > +	return string(buf, end, s, spec);  
> > > 
> > > Taking V4L2_PIX_FMT_Y16_BE as an example, this will print 'Y16 -BE'
> > > (without quotes). There are other 4CCs that contain spaces and would
> > > suffer from a similar issue. Even in little-endian format, it would
> > > result in additional spaces in the output string. Is this what we want ?
> > > Should the caller always enclose the 4CC in quotes or brackets for
> > > clarity ? Or should still be done here ?  
> > 
> > Good question. Space is indeed a valid character in a 4cc code.
> > 
> > If I omit one or more spaces, it will no longer be a 4cc, but a 3cc or even
> > a 2cc. Jokes aside, there are probably fair arguments both ways.
> > 
> > I presume there's no 4cc code where the location of a space would make a
> > difference but all of the spaces are trailing spaces.
> 
> Yes. I guess it doesn't make any sense to allow a 4cc code with an
> space before or in the middle.
> 
> Btw, on a quick search at the Internet for non-Linux definitions,
> a Fourcc code "Y8  " is actually shown at the lists as just "Y8", 
> e. g. removing the leading spaces:
> 
> 	https://www.fourcc.org/codecs.php
> 	http://abcavi.kibi.ru/fourcc.php
> 	https://softron.zendesk.com/hc/en-us/articles/207695697-List-of-FourCC-codes-for-video-codecs
> 	https://www.free-codecs.com/guides/guides.php?f=fourcc
> 
> One interesting detail there is that some tables show some codes 
> like "BGR(15)". While I'm not sure how this is encoded, I suspect
> that the fourcc is actually "BGR\x15".
> 
> We don't do that on V4L, nor we have plans to do so. Not sure if
> DRM would accept something like that. Of so, then the logic should
> have some special handler if the code is below 32.	

It is easy to achieve I think, with help of string_escape*() functions.

> > It's also worth noting that the formats printed are mostly for debugging
> > purpose and thus even getting a hypothetical case wrong is not a grave
> > issue. This would also support just printing them as-is though.
> > 
> > I'm leaning slightly towards omitting any spaces if the code has them. 
> 
> I would just remove trailing spaces, and then use a loop from the end
> to remove trailing spaces (and optionally handle codes ending with a
> value below 32, if are there any such case with DRM fourcc codes).
> 
> On the other hand, I don't mind if you prefer to use just one for()
> loop and just trip any spaces inside it.
> 
> > This is something that couldn't be done by using a macro...
> 
> Well, I suspect that it might be possible to write a macro
> for doing that too, for example using preprocessor concatenation
> logic that could produce the same results. If you do something 
> like that, however, I suspect that te macro would face some 
> portability issues, as, as far as I know, not all C compilers
> would handle string concatenation the same way.
> 
> Thanks,
> Mauro

-- 
With Best Regards,
Andy Shevchenko

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

[Rasmus Villemoes]

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

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

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.


> Suggested-by: Mauro Carvalho Chehab <mchehab@kernel.org>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> 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).

> - 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*.
> +
> +	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().

Rasmus

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

[Mauro Carvalho Chehab]

Em Fri, 3 Apr 2020 14:10:53 +0200
Rasmus Villemoes <linux@rasmusvillemoes.dk> escreveu:

> 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.

It is used on different subsystems. At least media, drm and input (yes, 
there are some input multi-touch devices with return images using
"GREY" fourcc - see drivers/input/touchscreen/sur40.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))
> 
> Or, for that matter, since it's for debugging, why not just print x with
> 0x%08x?

That's about what it has been done so far, using different solutions
on different places. Some display hex values, others display fourcc
(usually ignoring the BE case). We'd like to have a common solution
that won't be subsystem-specific and will handle it on a proper unified
way.

With regards to ex values, see for example the GREY format:

	V4L2_PIX_FMT_GREY ('GREY')

when someone reads 'GREY', this is easily understandable as a grey image
format, even by someone that it is not familiar with 4cc codes. Same is
true for several other widely used formats, like BGR and RGB.

If you see its hexa representation, 0x47524559 is a lot more obscure.

Thanks,
Mauro

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

[Joe Perches]

On Fri, 2020-04-03 at 14:10 +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))

Nothing really, it's a number of uses question.

For networking code,  print_mac was used before %pM.

After Linus floated the idea of %p<foo>, %pM was
introduced and all the DECLARE_MAC_BUF/print_mac
calls were converted.

%pM did reduce overall object size a fair amount.

How many instances of %p4cc could there be?

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

[Mauro Carvalho Chehab]

Em Fri, 03 Apr 2020 09:56:42 -0700
Joe Perches <joe@perches.com> escreveu:

> On Fri, 2020-04-03 at 14:10 +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))  
> 
> Nothing really, it's a number of uses question.
> 
> For networking code,  print_mac was used before %pM.
> 
> After Linus floated the idea of %p<foo>, %pM was
> introduced and all the DECLARE_MAC_BUF/print_mac
> calls were converted.
> 
> %pM did reduce overall object size a fair amount.
> 
> How many instances of %p4cc could there be?

That's hard to know... there are several places printing it
with different ways:

	$ git grep -i -E "(dev|pr)_(warn|dbg|info)" drivers/media|grep pixf|wc -l
	6
	$ git grep -i -E "print" drivers/media|grep pixf|wc -l
	1
	$ git grep print_fourcc|wc -l
	7
	$ git grep -i -E "(dev|pr)_(warn|dbg|info)" drivers/media|grep pixelf|wc -l
	10
	$ git grep -i -E "(dev|pr|v4l)_(warn|dbg|info)" drivers/media|grep format|wc -l
	60

I bet there are other places besides the above ones, but the thing is, as 
we currently lack a standard way, drivers still have their own ideas
about how to handle it. Each one does it differently.


Thanks,
Mauro

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

[Joe Perches]

On Fri, 2020-04-03 at 19:32 +0200, Mauro Carvalho Chehab wrote:
> Em Fri, 03 Apr 2020 09:56:42 -0700
> Joe Perches <joe@perches.com> escreveu:
[]
> > How many instances of %p4cc could there be?
> 
> That's hard to know... there are several places printing it
> with different ways:
> 
> 	$ git grep -i -E "(dev|pr)_(warn|dbg|info)" drivers/media|grep pixf|wc -l
> 	6
> 	$ git grep -i -E "print" drivers/media|grep pixf|wc -l
> 	1
> 	$ git grep print_fourcc|wc -l
> 	7
> 	$ git grep -i -E "(dev|pr)_(warn|dbg|info)" drivers/media|grep pixelf|wc -l
> 	10
> 	$ git grep -i -E "(dev|pr|v4l)_(warn|dbg|info)" drivers/media|grep format|wc -l
> 	60
> 
> I bet there are other places besides the above ones, but the thing is, as 
> we currently lack a standard way, drivers still have their own ideas
> about how to handle it. Each one does it differently.

My thought was ~100 uses was a minimum, rather like %pI6c.

That's pretty close already, so I suppose that's enough.

It _might_ be useful to use a CONFIG_MEDIA_SUPPORT guard
in lib/vsprintf for this.

cheers, Joe

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

[Andy Shevchenko]

On Fri, Apr 3, 2020 at 8:54 PM Joe Perches <joe@perches.com> wrote:
> On Fri, 2020-04-03 at 19:32 +0200, Mauro Carvalho Chehab wrote:
> > Em Fri, 03 Apr 2020 09:56:42 -0700
> > Joe Perches <joe@perches.com> escreveu:

> It _might_ be useful to use a CONFIG_MEDIA_SUPPORT guard
> in lib/vsprintf for this.

No need. FourCC, if Sakari makes it more generic, can be used for
other purposes, e.g. printing component names from the chips (not
related to media at all).

-- 
With Best Regards,
Andy Shevchenko

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

[Joe Perches]

On Fri, 2020-04-03 at 13:47 +0300, Sakari Ailus wrote:
> On Fri, Apr 03, 2020 at 01:24:49PM +0300, Laurent Pinchart wrote:
> > On Fri, Apr 03, 2020 at 12:11:56PM +0300, 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.
[]
> > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
[]
> > > +static noinline_for_stack
> > > +char *fourcc_string(char *buf, char *end, const u32 *fourcc,
> > > +		    struct printf_spec spec, const char *fmt)
[]
> > > +	if (fmt[1] != 'c' || fmt[2] != 'c')

This could check outside a format string if
the %p4 is at the end of the format string.

	printk("%p4", fourcc)

So this should verify

	if (!(fmt[1] == 'c' && fmt[2] == 'c'))

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

[Sakari Ailus]

Hi Joe,

On Fri, Apr 03, 2020 at 11:38:29AM -0700, Joe Perches wrote:
> On Fri, 2020-04-03 at 13:47 +0300, Sakari Ailus wrote:
> > On Fri, Apr 03, 2020 at 01:24:49PM +0300, Laurent Pinchart wrote:
> > > On Fri, Apr 03, 2020 at 12:11:56PM +0300, 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.
> []
> > > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c
> []
> > > > +static noinline_for_stack
> > > > +char *fourcc_string(char *buf, char *end, const u32 *fourcc,
> > > > +		    struct printf_spec spec, const char *fmt)
> []
> > > > +	if (fmt[1] != 'c' || fmt[2] != 'c')
> 
> This could check outside a format string if
> the %p4 is at the end of the format string.
> 
> 	printk("%p4", fourcc)
> 
> So this should verify
> 
> 	if (!(fmt[1] == 'c' && fmt[2] == 'c'))

How would these be different in functionality? fmt[2] won't be accessed if
fmt[1] is not 'c' (including '\0'), just like on the line above. I find the
original easier to read.

-- 
Regards,

Sakari Ailus

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

[Joe Perches]

On Sat, 2020-04-04 at 02:36 +0300, Sakari Ailus wrote:
> Hi Joe,

Hi Sakari.

> How would these be different in functionality? fmt[2] won't be accessed if
> fmt[1] is not 'c' (including '\0'), just like on the line above. I find the
> original easier to read.

Oops. You are right of course.

cheers, Joe

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

[Sakari Ailus]

Hi Andy,

On Fri, Apr 03, 2020 at 09:32:42PM +0300, Andy Shevchenko wrote:
> On Fri, Apr 3, 2020 at 8:54 PM Joe Perches <joe@perches.com> wrote:
> > On Fri, 2020-04-03 at 19:32 +0200, Mauro Carvalho Chehab wrote:
> > > Em Fri, 03 Apr 2020 09:56:42 -0700
> > > Joe Perches <joe@perches.com> escreveu:
> 
> > It _might_ be useful to use a CONFIG_MEDIA_SUPPORT guard
> > in lib/vsprintf for this.
> 
> No need. FourCC, if Sakari makes it more generic, can be used for
> other purposes, e.g. printing component names from the chips (not
> related to media at all).

Could you elaborate?

This could be already used on DRM, presumably, and that does not depend on
CONFIG_MEDIA_SUPPORT. I don't know how much there would be a need for that,
though, but this remains a possibility.

-- 
Sakari Ailus

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

[Laurent Pinchart]

Hi Sakari,

On Sat, Apr 04, 2020 at 03:14:25AM +0300, Sakari Ailus wrote:
> On Fri, Apr 03, 2020 at 09:32:42PM +0300, Andy Shevchenko wrote:
> > On Fri, Apr 3, 2020 at 8:54 PM Joe Perches <joe@perches.com> wrote:
> > > On Fri, 2020-04-03 at 19:32 +0200, Mauro Carvalho Chehab wrote:
> > > > Em Fri, 03 Apr 2020 09:56:42 -0700
> > > > Joe Perches <joe@perches.com> escreveu:
> > 
> > > It _might_ be useful to use a CONFIG_MEDIA_SUPPORT guard
> > > in lib/vsprintf for this.
> > 
> > No need. FourCC, if Sakari makes it more generic, can be used for
> > other purposes, e.g. printing component names from the chips (not
> > related to media at all).
> 
> Could you elaborate?
> 
> This could be already used on DRM, presumably, and that does not depend on
> CONFIG_MEDIA_SUPPORT. I don't know how much there would be a need for that,
> though, but this remains a possibility.

/**
 * drm_get_format_name - fill a string with a drm fourcc format's name
 * @format: format to compute name of
 * @buf: caller-supplied buffer
 */
const char *drm_get_format_name(uint32_t format, struct drm_format_name_buf *buf)
{
	snprintf(buf->str, sizeof(buf->str),
		 "%c%c%c%c %s-endian (0x%08x)",
		 printable_char(format & 0xff),
		 printable_char((format >> 8) & 0xff),
		 printable_char((format >> 16) & 0xff),
		 printable_char((format >> 24) & 0x7f),
		 format & DRM_FORMAT_BIG_ENDIAN ? "big" : "little",
		 format);

	return buf->str;
}
EXPORT_SYMBOL(drm_get_format_name);

I'm not advocating for one approach or the other in this case, but we
should standardize 4CC printing between the two subsystems.

-- 
Regards,

Laurent Pinchart

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

[Sakari Ailus]

Hi Laurent,

On Sat, Apr 04, 2020 at 03:21:47AM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Sat, Apr 04, 2020 at 03:14:25AM +0300, Sakari Ailus wrote:
> > On Fri, Apr 03, 2020 at 09:32:42PM +0300, Andy Shevchenko wrote:
> > > On Fri, Apr 3, 2020 at 8:54 PM Joe Perches <joe@perches.com> wrote:
> > > > On Fri, 2020-04-03 at 19:32 +0200, Mauro Carvalho Chehab wrote:
> > > > > Em Fri, 03 Apr 2020 09:56:42 -0700
> > > > > Joe Perches <joe@perches.com> escreveu:
> > > 
> > > > It _might_ be useful to use a CONFIG_MEDIA_SUPPORT guard
> > > > in lib/vsprintf for this.
> > > 
> > > No need. FourCC, if Sakari makes it more generic, can be used for
> > > other purposes, e.g. printing component names from the chips (not
> > > related to media at all).
> > 
> > Could you elaborate?
> > 
> > This could be already used on DRM, presumably, and that does not depend on
> > CONFIG_MEDIA_SUPPORT. I don't know how much there would be a need for that,
> > though, but this remains a possibility.
> 
> /**
>  * drm_get_format_name - fill a string with a drm fourcc format's name
>  * @format: format to compute name of
>  * @buf: caller-supplied buffer
>  */
> const char *drm_get_format_name(uint32_t format, struct drm_format_name_buf *buf)
> {
> 	snprintf(buf->str, sizeof(buf->str),
> 		 "%c%c%c%c %s-endian (0x%08x)",
> 		 printable_char(format & 0xff),
> 		 printable_char((format >> 8) & 0xff),
> 		 printable_char((format >> 16) & 0xff),
> 		 printable_char((format >> 24) & 0x7f),
> 		 format & DRM_FORMAT_BIG_ENDIAN ? "big" : "little",
> 		 format);
> 
> 	return buf->str;
> }
> EXPORT_SYMBOL(drm_get_format_name);
> 
> I'm not advocating for one approach or the other in this case, but we
> should standardize 4CC printing between the two subsystems.

IMO this format (with spaces removed from 4cc) would be fine for V4L2 as
well.

-- 
Regards,

Sakari Ailus

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@xs4all.nl/>

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@kernel.org>
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> > 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

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

[Jani Nikula]

On Mon, 06 Apr 2020, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> On Fri, Apr 03, 2020 at 02:10:53PM +0200, Rasmus Villemoes wrote:
>> 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.

Not arguing one way or another, just observing that
drm_get_format_name() abstracts that by using:

struct drm_format_name_buf {
	char str[32];
};


BR,
Jani.


-- 
Jani Nikula, Intel Open Source Graphics Center

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

[Mauro Carvalho Chehab]

Em Fri, 3 Apr 2020 21:32:42 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> escreveu:

> On Fri, Apr 3, 2020 at 8:54 PM Joe Perches <joe@perches.com> wrote:
> > On Fri, 2020-04-03 at 19:32 +0200, Mauro Carvalho Chehab wrote:  
> > > Em Fri, 03 Apr 2020 09:56:42 -0700
> > > Joe Perches <joe@perches.com> escreveu:  
> 
> > It _might_ be useful to use a CONFIG_MEDIA_SUPPORT guard
> > in lib/vsprintf for this.  
> 
> No need. FourCC, if Sakari makes it more generic, can be used for
> other purposes, e.g. printing component names from the chips (not
> related to media at all).
> 

Hmm... not 100% sure about what you're meaning with "component names".

At media, some vendors use a cc-like code to allow identifying the
name of the chip, retrieved on a common register via an I2C bus.
Omnivision uses, for example, uses a 2 bytes code:

	OV5670_CHIP_ID			0x5670
	OV5675_CHIP_ID			0x5675
	OV2680_CHIP_ID			0x2680
	OV5670_CHIP_ID			0x5670
	OV5675_CHIP_ID			0x5675

We used this at the em28xx driver to detect a camera sensor, and give
a name for the chip (see drivers/media/usb/em28xx/em28xx-camera.c):

		switch (id) {
		case 0x2642:
			name = "OV2640";
			dev->em28xx_sensor = EM28XX_OV2640;
			break;
		case 0x7648:
			name = "OV7648";
			break;
		case 0x7660:
			name = "OV7660";
			break;

Yet, this is not too reliable, as, for some products, they use something
different:

	OV8856_CHIP_ID			0x885a
	OV13858_CHIP_ID		0xd855

	OV9640 can either be 0x9648 or 0x9649, depending on its revision.

If you're referring to this kind of code, I don't think we can have
something generic.

Thanks,
Mauro

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

[Andy Shevchenko]

On Mon, Apr 6, 2020 at 10:46 AM Mauro Carvalho Chehab
<mchehab+huawei@kernel.org> wrote:
> Em Fri, 3 Apr 2020 21:32:42 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> escreveu:
> > On Fri, Apr 3, 2020 at 8:54 PM Joe Perches <joe@perches.com> wrote:
> > > On Fri, 2020-04-03 at 19:32 +0200, Mauro Carvalho Chehab wrote:
> > > > Em Fri, 03 Apr 2020 09:56:42 -0700
> > > > Joe Perches <joe@perches.com> escreveu:
> >
> > > It _might_ be useful to use a CONFIG_MEDIA_SUPPORT guard
> > > in lib/vsprintf for this.
> >
> > No need. FourCC, if Sakari makes it more generic, can be used for
> > other purposes, e.g. printing component names from the chips (not
> > related to media at all).
> >
>
> Hmm... not 100% sure about what you're meaning with "component names".

4cc is pretty much wide standard, media is just one of (famous) users of it.

As I emphasized the example I referring to has nothing to do with media.

Now, I have already two examples:
- component name inside hardware register (used by Synopsys DesignWare)
- CSRT table in ACPI uses this code for vendor ID.

-- 
With Best Regards,
Andy Shevchenko

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

[Joe Perches]

On Mon, 2020-04-06 at 13:44 +0300, Andy Shevchenko wrote:
> On Mon, Apr 6, 2020 at 10:46 AM Mauro Carvalho Chehab
> <mchehab+huawei@kernel.org> wrote:
> > Em Fri, 3 Apr 2020 21:32:42 +0300
> > Andy Shevchenko <andy.shevchenko@gmail.com> escreveu:
> > > On Fri, Apr 3, 2020 at 8:54 PM Joe Perches <joe@perches.com> wrote:
> > > > On Fri, 2020-04-03 at 19:32 +0200, Mauro Carvalho Chehab wrote:
> > > > > Em Fri, 03 Apr 2020 09:56:42 -0700
> > > > > Joe Perches <joe@perches.com> escreveu:
> > > > It _might_ be useful to use a CONFIG_MEDIA_SUPPORT guard
> > > > in lib/vsprintf for this.
> > > 
> > > No need. FourCC, if Sakari makes it more generic, can be used for
> > > other purposes, e.g. printing component names from the chips (not
> > > related to media at all).
> > > 
> > 
> > Hmm... not 100% sure about what you're meaning with "component names".
> 
> 4cc is pretty much wide standard, media is just one of (famous) users of it.
> 
> As I emphasized the example I referring to has nothing to do with media.
> 
> Now, I have already two examples:
> - component name inside hardware register (used by Synopsys DesignWare)
> - CSRT table in ACPI uses this code for vendor ID.

So if this is really u32_to_ascii, perhaps the "-BE" bit
should be separated and "%4pEp" could be used with some
renamed inline used like ERR_PTR so maybe something like
this might work?

static inline void * __must_check FOURCC(u32 val)
{
	return (void *)(unsigned long)cpu_to_be32(val);
}

void test_4cc(void)
{
	u32 val = 0x41424344;

	printk("4cc like: %4pE\n", FOURCC(val));
}