Re: [PATCH v4 5/7] media: vsp1: Refactor vsp1_video_complete_buffer() for later reuse

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Kieran Bingham <kieran.bingham@ideasonboard.com>
Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>,
	linux-media@vger.kernel.org, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH v4 5/7] media: vsp1: Refactor vsp1_video_complete_buffer() for later reuse
Date: Tue, 19 Feb 2019 01:43:42 +0200	[thread overview]
Message-ID: <20190218234342.GD5082@pendragon.ideasonboard.com> (raw)
In-Reply-To: <7b388ea1-fc35-01b9-64a4-7710068cb5e1@ideasonboard.com>

Hi Kieran,

On Sun, Feb 17, 2019 at 08:35:25PM +0000, Kieran Bingham wrote:
> On 17/02/2019 02:48, Laurent Pinchart wrote:
> > The vsp1_video_complete_buffer() function completes the current buffer
> > and returns a pointer to the next buffer. Split the code that completes
> > the buffer to a separate function for later reuse, and rename
> > vsp1_video_complete_buffer() to vsp1_video_complete_next_buffer().
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> 
> This looks good to me - except perhaps the documentation /might/ need
> some refresh. With or without updates there, the code changes look good
> to me:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> > ---
> >  drivers/media/platform/vsp1/vsp1_video.c | 35 ++++++++++++++----------
> >  1 file changed, 20 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/media/platform/vsp1/vsp1_video.c b/drivers/media/platform/vsp1/vsp1_video.c
> > index 328d686189be..cfbab16c4820 100644
> > --- a/drivers/media/platform/vsp1/vsp1_video.c
> > +++ b/drivers/media/platform/vsp1/vsp1_video.c
> > @@ -300,8 +300,22 @@ static int vsp1_video_pipeline_setup_partitions(struct vsp1_pipeline *pipe)
> >   * Pipeline Management
> >   */
> >  
> > +static void vsp1_video_complete_buffer(struct vsp1_video *video,
> > +				       struct vsp1_vb2_buffer *buffer)
> > +{
> > +	struct vsp1_pipeline *pipe = video->rwpf->entity.pipe;
> > +	unsigned int i;
> > +
> > +	buffer->buf.sequence = pipe->sequence;
> > +	buffer->buf.vb2_buf.timestamp = ktime_get_ns();
> > +	for (i = 0; i < buffer->buf.vb2_buf.num_planes; ++i)
> > +		vb2_set_plane_payload(&buffer->buf.vb2_buf, i,
> > +				      vb2_plane_size(&buffer->buf.vb2_buf, i));
> > +	vb2_buffer_done(&buffer->buf.vb2_buf, VB2_BUF_STATE_DONE);
> > +}
> > +
> >  /*
> > - * vsp1_video_complete_buffer - Complete the current buffer
> > + * vsp1_video_complete_next_buffer - Complete the current buffer
> 
> Should the brief be updated?
> It looks quite odd to call the function "complete next buffer" but with
> a brief saying "complete the current buffer".
> 
> Technically it's still correct, but it's more like
> "Complete the current buffer and return the next buffer"
> or such.

Good point. I'll update the brief to that.

> >   * @video: the video node
> >   *
> >   * This function completes the current buffer by filling its sequence number,
> 
> Is this still accurate enough to keep the text as is ?

It's still true, isn't it ?

> > @@ -310,13 +324,11 @@ static int vsp1_video_pipeline_setup_partitions(struct vsp1_pipeline *pipe)
> >   * Return the next queued buffer or NULL if the queue is empty.
> >   */
> >  static struct vsp1_vb2_buffer *
> > -vsp1_video_complete_buffer(struct vsp1_video *video)
> > +vsp1_video_complete_next_buffer(struct vsp1_video *video)
> >  {
> > -	struct vsp1_pipeline *pipe = video->rwpf->entity.pipe;
> > -	struct vsp1_vb2_buffer *next = NULL;
> > +	struct vsp1_vb2_buffer *next;
> >  	struct vsp1_vb2_buffer *done;
> >  	unsigned long flags;
> > -	unsigned int i;
> >  
> >  	spin_lock_irqsave(&video->irqlock, flags);
> >  
> > @@ -327,21 +339,14 @@ vsp1_video_complete_buffer(struct vsp1_video *video)
> >  
> >  	done = list_first_entry(&video->irqqueue,
> >  				struct vsp1_vb2_buffer, queue);
> > -
> >  	list_del(&done->queue);
> >  
> > -	if (!list_empty(&video->irqqueue))
> > -		next = list_first_entry(&video->irqqueue,
> > +	next = list_first_entry_or_null(&video->irqqueue,
> 
> That's a nice way to save a line.
> 
> >  					struct vsp1_vb2_buffer, queue);
> >  
> >  	spin_unlock_irqrestore(&video->irqlock, flags);
> >  
> > -	done->buf.sequence = pipe->sequence;
> > -	done->buf.vb2_buf.timestamp = ktime_get_ns();
> > -	for (i = 0; i < done->buf.vb2_buf.num_planes; ++i)
> > -		vb2_set_plane_payload(&done->buf.vb2_buf, i,
> > -				      vb2_plane_size(&done->buf.vb2_buf, i));
> > -	vb2_buffer_done(&done->buf.vb2_buf, VB2_BUF_STATE_DONE);
> > +	vsp1_video_complete_buffer(video, done);
> >  
> >  	return next;
> >  }
> > @@ -352,7 +357,7 @@ static void vsp1_video_frame_end(struct vsp1_pipeline *pipe,
> >  	struct vsp1_video *video = rwpf->video;
> >  	struct vsp1_vb2_buffer *buf;
> >  
> > -	buf = vsp1_video_complete_buffer(video);
> > +	buf = vsp1_video_complete_next_buffer(video);
> >  	if (buf == NULL)
> >  		return;
> >  

-- 
Regards,

Laurent Pinchart