* [PATCH 0/2] rkisp1: Two small fixes
@ 2021-06-25 8:23 Dafna Hirschfeld
2021-06-25 8:23 ` [PATCH 1/2] media: rkisp1: remove field 'vaddr' from 'rkisp1_buffer' Dafna Hirschfeld
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Dafna Hirschfeld @ 2021-06-25 8:23 UTC (permalink / raw)
To: linux-media
Cc: laurent.pinchart, dafna.hirschfeld, helen.koike, ezequiel,
hverkuil, kernel, dafna3, sakari.ailus, linux-rockchip, mchehab,
tfiga
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] media: rkisp1: remove field 'vaddr' from 'rkisp1_buffer'
2021-06-25 8:23 [PATCH 0/2] rkisp1: Two small fixes Dafna Hirschfeld
@ 2021-06-25 8:23 ` Dafna Hirschfeld
2021-06-26 0:51 ` Ezequiel Garcia
2021-06-25 8:23 ` [PATCH 2/2] media: rkisp1: cap: initialize dma buf address in 'buf_init' cb Dafna Hirschfeld
2021-06-28 5:46 ` [PATCH 0/2] rkisp1: Two small fixes Tomasz Figa
2 siblings, 1 reply; 9+ messages in thread
From: Dafna Hirschfeld @ 2021-06-25 8:23 UTC (permalink / raw)
To: linux-media
Cc: laurent.pinchart, dafna.hirschfeld, helen.koike, ezequiel,
hverkuil, kernel, dafna3, sakari.ailus, linux-rockchip, mchehab,
tfiga
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(¶ms->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(¶ms->config_lock);
list_add_tail(¶ms_buf->queue, ¶ms->params);
spin_unlock_irq(¶ms->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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] media: rkisp1: cap: initialize dma buf address in 'buf_init' cb
2021-06-25 8:23 [PATCH 0/2] rkisp1: Two small fixes Dafna Hirschfeld
2021-06-25 8:23 ` [PATCH 1/2] media: rkisp1: remove field 'vaddr' from 'rkisp1_buffer' Dafna Hirschfeld
@ 2021-06-25 8:23 ` Dafna Hirschfeld
2021-06-26 1:08 ` Ezequiel Garcia
2021-06-28 5:46 ` [PATCH 0/2] rkisp1: Two small fixes Tomasz Figa
2 siblings, 1 reply; 9+ messages in thread
From: Dafna Hirschfeld @ 2021-06-25 8:23 UTC (permalink / raw)
To: linux-media
Cc: laurent.pinchart, dafna.hirschfeld, helen.koike, ezequiel,
hverkuil, kernel, dafna3, sakari.ailus, linux-rockchip, mchehab,
tfiga
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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] media: rkisp1: remove field 'vaddr' from 'rkisp1_buffer'
2021-06-25 8:23 ` [PATCH 1/2] media: rkisp1: remove field 'vaddr' from 'rkisp1_buffer' Dafna Hirschfeld
@ 2021-06-26 0:51 ` Ezequiel Garcia
0 siblings, 0 replies; 9+ messages in thread
From: Ezequiel Garcia @ 2021-06-26 0:51 UTC (permalink / raw)
To: Dafna Hirschfeld, linux-media
Cc: laurent.pinchart, helen.koike, hverkuil, kernel, dafna3,
sakari.ailus, linux-rockchip, mchehab, tfiga
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] media: rkisp1: cap: initialize dma buf address in 'buf_init' cb
2021-06-25 8:23 ` [PATCH 2/2] media: rkisp1: cap: initialize dma buf address in 'buf_init' cb Dafna Hirschfeld
@ 2021-06-26 1:08 ` Ezequiel Garcia
2021-06-28 7:16 ` Laurent Pinchart
2021-07-12 13:21 ` Dafna Hirschfeld
0 siblings, 2 replies; 9+ messages in thread
From: Ezequiel Garcia @ 2021-06-26 1:08 UTC (permalink / raw)
To: Dafna Hirschfeld, linux-media
Cc: laurent.pinchart, helen.koike, hverkuil, kernel, dafna3,
sakari.ailus, linux-rockchip, mchehab, tfiga
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/2] rkisp1: Two small fixes
2021-06-25 8:23 [PATCH 0/2] rkisp1: Two small fixes Dafna Hirschfeld
2021-06-25 8:23 ` [PATCH 1/2] media: rkisp1: remove field 'vaddr' from 'rkisp1_buffer' Dafna Hirschfeld
2021-06-25 8:23 ` [PATCH 2/2] media: rkisp1: cap: initialize dma buf address in 'buf_init' cb Dafna Hirschfeld
@ 2021-06-28 5:46 ` Tomasz Figa
2 siblings, 0 replies; 9+ messages in thread
From: Tomasz Figa @ 2021-06-28 5:46 UTC (permalink / raw)
To: Dafna Hirschfeld
Cc: linux-media, laurent.pinchart, helen.koike, ezequiel, hverkuil,
kernel, dafna3, sakari.ailus, linux-rockchip, mchehab
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] media: rkisp1: cap: initialize dma buf address in 'buf_init' cb
2021-06-26 1:08 ` Ezequiel Garcia
@ 2021-06-28 7:16 ` Laurent Pinchart
2021-06-28 12:13 ` Ezequiel Garcia
2021-07-12 13:21 ` Dafna Hirschfeld
1 sibling, 1 reply; 9+ messages in thread
From: Laurent Pinchart @ 2021-06-28 7:16 UTC (permalink / raw)
To: Ezequiel Garcia
Cc: Dafna Hirschfeld, linux-media, helen.koike, hverkuil, kernel,
dafna3, sakari.ailus, linux-rockchip, mchehab, tfiga
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] media: rkisp1: cap: initialize dma buf address in 'buf_init' cb
2021-06-28 7:16 ` Laurent Pinchart
@ 2021-06-28 12:13 ` Ezequiel Garcia
0 siblings, 0 replies; 9+ messages in thread
From: Ezequiel Garcia @ 2021-06-28 12:13 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Dafna Hirschfeld, linux-media, helen.koike, hverkuil, kernel,
dafna3, sakari.ailus, linux-rockchip, mchehab, tfiga
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] media: rkisp1: cap: initialize dma buf address in 'buf_init' cb
2021-06-26 1:08 ` Ezequiel Garcia
2021-06-28 7:16 ` Laurent Pinchart
@ 2021-07-12 13:21 ` Dafna Hirschfeld
1 sibling, 0 replies; 9+ messages in thread
From: Dafna Hirschfeld @ 2021-07-12 13:21 UTC (permalink / raw)
To: Ezequiel Garcia, linux-media
Cc: laurent.pinchart, helen.koike, hverkuil, kernel, dafna3,
sakari.ailus, linux-rockchip, mchehab, tfiga
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);
>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-07-12 13:21 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-25 8:23 [PATCH 0/2] rkisp1: Two small fixes Dafna Hirschfeld
2021-06-25 8:23 ` [PATCH 1/2] media: rkisp1: remove field 'vaddr' from 'rkisp1_buffer' Dafna Hirschfeld
2021-06-26 0:51 ` Ezequiel Garcia
2021-06-25 8:23 ` [PATCH 2/2] media: rkisp1: cap: initialize dma buf address in 'buf_init' cb Dafna Hirschfeld
2021-06-26 1:08 ` Ezequiel Garcia
2021-06-28 7:16 ` Laurent Pinchart
2021-06-28 12:13 ` Ezequiel Garcia
2021-07-12 13:21 ` Dafna Hirschfeld
2021-06-28 5:46 ` [PATCH 0/2] rkisp1: Two small fixes Tomasz Figa
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).