Message-ID: <gemjiujyal7jf6gq7vd5wemriqkz7knxmkoe76fgmhhto6xzqu@zgy4r4ynaoe7>
Date: Thu, 10 Jul 2025 09:15:54 +0200
From: Jacopo Mondi <jacopo.mondi@...asonboard.com>
To: Dan Scally <dan.scally@...asonboard.com>
Cc: Jacopo Mondi <jacopo.mondi@...asonboard.com>, 
	Dafna Hirschfeld <dafna@...tmail.com>, Laurent Pinchart <laurent.pinchart@...asonboard.com>, 
	Keke Li <keke.li@...ogic.com>, Mauro Carvalho Chehab <mchehab@...nel.org>, 
	Heiko Stuebner <heiko@...ech.de>, Sakari Ailus <sakari.ailus@...ux.intel.com>, 
	linux-kernel@...r.kernel.org, linux-media@...r.kernel.org, linux-rockchip@...ts.infradead.org, 
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 1/8] media: uapi: Introduce V4L2 extensible params

Hi Dan

On Wed, Jul 09, 2025 at 02:18:07PM +0100, Dan Scally wrote:
> Hi Jacopo
>
> On 09/07/2025 12:53, Jacopo Mondi wrote:
> > Hi Dan,
> >     thanks for the comments
> >
> > On Wed, Jul 09, 2025 at 12:33:17PM +0100, Dan Scally wrote:
> > > Hi Jacopo - thanks for the patches
> > >
> > > On 08/07/2025 11:40, Jacopo Mondi wrote:
> > > > Introduce v4l2-extensible-params.h in the Linux kernel uAPI.
> > > >
> > > > The header defines two types that all drivers that use the extensible
> > > > parameters format for ISP configuration shall use to build their own
> > > > parameters format.
> > > >
> > > > The newly introduce type v4l2_params_block represent the
> > > > header to be prepend to each ISP configuration block and the
> > > > v4l2_params_buffer type represent the base type for the configuration
> > > > parameters buffer.
> > > >
> > > > The newly introduced header is not meant to be used directly by
> > > > applications which should instead use the platform-specific ones.
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@...asonboard.com>
> > > > ---
> > > >    MAINTAINERS                                       |   6 ++
> > > >    include/uapi/linux/media/v4l2-extensible-params.h | 106 ++++++++++++++++++++++
> > > >    2 files changed, 112 insertions(+)
> > > >
> > > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > > index 658543062bba3b7e600699d7271ffc89250ba7e5..49a9329e5fe8874bdbaca13946ea28bd80134cb3 100644
> > > > --- a/MAINTAINERS
> > > > +++ b/MAINTAINERS
> > > > @@ -25968,6 +25968,12 @@ F:	drivers/media/i2c/vd55g1.c
> > > >    F:	drivers/media/i2c/vd56g3.c
> > > >    F:	drivers/media/i2c/vgxy61.c
> > > > +V4L2 EXTENSIBLE PARAMETERS FORMAT
> > > > +M:	Jacopo Mondi <jacopo.mondi@...asonboard.com>
> > > > +L:	linux-media@...r.kernel.org
> > > > +S:	Maintained
> > > > +F:	include/uapi/linux/media/v4l2-extensible-params.h
> > > > +
> > > >    VF610 NAND DRIVER
> > > >    M:	Stefan Agner <stefan@...er.ch>
> > > >    L:	linux-mtd@...ts.infradead.org
> > > > diff --git a/include/uapi/linux/media/v4l2-extensible-params.h b/include/uapi/linux/media/v4l2-extensible-params.h
> > > > new file mode 100644
> > > > index 0000000000000000000000000000000000000000..ed37da433c6b1a34523b6a9befde5c0dee601cfb
> > > > --- /dev/null
> > > > +++ b/include/uapi/linux/media/v4l2-extensible-params.h
> > > > @@ -0,0 +1,106 @@
> > > > +/* SPDX-License-Identifier: ((GPL-2.0+ WITH Linux-syscall-note) OR MIT) */
> > > > +/*
> > > > + * Video4Linux2 extensible configuration parameters base types
> > > > + *
> > > > + * Copyright (C) 2025 Ideas On Board Oy
> > > > + * Author: Jacopo Mondi <jacopo.mondi@...asonboard.com>
> > > > + */
> > > > +
> > > > +#ifndef _UAPI_V4L2_PARAMS_H_
> > > > +#define _UAPI_V4L2_PARAMS_H_
> > > > +
> > > > +#ifndef _UAPI_V4L2_EXTENSIBLE_PARAMS_GUARD_
> > > > +/*
> > > > + * Note: each ISP driver exposes a different uAPI, where the types layout
> > > > + * match (more or less strictly) the hardware registers layout.
> > > > + *
> > > > + * This file defines the base types on which each ISP driver can implement its
> > > > + * own types that define its uAPI.
> > > > + *
> > > > + * This file is not meant to be included directly by applications which shall
> > > > + * instead only include the ISP-specific implementation.
> > > > + */
> > > > +#error "This file should not be included directly by applications"
> > > > +#endif
> > > > +
> > > > +#include <linux/types.h>
> > > > +
> > > > +/**
> > > > + * struct v4l2_params_block - V4L2 extensible parameters block header
> > > struct v4l2_params_block_header would be nicer I think
> > >
> > That's what I had started with :)
> >
> > I'm debated between a longer but more explicative name, or a shorter
> > one.
>
>
> I vote for longer here but only because I think the phrase "block" applies
> more properly to the likes of struct rkisp1_ext_params_bls_config for
> example.

Thanks, I've seen you made a patch to change this!

>
> >
> > > > + *
> > > > + * This structure represents the common part of all the ISP configuration
> > > > + * blocks. Each parameters block shall embed an instance of this structure type
> > > > + * as its first member, followed by the block-specific configuration data. The
> > > > + * driver inspects this common header to discern the block type and its size and
> > > > + * properly handle the block content by casting it to the correct block-specific
> > > > + * type.
> > > > + *
> > > > + * The @type field is one of the values enumerated by each platform-specific ISP
> > > > + * block types which specifies how the data should be interpreted by the driver.
> > > > + * The @size field specifies the size of the parameters block and is used by the
> > > > + * driver for validation purposes.
> > > > + *
> > > > + * The @flags field is a bitmask of platform-specific control flags.
> > > Since we're including flags in this base struct rather than a platform
> > > specific subclass I think perhaps we should centralise some flags (which I
> > > think is supported by the fact that all three implementations share the same
> > > flags so far). Perhaps we could reserve the bottom 8 bits for common flags
> > > (like ENABLE / DISABLE) and validate them centrally, and leave the top 8 for
> > > platform specific flags. I think we could then drop the platform specific
> > > validation for rkisp1 and c3 and just pass null to the helpers, since they
> > > do the same thing.
> > Yes, that's one of the things I was not sure about... if we should
> > centralize flags definition as well or not...
>
>
> I think probably the ability to have both centralised and platform specific ones would be worthwhile
>
> >
> > Knowing that Mali will use the same flags that the two existing
> > implementations already have is a good indication that we can probably
> > centralize at least the ENABLE/DISABLE ones
>
>
> Yeah
>
> >
> > > > + *
> > > > + * Userspace shall never use this type directly but use the platform specific
> > > > + * one with the associated data types.
> > > Why wouldn't userspace just use these directly? I could see why it might be
> > > difficult for the C3 and Rkisp1 which are merged, but for a new
> > > implementation couldn't they just use these objects without bothering to
> > > define their own?
> > >
> > mmm, my thinking was that each driver implementation shall define
> > their own types because I would expect that they will have to define
> > their own meta image format... For v4l2_params_buffer see below, for
> > the blocks it might be totally possible to use these type most
> > probably..
> >
> > > If we end up using these objects directly I think it would be nice to have
> > > the example code block from the platform specific headers documentation here
> > > too.
> > >
> > > > + *
> > > > + * - Rockchip RkISP1: :c:type:`rkisp1_ext_params_block_type`
> > > > + * - Amlogic C3: :c:type:`c3_isp_params_block_type`
> > > > + *
> > > > + * @type: The parameters block type (platform-specific)
> > > > + * @flags: A bitmask of block flags (platform-specific)
> > > > + * @size: Size (in bytes) of the parameters block, including this header
> > > > + */
> > > > +struct v4l2_params_block {
> > > > +	__u16 type;
> > > > +	__u16 flags;
> > > > +	__u32 size;
> > > > +} __attribute__((aligned(8)));
> > > > +
> > > > +/**
> > > > + * struct v4l2_params_buffer - V4L2 extensible parameters configuration
> > > > + *
> > > > + * This struct contains the configuration parameters of the ISP algorithms,
> > > > + * serialized by userspace into a data buffer. Each configuration parameter
> > > > + * block is represented by a block-specific structure which contains a
> > > > + * :c:type:`v4l2_params_block` entry as first member. Userspace populates
> > > > + * the @data buffer with configuration parameters for the blocks that it intends
> > > > + * to configure. As a consequence, the data buffer effective size changes
> > > > + * according to the number of ISP blocks that userspace intends to configure and
> > > > + * is set by userspace in the @data_size field.
> > > > + *
> > > > + * The parameters buffer is versioned by the @version field to allow modifying
> > > > + * and extending its definition. Userspace shall populate the @version field to
> > > > + * inform the driver about the version it intends to use. The driver will parse
> > > > + * and handle the @data buffer according to the data layout specific to the
> > > > + * indicated version and return an error if the desired version is not
> > > > + * supported.
> > > > + *
> > > > + * For each ISP block that userspace wants to configure, a block-specific
> > > > + * structure is appended to the @data buffer, one after the other without gaps
> > > > + * in between nor overlaps. Userspace shall populate the @data_size field with
> > > > + * the effective size, in bytes, of the @data buffer.
> > > > + *
> > > > + * Each ISP driver using the extensible parameters format shall define a
> > > > + * type which is type-convertible to this one, with the difference that the
> > > > + * @data member shall actually a memory buffer of platform-specific size and
> > > > + * not a pointer.
> > > Why not just use this object directly? We could provide a helper in
> > > v4l2-extensible-params.h that calculates the size of the buffer with a given
> > > data array size for the driver's convenience
> > The main reason I thought v4l2_params_buffer cannot be used is because
> > of the flexible-array at the end of the type
> >
> > struct v4l2_params_buffer {
> > 	__u32 version;
> > 	__u32 data_size;
> > 	__u8 data[];
> > };
> >
> > vs
> >
> > struct rkisp1_ext_params_cfg {
> > 	__u32 version;
> > 	__u32 data_size;
> > 	__u8 data[RKISP1_EXT_PARAMS_MAX_SIZE];
> > };
> >
> > I might have missed what you're suggesting here with the helper in
> > v4l2-extensible-params.h :)
>
> So I think a known size is needed to accomodate operations like "memcpy(dst,
> src, sizeof(rkisp1_ext_params_cfg))", but with something like...
>
>
> #define v4l2_params_buffer_size(max_params_size) \
>
>         (offsetof(struct v4l2_params_buffer, data) + max_params_size)
>
>
> then the above operation can be memcpy(dst,
> src, v4l2_params_buffer_size(RKISP1_EXT_PARAMS_MAX_SIZE)) instead

Fine for drivers indeed, my thinking was that we would need to reserve
space for userspace to write configuration blocks in...

However, buffers are allocated by videobuf2 and We could certainly
allocate the appropriate size using the vb2 queue 'buf_struct_size' member...

I can experiment with that indeed, in the meantime I wonder what
maintainers think about this ;)

Thanks
  j

>
>
> Unless I'm missing something that should be enough to drop the driver
> specific struct...it seems to work ok anyway
>
>
> Dan
>
> >
> >
> > >
> > > Thanks
> > >
> > > Dan
> > >
> > > > + *
> > > > + * Userspace shall never use this type directly but use the platform specific
> > > > + * one with the associated data types.
> > > > + *
> > > > + * - Rockchip RkISP1: :c:type:`rkisp1_ext_params_cfg`
> > > > + * - Amlogic C3: :c:type:`c3_isp_params_cfg`
> > > > + *
> > > > + * @version: The parameters buffer version (platform-specific)
> > > > + * @data_size: The configuration data effective size, excluding this header
> > > > + * @data: The configuration data
> > > > + */
> > > > +struct v4l2_params_buffer {
> > > > +	__u32 version;
> > > > +	__u32 data_size;
> > > > +	__u8 data[];
> > > > +};
> > > > +
> > > > +#endif /* _UAPI_V4L2_PARAMS_H_ */
> > > >

Powered by blists - more mailing lists