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+rene...@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+rene...@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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Reply via email to