| 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
Hi all, This set adds support for %p4cc printk modifier for printing V4L2 and DRM fourcc codes. The codes are cumbersome to print manually and by adding the modifier, this task is saved from the V4L2 and DRM frameworks as well as related drivers. DRM actually had it handled in a way (see 3rd patch) but the printk modifier makes printing the format easier even there. On V4L2 side it saves quite a few lines of repeating different implementations of printing the 4cc codes. Further work will include converting the V4L2 drivers doing the same, as well as converting DRM drivers from drm_get_format_name() to plain %p4cc. I left these out from this version since individual drivers are easier changed without dealing with multiple trees. If DRM folks would prefer to convert drivers to %p4cc directly instead I have no problem dropping the 3rd patch. Nearly all uses in DRM are in printk family of functions that can readily use %p4cc instead of the current arrangement that relies on caller-allocated temporary buffer. Since v6: - Don't drop spaces in fourcc codes. - Print unprintable characters as dot ('.') instead of hexadecimal number in parentheses. - Convert DRM from drm_get_format_name() to %p4cc. I wonder if this should be merged through the DRM tree, albeit it's probably unlikely to conflict with other changes. Further use of the function could be a problem. - Make tests more realistic. Since v5: - Added V4L2 core conversion to %p4cc, as well as change the DRM fourcc printing function to use %p4cc. - Add missing checkpatch.pl checks for %p4cc modifier. Sakari Ailus (3): lib/vsprintf: Add support for printing V4L2 and DRM fourccs v4l: ioctl: Use %p4cc printk modifier to print FourCC codes drm: Switch to %p4cc format modifier Documentation/core-api/printk-formats.rst | 16 ++++ 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 ++-- drivers/media/v4l2-core/v4l2-ioctl.c | 85 +++++-------------- include/drm/drm_fourcc.h | 1 - lib/test_printf.c | 17 ++++ lib/vsprintf.c | 39 +++++++++ scripts/checkpatch.pl | 6 +- 32 files changed, 161 insertions(+), 223 deletions(-)