diff mbox series

[v5,1/1] lib/vsprintf: Add support for printing V4L2 and DRM fourccs

Message ID 20201113105441.1427-1-sakari.ailus@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series [v5,1/1] lib/vsprintf: Add support for printing V4L2 and DRM fourccs | expand

Commit Message

Sakari Ailus Nov. 13, 2020, 10:54 a.m. UTC
Add a printk modifier %p4cc (for pixel format) for printing V4L2 and DRM
pixel formats denoted by fourccs. The fourcc 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 v4:

- Use correct destination for error string (was broken in v4)

- Remove unneeded comment "/* subtract parenthesis and the space from the
  size */".

 Documentation/core-api/printk-formats.rst | 16 +++++++
 lib/test_printf.c                         | 17 ++++++++
 lib/vsprintf.c                            | 51 +++++++++++++++++++++++
 3 files changed, 84 insertions(+)

Comments

Petr Mladek Nov. 20, 2020, 3:57 p.m. UTC | #1
On Fri 2020-11-13 12:54:41, Sakari Ailus wrote:
> Add a printk modifier %p4cc (for pixel format) for printing V4L2 and DRM
> pixel formats denoted by fourccs. The fourcc 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>

The last version looks fine to me.

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr
Sergey Senozhatsky Nov. 21, 2020, 3:05 a.m. UTC | #2
On (20/11/20 16:57), Petr Mladek wrote:
> On Fri 2020-11-13 12:54:41, Sakari Ailus wrote:
> > Add a printk modifier %p4cc (for pixel format) for printing V4L2 and DRM
> > pixel formats denoted by fourccs. The fourcc 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>
> 
> The last version looks fine to me.
> 
> Reviewed-by: Petr Mladek <pmladek@suse.com>

Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>

	-ss
Petr Mladek Dec. 10, 2020, 8:34 a.m. UTC | #3
On Fri 2020-11-13 12:54:41, Sakari Ailus wrote:
> Add a printk modifier %p4cc (for pixel format) for printing V4L2 and DRM
> pixel formats denoted by fourccs. The fourcc 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>

Andy, Rasmus,

the last version looks fine to me. I am going to push it.
Please, speak up if you are against it.

Best Regards,
Petr
Andy Shevchenko Dec. 10, 2020, 1:05 p.m. UTC | #4
On Thu, Dec 10, 2020 at 2:16 PM Petr Mladek <pmladek@suse.com> wrote:
> On Fri 2020-11-13 12:54:41, Sakari Ailus wrote:
> > Add a printk modifier %p4cc (for pixel format) for printing V4L2 and DRM
> > pixel formats denoted by fourccs. The fourcc 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>
>
> Andy, Rasmus,
>
> the last version looks fine to me. I am going to push it.
> Please, speak up if you are against it.

My concerns are:
- not so standard format of representation (why not to use
string_escape_mem() helper?) or is it?
- no compatibility with generic 4cc
  (I would rather have an additional specifier here for v4l2 cases.
OTOH generic %p4cc to me sounds like an equivalent to %4pEh (but we
have similar cases with MAC where %6ph is the same as %pM).

But I'm not insisting on them, consider it like just my 2 cents to the
discussion.
Sakari Ailus Dec. 10, 2020, 1:55 p.m. UTC | #5
Hi Andy,

On Thu, Dec 10, 2020 at 03:05:02PM +0200, Andy Shevchenko wrote:
> On Thu, Dec 10, 2020 at 2:16 PM Petr Mladek <pmladek@suse.com> wrote:
> > On Fri 2020-11-13 12:54:41, Sakari Ailus wrote:
> > > Add a printk modifier %p4cc (for pixel format) for printing V4L2 and DRM
> > > pixel formats denoted by fourccs. The fourcc 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>
> >
> > Andy, Rasmus,
> >
> > the last version looks fine to me. I am going to push it.
> > Please, speak up if you are against it.
> 
> My concerns are:
> - not so standard format of representation (why not to use
> string_escape_mem() helper?) or is it?

The format string may contain spaces that are not meant to be printed.
Other unprintable chacaters should not be present (at least not in V4L2
pixelformats). The hexadecimal representation is there to convey the
numerical value and that originally came from DRM, not V4L2.

> - no compatibility with generic 4cc
>   (I would rather have an additional specifier here for v4l2 cases.

What do you mean by "generic 4cc"? There are two users of 4cc codes in the
kernel that I know of: V4L2 and DRM. Something that does not refer to
in-memory pixel formats?

> OTOH generic %p4cc to me sounds like an equivalent to %4pEh (but we
> have similar cases with MAC where %6ph is the same as %pM).
> 
> But I'm not insisting on them, consider it like just my 2 cents to the
> discussion.

Ack.
Andy Shevchenko Dec. 10, 2020, 2:08 p.m. UTC | #6
On Thu, Dec 10, 2020 at 03:55:27PM +0200, Sakari Ailus wrote:
> On Thu, Dec 10, 2020 at 03:05:02PM +0200, Andy Shevchenko wrote:
> > On Thu, Dec 10, 2020 at 2:16 PM Petr Mladek <pmladek@suse.com> wrote:
> > > On Fri 2020-11-13 12:54:41, Sakari Ailus wrote:
> > > > Add a printk modifier %p4cc (for pixel format) for printing V4L2 and DRM
> > > > pixel formats denoted by fourccs. The fourcc 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>
> > >
> > > Andy, Rasmus,
> > >
> > > the last version looks fine to me. I am going to push it.
> > > Please, speak up if you are against it.
> > 
> > My concerns are:
> > - not so standard format of representation (why not to use
> > string_escape_mem() helper?) or is it?
> 
> The format string may contain spaces that are not meant to be printed.
> Other unprintable chacaters should not be present (at least not in V4L2
> pixelformats). The hexadecimal representation is there to convey the
> numerical value and that originally came from DRM, not V4L2.

Yes, but I mean that we usually anticipate the escaped characters in a form of
'\xNN' (hex) or '\NNN' (octal). The format '(NN)' is quite unusual to me.

> > - no compatibility with generic 4cc
> >   (I would rather have an additional specifier here for v4l2 cases.
> 
> What do you mean by "generic 4cc"? There are two users of 4cc codes in the
> kernel that I know of: V4L2 and DRM. Something that does not refer to
> in-memory pixel formats?

Of course. Everything else. 4cc is a generic term to describe something which
is of 4 characters long [1]. It's not limited by media file formats. And
moreover some (chip) vendors are using it as well (Synopsys).
Microsoft uses 4cc in CSRT ACPI table for vendor field and so on...

[1]: https://en.wikipedia.org/wiki/FourCC
Petr Mladek Dec. 11, 2020, 12:20 p.m. UTC | #7
On Thu 2020-12-10 16:08:05, Andy Shevchenko wrote:
> On Thu, Dec 10, 2020 at 03:55:27PM +0200, Sakari Ailus wrote:
> > On Thu, Dec 10, 2020 at 03:05:02PM +0200, Andy Shevchenko wrote:
> > > My concerns are:
> > > - not so standard format of representation (why not to use
> > > string_escape_mem() helper?) or is it?
> > 
> > The format string may contain spaces that are not meant to be printed.
> > Other unprintable chacaters should not be present (at least not in V4L2
> > pixelformats). The hexadecimal representation is there to convey the
> > numerical value and that originally came from DRM, not V4L2.
> 
> Yes, but I mean that we usually anticipate the escaped characters in a form of
> '\xNN' (hex) or '\NNN' (octal). The format '(NN)' is quite unusual to me.

It is true that I have been a bit confused when I saw it.


> > > - no compatibility with generic 4cc
> > >   (I would rather have an additional specifier here for v4l2 cases.
> > 
> > What do you mean by "generic 4cc"? There are two users of 4cc codes in the
> > kernel that I know of: V4L2 and DRM. Something that does not refer to
> > in-memory pixel formats?
> 
> Of course. Everything else. 4cc is a generic term to describe something which
> is of 4 characters long [1]. It's not limited by media file formats. And
> moreover some (chip) vendors are using it as well (Synopsys).
> Microsoft uses 4cc in CSRT ACPI table for vendor field and so on...

Honestly, I do not even know where exactly it is going to be used.

I did not have strong opinion. So I just followed the long discussions
about previous revisions. Some people loved it from the beginning.
Some people were concerned. Anyway, there were discussed only
implementation details in the last two revisions, so I assumed that
the idea was more or less accepted.

Would it help to send another revision with some existing DRM and
V4L2 code converted to use it?

It would help me/us to see how much different it is from the current
output. Also it will require ack from the affected subsystem
maintainers and developers.

Best Regqrds,
Petr
diff mbox series

Patch

diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
index 6d26c5c6ac48..080262d2e030 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -565,6 +565,22 @@  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, including format endianness and
+its numerical value as hexadecimal.
+
+Passed by reference.
+
+Examples::
+
+	%p4cc	BG12 little-endian (0x32314742)
+
 Thanks
 ======
 
diff --git a/lib/test_printf.c b/lib/test_printf.c
index 7ac87f18a10f..7fb042542660 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -649,6 +649,22 @@  static void __init fwnode_pointer(void)
 	software_node_unregister(&softnodes[0]);
 }
 
+static void __init fourcc_pointer(void)
+{
+	struct {
+		u32 code;
+		char *str;
+	} const try[] = {
+		{ 0x20104646, "FF(10) little-endian (0x20104646)", },
+		{ 0xa0104646, "FF(10) big-endian (0xa0104646)", },
+		{ 0x10111213, "(13)(12)(11)(10) little-endian (0x10111213)", },
+	};
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(try); i++)
+		test(try[i].str, "%p4cc", &try[i].code);
+}
+
 static void __init
 errptr(void)
 {
@@ -694,6 +710,7 @@  test_pointer(void)
 	flags();
 	errptr();
 	fwnode_pointer();
+	fourcc_pointer();
 }
 
 static void __init selftest(void)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 14c9a6af1b23..b07ee5b7de06 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1733,6 +1733,54 @@  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)
+{
+	char output[sizeof("(xx)(xx)(xx)(xx) little-endian (0x01234567)")];
+	char *p = output;
+	unsigned int i;
+	u32 val;
+
+	if (fmt[1] != 'c' || fmt[2] != 'c')
+		return error_string(buf, end, "(%p4?)", spec);
+
+	if (check_pointer(&buf, end, fourcc, spec))
+		return buf;
+
+	val = *fourcc & ~BIT(31);
+
+	for (i = 0; i < sizeof(*fourcc); i++) {
+		unsigned char c = val >> (i * 8);
+
+		/* Weed out spaces */
+		if (c == ' ')
+			continue;
+
+		/* Print non-control ASCII characters as-is */
+		if (isascii(c) && isprint(c)) {
+			*p++ = c;
+			continue;
+		}
+
+		*p++ = '(';
+		p = hex_byte_pack(p, c);
+		*p++ = ')';
+	}
+
+	strcpy(p, *fourcc & BIT(31) ? " big-endian" : " little-endian");
+	p += strlen(p);
+
+	*p++ = ' ';
+	*p++ = '(';
+	p = special_hex_number(p, output + sizeof(output) - 2, *fourcc,
+			       sizeof(u32));
+	*p++ = ')';
+	*p = '\0';
+
+	return string(buf, end, output, spec);
+}
+
 static noinline_for_stack
 char *address_val(char *buf, char *end, const void *addr,
 		  struct printf_spec spec, const char *fmt)
@@ -2162,6 +2210,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 endianness and raw numerical value.
  * - 'h[CDN]' For a variable-length buffer, it prints it as a hex string with
  *            a certain separator (' ' by default):
  *              C colon
@@ -2259,6 +2308,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':