Hi Laurent
On Wed, Jun 19, 2024 at 06:44:20PM GMT, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Wed, Jun 19, 2024 at 04:22:00PM +0200, Jacopo Mondi wrote:
> > On Wed, Jun 12, 2024 at 07:20:48PM GMT, Laurent Pinchart wrote:
> > > On Wed, Jun 12, 2024 at 03:28:05PM +0100, Daniel Scally wrote:
> > > > Hi Jacopo. As mentioned in the last patch, I think that this could be
> > > > squashed into 5/8, but a couple of comments below
> > >
> > > I think it should be moved earlier, yes, probably even before the
> > > introduction of extended parameters.
> >
> > I'm not sure I agree here
> >
> > Squashing would create a patch that does too many things. Moving it
> > before the introduction of the extensible format would not make much
> > sense, as there would be nothing to validate/copy for the fixed
> > format.
>
> I think the copy should be done for the fixed format too. See below.
>
> > I'll keep the ordering I have here if not a big deal.
> >
> > > > On 05/06/2024 17:54, Jacopo Mondi wrote:
> > > > > With the introduction of the extensible parameters format support in the
> > > > > rkisp1-param.c module, the RkISP1 driver now configures the ISP blocks
> > > > > by parsing the content of a data buffer of variable size provided by
> > > > > userspace through the V4L2 meta-output interface using the MMAP memory
> > > > > handling mode.
> > > > >
> > > > > As the parameters buffer is mapped in the userspace process memory,
> > > > > applications have access to the buffer content while the driver
> > > > > parses it.
> > > > >
> > > > > To prevent potential issues during the parameters buffer parsing and
> > > > > processing in the driver, implement three vb2_ops to
> > > > >
> > > > > 1) allocate a scratch buffer in the driver private buffer structure
> > > > > 2) validate the buffer content at VIDIOC_QBUF time
> > > > > 3) copy the content of the user provided configuration parameters
> > > > > in the driver-private scratch buffer
> > > > >
> > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx>
> > > > > ---
> > > > > .../platform/rockchip/rkisp1/rkisp1-params.c | 154 ++++++++++++++----
> > > > > 1 file changed, 124 insertions(+), 30 deletions(-)
> > > > >
> > > > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> > > > > index 4adaf084ce6e..003239e14511 100644
> > > > > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> > > > > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> > > > > @@ -5,6 +5,8 @@
> > > > > * Copyright (C) 2017 Rockchip Electronics Co., Ltd.
> > > > > */
> > > > >
> > > > > +#include <linux/string.h>
> > > > > +
> > > > > #include <media/v4l2-common.h>
> > > > > #include <media/v4l2-event.h>
> > > > > #include <media/v4l2-ioctl.h>
> > > > > @@ -1943,17 +1945,14 @@ static const struct rkisp1_ext_params_handler {
> > > > > };
> > > > >
> > > > > static int __rkisp1_ext_params_config(struct rkisp1_params *params,
> > > > > - struct rkisp1_ext_params_cfg *cfg,
> > > > > + struct rkisp1_params_buffer *buffer,
> > > > > u32 block_group_mask)
> > > > > {
> > > > > + struct rkisp1_ext_params_cfg *cfg = buffer->cfg;
> > >
> > > Maybe do this in the callers to avoid changing the prototype of this
> > > function and the other functions below.
> > >
> > > > > size_t block_offset = 0;
> > > > >
> > > > > - if (cfg->total_size > RKISP1_EXT_PARAMS_MAX_SIZE) {
> > > > > - dev_dbg(params->rkisp1->dev,
> > > > > - "Invalid parameters buffer size %llu\n",
> > > > > - cfg->total_size);
> > > > > - return -EINVAL;
> > > > > - }
> > > > > + if (WARN_ON(!cfg))
> > > > > + return -ENOMEM;
> > > > >
> > > > > /* Walk the list of parameter blocks and process them. */
> > > > > while (block_offset < cfg->total_size) {
> > > > > @@ -1965,25 +1964,13 @@ static int __rkisp1_ext_params_config(struct rkisp1_params *params,
> > > > > block_offset += block->size;
> > > > >
> > > > > /*
> > > > > - * Validate the block id and make sure the block group is in
> > > > > - * the list of groups to configure.
> > > > > + * Make sure the block group is in the list of groups to
> > > > > + * configure.
> > > > > */
> > > > > - if (block->type >= RKISP1_EXT_PARAMS_BLOCK_TYPE_SENTINEL) {
> > > > > - dev_dbg(params->rkisp1->dev,
> > > > > - "Invalid parameters block type\n");
> > > > > - return -EINVAL;
> > > > > - }
> > > > > -
> > > > > block_handler = &rkisp1_ext_params_handlers[block->type];
> > > > > if (!(block_handler->group & block_group_mask))
> > > > > continue;
> > > > >
> > > > > - if (block->size != block_handler->size) {
> > > > > - dev_dbg(params->rkisp1->dev,
> > > > > - "Invalid parameters block size\n");
> > > > > - return -EINVAL;
> > > > > - }
> > > > > -
> > > > > block_handler->handler(params, block);
> > > > > }
> > > > >
> > > > > @@ -1991,9 +1978,9 @@ static int __rkisp1_ext_params_config(struct rkisp1_params *params,
> > > > > }
> > > > >
> > > > > static int rkisp1_ext_params_config(struct rkisp1_params *params,
> > > > > - struct rkisp1_ext_params_cfg *cfg)
> > > > > + struct rkisp1_params_buffer *buffer)
> > > > > {
> > > > > - return __rkisp1_ext_params_config(params, cfg,
> > > > > + return __rkisp1_ext_params_config(params, buffer,
> > > > > RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS |
> > > > > RKISP1_EXT_PARAMS_BLOCK_GROUP_LSC |
> > > > > RKISP1_EXT_PARAMS_BLOCK_GROUP_MEAS);
> > > > > @@ -2001,17 +1988,17 @@ static int rkisp1_ext_params_config(struct rkisp1_params *params,
> > > > >
> > > > > static int
> > > > > rkisp1_ext_params_other_meas_config(struct rkisp1_params *params,
> > > > > - struct rkisp1_ext_params_cfg *cfg)
> > > > > + struct rkisp1_params_buffer *buffer)
> > > > > {
> > > > > - return __rkisp1_ext_params_config(params, cfg,
> > > > > + return __rkisp1_ext_params_config(params, buffer,
> > > > > RKISP1_EXT_PARAMS_BLOCK_GROUP_OTHERS |
> > > > > RKISP1_EXT_PARAMS_BLOCK_GROUP_MEAS);
> > > > > }
> > > > >
> > > > > static int rkisp1_ext_params_lsc_config(struct rkisp1_params *params,
> > > > > - struct rkisp1_ext_params_cfg *cfg)
> > > > > + struct rkisp1_params_buffer *buffer)
> > > > > {
> > > > > - return __rkisp1_ext_params_config(params, cfg,
> > > > > + return __rkisp1_ext_params_config(params, buffer,
> > > > > RKISP1_EXT_PARAMS_BLOCK_GROUP_LSC);
> > > > > }
> > > > >
> > > > > @@ -2057,7 +2044,7 @@ void rkisp1_params_isr(struct rkisp1_device *rkisp1)
> > > > > rkisp1_isp_isr_lsc_config(params, cfg);
> > > > > rkisp1_isp_isr_meas_config(params, cfg);
> > > > > } else {
> > > > > - ret = rkisp1_ext_params_config(params, cfg);
> > > > > + ret = rkisp1_ext_params_config(params, buf);
> > > > > }
> > > > >
> > > > > if (ret)
> > > > > @@ -2168,7 +2155,7 @@ int rkisp1_params_pre_configure(struct rkisp1_params *params,
> > > > > rkisp1_isp_isr_other_config(params, cfg);
> > > > > rkisp1_isp_isr_meas_config(params, cfg);
> > > > > } else {
> > > > > - ret = rkisp1_ext_params_other_meas_config(params, cfg);
> > > > > + ret = rkisp1_ext_params_other_meas_config(params, buf);
> > > > > }
> > > > >
> > > > > if (ret) {
> > > > > @@ -2215,7 +2202,7 @@ int rkisp1_params_post_configure(struct rkisp1_params *params)
> > > > > if (params->metafmt.dataformat == V4L2_META_FMT_RK_ISP1_PARAMS)
> > > > > rkisp1_isp_isr_lsc_config(params, cfg);
> > > > > else
> > > > > - ret = rkisp1_ext_params_lsc_config(params, cfg);
> > > > > + ret = rkisp1_ext_params_lsc_config(params, buf);
> > > > >
> > > > > if (ret)
> > > > > goto complete_and_unlock;
> > > > > @@ -2407,6 +2394,110 @@ static int rkisp1_params_vb2_queue_setup(struct vb2_queue *vq,
> > > > > return 0;
> > > > > }
> > > > >
> > > > > +static int rkisp1_params_vb2_buf_init(struct vb2_buffer *vb)
> > > > > +{
> > > > > + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > > > > + struct rkisp1_params_buffer *params_buf =
> > > > > + container_of(vbuf, struct rkisp1_params_buffer, vb);
> > >
> > > A to_rkisp1_params_buffer() inline function in the previous patch would
> > > be nice.
> > >
> > > > > + struct rkisp1_params *params = vb->vb2_queue->drv_priv;
> > > > > + size_t buf_size = params->metafmt.buffersize;
> > > > > +
> > > > > + if (params->metafmt.dataformat == V4L2_META_FMT_RK_ISP1_PARAMS) {
> > > > > + params_buf->cfg = NULL;
> > > > > + return 0;
> > > > > + }
> > >
> > > The problem is not restricted to the extensible format, how about doing
> > > the same for the legacy format ?
> >
> > Copying the buffer in ? Not sure I agree. The extensible format allows
> > userspace to change the memory layout of the buffer, if this happens
> > while the driver parses it, we'll ooops. With the fixed format the
> > memory layout is fixed, if userspace changes the content of the buffer
> > (ie the configuration parameters) after qbuf, well, it's their
> > problem.
>
> Can you guarantee that the driver will never ever crash or corrupt
> memory if that happens ? Especially if you take into account the fact
All of the mainline driver work in this way today. But ok if most
people think it's worth it, I'll do so and re-order the patches.
> that the compiler and CPU can reorder accesses, as well as split and
> merge accesses (i.e. reading the same memory location multiple times
> when it's accessed once only in the code), that seems hard to do.
>
> > Copying (and validation) is costly and if it serves to avoid
> > an ooops it's more than justified. In the other case I'm not sure.
>
> How expensive is the copy ?
>
For sure it doubles the allocated memory as we've one backing memory
buffer for each buffer requested by userspace.
