Re: [PATCH 4/7] media: rkisp1: Copy the parameters buffer

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 

Hi Jacopo,

Thank you for the patch.

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(&params->params))
> -		return false;
> +		return NULL;
>  
> -	*buf = list_first_entry(&params->params, struct rkisp1_params_buffer,
> +	return list_first_entry(&params->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(&params->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(&params->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(&params->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(&params_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).

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
[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux