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

[Kieran Bingham]

Hi Laurent

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 at 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 at 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.


>   * @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 ?



> @@ -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
--
Kieran