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