| Message ID | 376C9BD3-2F41-4511-BE52-1B8468FE2CB3@live.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Use proper printk format in appletbdrm | expand |
Hi Am 12.03.25 um 10:05 schrieb Aditya Garg: > From: Hector Martin <marcan@marcan.st> > > %p4cc is designed for DRM/V4L2 FourCCs with their specific quirks, but > it's useful to be able to print generic 4-character codes formatted as > an integer. Extend it to add format specifiers for printing generic > 32-bit FourCCs with various endian semantics: > > %p4ch Host byte order > %p4cn Network byte order > %p4cl Little-endian > %p4cb Big-endian That looks like someone trying to be too clever for their own good. Just my 2 cts. Best regards Thomas > > The endianness determines how bytes are interpreted as a u32, and the > FourCC is then always printed MSByte-first (this is the opposite of > V4L/DRM FourCCs). This covers most practical cases, e.g. %p4cn would > allow printing LSByte-first FourCCs stored in host endian order > (other than the hex form being in character order, not the integer > value). > > Acked-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > Signed-off-by: Hector Martin <marcan@marcan.st> > Signed-off-by: Aditya Garg <gargaditya08@live.com> > --- > Documentation/core-api/printk-formats.rst | 32 +++++++++++++++++++ > lib/test_printf.c | 39 +++++++++++++++++++---- > lib/vsprintf.c | 35 ++++++++++++++++---- > scripts/checkpatch.pl | 2 +- > 4 files changed, 94 insertions(+), 14 deletions(-) > > diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst > index ecccc0473..bd420e8aa 100644 > --- a/Documentation/core-api/printk-formats.rst > +++ b/Documentation/core-api/printk-formats.rst > @@ -648,6 +648,38 @@ Examples:: > %p4cc Y10 little-endian (0x20303159) > %p4cc NV12 big-endian (0xb231564e) > > +Generic FourCC code > +------------------- > + > +:: > + %p4c[hnlb] gP00 (0x67503030) > + > +Print a generic FourCC code, as both ASCII characters and its numerical > +value as hexadecimal. > + > +The generic FourCC code is always printed in the big-endian format, > +the most significant byte first. This is the opposite of V4L/DRM FourCCs. > + > +The additional ``h``, ``n``, ``l``, and ``b`` specifiers define what > +endianness is used to load the stored bytes. The data might be interpreted > +using the host byte order, network byte order, little-endian, or big-endian. > + > +Passed by reference. > + > +Examples for a little-endian machine, given &(u32)0x67503030:: > + > + %p4ch gP00 (0x67503030) > + %p4cn 00Pg (0x30305067) > + %p4cl gP00 (0x67503030) > + %p4cb 00Pg (0x30305067) > + > +Examples for a big-endian machine, given &(u32)0x67503030:: > + > + %p4ch gP00 (0x67503030) > + %p4cn 00Pg (0x30305067) > + %p4cl 00Pg (0x30305067) > + %p4cb gP00 (0x67503030) > + > Rust > ---- > > diff --git a/lib/test_printf.c b/lib/test_printf.c > index 59dbe4f9a..b9e8afc01 100644 > --- a/lib/test_printf.c > +++ b/lib/test_printf.c > @@ -776,21 +776,46 @@ static void __init fwnode_pointer(void) > software_node_unregister_node_group(group); > } > > +struct fourcc_struct { > + u32 code; > + const char *str; > +}; > + > +static void __init fourcc_pointer_test(const struct fourcc_struct *fc, size_t n, > + const char *fmt) > +{ > + size_t i; > + > + for (i = 0; i < n; i++) > + test(fc[i].str, fmt, &fc[i].code); > +} > + > static void __init fourcc_pointer(void) > { > - struct { > - u32 code; > - char *str; > - } const try[] = { > + static const struct fourcc_struct try_cc[] = { > { 0x3231564e, "NV12 little-endian (0x3231564e)", }, > { 0xb231564e, "NV12 big-endian (0xb231564e)", }, > { 0x10111213, ".... little-endian (0x10111213)", }, > { 0x20303159, "Y10 little-endian (0x20303159)", }, > }; > - unsigned int i; > + static const struct fourcc_struct try_ch[] = { > + { 0x41424344, "ABCD (0x41424344)", }, > + }; > + static const struct fourcc_struct try_cn[] = { > + { 0x41424344, "DCBA (0x44434241)", }, > + }; > + static const struct fourcc_struct try_cl[] = { > + { (__force u32)cpu_to_le32(0x41424344), "ABCD (0x41424344)", }, > + }; > + static const struct fourcc_struct try_cb[] = { > + { (__force u32)cpu_to_be32(0x41424344), "ABCD (0x41424344)", }, > + }; > > - for (i = 0; i < ARRAY_SIZE(try); i++) > - test(try[i].str, "%p4cc", &try[i].code); > + fourcc_pointer_test(try_cc, ARRAY_SIZE(try_cc), "%p4cc"); > + fourcc_pointer_test(try_ch, ARRAY_SIZE(try_ch), "%p4ch"); > + fourcc_pointer_test(try_cn, ARRAY_SIZE(try_cn), "%p4cn"); > + fourcc_pointer_test(try_cl, ARRAY_SIZE(try_cl), "%p4cl"); > + fourcc_pointer_test(try_cb, ARRAY_SIZE(try_cb), "%p4cb"); > } > > static void __init > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > index 56fe96319..56511a994 100644 > --- a/lib/vsprintf.c > +++ b/lib/vsprintf.c > @@ -1781,27 +1781,50 @@ char *fourcc_string(char *buf, char *end, const u32 *fourcc, > char output[sizeof("0123 little-endian (0x01234567)")]; > char *p = output; > unsigned int i; > + bool pixel_fmt = false; > u32 orig, val; > > - if (fmt[1] != 'c' || fmt[2] != 'c') > + if (fmt[1] != 'c') > return error_string(buf, end, "(%p4?)", spec); > > if (check_pointer(&buf, end, fourcc, spec)) > return buf; > > orig = get_unaligned(fourcc); > - val = orig & ~BIT(31); > + switch (fmt[2]) { > + case 'h': > + break; > + case 'n': > + orig = swab32(orig); > + break; > + case 'l': > + orig = (__force u32)cpu_to_le32(orig); > + break; > + case 'b': > + orig = (__force u32)cpu_to_be32(orig); > + break; > + case 'c': > + /* Pixel formats are printed LSB-first */ > + pixel_fmt = true; > + break; > + default: > + return error_string(buf, end, "(%p4?)", spec); > + } > + > + val = pixel_fmt ? swab32(orig & ~BIT(31)) : orig; > > for (i = 0; i < sizeof(u32); i++) { > - unsigned char c = val >> (i * 8); > + unsigned char c = val >> ((3 - i) * 8); > > /* Print non-control ASCII characters as-is, dot otherwise */ > *p++ = isascii(c) && isprint(c) ? c : '.'; > } > > - *p++ = ' '; > - strcpy(p, orig & BIT(31) ? "big-endian" : "little-endian"); > - p += strlen(p); > + if (pixel_fmt) { > + *p++ = ' '; > + strcpy(p, orig & BIT(31) ? "big-endian" : "little-endian"); > + p += strlen(p); > + } > > *p++ = ' '; > *p++ = '('; > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > index 7b28ad331..5595a0898 100755 > --- a/scripts/checkpatch.pl > +++ b/scripts/checkpatch.pl > @@ -6904,7 +6904,7 @@ sub process { > ($extension eq "f" && > defined $qualifier && $qualifier !~ /^w/) || > ($extension eq "4" && > - defined $qualifier && $qualifier !~ /^cc/)) { > + defined $qualifier && $qualifier !~ /^c[hnlbc]/)) { > $bad_specifier = $specifier; > last; > }
> On 12 Mar 2025, at 5:16 PM, Thomas Zimmermann <tzimmermann@suse.de> wrote: > > Hi > >> Am 12.03.25 um 10:05 schrieb Aditya Garg: >> From: Hector Martin <marcan@marcan.st> >> >> %p4cc is designed for DRM/V4L2 FourCCs with their specific quirks, but >> it's useful to be able to print generic 4-character codes formatted as >> an integer. Extend it to add format specifiers for printing generic >> 32-bit FourCCs with various endian semantics: >> >> %p4ch Host byte order >> %p4cn Network byte order >> %p4cl Little-endian >> %p4cb Big-endian > > That looks like someone trying to be too clever for their own good. Just my 2 cts. I don't understand what you are trying to say. Anyways, I thought it's obvious, but Petr's Ack is still left and thus cannot be merged into DRM for now unless he says so in this thread. > > Best regards > Thomas > >> >> The endianness determines how bytes are interpreted as a u32, and the >> FourCC is then always printed MSByte-first (this is the opposite of >> V4L/DRM FourCCs). This covers most practical cases, e.g. %p4cn would >> allow printing LSByte-first FourCCs stored in host endian order >> (other than the hex form being in character order, not the integer >> value). >> >> Acked-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> >> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >> Signed-off-by: Hector Martin <marcan@marcan.st> >> Signed-off-by: Aditya Garg <gargaditya08@live.com> >> --- >> Documentation/core-api/printk-formats.rst | 32 +++++++++++++++++++ >> lib/test_printf.c | 39 +++++++++++++++++++---- >> lib/vsprintf.c | 35 ++++++++++++++++---- >> scripts/checkpatch.pl | 2 +- >> 4 files changed, 94 insertions(+), 14 deletions(-) >> >> diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst >> index ecccc0473..bd420e8aa 100644 >> --- a/Documentation/core-api/printk-formats.rst >> +++ b/Documentation/core-api/printk-formats.rst >> @@ -648,6 +648,38 @@ Examples:: >> %p4cc Y10 little-endian (0x20303159) >> %p4cc NV12 big-endian (0xb231564e) >> +Generic FourCC code >> +------------------- >> + >> +:: >> + %p4c[hnlb] gP00 (0x67503030) >> + >> +Print a generic FourCC code, as both ASCII characters and its numerical >> +value as hexadecimal. >> + >> +The generic FourCC code is always printed in the big-endian format, >> +the most significant byte first. This is the opposite of V4L/DRM FourCCs. >> + >> +The additional ``h``, ``n``, ``l``, and ``b`` specifiers define what >> +endianness is used to load the stored bytes. The data might be interpreted >> +using the host byte order, network byte order, little-endian, or big-endian. >> + >> +Passed by reference. >> + >> +Examples for a little-endian machine, given &(u32)0x67503030:: >> + >> + %p4ch gP00 (0x67503030) >> + %p4cn 00Pg (0x30305067) >> + %p4cl gP00 (0x67503030) >> + %p4cb 00Pg (0x30305067) >> + >> +Examples for a big-endian machine, given &(u32)0x67503030:: >> + >> + %p4ch gP00 (0x67503030) >> + %p4cn 00Pg (0x30305067) >> + %p4cl 00Pg (0x30305067) >> + %p4cb gP00 (0x67503030) >> + >> Rust >> ---- >> diff --git a/lib/test_printf.c b/lib/test_printf.c >> index 59dbe4f9a..b9e8afc01 100644 >> --- a/lib/test_printf.c >> +++ b/lib/test_printf.c >> @@ -776,21 +776,46 @@ static void __init fwnode_pointer(void) >> software_node_unregister_node_group(group); >> } >> +struct fourcc_struct { >> + u32 code; >> + const char *str; >> +}; >> + >> +static void __init fourcc_pointer_test(const struct fourcc_struct *fc, size_t n, >> + const char *fmt) >> +{ >> + size_t i; >> + >> + for (i = 0; i < n; i++) >> + test(fc[i].str, fmt, &fc[i].code); >> +} >> + >> static void __init fourcc_pointer(void) >> { >> - struct { >> - u32 code; >> - char *str; >> - } const try[] = { >> + static const struct fourcc_struct try_cc[] = { >> { 0x3231564e, "NV12 little-endian (0x3231564e)", }, >> { 0xb231564e, "NV12 big-endian (0xb231564e)", }, >> { 0x10111213, ".... little-endian (0x10111213)", }, >> { 0x20303159, "Y10 little-endian (0x20303159)", }, >> }; >> - unsigned int i; >> + static const struct fourcc_struct try_ch[] = { >> + { 0x41424344, "ABCD (0x41424344)", }, >> + }; >> + static const struct fourcc_struct try_cn[] = { >> + { 0x41424344, "DCBA (0x44434241)", }, >> + }; >> + static const struct fourcc_struct try_cl[] = { >> + { (__force u32)cpu_to_le32(0x41424344), "ABCD (0x41424344)", }, >> + }; >> + static const struct fourcc_struct try_cb[] = { >> + { (__force u32)cpu_to_be32(0x41424344), "ABCD (0x41424344)", }, >> + }; >> - for (i = 0; i < ARRAY_SIZE(try); i++) >> - test(try[i].str, "%p4cc", &try[i].code); >> + fourcc_pointer_test(try_cc, ARRAY_SIZE(try_cc), "%p4cc"); >> + fourcc_pointer_test(try_ch, ARRAY_SIZE(try_ch), "%p4ch"); >> + fourcc_pointer_test(try_cn, ARRAY_SIZE(try_cn), "%p4cn"); >> + fourcc_pointer_test(try_cl, ARRAY_SIZE(try_cl), "%p4cl"); >> + fourcc_pointer_test(try_cb, ARRAY_SIZE(try_cb), "%p4cb"); >> } >> static void __init >> diff --git a/lib/vsprintf.c b/lib/vsprintf.c >> index 56fe96319..56511a994 100644 >> --- a/lib/vsprintf.c >> +++ b/lib/vsprintf.c >> @@ -1781,27 +1781,50 @@ char *fourcc_string(char *buf, char *end, const u32 *fourcc, >> char output[sizeof("0123 little-endian (0x01234567)")]; >> char *p = output; >> unsigned int i; >> + bool pixel_fmt = false; >> u32 orig, val; >> - if (fmt[1] != 'c' || fmt[2] != 'c') >> + if (fmt[1] != 'c') >> return error_string(buf, end, "(%p4?)", spec); >> if (check_pointer(&buf, end, fourcc, spec)) >> return buf; >> orig = get_unaligned(fourcc); >> - val = orig & ~BIT(31); >> + switch (fmt[2]) { >> + case 'h': >> + break; >> + case 'n': >> + orig = swab32(orig); >> + break; >> + case 'l': >> + orig = (__force u32)cpu_to_le32(orig); >> + break; >> + case 'b': >> + orig = (__force u32)cpu_to_be32(orig); >> + break; >> + case 'c': >> + /* Pixel formats are printed LSB-first */ >> + pixel_fmt = true; >> + break; >> + default: >> + return error_string(buf, end, "(%p4?)", spec); >> + } >> + >> + val = pixel_fmt ? swab32(orig & ~BIT(31)) : orig; >> for (i = 0; i < sizeof(u32); i++) { >> - unsigned char c = val >> (i * 8); >> + unsigned char c = val >> ((3 - i) * 8); >> /* Print non-control ASCII characters as-is, dot otherwise */ >> *p++ = isascii(c) && isprint(c) ? c : '.'; >> } >> - *p++ = ' '; >> - strcpy(p, orig & BIT(31) ? "big-endian" : "little-endian"); >> - p += strlen(p); >> + if (pixel_fmt) { >> + *p++ = ' '; >> + strcpy(p, orig & BIT(31) ? "big-endian" : "little-endian"); >> + p += strlen(p); >> + } >> *p++ = ' '; >> *p++ = '('; >> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl >> index 7b28ad331..5595a0898 100755 >> --- a/scripts/checkpatch.pl >> +++ b/scripts/checkpatch.pl >> @@ -6904,7 +6904,7 @@ sub process { >> ($extension eq "f" && >> defined $qualifier && $qualifier !~ /^w/) || >> ($extension eq "4" && >> - defined $qualifier && $qualifier !~ /^cc/)) { >> + defined $qualifier && $qualifier !~ /^c[hnlbc]/)) { >> $bad_specifier = $specifier; >> last; >> } > > -- > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Frankenstrasse 146, 90461 Nuernberg, Germany > GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman > HRB 36809 (AG Nuernberg) >
Hi Am 12.03.25 um 12:49 schrieb Aditya Garg: > >> On 12 Mar 2025, at 5:16 PM, Thomas Zimmermann <tzimmermann@suse.de> wrote: >> >> Hi >> >>> Am 12.03.25 um 10:05 schrieb Aditya Garg: >>> From: Hector Martin <marcan@marcan.st> >>> >>> %p4cc is designed for DRM/V4L2 FourCCs with their specific quirks, but >>> it's useful to be able to print generic 4-character codes formatted as >>> an integer. Extend it to add format specifiers for printing generic >>> 32-bit FourCCs with various endian semantics: >>> >>> %p4ch Host byte order >>> %p4cn Network byte order >>> %p4cl Little-endian >>> %p4cb Big-endian >> That looks like someone trying to be too clever for their own good. Just my 2 cts. > I don't understand what you are trying to say. Anyways, I thought it's obvious, but Petr's Ack is still left and thus cannot be merged into DRM for now unless he says so in this thread. I'm trying to say that the author of this patch found the %p4cc functionality and over-generalized the feature. Source code should express the idea of what it's doing in clear terms. %p4ch somehow doesn't do that for me. Printing 4 bytes in various orders without context seems arbitrary and confusing. (I don't really have a say here. I'm just asking to reconsider this change.) Best regards Thomas >> Best regards >> Thomas >> >>> The endianness determines how bytes are interpreted as a u32, and the >>> FourCC is then always printed MSByte-first (this is the opposite of >>> V4L/DRM FourCCs). This covers most practical cases, e.g. %p4cn would >>> allow printing LSByte-first FourCCs stored in host endian order >>> (other than the hex form being in character order, not the integer >>> value). >>> >>> Acked-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> >>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >>> Signed-off-by: Hector Martin <marcan@marcan.st> >>> Signed-off-by: Aditya Garg <gargaditya08@live.com> >>> --- >>> Documentation/core-api/printk-formats.rst | 32 +++++++++++++++++++ >>> lib/test_printf.c | 39 +++++++++++++++++++---- >>> lib/vsprintf.c | 35 ++++++++++++++++---- >>> scripts/checkpatch.pl | 2 +- >>> 4 files changed, 94 insertions(+), 14 deletions(-) >>> >>> diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst >>> index ecccc0473..bd420e8aa 100644 >>> --- a/Documentation/core-api/printk-formats.rst >>> +++ b/Documentation/core-api/printk-formats.rst >>> @@ -648,6 +648,38 @@ Examples:: >>> %p4cc Y10 little-endian (0x20303159) >>> %p4cc NV12 big-endian (0xb231564e) >>> +Generic FourCC code >>> +------------------- >>> + >>> +:: >>> + %p4c[hnlb] gP00 (0x67503030) >>> + >>> +Print a generic FourCC code, as both ASCII characters and its numerical >>> +value as hexadecimal. >>> + >>> +The generic FourCC code is always printed in the big-endian format, >>> +the most significant byte first. This is the opposite of V4L/DRM FourCCs. >>> + >>> +The additional ``h``, ``n``, ``l``, and ``b`` specifiers define what >>> +endianness is used to load the stored bytes. The data might be interpreted >>> +using the host byte order, network byte order, little-endian, or big-endian. >>> + >>> +Passed by reference. >>> + >>> +Examples for a little-endian machine, given &(u32)0x67503030:: >>> + >>> + %p4ch gP00 (0x67503030) >>> + %p4cn 00Pg (0x30305067) >>> + %p4cl gP00 (0x67503030) >>> + %p4cb 00Pg (0x30305067) >>> + >>> +Examples for a big-endian machine, given &(u32)0x67503030:: >>> + >>> + %p4ch gP00 (0x67503030) >>> + %p4cn 00Pg (0x30305067) >>> + %p4cl 00Pg (0x30305067) >>> + %p4cb gP00 (0x67503030) >>> + >>> Rust >>> ---- >>> diff --git a/lib/test_printf.c b/lib/test_printf.c >>> index 59dbe4f9a..b9e8afc01 100644 >>> --- a/lib/test_printf.c >>> +++ b/lib/test_printf.c >>> @@ -776,21 +776,46 @@ static void __init fwnode_pointer(void) >>> software_node_unregister_node_group(group); >>> } >>> +struct fourcc_struct { >>> + u32 code; >>> + const char *str; >>> +}; >>> + >>> +static void __init fourcc_pointer_test(const struct fourcc_struct *fc, size_t n, >>> + const char *fmt) >>> +{ >>> + size_t i; >>> + >>> + for (i = 0; i < n; i++) >>> + test(fc[i].str, fmt, &fc[i].code); >>> +} >>> + >>> static void __init fourcc_pointer(void) >>> { >>> - struct { >>> - u32 code; >>> - char *str; >>> - } const try[] = { >>> + static const struct fourcc_struct try_cc[] = { >>> { 0x3231564e, "NV12 little-endian (0x3231564e)", }, >>> { 0xb231564e, "NV12 big-endian (0xb231564e)", }, >>> { 0x10111213, ".... little-endian (0x10111213)", }, >>> { 0x20303159, "Y10 little-endian (0x20303159)", }, >>> }; >>> - unsigned int i; >>> + static const struct fourcc_struct try_ch[] = { >>> + { 0x41424344, "ABCD (0x41424344)", }, >>> + }; >>> + static const struct fourcc_struct try_cn[] = { >>> + { 0x41424344, "DCBA (0x44434241)", }, >>> + }; >>> + static const struct fourcc_struct try_cl[] = { >>> + { (__force u32)cpu_to_le32(0x41424344), "ABCD (0x41424344)", }, >>> + }; >>> + static const struct fourcc_struct try_cb[] = { >>> + { (__force u32)cpu_to_be32(0x41424344), "ABCD (0x41424344)", }, >>> + }; >>> - for (i = 0; i < ARRAY_SIZE(try); i++) >>> - test(try[i].str, "%p4cc", &try[i].code); >>> + fourcc_pointer_test(try_cc, ARRAY_SIZE(try_cc), "%p4cc"); >>> + fourcc_pointer_test(try_ch, ARRAY_SIZE(try_ch), "%p4ch"); >>> + fourcc_pointer_test(try_cn, ARRAY_SIZE(try_cn), "%p4cn"); >>> + fourcc_pointer_test(try_cl, ARRAY_SIZE(try_cl), "%p4cl"); >>> + fourcc_pointer_test(try_cb, ARRAY_SIZE(try_cb), "%p4cb"); >>> } >>> static void __init >>> diff --git a/lib/vsprintf.c b/lib/vsprintf.c >>> index 56fe96319..56511a994 100644 >>> --- a/lib/vsprintf.c >>> +++ b/lib/vsprintf.c >>> @@ -1781,27 +1781,50 @@ char *fourcc_string(char *buf, char *end, const u32 *fourcc, >>> char output[sizeof("0123 little-endian (0x01234567)")]; >>> char *p = output; >>> unsigned int i; >>> + bool pixel_fmt = false; >>> u32 orig, val; >>> - if (fmt[1] != 'c' || fmt[2] != 'c') >>> + if (fmt[1] != 'c') >>> return error_string(buf, end, "(%p4?)", spec); >>> if (check_pointer(&buf, end, fourcc, spec)) >>> return buf; >>> orig = get_unaligned(fourcc); >>> - val = orig & ~BIT(31); >>> + switch (fmt[2]) { >>> + case 'h': >>> + break; >>> + case 'n': >>> + orig = swab32(orig); >>> + break; >>> + case 'l': >>> + orig = (__force u32)cpu_to_le32(orig); >>> + break; >>> + case 'b': >>> + orig = (__force u32)cpu_to_be32(orig); >>> + break; >>> + case 'c': >>> + /* Pixel formats are printed LSB-first */ >>> + pixel_fmt = true; >>> + break; >>> + default: >>> + return error_string(buf, end, "(%p4?)", spec); >>> + } >>> + >>> + val = pixel_fmt ? swab32(orig & ~BIT(31)) : orig; >>> for (i = 0; i < sizeof(u32); i++) { >>> - unsigned char c = val >> (i * 8); >>> + unsigned char c = val >> ((3 - i) * 8); >>> /* Print non-control ASCII characters as-is, dot otherwise */ >>> *p++ = isascii(c) && isprint(c) ? c : '.'; >>> } >>> - *p++ = ' '; >>> - strcpy(p, orig & BIT(31) ? "big-endian" : "little-endian"); >>> - p += strlen(p); >>> + if (pixel_fmt) { >>> + *p++ = ' '; >>> + strcpy(p, orig & BIT(31) ? "big-endian" : "little-endian"); >>> + p += strlen(p); >>> + } >>> *p++ = ' '; >>> *p++ = '('; >>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl >>> index 7b28ad331..5595a0898 100755 >>> --- a/scripts/checkpatch.pl >>> +++ b/scripts/checkpatch.pl >>> @@ -6904,7 +6904,7 @@ sub process { >>> ($extension eq "f" && >>> defined $qualifier && $qualifier !~ /^w/) || >>> ($extension eq "4" && >>> - defined $qualifier && $qualifier !~ /^cc/)) { >>> + defined $qualifier && $qualifier !~ /^c[hnlbc]/)) { >>> $bad_specifier = $specifier; >>> last; >>> } >> -- >> -- >> Thomas Zimmermann >> Graphics Driver Developer >> SUSE Software Solutions Germany GmbH >> Frankenstrasse 146, 90461 Nuernberg, Germany >> GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman >> HRB 36809 (AG Nuernberg) >>
> On 12 Mar 2025, at 5:29 PM, Thomas Zimmermann <tzimmermann@suse.de> wrote: > > Hi > >> Am 12.03.25 um 12:49 schrieb Aditya Garg: >> >>>> On 12 Mar 2025, at 5:16 PM, Thomas Zimmermann <tzimmermann@suse.de> wrote: >>> >>> Hi >>> >>>> Am 12.03.25 um 10:05 schrieb Aditya Garg: >>>> From: Hector Martin <marcan@marcan.st> >>>> >>>> %p4cc is designed for DRM/V4L2 FourCCs with their specific quirks, but >>>> it's useful to be able to print generic 4-character codes formatted as >>>> an integer. Extend it to add format specifiers for printing generic >>>> 32-bit FourCCs with various endian semantics: >>>> >>>> %p4ch Host byte order >>>> %p4cn Network byte order >>>> %p4cl Little-endian >>>> %p4cb Big-endian >>> That looks like someone trying to be too clever for their own good. Just my 2 cts. >> I don't understand what you are trying to say. Anyways, I thought it's obvious, but Petr's Ack is still left and thus cannot be merged into DRM for now unless he says so in this thread. > > I'm trying to say that the author of this patch found the %p4cc functionality and over-generalized the feature. Source code should express the idea of what it's doing in clear terms. %p4ch somehow doesn't do that for me. Printing 4 bytes in various orders without context seems arbitrary and confusing. > > (I don't really have a say here. I'm just asking to reconsider this change.) Ah I see. I'll checkout the macros you sent. The Asahi Linux SMC drivers would need these as well, so I'll probably first wait for the vsprintf maintainers and also Asahi Linux maintainers for their views. > > Best regards > Thomas > >>> Best regards >>> Thomas >>> >>>> The endianness determines how bytes are interpreted as a u32, and the >>>> FourCC is then always printed MSByte-first (this is the opposite of >>>> V4L/DRM FourCCs). This covers most practical cases, e.g. %p4cn would >>>> allow printing LSByte-first FourCCs stored in host endian order >>>> (other than the hex form being in character order, not the integer >>>> value). >>>> >>>> Acked-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> >>>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >>>> Signed-off-by: Hector Martin <marcan@marcan.st> >>>> Signed-off-by: Aditya Garg <gargaditya08@live.com> >>>> --- >>>> Documentation/core-api/printk-formats.rst | 32 +++++++++++++++++++ >>>> lib/test_printf.c | 39 +++++++++++++++++++---- >>>> lib/vsprintf.c | 35 ++++++++++++++++---- >>>> scripts/checkpatch.pl | 2 +- >>>> 4 files changed, 94 insertions(+), 14 deletions(-) >>>> >>>> diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst >>>> index ecccc0473..bd420e8aa 100644 >>>> --- a/Documentation/core-api/printk-formats.rst >>>> +++ b/Documentation/core-api/printk-formats.rst >>>> @@ -648,6 +648,38 @@ Examples:: >>>> %p4cc Y10 little-endian (0x20303159) >>>> %p4cc NV12 big-endian (0xb231564e) >>>> +Generic FourCC code >>>> +------------------- >>>> + >>>> +:: >>>> + %p4c[hnlb] gP00 (0x67503030) >>>> + >>>> +Print a generic FourCC code, as both ASCII characters and its numerical >>>> +value as hexadecimal. >>>> + >>>> +The generic FourCC code is always printed in the big-endian format, >>>> +the most significant byte first. This is the opposite of V4L/DRM FourCCs. >>>> + >>>> +The additional ``h``, ``n``, ``l``, and ``b`` specifiers define what >>>> +endianness is used to load the stored bytes. The data might be interpreted >>>> +using the host byte order, network byte order, little-endian, or big-endian. >>>> + >>>> +Passed by reference. >>>> + >>>> +Examples for a little-endian machine, given &(u32)0x67503030:: >>>> + >>>> + %p4ch gP00 (0x67503030) >>>> + %p4cn 00Pg (0x30305067) >>>> + %p4cl gP00 (0x67503030) >>>> + %p4cb 00Pg (0x30305067) >>>> + >>>> +Examples for a big-endian machine, given &(u32)0x67503030:: >>>> + >>>> + %p4ch gP00 (0x67503030) >>>> + %p4cn 00Pg (0x30305067) >>>> + %p4cl 00Pg (0x30305067) >>>> + %p4cb gP00 (0x67503030) >>>> + >>>> Rust >>>> ---- >>>> diff --git a/lib/test_printf.c b/lib/test_printf.c >>>> index 59dbe4f9a..b9e8afc01 100644 >>>> --- a/lib/test_printf.c >>>> +++ b/lib/test_printf.c >>>> @@ -776,21 +776,46 @@ static void __init fwnode_pointer(void) >>>> software_node_unregister_node_group(group); >>>> } >>>> +struct fourcc_struct { >>>> + u32 code; >>>> + const char *str; >>>> +}; >>>> + >>>> +static void __init fourcc_pointer_test(const struct fourcc_struct *fc, size_t n, >>>> + const char *fmt) >>>> +{ >>>> + size_t i; >>>> + >>>> + for (i = 0; i < n; i++) >>>> + test(fc[i].str, fmt, &fc[i].code); >>>> +} >>>> + >>>> static void __init fourcc_pointer(void) >>>> { >>>> - struct { >>>> - u32 code; >>>> - char *str; >>>> - } const try[] = { >>>> + static const struct fourcc_struct try_cc[] = { >>>> { 0x3231564e, "NV12 little-endian (0x3231564e)", }, >>>> { 0xb231564e, "NV12 big-endian (0xb231564e)", }, >>>> { 0x10111213, ".... little-endian (0x10111213)", }, >>>> { 0x20303159, "Y10 little-endian (0x20303159)", }, >>>> }; >>>> - unsigned int i; >>>> + static const struct fourcc_struct try_ch[] = { >>>> + { 0x41424344, "ABCD (0x41424344)", }, >>>> + }; >>>> + static const struct fourcc_struct try_cn[] = { >>>> + { 0x41424344, "DCBA (0x44434241)", }, >>>> + }; >>>> + static const struct fourcc_struct try_cl[] = { >>>> + { (__force u32)cpu_to_le32(0x41424344), "ABCD (0x41424344)", }, >>>> + }; >>>> + static const struct fourcc_struct try_cb[] = { >>>> + { (__force u32)cpu_to_be32(0x41424344), "ABCD (0x41424344)", }, >>>> + }; >>>> - for (i = 0; i < ARRAY_SIZE(try); i++) >>>> - test(try[i].str, "%p4cc", &try[i].code); >>>> + fourcc_pointer_test(try_cc, ARRAY_SIZE(try_cc), "%p4cc"); >>>> + fourcc_pointer_test(try_ch, ARRAY_SIZE(try_ch), "%p4ch"); >>>> + fourcc_pointer_test(try_cn, ARRAY_SIZE(try_cn), "%p4cn"); >>>> + fourcc_pointer_test(try_cl, ARRAY_SIZE(try_cl), "%p4cl"); >>>> + fourcc_pointer_test(try_cb, ARRAY_SIZE(try_cb), "%p4cb"); >>>> } >>>> static void __init >>>> diff --git a/lib/vsprintf.c b/lib/vsprintf.c >>>> index 56fe96319..56511a994 100644 >>>> --- a/lib/vsprintf.c >>>> +++ b/lib/vsprintf.c >>>> @@ -1781,27 +1781,50 @@ char *fourcc_string(char *buf, char *end, const u32 *fourcc, >>>> char output[sizeof("0123 little-endian (0x01234567)")]; >>>> char *p = output; >>>> unsigned int i; >>>> + bool pixel_fmt = false; >>>> u32 orig, val; >>>> - if (fmt[1] != 'c' || fmt[2] != 'c') >>>> + if (fmt[1] != 'c') >>>> return error_string(buf, end, "(%p4?)", spec); >>>> if (check_pointer(&buf, end, fourcc, spec)) >>>> return buf; >>>> orig = get_unaligned(fourcc); >>>> - val = orig & ~BIT(31); >>>> + switch (fmt[2]) { >>>> + case 'h': >>>> + break; >>>> + case 'n': >>>> + orig = swab32(orig); >>>> + break; >>>> + case 'l': >>>> + orig = (__force u32)cpu_to_le32(orig); >>>> + break; >>>> + case 'b': >>>> + orig = (__force u32)cpu_to_be32(orig); >>>> + break; >>>> + case 'c': >>>> + /* Pixel formats are printed LSB-first */ >>>> + pixel_fmt = true; >>>> + break; >>>> + default: >>>> + return error_string(buf, end, "(%p4?)", spec); >>>> + } >>>> + >>>> + val = pixel_fmt ? swab32(orig & ~BIT(31)) : orig; >>>> for (i = 0; i < sizeof(u32); i++) { >>>> - unsigned char c = val >> (i * 8); >>>> + unsigned char c = val >> ((3 - i) * 8); >>>> /* Print non-control ASCII characters as-is, dot otherwise */ >>>> *p++ = isascii(c) && isprint(c) ? c : '.'; >>>> } >>>> - *p++ = ' '; >>>> - strcpy(p, orig & BIT(31) ? "big-endian" : "little-endian"); >>>> - p += strlen(p); >>>> + if (pixel_fmt) { >>>> + *p++ = ' '; >>>> + strcpy(p, orig & BIT(31) ? "big-endian" : "little-endian"); >>>> + p += strlen(p); >>>> + } >>>> *p++ = ' '; >>>> *p++ = '('; >>>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl >>>> index 7b28ad331..5595a0898 100755 >>>> --- a/scripts/checkpatch.pl >>>> +++ b/scripts/checkpatch.pl >>>> @@ -6904,7 +6904,7 @@ sub process { >>>> ($extension eq "f" && >>>> defined $qualifier && $qualifier !~ /^w/) || >>>> ($extension eq "4" && >>>> - defined $qualifier && $qualifier !~ /^cc/)) { >>>> + defined $qualifier && $qualifier !~ /^c[hnlbc]/)) { >>>> $bad_specifier = $specifier; >>>> last; >>>> } >>> -- >>> -- >>> Thomas Zimmermann >>> Graphics Driver Developer >>> SUSE Software Solutions Germany GmbH >>> Frankenstrasse 146, 90461 Nuernberg, Germany >>> GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman >>> HRB 36809 (AG Nuernberg) >>> > > -- > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Frankenstrasse 146, 90461 Nuernberg, Germany > GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman > HRB 36809 (AG Nuernberg) >
> On 12 Mar 2025, at 5:33 PM, Aditya Garg <gargaditya08@live.com> wrote: > > > >> On 12 Mar 2025, at 5:29 PM, Thomas Zimmermann <tzimmermann@suse.de> wrote: >> >> Hi >> >>>> Am 12.03.25 um 12:49 schrieb Aditya Garg: >>> >>>>> On 12 Mar 2025, at 5:16 PM, Thomas Zimmermann <tzimmermann@suse.de> wrote: >>>> >>>> Hi >>>> >>>>> Am 12.03.25 um 10:05 schrieb Aditya Garg: >>>>> From: Hector Martin <marcan@marcan.st> >>>>> >>>>> %p4cc is designed for DRM/V4L2 FourCCs with their specific quirks, but >>>>> it's useful to be able to print generic 4-character codes formatted as >>>>> an integer. Extend it to add format specifiers for printing generic >>>>> 32-bit FourCCs with various endian semantics: >>>>> >>>>> %p4ch Host byte order >>>>> %p4cn Network byte order >>>>> %p4cl Little-endian >>>>> %p4cb Big-endian >>>> That looks like someone trying to be too clever for their own good. Just my 2 cts. >>> I don't understand what you are trying to say. Anyways, I thought it's obvious, but Petr's Ack is still left and thus cannot be merged into DRM for now unless he says so in this thread. >> >> I'm trying to say that the author of this patch found the %p4cc functionality and over-generalized the feature. Source code should express the idea of what it's doing in clear terms. %p4ch somehow doesn't do that for me. Printing 4 bytes in various orders without context seems arbitrary and confusing. >> >> (I don't really have a say here. I'm just asking to reconsider this change.) > > Ah I see. I'll checkout the macros you sent. The Asahi Linux SMC drivers would need these as well, so I'll probably first wait for the vsprintf maintainers and also Asahi Linux maintainers for their views. >> You also might have noticed that the FourCCs actually make sense in this driver. Eg: REDY, CLRD stand for APPLETBDRM_MSG_SIGNAL_READINESS and APPLETBDRM_MSG_CLEAR_DISPLAY respectively. >> Best regards >> Thomas >> >>>> Best regards >>>> Thomas >>>> >>>>> The endianness determines how bytes are interpreted as a u32, and the >>>>> FourCC is then always printed MSByte-first (this is the opposite of >>>>> V4L/DRM FourCCs). This covers most practical cases, e.g. %p4cn would >>>>> allow printing LSByte-first FourCCs stored in host endian order >>>>> (other than the hex form being in character order, not the integer >>>>> value). >>>>> >>>>> Acked-by: Rasmus Villemoes <linux@rasmusvillemoes.dk> >>>>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> >>>>> Signed-off-by: Hector Martin <marcan@marcan.st> >>>>> Signed-off-by: Aditya Garg <gargaditya08@live.com> >>>>> --- >>>>> Documentation/core-api/printk-formats.rst | 32 +++++++++++++++++++ >>>>> lib/test_printf.c | 39 +++++++++++++++++++---- >>>>> lib/vsprintf.c | 35 ++++++++++++++++---- >>>>> scripts/checkpatch.pl | 2 +- >>>>> 4 files changed, 94 insertions(+), 14 deletions(-) >>>>> >>>>> diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst >>>>> index ecccc0473..bd420e8aa 100644 >>>>> --- a/Documentation/core-api/printk-formats.rst >>>>> +++ b/Documentation/core-api/printk-formats.rst >>>>> @@ -648,6 +648,38 @@ Examples:: >>>>> %p4cc Y10 little-endian (0x20303159) >>>>> %p4cc NV12 big-endian (0xb231564e) >>>>> +Generic FourCC code >>>>> +------------------- >>>>> + >>>>> +:: >>>>> + %p4c[hnlb] gP00 (0x67503030) >>>>> + >>>>> +Print a generic FourCC code, as both ASCII characters and its numerical >>>>> +value as hexadecimal. >>>>> + >>>>> +The generic FourCC code is always printed in the big-endian format, >>>>> +the most significant byte first. This is the opposite of V4L/DRM FourCCs. >>>>> + >>>>> +The additional ``h``, ``n``, ``l``, and ``b`` specifiers define what >>>>> +endianness is used to load the stored bytes. The data might be interpreted >>>>> +using the host byte order, network byte order, little-endian, or big-endian. >>>>> + >>>>> +Passed by reference. >>>>> + >>>>> +Examples for a little-endian machine, given &(u32)0x67503030:: >>>>> + >>>>> + %p4ch gP00 (0x67503030) >>>>> + %p4cn 00Pg (0x30305067) >>>>> + %p4cl gP00 (0x67503030) >>>>> + %p4cb 00Pg (0x30305067) >>>>> + >>>>> +Examples for a big-endian machine, given &(u32)0x67503030:: >>>>> + >>>>> + %p4ch gP00 (0x67503030) >>>>> + %p4cn 00Pg (0x30305067) >>>>> + %p4cl 00Pg (0x30305067) >>>>> + %p4cb gP00 (0x67503030) >>>>> + >>>>> Rust >>>>> ---- >>>>> diff --git a/lib/test_printf.c b/lib/test_printf.c >>>>> index 59dbe4f9a..b9e8afc01 100644 >>>>> --- a/lib/test_printf.c >>>>> +++ b/lib/test_printf.c >>>>> @@ -776,21 +776,46 @@ static void __init fwnode_pointer(void) >>>>> software_node_unregister_node_group(group); >>>>> } >>>>> +struct fourcc_struct { >>>>> + u32 code; >>>>> + const char *str; >>>>> +}; >>>>> + >>>>> +static void __init fourcc_pointer_test(const struct fourcc_struct *fc, size_t n, >>>>> + const char *fmt) >>>>> +{ >>>>> + size_t i; >>>>> + >>>>> + for (i = 0; i < n; i++) >>>>> + test(fc[i].str, fmt, &fc[i].code); >>>>> +} >>>>> + >>>>> static void __init fourcc_pointer(void) >>>>> { >>>>> - struct { >>>>> - u32 code; >>>>> - char *str; >>>>> - } const try[] = { >>>>> + static const struct fourcc_struct try_cc[] = { >>>>> { 0x3231564e, "NV12 little-endian (0x3231564e)", }, >>>>> { 0xb231564e, "NV12 big-endian (0xb231564e)", }, >>>>> { 0x10111213, ".... little-endian (0x10111213)", }, >>>>> { 0x20303159, "Y10 little-endian (0x20303159)", }, >>>>> }; >>>>> - unsigned int i; >>>>> + static const struct fourcc_struct try_ch[] = { >>>>> + { 0x41424344, "ABCD (0x41424344)", }, >>>>> + }; >>>>> + static const struct fourcc_struct try_cn[] = { >>>>> + { 0x41424344, "DCBA (0x44434241)", }, >>>>> + }; >>>>> + static const struct fourcc_struct try_cl[] = { >>>>> + { (__force u32)cpu_to_le32(0x41424344), "ABCD (0x41424344)", }, >>>>> + }; >>>>> + static const struct fourcc_struct try_cb[] = { >>>>> + { (__force u32)cpu_to_be32(0x41424344), "ABCD (0x41424344)", }, >>>>> + }; >>>>> - for (i = 0; i < ARRAY_SIZE(try); i++) >>>>> - test(try[i].str, "%p4cc", &try[i].code); >>>>> + fourcc_pointer_test(try_cc, ARRAY_SIZE(try_cc), "%p4cc"); >>>>> + fourcc_pointer_test(try_ch, ARRAY_SIZE(try_ch), "%p4ch"); >>>>> + fourcc_pointer_test(try_cn, ARRAY_SIZE(try_cn), "%p4cn"); >>>>> + fourcc_pointer_test(try_cl, ARRAY_SIZE(try_cl), "%p4cl"); >>>>> + fourcc_pointer_test(try_cb, ARRAY_SIZE(try_cb), "%p4cb"); >>>>> } >>>>> static void __init >>>>> diff --git a/lib/vsprintf.c b/lib/vsprintf.c >>>>> index 56fe96319..56511a994 100644 >>>>> --- a/lib/vsprintf.c >>>>> +++ b/lib/vsprintf.c >>>>> @@ -1781,27 +1781,50 @@ char *fourcc_string(char *buf, char *end, const u32 *fourcc, >>>>> char output[sizeof("0123 little-endian (0x01234567)")]; >>>>> char *p = output; >>>>> unsigned int i; >>>>> + bool pixel_fmt = false; >>>>> u32 orig, val; >>>>> - if (fmt[1] != 'c' || fmt[2] != 'c') >>>>> + if (fmt[1] != 'c') >>>>> return error_string(buf, end, "(%p4?)", spec); >>>>> if (check_pointer(&buf, end, fourcc, spec)) >>>>> return buf; >>>>> orig = get_unaligned(fourcc); >>>>> - val = orig & ~BIT(31); >>>>> + switch (fmt[2]) { >>>>> + case 'h': >>>>> + break; >>>>> + case 'n': >>>>> + orig = swab32(orig); >>>>> + break; >>>>> + case 'l': >>>>> + orig = (__force u32)cpu_to_le32(orig); >>>>> + break; >>>>> + case 'b': >>>>> + orig = (__force u32)cpu_to_be32(orig); >>>>> + break; >>>>> + case 'c': >>>>> + /* Pixel formats are printed LSB-first */ >>>>> + pixel_fmt = true; >>>>> + break; >>>>> + default: >>>>> + return error_string(buf, end, "(%p4?)", spec); >>>>> + } >>>>> + >>>>> + val = pixel_fmt ? swab32(orig & ~BIT(31)) : orig; >>>>> for (i = 0; i < sizeof(u32); i++) { >>>>> - unsigned char c = val >> (i * 8); >>>>> + unsigned char c = val >> ((3 - i) * 8); >>>>> /* Print non-control ASCII characters as-is, dot otherwise */ >>>>> *p++ = isascii(c) && isprint(c) ? c : '.'; >>>>> } >>>>> - *p++ = ' '; >>>>> - strcpy(p, orig & BIT(31) ? "big-endian" : "little-endian"); >>>>> - p += strlen(p); >>>>> + if (pixel_fmt) { >>>>> + *p++ = ' '; >>>>> + strcpy(p, orig & BIT(31) ? "big-endian" : "little-endian"); >>>>> + p += strlen(p); >>>>> + } >>>>> *p++ = ' '; >>>>> *p++ = '('; >>>>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl >>>>> index 7b28ad331..5595a0898 100755 >>>>> --- a/scripts/checkpatch.pl >>>>> +++ b/scripts/checkpatch.pl >>>>> @@ -6904,7 +6904,7 @@ sub process { >>>>> ($extension eq "f" && >>>>> defined $qualifier && $qualifier !~ /^w/) || >>>>> ($extension eq "4" && >>>>> - defined $qualifier && $qualifier !~ /^cc/)) { >>>>> + defined $qualifier && $qualifier !~ /^c[hnlbc]/)) { >>>>> $bad_specifier = $specifier; >>>>> last; >>>>> } >>>> -- >>>> -- >>>> Thomas Zimmermann >>>> Graphics Driver Developer >>>> SUSE Software Solutions Germany GmbH >>>> Frankenstrasse 146, 90461 Nuernberg, Germany >>>> GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman >>>> HRB 36809 (AG Nuernberg) >>>> >> >> -- >> -- >> Thomas Zimmermann >> Graphics Driver Developer >> SUSE Software Solutions Germany GmbH >> Frankenstrasse 146, 90461 Nuernberg, Germany >> GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman >> HRB 36809 (AG Nuernberg) >>
Hi, On Wed, Mar 12, 2025, at 13:03, Aditya Garg wrote: >> On 12 Mar 2025, at 5:29 PM, Thomas Zimmermann <tzimmermann@suse.de> wrote: >> >> Hi >> >>> Am 12.03.25 um 12:49 schrieb Aditya Garg: >>> >>>>> On 12 Mar 2025, at 5:16 PM, Thomas Zimmermann <tzimmermann@suse.de> wrote: >>>> >>>> Hi >>>> >>>>> Am 12.03.25 um 10:05 schrieb Aditya Garg: >>>>> From: Hector Martin <marcan@marcan.st> >>>>> >>>>> %p4cc is designed for DRM/V4L2 FourCCs with their specific quirks, but >>>>> it's useful to be able to print generic 4-character codes formatted as >>>>> an integer. Extend it to add format specifiers for printing generic >>>>> 32-bit FourCCs with various endian semantics: >>>>> >>>>> %p4ch Host byte order >>>>> %p4cn Network byte order >>>>> %p4cl Little-endian >>>>> %p4cb Big-endian >>>> That looks like someone trying to be too clever for their own good. Just my 2 cts. >>> I don't understand what you are trying to say. Anyways, I thought it's obvious, but Petr's Ack is still left and thus cannot be merged into DRM for now unless he says so in this thread. >> >> I'm trying to say that the author of this patch found the %p4cc functionality and over-generalized the feature. Source code should express the idea of what it's doing in clear terms. %p4ch somehow doesn't do that for me. Printing 4 bytes in various orders without context seems arbitrary and confusing. >> >> (I don't really have a say here. I'm just asking to reconsider this change.) > > Ah I see. I'll checkout the macros you sent. The Asahi Linux SMC > drivers would need these as well, so I'll probably first wait for the > vsprintf maintainers and also Asahi Linux maintainers for their views. I don't have a strong opinion either way: for SMC I just need to print FourCC keys for debugging / information in a few places. I'm preparing the SMC driver for upstreaming again (after a two year delay :-() and was just going to use macros to print the SMC FourCC keys similar to DRM_MODE_FMT/DRM_MODE_ARG for now to keep the series smaller and revisit the topic later. Right now I have these in my local tree (only compile tested so far): #define SMC_KEY_FMT "%c%c%c%c (0x%08x)" #define SMC_KEY_ARG(k) (k)>>24, (k)>>16, (k)>>8, (k), (k) which are then used like this: dev_info(dev, "Initialized (%d keys " SMC_KEY_FMT " .. " SMC_KEY_FMT ")\n", smc->key_count, SMC_KEY_ARG(smc->first_key), SMC_KEY_ARG(smc->last_key)); Best, Sven
> On 12 Mar 2025, at 9:05 PM, Sven Peter <sven@svenpeter.dev> wrote: > > Hi, > > > On Wed, Mar 12, 2025, at 13:03, Aditya Garg wrote: >>>> On 12 Mar 2025, at 5:29 PM, Thomas Zimmermann <tzimmermann@suse.de> wrote: >>> >>> Hi >>> >>>> Am 12.03.25 um 12:49 schrieb Aditya Garg: >>>> >>>>>> On 12 Mar 2025, at 5:16 PM, Thomas Zimmermann <tzimmermann@suse.de> wrote: >>>>> >>>>> Hi >>>>> >>>>>> Am 12.03.25 um 10:05 schrieb Aditya Garg: >>>>>> From: Hector Martin <marcan@marcan.st> >>>>>> >>>>>> %p4cc is designed for DRM/V4L2 FourCCs with their specific quirks, but >>>>>> it's useful to be able to print generic 4-character codes formatted as >>>>>> an integer. Extend it to add format specifiers for printing generic >>>>>> 32-bit FourCCs with various endian semantics: >>>>>> >>>>>> %p4ch Host byte order >>>>>> %p4cn Network byte order >>>>>> %p4cl Little-endian >>>>>> %p4cb Big-endian >>>>> That looks like someone trying to be too clever for their own good. Just my 2 cts. >>>> I don't understand what you are trying to say. Anyways, I thought it's obvious, but Petr's Ack is still left and thus cannot be merged into DRM for now unless he says so in this thread. >>> >>> I'm trying to say that the author of this patch found the %p4cc functionality and over-generalized the feature. Source code should express the idea of what it's doing in clear terms. %p4ch somehow doesn't do that for me. Printing 4 bytes in various orders without context seems arbitrary and confusing. >>> >>> (I don't really have a say here. I'm just asking to reconsider this change.) >> >> Ah I see. I'll checkout the macros you sent. The Asahi Linux SMC >> drivers would need these as well, so I'll probably first wait for the >> vsprintf maintainers and also Asahi Linux maintainers for their views. > > I don't have a strong opinion either way: for SMC I just need to print > FourCC keys for debugging / information in a few places. > > I'm preparing the SMC driver for upstreaming again (after a two year delay :-() > and was just going to use macros to print the SMC FourCC keys similar to > DRM_MODE_FMT/DRM_MODE_ARG for now to keep the series smaller and revisit > the topic later. > > Right now I have these in my local tree (only compile tested so far): > > #define SMC_KEY_FMT "%c%c%c%c (0x%08x)" > #define SMC_KEY_ARG(k) (k)>>24, (k)>>16, (k)>>8, (k), (k) That seems to be a nice alternative, which I guess Thomas was also suggesting. > which are then used like this: > > dev_info(dev, > "Initialized (%d keys " SMC_KEY_FMT " .. " SMC_KEY_FMT ")\n", > smc->key_count, SMC_KEY_ARG(smc->first_key), > SMC_KEY_ARG(smc->last_key)); > > Best, > > Sven
On Wed, Mar 12, 2025 at 07:14:36PM +0000, Aditya Garg wrote: > > On 12 Mar 2025, at 9:05 PM, Sven Peter <sven@svenpeter.dev> wrote: > > On Wed, Mar 12, 2025, at 13:03, Aditya Garg wrote: ... > > I don't have a strong opinion either way: for SMC I just need to print > > FourCC keys for debugging / information in a few places. > > > > I'm preparing the SMC driver for upstreaming again (after a two year delay :-() > > and was just going to use macros to print the SMC FourCC keys similar to > > DRM_MODE_FMT/DRM_MODE_ARG for now to keep the series smaller and revisit > > the topic later. > > > > Right now I have these in my local tree (only compile tested so far): > > > > #define SMC_KEY_FMT "%c%c%c%c (0x%08x)" > > #define SMC_KEY_ARG(k) (k)>>24, (k)>>16, (k)>>8, (k), (k) > > That seems to be a nice alternative, which I guess Thomas was also suggesting. I don't think it's "nice". Each of the approaches has pros and cons. You can start from bloat-o-meter here and compare it with your %p extension. Also, can you show the bloat-o-meter output for the vsprintf.c? > > which are then used like this: > > > > dev_info(dev, > > "Initialized (%d keys " SMC_KEY_FMT " .. " SMC_KEY_FMT ")\n", > > smc->key_count, SMC_KEY_ARG(smc->first_key), > > SMC_KEY_ARG(smc->last_key));
> On 13 Mar 2025, at 12:58 AM, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Wed, Mar 12, 2025 at 07:14:36PM +0000, Aditya Garg wrote: >>>> On 12 Mar 2025, at 9:05 PM, Sven Peter <sven@svenpeter.dev> wrote: >>> On Wed, Mar 12, 2025, at 13:03, Aditya Garg wrote: > > ... > >>> I don't have a strong opinion either way: for SMC I just need to print >>> FourCC keys for debugging / information in a few places. >>> >>> I'm preparing the SMC driver for upstreaming again (after a two year delay :-() >>> and was just going to use macros to print the SMC FourCC keys similar to >>> DRM_MODE_FMT/DRM_MODE_ARG for now to keep the series smaller and revisit >>> the topic later. >>> >>> Right now I have these in my local tree (only compile tested so far): >>> >>> #define SMC_KEY_FMT "%c%c%c%c (0x%08x)" >>> #define SMC_KEY_ARG(k) (k)>>24, (k)>>16, (k)>>8, (k), (k) >> >> That seems to be a nice alternative, which I guess Thomas was also suggesting. > > I don't think it's "nice". Each of the approaches has pros and cons. I would prefer vsprintf, but if it's not there, that remains as nice right? > You can start from bloat-o-meter here and compare it with your %p extension. > > Also, can you show the bloat-o-meter output for the vsprintf.c? vsprintf isn't a kernel module, is it? I'll have to compile a new kernel I guess. > >>> which are then used like this: >>> >>> dev_info(dev, >>> "Initialized (%d keys " SMC_KEY_FMT " .. " SMC_KEY_FMT ")\n", >>> smc->key_count, SMC_KEY_ARG(smc->first_key), >>> SMC_KEY_ARG(smc->last_key)); > > -- > With Best Regards, > Andy Shevchenko > >
On Wed, Mar 12, 2025 at 07:35:54PM +0000, Aditya Garg wrote: > > On 13 Mar 2025, at 12:58 AM, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Wed, Mar 12, 2025 at 07:14:36PM +0000, Aditya Garg wrote: > >>>> On 12 Mar 2025, at 9:05 PM, Sven Peter <sven@svenpeter.dev> wrote: > >>> On Wed, Mar 12, 2025, at 13:03, Aditya Garg wrote: ... > >>> I don't have a strong opinion either way: for SMC I just need to print > >>> FourCC keys for debugging / information in a few places. > >>> > >>> I'm preparing the SMC driver for upstreaming again (after a two year delay :-() > >>> and was just going to use macros to print the SMC FourCC keys similar to > >>> DRM_MODE_FMT/DRM_MODE_ARG for now to keep the series smaller and revisit > >>> the topic later. > >>> > >>> Right now I have these in my local tree (only compile tested so far): > >>> > >>> #define SMC_KEY_FMT "%c%c%c%c (0x%08x)" > >>> #define SMC_KEY_ARG(k) (k)>>24, (k)>>16, (k)>>8, (k), (k) > >> > >> That seems to be a nice alternative, which I guess Thomas was also suggesting. > > > > I don't think it's "nice". Each of the approaches has pros and cons. > > I would prefer vsprintf, but if it's not there, that remains as nice right? Nope, it remains us with the only approach (besides copy'n'paste everywhere which is error prone). > > You can start from bloat-o-meter here and compare it with your %p extension. > > > > Also, can you show the bloat-o-meter output for the vsprintf.c? > > vsprintf isn't a kernel module, is it? I'll have to compile a new kernel I guess. You can just compile one file. We need an object out of it, we don't it to be linked. > >>> which are then used like this: > >>> > >>> dev_info(dev, > >>> "Initialized (%d keys " SMC_KEY_FMT " .. " SMC_KEY_FMT ")\n", > >>> smc->key_count, SMC_KEY_ARG(smc->first_key), > >>> SMC_KEY_ARG(smc->last_key));
diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst index ecccc0473..bd420e8aa 100644 --- a/Documentation/core-api/printk-formats.rst +++ b/Documentation/core-api/printk-formats.rst @@ -648,6 +648,38 @@ Examples:: %p4cc Y10 little-endian (0x20303159) %p4cc NV12 big-endian (0xb231564e) +Generic FourCC code +------------------- + +:: + %p4c[hnlb] gP00 (0x67503030) + +Print a generic FourCC code, as both ASCII characters and its numerical +value as hexadecimal. + +The generic FourCC code is always printed in the big-endian format, +the most significant byte first. This is the opposite of V4L/DRM FourCCs. + +The additional ``h``, ``n``, ``l``, and ``b`` specifiers define what +endianness is used to load the stored bytes. The data might be interpreted +using the host byte order, network byte order, little-endian, or big-endian. + +Passed by reference. + +Examples for a little-endian machine, given &(u32)0x67503030:: + + %p4ch gP00 (0x67503030) + %p4cn 00Pg (0x30305067) + %p4cl gP00 (0x67503030) + %p4cb 00Pg (0x30305067) + +Examples for a big-endian machine, given &(u32)0x67503030:: + + %p4ch gP00 (0x67503030) + %p4cn 00Pg (0x30305067) + %p4cl 00Pg (0x30305067) + %p4cb gP00 (0x67503030) + Rust ---- diff --git a/lib/test_printf.c b/lib/test_printf.c index 59dbe4f9a..b9e8afc01 100644 --- a/lib/test_printf.c +++ b/lib/test_printf.c @@ -776,21 +776,46 @@ static void __init fwnode_pointer(void) software_node_unregister_node_group(group); } +struct fourcc_struct { + u32 code; + const char *str; +}; + +static void __init fourcc_pointer_test(const struct fourcc_struct *fc, size_t n, + const char *fmt) +{ + size_t i; + + for (i = 0; i < n; i++) + test(fc[i].str, fmt, &fc[i].code); +} + static void __init fourcc_pointer(void) { - struct { - u32 code; - char *str; - } const try[] = { + static const struct fourcc_struct try_cc[] = { { 0x3231564e, "NV12 little-endian (0x3231564e)", }, { 0xb231564e, "NV12 big-endian (0xb231564e)", }, { 0x10111213, ".... little-endian (0x10111213)", }, { 0x20303159, "Y10 little-endian (0x20303159)", }, }; - unsigned int i; + static const struct fourcc_struct try_ch[] = { + { 0x41424344, "ABCD (0x41424344)", }, + }; + static const struct fourcc_struct try_cn[] = { + { 0x41424344, "DCBA (0x44434241)", }, + }; + static const struct fourcc_struct try_cl[] = { + { (__force u32)cpu_to_le32(0x41424344), "ABCD (0x41424344)", }, + }; + static const struct fourcc_struct try_cb[] = { + { (__force u32)cpu_to_be32(0x41424344), "ABCD (0x41424344)", }, + }; - for (i = 0; i < ARRAY_SIZE(try); i++) - test(try[i].str, "%p4cc", &try[i].code); + fourcc_pointer_test(try_cc, ARRAY_SIZE(try_cc), "%p4cc"); + fourcc_pointer_test(try_ch, ARRAY_SIZE(try_ch), "%p4ch"); + fourcc_pointer_test(try_cn, ARRAY_SIZE(try_cn), "%p4cn"); + fourcc_pointer_test(try_cl, ARRAY_SIZE(try_cl), "%p4cl"); + fourcc_pointer_test(try_cb, ARRAY_SIZE(try_cb), "%p4cb"); } static void __init diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 56fe96319..56511a994 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -1781,27 +1781,50 @@ char *fourcc_string(char *buf, char *end, const u32 *fourcc, char output[sizeof("0123 little-endian (0x01234567)")]; char *p = output; unsigned int i; + bool pixel_fmt = false; u32 orig, val; - if (fmt[1] != 'c' || fmt[2] != 'c') + if (fmt[1] != 'c') return error_string(buf, end, "(%p4?)", spec); if (check_pointer(&buf, end, fourcc, spec)) return buf; orig = get_unaligned(fourcc); - val = orig & ~BIT(31); + switch (fmt[2]) { + case 'h': + break; + case 'n': + orig = swab32(orig); + break; + case 'l': + orig = (__force u32)cpu_to_le32(orig); + break; + case 'b': + orig = (__force u32)cpu_to_be32(orig); + break; + case 'c': + /* Pixel formats are printed LSB-first */ + pixel_fmt = true; + break; + default: + return error_string(buf, end, "(%p4?)", spec); + } + + val = pixel_fmt ? swab32(orig & ~BIT(31)) : orig; for (i = 0; i < sizeof(u32); i++) { - unsigned char c = val >> (i * 8); + unsigned char c = val >> ((3 - i) * 8); /* Print non-control ASCII characters as-is, dot otherwise */ *p++ = isascii(c) && isprint(c) ? c : '.'; } - *p++ = ' '; - strcpy(p, orig & BIT(31) ? "big-endian" : "little-endian"); - p += strlen(p); + if (pixel_fmt) { + *p++ = ' '; + strcpy(p, orig & BIT(31) ? "big-endian" : "little-endian"); + p += strlen(p); + } *p++ = ' '; *p++ = '('; diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 7b28ad331..5595a0898 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -6904,7 +6904,7 @@ sub process { ($extension eq "f" && defined $qualifier && $qualifier !~ /^w/) || ($extension eq "4" && - defined $qualifier && $qualifier !~ /^cc/)) { + defined $qualifier && $qualifier !~ /^c[hnlbc]/)) { $bad_specifier = $specifier; last; }