On Sat, Jun 29, 2024 at 04:31:49PM +0300, Laurent Pinchart wrote:
> On Fri, Jun 21, 2024 at 04:54:02PM +0200, Jacopo Mondi wrote:
> > The ISP parameters buffers are queued by userspace to the params video
> > device and appended by the driver to the list of available ones for
>
> s/ones/buffers/
>
> > later consumption.
> >
> > As the parameters buffer is mapped in the userspace process memory,
> > applications have access to the buffer content after the buffer has
>
> s/content/contents/
>
> > been queued.
> >
> > To prevent userspace from modifying the content of the parameters buffer
>
> s/content/contents/
>
> > after it has been queued to the video device, add to 'struct
> > rkisp1_params_buffer' a scratch buffer where to copy the parameters.
> >
> > Allocate the scratch buffer in the vb2 buf_init() operation and copy the
> > buffer content in the buf_prepare() operation. Release the scratch
>
> s/Release/Free/
>
> > buffer in the newly introduced buf_cleanup() operation handler.
> >
> > Modify the ISP configuration function to access the ISP configuration
> > from the cached copy of the parameters buffer instead of using the
> > userspace-mapped one.
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx>
> > ---
> > .../platform/rockchip/rkisp1/rkisp1-common.h | 2 +
> > .../platform/rockchip/rkisp1/rkisp1-params.c | 76 ++++++++++++++-----
> > 2 files changed, 57 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > index a615bbb0255e..cdc7cc64ebd5 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> > @@ -250,10 +250,12 @@ struct rkisp1_buffer {
> > *
> > * @vb: vb2 buffer
> > * @queue: entry of the buffer in the queue
> > + * @cfg: scratch buffer used for caching the ISP configuration parameters
> > */
> > struct rkisp1_params_buffer {
> > struct vb2_v4l2_buffer vb;
> > struct list_head queue;
> > + struct rkisp1_params_cfg *cfg;
> > };
>
> You can add
>
> static inline struct rkisp1_params_buffer *
> to_rkisp1_params_buffer(struct vb2_v4l2_buffer *vbuf)
> {
> return container_of(vbuf, struct rkisp1_params_buffer, vb);
> }
>
> and use it below.
>
> >
> > /*
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
> > index 2844e55bc4f2..c081b41d6212 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>
> > @@ -1501,18 +1503,14 @@ static void rkisp1_isp_isr_meas_config(struct rkisp1_params *params,
> > }
> > }
> >
> > -static bool rkisp1_params_get_buffer(struct rkisp1_params *params,
> > - struct rkisp1_params_buffer **buf,
> > - struct rkisp1_params_cfg **cfg)
> > +static struct rkisp1_params_buffer *
> > +rkisp1_params_get_buffer(struct rkisp1_params *params)
> > {
> > if (list_empty(¶ms->params))
> > - return false;
> > + return NULL;
> >
> > - *buf = list_first_entry(¶ms->params, struct rkisp1_params_buffer,
> > + return list_first_entry(¶ms->params, struct rkisp1_params_buffer,
> > queue);
> > - *cfg = vb2_plane_vaddr(&(*buf)->vb.vb2_buf, 0);
> > -
> > - return true;
>
> There's a nice helper you can use:
>
> return list_first_entry_or_null(¶ms->params,
> struct rkisp1_params_buffer, queue);
>
> You could possibly even use that directly below and drop this function.
> Up to you.
>
> > }
> >
> > static void rkisp1_params_complete_buffer(struct rkisp1_params *params,
> > @@ -1528,17 +1526,17 @@ static void rkisp1_params_complete_buffer(struct rkisp1_params *params,
> > void rkisp1_params_isr(struct rkisp1_device *rkisp1)
> > {
> > struct rkisp1_params *params = &rkisp1->params;
> > - struct rkisp1_params_cfg *new_params;
> > struct rkisp1_params_buffer *cur_buf;
> >
> > spin_lock(¶ms->config_lock);
> >
> > - if (!rkisp1_params_get_buffer(params, &cur_buf, &new_params))
> > + cur_buf = rkisp1_params_get_buffer(params);
> > + if (!cur_buf)
> > goto unlock;
> >
> > - rkisp1_isp_isr_other_config(params, new_params);
> > - rkisp1_isp_isr_lsc_config(params, new_params);
> > - rkisp1_isp_isr_meas_config(params, new_params);
> > + rkisp1_isp_isr_other_config(params, cur_buf->cfg);
> > + rkisp1_isp_isr_lsc_config(params, cur_buf->cfg);
> > + rkisp1_isp_isr_meas_config(params, cur_buf->cfg);
> >
> > /* update shadow register immediately */
> > rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL,
> > @@ -1604,7 +1602,6 @@ void rkisp1_params_pre_configure(struct rkisp1_params *params,
> > enum v4l2_ycbcr_encoding ycbcr_encoding)
> > {
> > struct rkisp1_cif_isp_hst_config hst = rkisp1_hst_params_default_config;
> > - struct rkisp1_params_cfg *new_params;
> > struct rkisp1_params_buffer *cur_buf;
> >
> > params->quantization = quantization;
> > @@ -1634,11 +1631,12 @@ void rkisp1_params_pre_configure(struct rkisp1_params *params,
> >
> > /* apply the first buffer if there is one already */
> >
> > - if (!rkisp1_params_get_buffer(params, &cur_buf, &new_params))
> > + cur_buf = rkisp1_params_get_buffer(params);
> > + if (!cur_buf)
> > goto unlock;
> >
> > - rkisp1_isp_isr_other_config(params, new_params);
> > - rkisp1_isp_isr_meas_config(params, new_params);
> > + rkisp1_isp_isr_other_config(params, cur_buf->cfg);
> > + rkisp1_isp_isr_meas_config(params, cur_buf->cfg);
> >
> > /* update shadow register immediately */
> > rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL,
> > @@ -1650,7 +1648,6 @@ void rkisp1_params_pre_configure(struct rkisp1_params *params,
> >
> > void rkisp1_params_post_configure(struct rkisp1_params *params)
> > {
> > - struct rkisp1_params_cfg *new_params;
> > struct rkisp1_params_buffer *cur_buf;
> >
> > spin_lock_irq(¶ms->config_lock);
> > @@ -1663,11 +1660,11 @@ void rkisp1_params_post_configure(struct rkisp1_params *params)
> > * ordering doesn't affect other ISP versions negatively, do so
> > * unconditionally.
> > */
> > -
> > - if (!rkisp1_params_get_buffer(params, &cur_buf, &new_params))
> > + cur_buf = rkisp1_params_get_buffer(params);
> > + if (!cur_buf)
> > goto unlock;
> >
> > - rkisp1_isp_isr_lsc_config(params, new_params);
> > + rkisp1_isp_isr_lsc_config(params, cur_buf->cfg);
> >
> > /* update shadow register immediately */
> > rkisp1_param_set_bits(params, RKISP1_CIF_ISP_CTRL,
> > @@ -1819,6 +1816,29 @@ 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);
> > +
> > + params_buf->cfg = kvmalloc(sizeof(*params_buf->cfg), GFP_KERNEL);
> > + if (!params_buf->cfg)
> > + return -ENOMEM;
> > +
> > + return 0;
> > +}
> > +
> > +static void rkisp1_params_vb2_buf_cleanup(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);
> > +
> > + kvfree(params_buf->cfg);
> > + params_buf->cfg = NULL;
> > +}
> > +
> > static void rkisp1_params_vb2_buf_queue(struct vb2_buffer *vb)
> > {
> > struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > @@ -1834,11 +1854,23 @@ static void rkisp1_params_vb2_buf_queue(struct vb2_buffer *vb)
> >
> > static int rkisp1_params_vb2_buf_prepare(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);
> > + struct rkisp1_params_cfg *cfg =
> > + vb2_plane_vaddr(¶ms_buf->vb.vb2_buf, 0);
> > +
> > if (vb2_plane_size(vb, 0) < sizeof(struct rkisp1_params_cfg))
> > return -EINVAL;
> >
> > vb2_set_plane_payload(vb, 0, sizeof(struct rkisp1_params_cfg));
> >
> > + /*
> > + * Copy the parameters buffer to the internal scratch buffer to avoid
> > + * userspace modifying the buffer content while the driver processes it.
> > + */
> > + memcpy(params_buf->cfg, cfg, sizeof(*cfg));
>
> You need a copy_from_user() (and include uaccess.h).
If anyone read this thread in the future: I was wrong here, cfg is a
kernel-mapped address, so a plain memcpy() is all that is required.
> Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>
> > +
> > return 0;
> > }
> >
> > @@ -1863,6 +1895,8 @@ static void rkisp1_params_vb2_stop_streaming(struct vb2_queue *vq)
> >
> > static const struct vb2_ops rkisp1_params_vb2_ops = {
> > .queue_setup = rkisp1_params_vb2_queue_setup,
> > + .buf_init = rkisp1_params_vb2_buf_init,
> > + .buf_cleanup = rkisp1_params_vb2_buf_cleanup,
> > .wait_prepare = vb2_ops_wait_prepare,
> > .wait_finish = vb2_ops_wait_finish,
> > .buf_queue = rkisp1_params_vb2_buf_queue,
--
Regards,
Laurent Pinchart
