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

From: Ezequiel Garcia <ezequiel@collabora.com>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>,
	 linux-media@vger.kernel.org, helen.koike@collabora.com,
	hverkuil@xs4all.nl, kernel@collabora.com, dafna3@gmail.com,
	sakari.ailus@linux.intel.com,
	 linux-rockchip@lists.infradead.org, mchehab@kernel.org,
	tfiga@chromium.org
Subject: Re: [PATCH 2/2] media: rkisp1: cap: initialize dma buf address in 'buf_init' cb
Date: Mon, 28 Jun 2021 09:13:45 -0300	[thread overview]
Message-ID: <ccd40da84dba10f13f5d385850afaa7470b72105.camel@collabora.com> (raw)
In-Reply-To: <YNl3TPzoNhUTpkDO@pendragon.ideasonboard.com>

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


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip