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