| Message ID | 20210625082309.24944-3-dafna.hirschfeld@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rkisp1: Two small fixes | expand |
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);
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 ?
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.
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); >
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,
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(-)