Message ID | E1oVYUS-005CmS-IA@rmk-PC.armlinux.org.uk (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Add Apple Mac System Management Controller GPIOs | expand |
On 06/09/2022 15.19, Russell King (Oracle) wrote: > 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: > > Documentation/core-api/printk-formats.rst | 32 +++++++++++++++++++++ > lib/vsprintf.c | 35 +++++++++++++++++++---- > 2 files changed, 61 insertions(+), 6 deletions(-) Please also update lib/test_printf.c with a few test cases when modifying/extending vsnprintf(). > diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst > index 5e89497ba314..22c33398ec02 100644 > --- a/Documentation/core-api/printk-formats.rst > +++ b/Documentation/core-api/printk-formats.rst > @@ -625,6 +625,38 @@ Passed by reference. > %p4cc Y10 little-endian (0x20303159) > %p4cc NV12 big-endian (0xb231564e) > > +Generic FourCC code > +------------------- > + > +:: > + %p4c[hnbl] gP00 (0x67503030) > + Typo, I think. hrbl ? Rasmus
On Tue 2022-09-06 14:19:44, Russell King wrote: > 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-endian > %p4cl Little-endian > %p4cb Big-endian > %p4cr Reverse-endian > > 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. %p4cr would > allow printing LSByte-first FOURCCs stored in host endian order > (other than the hex form being in character order, not the integer > value). > > --- a/Documentation/core-api/printk-formats.rst > +++ b/Documentation/core-api/printk-formats.rst > @@ -625,6 +625,38 @@ Passed by reference. > %p4cc Y10 little-endian (0x20303159) > %p4cc NV12 big-endian (0xb231564e) > > +Generic FourCC code > +------------------- > + > +:: > + %p4c[hnbl] gP00 (0x67503030) > + > +Print a generic FourCC code, as both ASCII characters and its numerical > +value as hexadecimal. > + > +The additional ``h``, ``r``, ``b``, and ``l`` specifiers are used to specify > +host, reversed, big or little endian order data respectively. Host endian > +order means the data is interpreted as a 32-bit integer and the most > +significant byte is printed first; that is, the character code as printed > +matches the byte order stored in memory on big-endian systems, and is reversed > +on little-endian systems. > + > +Passed by reference. > + > +Examples for a little-endian machine, given &(u32)0x67503030:: > + > + %p4ch gP00 (0x67503030) > + %p4cl gP00 (0x67503030) > + %p4cb 00Pg (0x30305067) > + %p4cr 00Pg (0x30305067) Nit: I would prefer to keep the same order (h,r,b,l) everywhere. I guess that you wanted to show exactly the same results next to each other. But it is not the case on big-endian anyway. > + > +Examples for a big-endian machine, given &(u32)0x67503030:: > + > + %p4ch gP00 (0x67503030) > + %p4cl 00Pg (0x30305067) > + %p4cb gP00 (0x67503030) > + %p4cr 00Pg (0x30305067) Same here. > + > Thanks > ====== > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > index 3c1853a9d1c0..31707499f90f 100644 > --- a/lib/vsprintf.c > +++ b/lib/vsprintf.c > @@ -1757,27 +1757,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 pix_fmt = false; Nit: I would prefer "pixel_fmt". I am not a graphics guy and wondered what "pix" did stands for ;-) > 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': > + val = orig; > + break; > + case 'r': > + val = orig = swab32(orig); > + break; > + case 'l': > + val = orig = le32_to_cpu(orig); > + break; > + case 'b': > + val = orig = be32_to_cpu(orig); > + break; > + case 'c': > + /* Pixel formats are printed LSB-first */ > + val = swab32(orig & ~BIT(31)); > + pix_fmt = true; > + break; > + default: > + return error_string(buf, end, "(%p4?)", spec); > + } > > for (i = 0; i < sizeof(u32); i++) { > - unsigned char c = val >> (i * 8); > + unsigned char c = val >> ((3 - i) * 8); This hardcodes '3' but the for-cycle uses i < sizeof(u32). We should be consistent. A solution would be: int i; for (i = sizeof(u32); --i >= 0;) { unsigned char c = val >> (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 (pix_fmt) { > + *p++ = ' '; > + strcpy(p, orig & BIT(31) ? "big-endian" : "little-endian"); > + p += strlen(p); > + } > > *p++ = ' '; > *p++ = '('; Best Regards, Petr
On Wed, Oct 19, 2022 at 12:00:23PM +0200, Petr Mladek wrote: > On Tue 2022-09-06 14:19:44, Russell King wrote: > > From: Hector Martin <marcan@marcan.st> ... > > for (i = 0; i < sizeof(u32); i++) { > > - unsigned char c = val >> (i * 8); > > + unsigned char c = val >> ((3 - i) * 8); > > This hardcodes '3' but the for-cycle uses i < sizeof(u32). > We should be consistent. > > A solution would be: > > int i; > > for (i = sizeof(u32); --i >= 0;) { > unsigned char c = val >> (i * 8); With while-loop it might be more readable: unsigned int i = sizeof(u32); while (i--) { ... } > > /* Print non-control ASCII characters as-is, dot otherwise */ > > *p++ = isascii(c) && isprint(c) ? c : '.'; > > }
On Wed, Oct 19, 2022 at 12:00:23PM +0200, Petr Mladek wrote: > On Tue 2022-09-06 14:19:44, Russell King wrote: > > From: Hector Martin <marcan@marcan.st> > > ... > > +Generic FourCC code > > +------------------- > > + > > +:: > > + %p4c[hnbl] gP00 (0x67503030) > > + > > +Print a generic FourCC code, as both ASCII characters and its numerical > > +value as hexadecimal. > > + > > +The additional ``h``, ``r``, ``b``, and ``l`` specifiers are used to specify > > +host, reversed, big or little endian order data respectively. Host endian > > +order means the data is interpreted as a 32-bit integer and the most > > +significant byte is printed first; that is, the character code as printed > > +matches the byte order stored in memory on big-endian systems, and is reversed > > +on little-endian systems. > > + > > +Passed by reference. > > + > > +Examples for a little-endian machine, given &(u32)0x67503030:: > > + > > + %p4ch gP00 (0x67503030) > > + %p4cl gP00 (0x67503030) > > + %p4cb 00Pg (0x30305067) > > + %p4cr 00Pg (0x30305067) > > Nit: I would prefer to keep the same order (h,r,b,l) everywhere. > > I guess that you wanted to show exactly the same results next > to each other. But it is not the case on big-endian anyway. This is straight from the Asahi kernel tree, and is unmodified. I'm guessing you're use of "you" here refers to Hector rather than me. So, Hector, any opinions on Petr's comments please? Thanks.
On 19/10/2022 20.48, Russell King (Oracle) wrote: > On Wed, Oct 19, 2022 at 12:00:23PM +0200, Petr Mladek wrote: >> On Tue 2022-09-06 14:19:44, Russell King wrote: >>> From: Hector Martin <marcan@marcan.st> >>> > ... >>> +Generic FourCC code >>> +------------------- >>> + >>> +:: >>> + %p4c[hnbl] gP00 (0x67503030) >>> + >>> +Print a generic FourCC code, as both ASCII characters and its numerical >>> +value as hexadecimal. >>> + >>> +The additional ``h``, ``r``, ``b``, and ``l`` specifiers are used to specify >>> +host, reversed, big or little endian order data respectively. Host endian >>> +order means the data is interpreted as a 32-bit integer and the most >>> +significant byte is printed first; that is, the character code as printed >>> +matches the byte order stored in memory on big-endian systems, and is reversed >>> +on little-endian systems. >>> + >>> +Passed by reference. >>> + >>> +Examples for a little-endian machine, given &(u32)0x67503030:: >>> + >>> + %p4ch gP00 (0x67503030) >>> + %p4cl gP00 (0x67503030) >>> + %p4cb 00Pg (0x30305067) >>> + %p4cr 00Pg (0x30305067) >> >> Nit: I would prefer to keep the same order (h,r,b,l) everywhere. >> >> I guess that you wanted to show exactly the same results next >> to each other. But it is not the case on big-endian anyway. > > This is straight from the Asahi kernel tree, and is unmodified. I'm > guessing you're use of "you" here refers to Hector rather than me. > > So, Hector, any opinions on Petr's comments please? Fine with me, feel free to make the changes :) - Hector
diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst index 5e89497ba314..22c33398ec02 100644 --- a/Documentation/core-api/printk-formats.rst +++ b/Documentation/core-api/printk-formats.rst @@ -625,6 +625,38 @@ Passed by reference. %p4cc Y10 little-endian (0x20303159) %p4cc NV12 big-endian (0xb231564e) +Generic FourCC code +------------------- + +:: + %p4c[hnbl] gP00 (0x67503030) + +Print a generic FourCC code, as both ASCII characters and its numerical +value as hexadecimal. + +The additional ``h``, ``r``, ``b``, and ``l`` specifiers are used to specify +host, reversed, big or little endian order data respectively. Host endian +order means the data is interpreted as a 32-bit integer and the most +significant byte is printed first; that is, the character code as printed +matches the byte order stored in memory on big-endian systems, and is reversed +on little-endian systems. + +Passed by reference. + +Examples for a little-endian machine, given &(u32)0x67503030:: + + %p4ch gP00 (0x67503030) + %p4cl gP00 (0x67503030) + %p4cb 00Pg (0x30305067) + %p4cr 00Pg (0x30305067) + +Examples for a big-endian machine, given &(u32)0x67503030:: + + %p4ch gP00 (0x67503030) + %p4cl 00Pg (0x30305067) + %p4cb gP00 (0x67503030) + %p4cr 00Pg (0x30305067) + Thanks ====== diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 3c1853a9d1c0..31707499f90f 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -1757,27 +1757,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 pix_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': + val = orig; + break; + case 'r': + val = orig = swab32(orig); + break; + case 'l': + val = orig = le32_to_cpu(orig); + break; + case 'b': + val = orig = be32_to_cpu(orig); + break; + case 'c': + /* Pixel formats are printed LSB-first */ + val = swab32(orig & ~BIT(31)); + pix_fmt = true; + break; + default: + return error_string(buf, end, "(%p4?)", spec); + } 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 (pix_fmt) { + *p++ = ' '; + strcpy(p, orig & BIT(31) ? "big-endian" : "little-endian"); + p += strlen(p); + } *p++ = ' '; *p++ = '(';