Message ID | 20210215114030.11862-1-sakari.ailus@linux.intel.com |
---|---|
Headers | show |
Series | Add %p4cc printk modifier for V4L2 and DRM fourcc codes | expand |
Hi Andy, On Mon, Feb 15, 2021 at 03:31:29PM +0200, Andy Shevchenko wrote: > On Mon, Feb 15, 2021 at 01:40:28PM +0200, Sakari Ailus wrote: > > Add a printk modifier %p4cc (for pixel format) for printing V4L2 and DRM > > pixel formats denoted by fourccs. The fourcc encoding is the same for both > > so the same implementation can be used. > > This version I almost like, feel free to add > Reviewed-by: From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > after considering addressing below nit-picks. > > > Suggested-by: Mauro Carvalho Chehab <mchehab@kernel.org> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > Reviewed-by: Petr Mladek <pmladek@suse.com> > > Reviewed-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com> > > --- > > Documentation/core-api/printk-formats.rst | 16 ++++++++++ > > lib/test_printf.c | 17 ++++++++++ > > lib/vsprintf.c | 39 +++++++++++++++++++++++ > > scripts/checkpatch.pl | 6 ++-- > > 4 files changed, 76 insertions(+), 2 deletions(-) > > > > diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst > > index 160e710d992f..da2aa065dc42 100644 > > --- a/Documentation/core-api/printk-formats.rst > > +++ b/Documentation/core-api/printk-formats.rst > > @@ -567,6 +567,22 @@ For printing netdev_features_t. > > > > Passed by reference. > > > > +V4L2 and DRM FourCC code (pixel format) > > +--------------------------------------- > > + > > +:: > > + > > + %p4cc > > + > > +Print a FourCC code used by V4L2 or DRM, including format endianness and > > +its numerical value as hexadecimal. > > + > > +Passed by reference. > > + > > +Examples:: > > + > > + %p4cc BG12 little-endian (0x32314742) > > No examples with spaces / non-printable / non-ascii characters I can sure add an example that has a space. But do you think I really should add an example where invalid information is being printed? > > > + > > Thanks > > ====== > > > > diff --git a/lib/test_printf.c b/lib/test_printf.c > > index 7d60f24240a4..9848510a2786 100644 > > --- a/lib/test_printf.c > > +++ b/lib/test_printf.c > > @@ -647,6 +647,22 @@ static void __init fwnode_pointer(void) > > software_node_unregister_nodes(softnodes); > > } > > > > +static void __init fourcc_pointer(void) > > +{ > > + struct { > > + u32 code; > > + char *str; > > + } const try[] = { > > + { 0x3231564e, "NV12 little-endian (0x3231564e)", }, > > + { 0xb231564e, "NV12 big-endian (0xb231564e)", }, > > + { 0x10111213, ".... little-endian (0x10111213)", }, > > + }; > > + unsigned int i; > > + > > + for (i = 0; i < ARRAY_SIZE(try); i++) > > + test(try[i].str, "%p4cc", &try[i].code); > > +} > > + > > static void __init > > errptr(void) > > { > > @@ -692,6 +708,7 @@ test_pointer(void) > > flags(); > > errptr(); > > fwnode_pointer(); > > + fourcc_pointer(); > > } > > > > static void __init selftest(void) > > diff --git a/lib/vsprintf.c b/lib/vsprintf.c > > index 3b53c73580c5..432b5a2d1e90 100644 > > --- a/lib/vsprintf.c > > +++ b/lib/vsprintf.c > > @@ -1733,6 +1733,42 @@ char *netdev_bits(char *buf, char *end, const void *addr, > > return special_hex_number(buf, end, num, size); > > } > > > > +static noinline_for_stack > > +char *fourcc_string(char *buf, char *end, const u32 *fourcc, > > + struct printf_spec spec, const char *fmt) > > +{ > > + char output[sizeof("1234 little-endian (0x01234567)")]; > > 1234 -> ABCD ? (Or XY12 to be closer to the reality) I count in numbers... albeit the hexadecimal number there starts from zero. I guess both would work though. 0123 would be consistent.
On Mon, Feb 15, 2021 at 03:56:50PM +0200, Sakari Ailus wrote: > On Mon, Feb 15, 2021 at 03:31:29PM +0200, Andy Shevchenko wrote: > > On Mon, Feb 15, 2021 at 01:40:28PM +0200, Sakari Ailus wrote: > > > Add a printk modifier %p4cc (for pixel format) for printing V4L2 and DRM > > > pixel formats denoted by fourccs. The fourcc encoding is the same for both > > > so the same implementation can be used. > > > > This version I almost like, feel free to add > > Reviewed-by: From: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > after considering addressing below nit-picks. > > > +Examples:: > > > + > > > + %p4cc BG12 little-endian (0x32314742) > > > > No examples with spaces / non-printable / non-ascii characters > > I can sure add an example that has a space. But do you think I really > should add an example where invalid information is being printed? I think you have to provide better coverage of what user can get out of this. Perhaps one example with space and non-printable character is enough. > > > + char output[sizeof("1234 little-endian (0x01234567)")]; > > > > 1234 -> ABCD ? (Or XY12 to be closer to the reality) > > I count in numbers... albeit the hexadecimal number there starts from zero. > > I guess both would work though. > > 0123 would be consistent. Since letters can be printed the above is confusing a bit. I think XY12 is closer to the reality than 0123.
On Mon, Feb 15, 2021 at 07:26:55PM +0200, Sakari Ailus wrote: > On Mon, Feb 15, 2021 at 03:41:14PM +0200, Andy Shevchenko wrote: > ... > > > + seq_printf(m, "\t\tuapi: [FB:%d] ", fb ? fb->base.id : 0); > > > + if (fb) > > > + seq_printf(m, "%p4cc", &fb->format->format); > > > + else > > > + seq_puts(m, "n/a"); > > > > > + seq_printf(m, ",0x%llx,%dx%d, visible=%s, src=" DRM_RECT_FP_FMT ", dst=" DRM_RECT_FMT ", rotation=%s\n", > > > > Why not to keep two seq_printf() calls? > > > > if (fb) { > > seq_printf(); > > } else { > > seq_printf(); > > } > > > > ? > > I could, but it'd repeat a lot of the same format string that is very > complicated right now. Therefore I thought it's better to split. It's fine, why not? > Or do you mean seq_printf() vs. seq_puts()? checkpatch.pl (rightly) warns > about it. If it doesn't take run-time parameters, then definitely if (fb) seq_printf(); else seq_puts(); > > > fb ? fb->modifier : 0, > > > fb ? fb->width : 0, fb ? fb->height : 0, > > > plane_visibility(plane_state),
On Tue 2021-02-16 09:37:45, Thomas Zimmermann wrote: > Hi > > Am 15.02.21 um 12:40 schrieb Sakari Ailus: > > Switch DRM drivers from drm_get_format_name() to %p4cc. This gets rid of a > > large number of temporary variables at the same time. > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 5 ++-- > > drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 5 ++-- > > drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 5 ++-- > > drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 5 ++-- > > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 ++-- > > .../arm/display/komeda/komeda_format_caps.h | 11 -------- > > .../arm/display/komeda/komeda_framebuffer.c | 4 +-- > > .../gpu/drm/arm/display/komeda/komeda_plane.c | 6 ++--- > > drivers/gpu/drm/arm/malidp_mw.c | 7 ++---- > > drivers/gpu/drm/drm_atomic.c | 8 ++---- > > drivers/gpu/drm/drm_crtc.c | 7 ++---- > > drivers/gpu/drm/drm_fourcc.c | 25 ------------------- > > drivers/gpu/drm/drm_framebuffer.c | 11 +++----- > > drivers/gpu/drm/drm_mipi_dbi.c | 5 ++-- > > drivers/gpu/drm/drm_plane.c | 8 ++---- > > .../gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 5 ++-- > > drivers/gpu/drm/i915/display/intel_display.c | 14 +++-------- > > .../drm/i915/display/intel_display_debugfs.c | 19 ++++++-------- > > drivers/gpu/drm/i915/display/intel_sprite.c | 6 ++--- > > drivers/gpu/drm/mcde/mcde_display.c | 6 ++--- > > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 6 ++--- > > drivers/gpu/drm/nouveau/nouveau_display.c | 9 +++---- > > drivers/gpu/drm/radeon/atombios_crtc.c | 10 +++----- > > drivers/gpu/drm/sun4i/sun4i_backend.c | 6 ++--- > > drivers/gpu/drm/vkms/vkms_writeback.c | 7 ++---- > > drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 15 +++++------ > > include/drm/drm_fourcc.h | 1 - > > 27 files changed, 64 insertions(+), 157 deletions(-) > > This is a nice patchset. For the driver-related changes: > > Acked-by: Thomas Zimmermann <tzimmermann@suse.de> > > But landing this patch will certainly give us build errors. There are at > least 3 git trees involved: drm-misc-next, amd and i915. I'd expect at least > one of them to have additional changes that still require > drm_get_format_name(). > > IMHO we should remove drm_get_format_name() in a later patch. Please remove > the changes in drm_fourcc.{c,h} from this patch and maybe add a TODO comment > to the declaration that the function is supposed to be removed. > > I would merge the patchset through drm-misc-next. And the final removal > patch during the next cycle. Ok? Sounds like a plan. I am fine with it from the vsprintf side. Best Regards, Petr