[PATCH 0/2] rkisp1: Two small fixes

[PATCH 0/2] rkisp1: Two small fixes

[Dafna Hirschfeld]

Two small fixes to rkisp1
1. remove useless vaddr field
2. set the dma address of the capture buffers only once in 'buf_init' callabck
instead of each time the buffer is queued

Dafna Hirschfeld (2):
  media: rkisp1: remove field 'vaddr' from 'rkisp1_buffer'
  media: rkisp1: cap: initialize dma buf address in 'buf_init' cb

 .../media/platform/rockchip/rkisp1/rkisp1-capture.c  | 12 +++++++++++-
 .../media/platform/rockchip/rkisp1/rkisp1-common.h   |  6 +-----
 .../media/platform/rockchip/rkisp1/rkisp1-params.c   |  3 +--
 .../media/platform/rockchip/rkisp1/rkisp1-stats.c    |  6 ++----
 4 files changed, 15 insertions(+), 12 deletions(-)

-- 
2.17.1

[PATCH 1/2] media: rkisp1: remove field 'vaddr' from 'rkisp1_buffer'

[Dafna Hirschfeld]

The virtual address can be accessed using vb2_plane_vaddr
therefore there is no need to save it in an extra field in
'rkisp1_buffer'. Remove it.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 drivers/media/platform/rockchip/rkisp1/rkisp1-common.h | 6 +-----
 drivers/media/platform/rockchip/rkisp1/rkisp1-params.c | 3 +--
 drivers/media/platform/rockchip/rkisp1/rkisp1-stats.c  | 6 ++----
 3 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
index 038c303a8aed..bb73f4e17b66 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
@@ -156,15 +156,11 @@ struct rkisp1_vdev_node {
  * @vb:		vb2 buffer
  * @queue:	entry of the buffer in the queue
  * @buff_addr:	dma addresses of each plane, used only by the capture devices: selfpath, mainpath
- * @vaddr:	virtual address for buffers used by params and stats devices
  */
 struct rkisp1_buffer {
 	struct vb2_v4l2_buffer vb;
 	struct list_head queue;
-	union {
-		u32 buff_addr[VIDEO_MAX_PLANES];
-		void *vaddr;
-	};
+	u32 buff_addr[VIDEO_MAX_PLANES];
 };
 
 /*
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
index 529c6e21815f..8fa5b0abf1f9 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-params.c
@@ -1143,7 +1143,7 @@ static void rkisp1_params_apply_params_cfg(struct rkisp1_params *params,
 	cur_buf = list_first_entry(&params->params,
 				   struct rkisp1_buffer, queue);
 
-	new_params = (struct rkisp1_params_cfg *)(cur_buf->vaddr);
+	new_params = (struct rkisp1_params_cfg *)vb2_plane_vaddr(&cur_buf->vb.vb2_buf, 0);
 
 	rkisp1_isp_isr_other_config(params, new_params);
 	rkisp1_isp_isr_meas_config(params, new_params);
@@ -1382,7 +1382,6 @@ static void rkisp1_params_vb2_buf_queue(struct vb2_buffer *vb)
 	struct vb2_queue *vq = vb->vb2_queue;
 	struct rkisp1_params *params = vq->drv_priv;
 
-	params_buf->vaddr = vb2_plane_vaddr(vb, 0);
 	spin_lock_irq(&params->config_lock);
 	list_add_tail(&params_buf->queue, &params->params);
 	spin_unlock_irq(&params->config_lock);
diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-stats.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-stats.c
index c1d07a2e8839..e88bdd612d71 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-stats.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-stats.c
@@ -112,7 +112,6 @@ static void rkisp1_stats_vb2_buf_queue(struct vb2_buffer *vb)
 	struct vb2_queue *vq = vb->vb2_queue;
 	struct rkisp1_stats *stats_dev = vq->drv_priv;
 
-	stats_buf->vaddr = vb2_plane_vaddr(vb, 0);
 
 	spin_lock_irq(&stats_dev->lock);
 	list_add_tail(&stats_buf->queue, &stats_dev->stat);
@@ -305,9 +304,8 @@ rkisp1_stats_send_measurement(struct rkisp1_stats *stats, u32 isp_ris)
 	if (!cur_buf)
 		return;
 
-	cur_stat_buf =
-		(struct rkisp1_stat_buffer *)(cur_buf->vaddr);
-
+	cur_stat_buf = (struct rkisp1_stat_buffer *)
+			vb2_plane_vaddr(&cur_buf->vb.vb2_buf, 0);
 	if (isp_ris & RKISP1_CIF_ISP_AWB_DONE)
 		rkisp1_stats_get_awb_meas(stats, cur_stat_buf);
 
-- 
2.17.1

Re: [PATCH 1/2] media: rkisp1: remove field 'vaddr' from 'rkisp1_buffer'

[Ezequiel Garcia]

Hi Dafna,

On Fri, 2021-06-25 at 11:23 +0300, Dafna Hirschfeld wrote:
> The virtual address can be accessed using vb2_plane_vaddr
> therefore there is no need to save it in an extra field in
> 'rkisp1_buffer'. Remove it.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>

Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>

-- 
Kindly,
Ezequiel

[PATCH 2/2] media: rkisp1: cap: initialize dma buf address in 'buf_init' cb

[Dafna Hirschfeld]

Initializing the dma addresses of the capture buffers can
move to the 'buf_init' callback, since it is enough to do
it once for each buffer and not every time it is queued.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
---
 .../media/platform/rockchip/rkisp1/rkisp1-capture.c  | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
index 60cd2200e7ae..41988eb0ec0a 100644
--- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
+++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
@@ -750,7 +750,7 @@ static int rkisp1_vb2_queue_setup(struct vb2_queue *queue,
 	return 0;
 }
 
-static void rkisp1_vb2_buf_queue(struct vb2_buffer *vb)
+static int rkisp1_vb2_buf_init(struct vb2_buffer *vb)
 {
 	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
 	struct rkisp1_buffer *ispbuf =
@@ -780,6 +780,15 @@ static void rkisp1_vb2_buf_queue(struct vb2_buffer *vb)
 	if (cap->pix.info->comp_planes == 3 && cap->pix.cfg->uv_swap)
 		swap(ispbuf->buff_addr[RKISP1_PLANE_CR],
 		     ispbuf->buff_addr[RKISP1_PLANE_CB]);
+	return 0;
+}
+
+static void rkisp1_vb2_buf_queue(struct vb2_buffer *vb)
+{
+	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
+	struct rkisp1_buffer *ispbuf =
+		container_of(vbuf, struct rkisp1_buffer, vb);
+	struct rkisp1_capture *cap = vb->vb2_queue->drv_priv;
 
 	spin_lock_irq(&cap->buf.lock);
 	list_add_tail(&ispbuf->queue, &cap->buf.queue);
@@ -1039,6 +1048,7 @@ rkisp1_vb2_start_streaming(struct vb2_queue *queue, unsigned int count)
 
 static const struct vb2_ops rkisp1_vb2_ops = {
 	.queue_setup = rkisp1_vb2_queue_setup,
+	.buf_init = rkisp1_vb2_buf_init,
 	.buf_queue = rkisp1_vb2_buf_queue,
 	.buf_prepare = rkisp1_vb2_buf_prepare,
 	.wait_prepare = vb2_ops_wait_prepare,
-- 
2.17.1

Re: [PATCH 2/2] media: rkisp1: cap: initialize dma buf address in 'buf_init' cb

[Ezequiel Garcia]

Hi Dafna,

On Fri, 2021-06-25 at 11:23 +0300, Dafna Hirschfeld wrote:
> Initializing the dma addresses of the capture buffers can
> move to the 'buf_init' callback, since it is enough to do
> it once for each buffer and not every time it is queued.
> 

Are you able to measure any impact with this change?

> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  .../media/platform/rockchip/rkisp1/rkisp1-capture.c  | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> index 60cd2200e7ae..41988eb0ec0a 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> @@ -750,7 +750,7 @@ static int rkisp1_vb2_queue_setup(struct vb2_queue *queue,
>         return 0;
>  }
>  
> -static void rkisp1_vb2_buf_queue(struct vb2_buffer *vb)
> +static int rkisp1_vb2_buf_init(struct vb2_buffer *vb)
>  {
>         struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>         struct rkisp1_buffer *ispbuf =

Since you are interested at these kind of cleanups, you can
do something like:

+enum rkisp1_plane {
+       RKISP1_PLANE_Y  = 0,
+       RKISP1_PLANE_CB = 1,
+       RKISP1_PLANE_CR = 2,
+       RKISP1_NUM_PLANES = 3
+};
+
 /*
  * struct rkisp1_buffer - A container for the vb2 buffers used by the video devices:
  *                       params, stats, mainpath, selfpath
@@ -160,7 +167,7 @@ struct rkisp1_vdev_node {
 struct rkisp1_buffer {
        struct vb2_v4l2_buffer vb;
        struct list_head queue;
-       u32 buff_addr[VIDEO_MAX_PLANES];
+       u32 buff_addr[RKISP1_NUM_PLANES];
 };

And then you can get rid of the memset, and rely on
vb2_dma_contig_plane_dma_addr returning NULL.

@@ -759,8 +753,7 @@ static void rkisp1_vb2_buf_queue(struct vb2_buffer *vb)
        const struct v4l2_pix_format_mplane *pixm = &cap->pix.fmt;
        unsigned int i;
 
-       memset(ispbuf->buff_addr, 0, sizeof(ispbuf->buff_addr));
-       for (i = 0; i < pixm->num_planes; i++)
+       for (i = 0; i < RKISP1_NUM_PLANES; i++)
                ispbuf->buff_addr[i] = vb2_dma_contig_plane_dma_addr(vb, i);

-- 
Kindly,
Ezequiel

Re: [PATCH 2/2] media: rkisp1: cap: initialize dma buf address in 'buf_init' cb

[Laurent Pinchart]

On Fri, Jun 25, 2021 at 10:08:44PM -0300, Ezequiel Garcia wrote:
> On Fri, 2021-06-25 at 11:23 +0300, Dafna Hirschfeld wrote:
> > Initializing the dma addresses of the capture buffers can
> > move to the 'buf_init' callback, since it is enough to do
> > it once for each buffer and not every time it is queued.
> 
> Are you able to measure any impact with this change?
> 
> > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> > ---
> >  .../media/platform/rockchip/rkisp1/rkisp1-capture.c  | 12 +++++++++++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> > index 60cd2200e7ae..41988eb0ec0a 100644
> > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> > @@ -750,7 +750,7 @@ static int rkisp1_vb2_queue_setup(struct vb2_queue *queue,
> >         return 0;
> >  }
> >  
> > -static void rkisp1_vb2_buf_queue(struct vb2_buffer *vb)
> > +static int rkisp1_vb2_buf_init(struct vb2_buffer *vb)
> >  {
> >         struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> >         struct rkisp1_buffer *ispbuf =
> 
> Since you are interested at these kind of cleanups, you can
> do something like:
> 
> +enum rkisp1_plane {
> +       RKISP1_PLANE_Y  = 0,
> +       RKISP1_PLANE_CB = 1,
> +       RKISP1_PLANE_CR = 2,
> +       RKISP1_NUM_PLANES = 3
> +};
> +
>  /*
>   * struct rkisp1_buffer - A container for the vb2 buffers used by the video devices:
>   *                       params, stats, mainpath, selfpath
> @@ -160,7 +167,7 @@ struct rkisp1_vdev_node {
>  struct rkisp1_buffer {
>         struct vb2_v4l2_buffer vb;
>         struct list_head queue;
> -       u32 buff_addr[VIDEO_MAX_PLANES];
> +       u32 buff_addr[RKISP1_NUM_PLANES];
>  };
> 
> And then you can get rid of the memset, and rely on
> vb2_dma_contig_plane_dma_addr returning NULL.
> 
> @@ -759,8 +753,7 @@ static void rkisp1_vb2_buf_queue(struct vb2_buffer *vb)
>         const struct v4l2_pix_format_mplane *pixm = &cap->pix.fmt;
>         unsigned int i;
>  
> -       memset(ispbuf->buff_addr, 0, sizeof(ispbuf->buff_addr));
> -       for (i = 0; i < pixm->num_planes; i++)
> +       for (i = 0; i < RKISP1_NUM_PLANES; i++)

This should use ARRAY_SIZE().

>                 ispbuf->buff_addr[i] = vb2_dma_contig_plane_dma_addr(vb, i);

But will vb2_dma_contig_plane_dma_addr() be happy when i is larger than
the number of planes ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/2] media: rkisp1: cap: initialize dma buf address in 'buf_init' cb

[Ezequiel Garcia]

On Mon, 2021-06-28 at 10:16 +0300, Laurent Pinchart wrote:
> On Fri, Jun 25, 2021 at 10:08:44PM -0300, Ezequiel Garcia wrote:
> > On Fri, 2021-06-25 at 11:23 +0300, Dafna Hirschfeld wrote:
> > > Initializing the dma addresses of the capture buffers can
> > > move to the 'buf_init' callback, since it is enough to do
> > > it once for each buffer and not every time it is queued.
> > 
> > Are you able to measure any impact with this change?
> > 
> > > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> > > ---
> > >  .../media/platform/rockchip/rkisp1/rkisp1-capture.c  | 12 +++++++++++-
> > >  1 file changed, 11 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> > > index 60cd2200e7ae..41988eb0ec0a 100644
> > > --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> > > +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
> > > @@ -750,7 +750,7 @@ static int rkisp1_vb2_queue_setup(struct vb2_queue *queue,
> > >         return 0;
> > >  }
> > >  
> > > -static void rkisp1_vb2_buf_queue(struct vb2_buffer *vb)
> > > +static int rkisp1_vb2_buf_init(struct vb2_buffer *vb)
> > >  {
> > >         struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> > >         struct rkisp1_buffer *ispbuf =
> > 
> > Since you are interested at these kind of cleanups, you can
> > do something like:
> > 
> > +enum rkisp1_plane {
> > +       RKISP1_PLANE_Y  = 0,
> > +       RKISP1_PLANE_CB = 1,
> > +       RKISP1_PLANE_CR = 2,
> > +       RKISP1_NUM_PLANES = 3
> > +};
> > +
> >  /*
> >   * struct rkisp1_buffer - A container for the vb2 buffers used by the video devices:
> >   *                       params, stats, mainpath, selfpath
> > @@ -160,7 +167,7 @@ struct rkisp1_vdev_node {
> >  struct rkisp1_buffer {
> >         struct vb2_v4l2_buffer vb;
> >         struct list_head queue;
> > -       u32 buff_addr[VIDEO_MAX_PLANES];
> > +       u32 buff_addr[RKISP1_NUM_PLANES];
> >  };
> > 
> > And then you can get rid of the memset, and rely on
> > vb2_dma_contig_plane_dma_addr returning NULL.
> > 
> > @@ -759,8 +753,7 @@ static void rkisp1_vb2_buf_queue(struct vb2_buffer *vb)
> >         const struct v4l2_pix_format_mplane *pixm = &cap->pix.fmt;
> >         unsigned int i;
> >  
> > -       memset(ispbuf->buff_addr, 0, sizeof(ispbuf->buff_addr));
> > -       for (i = 0; i < pixm->num_planes; i++)
> > +       for (i = 0; i < RKISP1_NUM_PLANES; i++)
> 
> This should use ARRAY_SIZE().
> 
> >                 ispbuf->buff_addr[i] = vb2_dma_contig_plane_dma_addr(vb, i);
> 
> But will vb2_dma_contig_plane_dma_addr() be happy when i is larger than
> the number of planes ?
> 

Well, vb2_plane_cookie handles it. TBH, not sure it's a behavior we should
rely on, but it's there.
-- 
Kindly,
Ezequiel

Re: [PATCH 2/2] media: rkisp1: cap: initialize dma buf address in 'buf_init' cb

[Dafna Hirschfeld]

Hi

On 26.06.21 03:08, Ezequiel Garcia wrote:
> Hi Dafna,
> 
> On Fri, 2021-06-25 at 11:23 +0300, Dafna Hirschfeld wrote:
>> Initializing the dma addresses of the capture buffers can
>> move to the 'buf_init' callback, since it is enough to do
>> it once for each buffer and not every time it is queued.
>>
> 
> Are you able to measure any impact with this change?

I didn't measure the impact, I just thought it is a more correct
use of the API.
You think it worth measuring the impact?

Thanks,
Dafna

> 
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>> ---
>>   .../media/platform/rockchip/rkisp1/rkisp1-capture.c  | 12 +++++++++++-
>>   1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
>> index 60cd2200e7ae..41988eb0ec0a 100644
>> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
>> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-capture.c
>> @@ -750,7 +750,7 @@ static int rkisp1_vb2_queue_setup(struct vb2_queue *queue,
>>          return 0;
>>   }
>>   
>> -static void rkisp1_vb2_buf_queue(struct vb2_buffer *vb)
>> +static int rkisp1_vb2_buf_init(struct vb2_buffer *vb)
>>   {
>>          struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>>          struct rkisp1_buffer *ispbuf =
> 
> Since you are interested at these kind of cleanups, you can
> do something like:
> 
> +enum rkisp1_plane {
> +       RKISP1_PLANE_Y  = 0,
> +       RKISP1_PLANE_CB = 1,
> +       RKISP1_PLANE_CR = 2,
> +       RKISP1_NUM_PLANES = 3
> +};
> +
>   /*
>    * struct rkisp1_buffer - A container for the vb2 buffers used by the video devices:
>    *                       params, stats, mainpath, selfpath
> @@ -160,7 +167,7 @@ struct rkisp1_vdev_node {
>   struct rkisp1_buffer {
>          struct vb2_v4l2_buffer vb;
>          struct list_head queue;
> -       u32 buff_addr[VIDEO_MAX_PLANES];
> +       u32 buff_addr[RKISP1_NUM_PLANES];
>   };
> 
> And then you can get rid of the memset, and rely on
> vb2_dma_contig_plane_dma_addr returning NULL.
> 
> @@ -759,8 +753,7 @@ static void rkisp1_vb2_buf_queue(struct vb2_buffer *vb)
>          const struct v4l2_pix_format_mplane *pixm = &cap->pix.fmt;
>          unsigned int i;
>   
> -       memset(ispbuf->buff_addr, 0, sizeof(ispbuf->buff_addr));
> -       for (i = 0; i < pixm->num_planes; i++)
> +       for (i = 0; i < RKISP1_NUM_PLANES; i++)
>                  ispbuf->buff_addr[i] = vb2_dma_contig_plane_dma_addr(vb, i);
> 

Re: [PATCH 0/2] rkisp1: Two small fixes

[Tomasz Figa]

On Fri, Jun 25, 2021 at 11:23:07AM +0300, Dafna Hirschfeld wrote:
> Two small fixes to rkisp1
> 1. remove useless vaddr field
> 2. set the dma address of the capture buffers only once in 'buf_init' callabck
> instead of each time the buffer is queued
> 
> Dafna Hirschfeld (2):
>   media: rkisp1: remove field 'vaddr' from 'rkisp1_buffer'
>   media: rkisp1: cap: initialize dma buf address in 'buf_init' cb
> 
>  .../media/platform/rockchip/rkisp1/rkisp1-capture.c  | 12 +++++++++++-
>  .../media/platform/rockchip/rkisp1/rkisp1-common.h   |  6 +-----
>  .../media/platform/rockchip/rkisp1/rkisp1-params.c   |  3 +--
>  .../media/platform/rockchip/rkisp1/rkisp1-stats.c    |  6 ++----
>  4 files changed, 15 insertions(+), 12 deletions(-)

For both patches:

Reviewed-by; Tomasz Figa <tfiga@chromium.org>

Best regards,
Tomasz