Hi Laurent
On Wed, Jun 19, 2024 at 07:11:01PM GMT, Laurent Pinchart wrote:
> On Wed, Jun 19, 2024 at 05:55:12PM +0200, Jacopo Mondi wrote:
> > 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.
>
> Unless the memcpy really hinders performances, I think we should ensure
> safety first and foremest.
>
I haven't run any test, but I doubt the performance penality is
terrible
> > > 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.
>
> True, but parameter buffers are quite small. I haven't checked but I
> would expect everything to hold in one page.
True that.
Ok, safety first, I'll add support for copying in the fixed format as
well!
Thanks
j
>
> --
> Regards,
>
> Laurent Pinchart
