| Message ID | 20190217024852.23328-6-laurent.pinchart+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | VSP1: Display writeback support | expand |
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@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. > * @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; > >
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; > >
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 * @video: the video node * * This function completes the current buffer by filling its sequence number, @@ -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, 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;
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> --- drivers/media/platform/vsp1/vsp1_video.c | 35 ++++++++++++++---------- 1 file changed, 20 insertions(+), 15 deletions(-)