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