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@xxxxxxxxxxxxx>
> > ---
> > .../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
