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':
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);
+}
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.
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.
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.
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);
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...
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... 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
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... 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
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'))
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.
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
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
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
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?
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
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
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).
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.
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.
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.
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
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.
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)); }
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.
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.
dri-devel@lists.freedesktop.org
-
Andy Shevchenko -
Andy Shevchenko -
Jani Nikula -
Joe Perches -
Laurent Pinchart -
Mauro Carvalho Chehab -
Mauro Carvalho Chehab -
Rasmus Villemoes -
Sakari Ailus