Re: [PATCH v3 8/9] media: platform: broadcom: Add bcm2835-isp driver

[Jai Luthra]

Hi Paul,

Quoting Paul Elder (2026-09-04 14:47:39)
> Hi Jai,
> 
> Thanks for the patch.
> 

Thank you for the detailed review.

> Quoting Jai Luthra (2026-07-17 20:04:23)
> > From: Naushir Patuck <naush@xxxxxxxxxxxxxxx>
> > 
> > The BCM2835 ISP is a fixed-function hardware block that performs image
> > processing on Bayer, RGB, and YUV frames. It produces two processed
> > video outputs at different resolutions and can generate statistics for
> > Bayer inputs. The hardware is not accessible directly, so this driver
> > programs it through the VCHIQ MMAL interface.
> > 
> > As the topology cannot be represented as a simple V4L2 M2M device, we
> > register a media device with 5 video nodes: one output (sink), two
> > capture (processed frames), one metadata capture (statistics), and one
> > metadata output (config parameters). The VCHIQ firmware supports two
> > concurrent users, so instantiate two identical media graphs.
> > 
> > ISP configuration uses the V4L2 extensible parameters framework through
> > the dedicated params node, allowing userspace to batch multiple
> > parameter blocks (black level, gamma, CCM, lens shading, etc.) in a
> > single buffer that is applied on the next frame boundary.
> > 
> > Signed-off-by: Naushir Patuck <naush@xxxxxxxxxxxxxxx>
> > Co-developed-by: Jai Luthra <jai.luthra@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Jai Luthra <jai.luthra@xxxxxxxxxxxxxxxx>
> > ---
> > Changes in v3:
> > - Rebase on v7.2-rc1
> > - Drop redundant buffer list and locks for params (we send them
> >   immediately to firmware on queue)
> > - Drop .stop_streaming() for params node
> > - Minor cleanups
> > ---

[...]

> > diff --git a/Documentation/userspace-api/media/v4l/metafmt-bcm2835-isp.rst b/Documentation/userspace-api/media/v4l/metafmt-bcm2835-isp.rst
> > new file mode 100644
> > index 000000000000..6ed1d1e7890f
> > --- /dev/null
> > +++ b/Documentation/userspace-api/media/v4l/metafmt-bcm2835-isp.rst
> > @@ -0,0 +1,101 @@
> > +.. SPDX-License-Identifier: GPL-2.0
> > +.. c:namespace:: V4L
> > +
> > +***********************************************************************************
> > +V4L2_META_FMT_BCM2835_ISP_STATS ('BSTA'), V4L2_META_FMT_BCM2835_ISP_PARAMS ('BCMP')
> > +***********************************************************************************
> > +
> > +.. _v4l2-meta-fmt-bcm2835-isp-stats:
> > +
> > +BCM2835 ISP Statistics
> > +======================
> > +
> > +The BCM2835 ISP hardware calculate image statistics for an input Bayer frame.
> > +These statistics are obtained from the "bcm2835-isp0-capture3" device node
> > +using the :c:type:`v4l2_meta_format` interface. They are formatted as described
> > +by the :c:type:`bcm2835_isp_stats` structure below.
> 
> Since this is a new upstream driver, I personally would've preferred extensible
> stats. Is there a reason for preferring nonextensible stats?
> 

Yes, the VC4 firmware-based ISP populates the statistics struct and this
driver passes it to the userspace without modifications. 

The current layout is already "extensible" i.e. it does have u32 fields for
version and size, but it doesn't use blocks with headers like v4l2-isp.h
expects.

The ISP features are mostly stable and unlikely to change, but in the
unlikely case that we do end up requiring an extension to the stats, it
should be possible to switch to v4l2-isp stat blocks at that point.

For now, I prefered avoiding firmware changes.

[...]

> > +struct bcm2835_isp_params_buffer {
> > +       struct vb2_v4l2_buffer vb;
> > +       struct list_head list;
> > +       void *config;
> > +};
> > +
> > +#define to_bcm2835_isp_params_buf(vbuf) \
> > +       container_of(vbuf, struct bcm2835_isp_params_buffer, vb)
> > +
> > +static int isp_set_param(struct bcm2835_isp_params *params, u32 parameter, void
> 
> I'd put void on the next line.
> 

Oops, will do.

> > +                        *value, u32 value_size)
> > +{
> > +       return vchiq_mmal_port_parameter_set(params->mmal_instance, params->port,
> > +                                            parameter, value, value_size);
> > +}
> > +
> > +static int map_ls_table(struct bcm2835_isp_params *params,
> > +                       struct dma_buf *dmabuf,
> > +                       const struct bcm2835_isp_lens_shading *v4l2_ls)
> > +{
> > +       void *vcsm_handle;
> > +       int ret;
> > +
> > +       if (IS_ERR_OR_NULL(dmabuf))
> > +               return -EINVAL;
> 
> Isn't this already checked for in the sole caller
> bcm2835_isp_params_lens_shading()?
> 

Indeed, I've reworked a lot of this in the next revision, along with
integrating Dave's helpful patches that batch together parameter updates to
the firmware in a single IPC message, saving multiple milliseconds.

> > +
> > +       /*
> > +        * struct bcm2835_isp_lens_shading and struct
> > +        * mmal_parameter_lens_shading_v2 match so that we can do a
> > +        * simple memcpy here.
> 
> I was a bit concerned about manually synchronizing these structs but I guess
> neither is expected to change I suppose it's ok.
> 
> > +        * Only the dmabuf to the actual table needs any manipulation.
> > +        */
> > +       memcpy(&params->ls, v4l2_ls, sizeof(params->ls));
> > +       ret = vc_sm_cma_import_dmabuf(dmabuf, &vcsm_handle);
> > +       if (ret) {
> > +               dma_buf_put(dmabuf);
> 
> Wouldn't this result in a double dma_buf_put? afaict it's already put in the
> error-handling path of vc_sm_cma_import_dmabuf_internal.
> 

Good catch, will drop.

> > +               return ret;
> > +       }
> > +
> > +       params->ls.mem_handle_table = vc_sm_cma_int_handle(vcsm_handle);
> > +       params->last_ls_dmabuf = dmabuf;
> > +
> > +       vc_sm_cma_free(vcsm_handle);
> > +
> > +       return 0;
> > +}

[...]

> > +struct bcm2835_isp_params *
> > +bcm2835_isp_params_register(struct v4l2_device *v4l2_dev, struct device *dev,
> > +                           struct vchiq_mmal_instance *mmal_instance,
> > +                           struct vchiq_mmal_port *port, int video_nr)
> > +{
> > +       struct bcm2835_isp_params *params;
> > +       struct video_device *vdev;
> > +       struct vb2_queue *q;
> > +       int ret;
> > +
> > +       params = devm_kzalloc(dev, sizeof(*params), GFP_KERNEL);
> > +       if (!params)
> > +               return ERR_PTR(-ENOMEM);
> > +
> > +       params->dev = dev;
> > +       params->v4l2_dev = v4l2_dev;
> > +       params->mmal_instance = mmal_instance;
> > +       params->port = port;
> > +
> > +       mutex_init(&params->lock);
> > +
> > +       /* Initialize vb2 queue */
> > +       q = &params->queue;
> > +       q->type = V4L2_BUF_TYPE_META_OUTPUT;
> > +       q->io_modes = VB2_MMAP | VB2_DMABUF;
> > +       q->drv_priv = params;
> > +       q->ops = &bcm2835_isp_params_vb2_ops;
> > +       q->mem_ops = &vb2_dma_contig_memops;
> > +       q->buf_struct_size = sizeof(struct bcm2835_isp_params_buffer);
> > +       q->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_COPY;
> > +       q->dev = dev;
> > +       q->lock = &params->lock;
> > +
> > +       ret = vb2_queue_init(q);
> > +       if (ret) {
> > +               dev_err(dev, "Failed to init params vb2 queue\n");
> > +               return ERR_PTR(ret);
> 
> Do you need to destroy the mutex here (and in the below error handling too)?
> 

Yikes.. will do.

> > +       }
> > +

[...]

> > +#define BCM2835_ISP_NAME "bcm2835-isp"
> > +#define BCM2835_ISP_ENTITY_NAME_LEN 32
> > +
> > +#define BCM2835_ISP_NUM_OUTPUTS 1
> > +#define BCM2835_ISP_NUM_CAPTURES 2
> > +#define BCM2835_ISP_NUM_METADATA 1
> > +#define BCM2835_ISP_NUM_PARAMS 1
> > +
> > +#define BCM2835_ISP_NUM_NODES                                          \
> > +               (BCM2835_ISP_NUM_OUTPUTS + BCM2835_ISP_NUM_CAPTURES +   \
> > +                BCM2835_ISP_NUM_METADATA)
> > +#define BCM2835_ISP_PARAMS_PAD BCM2835_ISP_NUM_NODES
> > +#define BCM2835_ISP_NUM_ENTITY_PADS                                    \
> > +               (BCM2835_ISP_NUM_NODES + BCM2835_ISP_NUM_PARAMS)
> 
> I was wondering why params is separate but I see it's a special node. 
> 

Yes, the other videos nodes are "ports" on the firmware, directly dealing
with the video/metadata buffers. The parameter buffers otoh are completely
ingested by this driver as the firmware API for configuring the ISP is
through IPC (MMAL) messages on the video ingest port.

[...]

> > +static int populate_qdata_fmt(struct v4l2_format *f,
> > +                             struct bcm2835_isp_node *node)
> > +{
> > +       struct bcm2835_isp_dev *dev = node_get_dev(node);
> > +       struct bcm2835_isp_q_data *q_data = &node->q_data;
> > +       int ret;
> > +
> > +       if (!node_is_stats(node)) {
> > +               v4l2_dbg(1, debug, &dev->v4l2_dev,
> > +                        "%s: Setting pix format for type %d, wxh: %ux%u, fmt: %08x, size %u\n",
> > +                        __func__, f->type, f->fmt.pix.width, f->fmt.pix.height,
> > +                        f->fmt.pix.pixelformat, f->fmt.pix.sizeimage);
> > +
> > +               q_data->fmt = find_format(f, node);
> > +               q_data->width = f->fmt.pix.width;
> > +               q_data->height = f->fmt.pix.height;
> > +
> > +               /* All parameters should have been set correctly by try_fmt */
> > +               q_data->bytesperline = f->fmt.pix.bytesperline;
> > +               q_data->sizeimage = f->fmt.pix.sizeimage;
> > +               q_data->ycbcr_enc = f->fmt.pix.ycbcr_enc;
> > +               q_data->xfer_func = f->fmt.pix.xfer_func;
> > +               q_data->quantization = f->fmt.pix.quantization;
> > +
> > +               /* We must indicate which of the allowed colour spaces we have. */
> > +               q_data->colorspace = f->fmt.pix.colorspace;
> > +       } else {
> > +               v4l2_dbg(1, debug, &dev->v4l2_dev,
> > +                        "%s: Setting meta format for fmt: %08x, size %u\n",
> > +                        __func__, f->fmt.meta.dataformat,
> > +                        f->fmt.meta.buffersize);
> > +
> > +               q_data->fmt = find_format(f, node);
> > +               q_data->width = 0;
> > +               q_data->height = 0;
> > +               q_data->bytesperline = 0;
> > +               q_data->sizeimage = f->fmt.meta.buffersize;
> > +
> > +               /* This won't mean anything for metadata, but may as well fill it in. */
> > +               q_data->colorspace = V4L2_COLORSPACE_DEFAULT;
> > +       }
> > +
> > +       v4l2_dbg(1, debug, &dev->v4l2_dev,
> > +                "%s: Calculated bpl as %u, size %u\n", __func__,
> > +                q_data->bytesperline, q_data->sizeimage);
> > +
> > +       setup_mmal_port_format(node, node->port);
> > +       ret = vchiq_mmal_port_set_format(dev->mmal_instance, node->port);
> > +       if (ret) {
> > +               v4l2_err(&dev->v4l2_dev,
> > +                        "%s: Failed vchiq_mmal_port_set_format on port, ret %d\n",
> > +                        __func__, ret);
> > +               ret = -EINVAL;
> > +       }
> > +
> > +       if (q_data->sizeimage < node->port->minimum_buffer.size) {
> > +               v4l2_err(&dev->v4l2_dev,
> > +                        "%s: Current buffer size of %u < min buf size %u - driver mismatch to MMAL\n",
> > +                        __func__,
> > +                        q_data->sizeimage,
> > +                        node->port->minimum_buffer.size);
> 
> Will this cause any serious problems or will the stream just kind of not
> stream?
> 

That's a good question, I'm afraid I don't understand the firmware side
enough to answer it, so I'll defer it to Dave or Naush.

>From my testing with different resolutions, the firmware always returned
minimum_buffer.size == sizeimage.

I will add a ret = -EINVAL here in the next revision so this error
propagates to the userspace when they call S_FMT.

While we usually want to avoid errors on S_FMT for size issues that could
be updated by the driver, in this case a failure likely means something is
broken with the firmware, similar to an error with
vchiq_mmal_port_set_format() above.

> > +       }
> > +
> > +       v4l2_dbg(1, debug, &dev->v4l2_dev,
> > +                "%s: Set format for type %d, wxh: %dx%d, fmt: %08x, size %u\n",
> > +                __func__, f->type, q_data->width, q_data->height,
> > +                q_data->fmt->fourcc, q_data->sizeimage);
> > +
> > +       return ret;
> > +}
> > +
> > +int bcm2835_isp_node_querycap(struct file *file, void *priv,
> > +                             struct v4l2_capability *cap)
> > +{
> > +       strscpy(cap->driver, BCM2835_ISP_NAME, sizeof(cap->driver));
> > +       strscpy(cap->card, BCM2835_ISP_NAME, sizeof(cap->card));
> > +       snprintf(cap->bus_info, sizeof(cap->bus_info), "platform:%s",
> > +                BCM2835_ISP_NAME);
> > +
> > +       return 0;
> > +}
> > +
> > +static int bcm2835_isp_node_g_fmt(struct file *file, void *priv,
> > +                                 struct v4l2_format *f)
> > +{
> > +       struct bcm2835_isp_node *node = video_drvdata(file);
> > +
> > +       if (f->type != node->queue.type)
> > +               return -EINVAL;
> > +
> > +       if (node_is_stats(node)) {
> > +               f->fmt.meta.dataformat = V4L2_META_FMT_BCM2835_ISP_STATS;
> > +               f->fmt.meta.buffersize =
> > +                       node->port->minimum_buffer.size;
> > +       } else {
> > +               struct bcm2835_isp_q_data *q_data = &node->q_data;
> > +
> > +               f->fmt.pix.width = q_data->width;
> > +               f->fmt.pix.height = q_data->height;
> > +               f->fmt.pix.field = V4L2_FIELD_NONE;
> > +               f->fmt.pix.pixelformat = q_data->fmt->fourcc;
> > +               f->fmt.pix.bytesperline = q_data->bytesperline;
> > +               f->fmt.pix.sizeimage = q_data->sizeimage;
> > +               f->fmt.pix.colorspace = q_data->colorspace;
> > +               f->fmt.pix.ycbcr_enc = q_data->ycbcr_enc;
> > +               f->fmt.pix.xfer_func = q_data->xfer_func;
> > +               f->fmt.pix.quantization = q_data->quantization;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static int bcm2835_isp_node_enum_fmt(struct file *file, void  *priv,
> 
> s/  / /
> 

Argh, will fix.

> > +                                    struct v4l2_fmtdesc *f)
> > +{
> > +       struct bcm2835_isp_node *node = video_drvdata(file);
> > +
> > +       if (f->type != node->queue.type)
> > +               return -EINVAL;
> > +
> > +       if (f->index < node->num_supported_fmts) {
> > +               /* Format found */
> > +               f->pixelformat = node->supported_fmts[f->index]->fourcc;
> > +               f->flags = 0;
> > +               return 0;
> > +       }
> > +
> > +       return -EINVAL;
> > +}
> > +
> > +static int bcm2835_isp_enum_framesizes(struct file *file, void *priv,
> > +                                      struct v4l2_frmsizeenum *fsize)
> > +{
> > +       struct bcm2835_isp_node *node = video_drvdata(file);
> > +       struct bcm2835_isp_dev *dev = node_get_dev(node);
> > +       const struct bcm2835_isp_fmt *fmt;
> > +
> > +       if (node_is_stats(node) || fsize->index)
> > +               return -EINVAL;
> > +
> > +       fmt = find_format_by_fourcc(fsize->pixel_format, node);
> > +       if (!fmt) {
> > +               v4l2_err(&dev->v4l2_dev, "Invalid pixel code: %x\n",
> > +                        fsize->pixel_format);
> > +               return -EINVAL;
> > +       }
> > +
> > +       fsize->type = V4L2_FRMSIZE_TYPE_STEPWISE;
> > +       fsize->stepwise.min_width = MIN_DIM;
> > +       fsize->stepwise.max_width = MAX_DIM;
> > +       fsize->stepwise.step_width = fmt->step_size;
> > +
> > +       fsize->stepwise.min_height = MIN_DIM;
> > +       fsize->stepwise.max_height = MAX_DIM;
> > +       fsize->stepwise.step_height = fmt->step_size;
> > +
> > +       return 0;
> > +}
> > +
> > +static int bcm2835_isp_node_try_fmt(struct file *file, void *priv,
> > +                                   struct v4l2_format *f)
> > +{
> > +       struct bcm2835_isp_node *node = video_drvdata(file);
> > +       const struct bcm2835_isp_fmt *fmt;
> > +
> > +       if (f->type != node->queue.type)
> > +               return -EINVAL;
> > +
> > +       fmt = find_format(f, node);
> > +       if (!fmt)
> > +               fmt = get_default_format(node);
> > +
> > +       if (!node_is_stats(node)) {
> > +               bool is_rgb;
> > +
> > +               f->fmt.pix.width = max(min(f->fmt.pix.width, MAX_DIM),
> > +                                      MIN_DIM);
> > +               f->fmt.pix.height = max(min(f->fmt.pix.height, MAX_DIM),
> > +                                       MIN_DIM);
> 
> Why not clamp_t?
> 

Indeed, switching to it now.

> > +
> > +               f->fmt.pix.pixelformat = fmt->fourcc;
> > +

[...]

> > diff --git a/include/uapi/linux/bcm2835-isp.h b/include/uapi/linux/bcm2835-isp.h
> > new file mode 100644
> > index 000000000000..85b7f56edf5a
> > --- /dev/null
> > +++ b/include/uapi/linux/bcm2835-isp.h
> > @@ -0,0 +1,530 @@
> > +/* SPDX-License-Identifier: ((GPL-2.0+ WITH Linux-syscall-note) OR BSD-3-Clause) */
> > +/*
> > + * bcm2835-isp.h
> > + *
> > + * BCM2835 ISP driver - user space header file.
> > + *
> > + * Copyright © 2019-2026 Raspberry Pi (Trading) Ltd.
> > + *
> > + * Author: Naushir Patuck (naush@xxxxxxxxxxxxxxx)
> > + *
> > + */
> > +
> > +#ifndef __BCM2835_ISP_H_
> > +#define __BCM2835_ISP_H_
> > +
> > +#include <linux/media/v4l2-isp.h>
> > +
> > +/*
> > + * All structs below are directly mapped onto the equivalent structs in
> > + * drivers/staging/vc04_services/vchiq-mmal/mmal-parameters.h
> 
> This path probably needs to be updated.
> 

Will do.

> > + * for convenience.
> > + */
> > +
> > +/**
> > + * struct bcm2835_isp_rational - Rational value type.
> > + *
> > + * @num:       Numerator.
> > + * @den:       Denominator.
> > + */
> > +struct bcm2835_isp_rational {
> > +       __s32 num;
> > +       __u32 den;
> > +};
> > +
> > +/**
> > + * struct bcm2835_isp_ccm - Colour correction matrix.
> > + *
> > + * @ccm:       3x3 correction matrix coefficients.
> > + * @offsets:   1x3 correction offsets.
> > + */
> > +struct bcm2835_isp_ccm {
> > +       struct bcm2835_isp_rational ccm[3][3];
> > +       __s32 offsets[3];
> > +};
> > +
> > +/**
> > + * struct bcm2835_isp_custom_ccm - Custom CCM configuration.
> > + *
> > + * @enabled:   Enable custom CCM.
> > + * @ccm:       Custom CCM coefficients and offsets.
> > + */
> > +struct bcm2835_isp_custom_ccm {
> > +       __u32 enabled;
> > +       struct bcm2835_isp_ccm ccm;
> > +};
> > +
> > +/**
> > + * enum bcm2835_isp_gain_format - format of the gains in the lens shading
> > + *                               tables.
> > + *
> > + * @GAIN_FORMAT_U0P8_1:                Gains are u0.8 format, starting at 1.0
> > + * @GAIN_FORMAT_U1P7_0:                Gains are u1.7 format, starting at 0.0
> > + * @GAIN_FORMAT_U1P7_1:                Gains are u1.7 format, starting at 1.0
> > + * @GAIN_FORMAT_U2P6_0:                Gains are u2.6 format, starting at 0.0
> > + * @GAIN_FORMAT_U2P6_1:                Gains are u2.6 format, starting at 1.0
> > + * @GAIN_FORMAT_U3P5_0:                Gains are u3.5 format, starting at 0.0
> > + * @GAIN_FORMAT_U3P5_1:                Gains are u3.5 format, starting at 1.0
> > + * @GAIN_FORMAT_U4P10:         Gains are u4.10 format, starting at 0.0
> > + */
> > +enum bcm2835_isp_gain_format {
> > +       GAIN_FORMAT_U0P8_1 = 0,
> > +       GAIN_FORMAT_U1P7_0 = 1,
> > +       GAIN_FORMAT_U1P7_1 = 2,
> > +       GAIN_FORMAT_U2P6_0 = 3,
> > +       GAIN_FORMAT_U2P6_1 = 4,
> > +       GAIN_FORMAT_U3P5_0 = 5,
> > +       GAIN_FORMAT_U3P5_1 = 6,
> > +       GAIN_FORMAT_U4P10  = 7,
> > +};
> > +
> > +/**
> > + * struct bcm2835_isp_lens_shading - Lens shading tables.
> > + *
> > + * @enabled:           Enable lens shading.
> > + * @grid_cell_size:    Size of grid cells in samples (16, 32, 64, 128 or 256).
> > + * @grid_width:                Width of lens shading tables in grid cells.
> > + * @grid_stride:       Row to row distance (in grid cells) between grid cells
> > + *                     in the same horizontal location.
> > + * @grid_height:       Height of lens shading tables in grid cells.
> > + * @dmabuf:            dmabuf file handle containing the table.
> > + * @ref_transform:     Reference transform - unsupported, please pass zero.
> 
> If this is unsupported, why is it exposed? Is there a possible future where
> this will be used?
> 

Ah, this is mostly for backward compatibility. More on this just below.

> > + * @corner_sampled:    Whether the gains are sampled at the corner points
> > + *                     of the grid cells or in the cell centres.
> > + * @gain_format:       Format of the gains (see enum &bcm2835_isp_gain_format).
> > + */
> > +struct bcm2835_isp_lens_shading {
> > +       __u32 enabled;
> > +       __u32 grid_cell_size;
> > +       __u32 grid_width;
> > +       __u32 grid_stride;
> > +       __u32 grid_height;
> > +       __s32 dmabuf;
> > +       __u32 ref_transform;
> > +       __u32 corner_sampled;
> > +       __u32 gain_format;
> > +};

[...]

> > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> > index eda4492e40dc..f691a0d4e119 100644
> > --- a/include/uapi/linux/videodev2.h
> > +++ b/include/uapi/linux/videodev2.h
> > @@ -870,6 +870,8 @@ struct v4l2_pix_format {
> >  #define V4L2_META_FMT_D4XX        v4l2_fourcc('D', '4', 'X', 'X') /* D4XX Payload Header metadata */
> >  #define V4L2_META_FMT_UVC_MSXU_1_5  v4l2_fourcc('U', 'V', 'C', 'M') /* UVC MSXU metadata */
> >  #define V4L2_META_FMT_VIVID      v4l2_fourcc('V', 'I', 'V', 'D') /* Vivid Metadata */
> > +#define V4L2_META_FMT_BCM2835_ISP_STATS v4l2_fourcc('B', 'S', 'T', 'A') /* BCM2835 ISP stats */
> 
> This is kind of bikeshedding but why not BCMS?
> 

The downstream used BSTA instead. I debated whether I should call the
params one BPAR, but that sounded likely to conflict with something in
future.

The initial idea was to not add any breaking changes to the uAPI compared
to the downstream driver for backward compatibility, given Raspberry Pis
are widely used. That's of course not possible since we've switched to
using parameter buffers (a much cleaner solution IMO) instead of custom
V4L2 controls for the ISP configuration.

But it is still possible for the downstream tree to expose both the V4L2
controls along with parameter buffers if they think that might ease
transition for their users. I've split my libcamera series for param
buffers into modular patches for the same reason.

Tiny tweaks to the uAPI like the FOURCC here, or updating
s/ref_transform/reserved in the LS struct are trivial things that might
make that cumbersome or even impossible, so I wanted to avoid those.

> 
> Otherwise this is looking pretty good imo.

Thanks again for your review and tests.

--
Jai

> 
> 
> Paul
> 
> > +#define V4L2_META_FMT_BCM2835_ISP_PARAMS v4l2_fourcc('B', 'C', 'M', 'P') /* BCM2835 ISP params */
> >  
> >  /* Vendor specific - used for RK_ISP1 camera sub-system */
> >  #define V4L2_META_FMT_RK_ISP1_PARAMS   v4l2_fourcc('R', 'K', '1', 'P') /* Rockchip ISP1 3A Parameters */
> > 
> > -- 
> > 2.54.0
> > 
> >