[PATCH 0/5] media: Access videobuf2 buffers via an accessor

[PATCH 0/5] media: Access videobuf2 buffers via an accessor

[Ezequiel Garcia]

Hi,

This patchset introduces a new vb2_get_buffer accessor and then
uses it on all the drivers that are accessing videobuf2
private buffer array directly.

I'm skipping Intel IPU3 driver here, since the code goes beyond
just accessing the buffer. It also modifies the buffer queue
directly. I believe this driver would need some more cleanup
and love from its maintainers.

Note that OMAP2/OMAP3 display driver is videobuf1 and so not
affected by this change.

Lastly, note that I'm doing the minimum changes to drivers I can't test,
only using the new accessor and avoiding any further changes.
`
Thanks,
Ezequiel

Ezequiel Garcia (5):
  media: vb2: Introduce a vb2_get_buffer accessor
  media: mtk-jpeg: Use vb2_get_buffer
  media: mtk-vcodec: Use vb2_get_buffer
  media: sti: Use vb2_get_buffer
  media: rockchip: Use vb2_get_buffer

 .../media/platform/mtk-jpeg/mtk_jpeg_core.c    |  2 +-
 .../media/platform/mtk-vcodec/mtk_vcodec_enc.c | 12 +++++++++---
 drivers/media/platform/sti/hva/hva-v4l2.c      |  4 +++-
 .../media/rockchip/vpu/rockchip_vpu_drv.c      |  9 ++++++---
 include/media/videobuf2-core.h                 | 18 ++++++++++++++++++
 5 files changed, 37 insertions(+), 8 deletions(-)

-- 
2.20.1

[PATCH 1/5] media: vb2: Introduce a vb2_get_buffer accessor

[Ezequiel Garcia]

Some drivers need to access a vb2 buffer from its
queue index. Introduce an accessor to abstract this,
and avoid drivers from accessing private members.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 include/media/videobuf2-core.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index c03ef7cc5071..7b1729320b16 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -1163,6 +1163,24 @@ static inline void vb2_clear_last_buffer_dequeued(struct vb2_queue *q)
 	q->last_buffer_dequeued = false;
 }
 
+/**
+ * vb2_get_buffer() - get a buffer from a queue
+ * @q:		pointer to &struct vb2_queue with videobuf2 queue.
+ * @index:	buffer index
+ *
+ * This function obtains a buffer from a queue, by its index.
+ * Keep in mind that there is no refcounting involved in this
+ * operation, so the buffer lifetime should be taken into
+ * consideration.
+ */
+static inline struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q,
+						unsigned int index)
+{
+	if (q->num_buffers > 0 && index < q->num_buffers)
+		return q->bufs[index];
+	return NULL;
+}
+
 /*
  * The following functions are not part of the vb2 core API, but are useful
  * functions for videobuf2-*.
-- 
2.20.1

Re: [PATCH 1/5] media: vb2: Introduce a vb2_get_buffer accessor

[Boris Brezillon]

On Thu,  6 Jun 2019 12:44:22 -0300
Ezequiel Garcia <ezequiel@collabora.com> wrote:

> Some drivers need to access a vb2 buffer from its
> queue index. Introduce an accessor to abstract this,
> and avoid drivers from accessing private members.
> 
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
>  include/media/videobuf2-core.h | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index c03ef7cc5071..7b1729320b16 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -1163,6 +1163,24 @@ static inline void vb2_clear_last_buffer_dequeued(struct vb2_queue *q)
>  	q->last_buffer_dequeued = false;
>  }
>  
> +/**
> + * vb2_get_buffer() - get a buffer from a queue
> + * @q:		pointer to &struct vb2_queue with videobuf2 queue.
> + * @index:	buffer index
> + *
> + * This function obtains a buffer from a queue, by its index.
> + * Keep in mind that there is no refcounting involved in this
> + * operation, so the buffer lifetime should be taken into
> + * consideration.
> + */
> +static inline struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q,
> +						unsigned int index)
> +{
> +	if (q->num_buffers > 0 && index < q->num_buffers)

No need to check q->num_buffers > 0 because in that case
index < q->num_buffers is always false.

Looks good otherwise

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

> +		return q->bufs[index];
> +	return NULL;
> +}
> +
>  /*
>   * The following functions are not part of the vb2 core API, but are useful
>   * functions for videobuf2-*.

[PATCH 2/5] media: mtk-jpeg: Use vb2_get_buffer

[Ezequiel Garcia]

Use the newly introduced vb2_get_buffer API and avoid
accessing buffers in the queue directly.

Cc: Rick Chang <rick.chang@mediatek.com>
Cc: Bin Liu <bin.liu@mediatek.com>
Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
index 3b199662cb34..f8f808abada1 100644
--- a/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
+++ b/drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c
@@ -526,7 +526,7 @@ static int mtk_jpeg_qbuf(struct file *file, void *priv, struct v4l2_buffer *buf)
 		return -EINVAL;
 	}
 
-	vb = vq->bufs[buf->index];
+	vb = vb2_get_buffer(vq, buf->index);
 	jpeg_src_buf = mtk_jpeg_vb2_to_srcbuf(vb);
 	jpeg_src_buf->flags = (buf->m.planes[0].bytesused == 0) ?
 		MTK_JPEG_BUF_FLAGS_LAST_FRAME : MTK_JPEG_BUF_FLAGS_INIT;
-- 
2.20.1

[PATCH 3/5] media: mtk-vcodec: Use vb2_get_buffer

[Ezequiel Garcia]

Use the newly introduced vb2_get_buffer API and avoid
accessing buffers in the queue directly.

Cc: Tiffany Lin <tiffany.lin@mediatek.com>
Cc: Andrew-CT Chen <andrew-ct.chen@mediatek.com>
Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c
index 2c92ee4f0c8c..a26b7269d6f1 100644
--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c
@@ -872,11 +872,17 @@ static int vb2ops_venc_start_streaming(struct vb2_queue *q, unsigned int count)
 
 err_set_param:
 	for (i = 0; i < q->num_buffers; ++i) {
-		if (q->bufs[i]->state == VB2_BUF_STATE_ACTIVE) {
+		struct vb2_buffer *buf = vb2_get_buffer(q, i);
+
+		/*
+		 * FIXME: This check is not needed as only active buffers
+		 * can be marked as done.
+		 */
+		if (buf->state == VB2_BUF_STATE_ACTIVE) {
 			mtk_v4l2_debug(0, "[%d] id=%d, type=%d, %d -> VB2_BUF_STATE_QUEUED",
 					ctx->id, i, q->type,
-					(int)q->bufs[i]->state);
-			v4l2_m2m_buf_done(to_vb2_v4l2_buffer(q->bufs[i]),
+					(int)buf->state);
+			v4l2_m2m_buf_done(to_vb2_v4l2_buffer(buf),
 					VB2_BUF_STATE_QUEUED);
 		}
 	}
-- 
2.20.1

[PATCH 4/5] media: sti: Use vb2_get_buffer

[Ezequiel Garcia]

Use the newly introduced vb2_get_buffer API and avoid
accessing buffers in the queue directly.

Cc: Jean-Christophe Trotin <jean-christophe.trotin@st.com>
Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/media/platform/sti/hva/hva-v4l2.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/sti/hva/hva-v4l2.c b/drivers/media/platform/sti/hva/hva-v4l2.c
index c42623dccfd6..64004d15a9c9 100644
--- a/drivers/media/platform/sti/hva/hva-v4l2.c
+++ b/drivers/media/platform/sti/hva/hva-v4l2.c
@@ -566,6 +566,7 @@ static int hva_qbuf(struct file *file, void *priv, struct v4l2_buffer *buf)
 		 */
 		struct vb2_queue *vq;
 		struct hva_stream *stream;
+		struct vb2_buffer *vb2_buf;
 
 		vq = v4l2_m2m_get_vq(ctx->fh.m2m_ctx, buf->type);
 
@@ -575,7 +576,8 @@ static int hva_qbuf(struct file *file, void *priv, struct v4l2_buffer *buf)
 			return -EINVAL;
 		}
 
-		stream = (struct hva_stream *)vq->bufs[buf->index];
+		vb2_buf = vb2_get_buffer(vq, buf->index);
+		stream = to_hva_stream(to_vb2_v4l2_buffer(vb2_buf));
 		stream->bytesused = buf->bytesused;
 	}
 
-- 
2.20.1

[PATCH 5/5] media: rockchip: Use vb2_get_buffer

[Ezequiel Garcia]

Use the newly introduced vb2_get_buffer API and avoid
accessing buffers in the queue directly.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
 drivers/staging/media/rockchip/vpu/rockchip_vpu_drv.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/media/rockchip/vpu/rockchip_vpu_drv.c b/drivers/staging/media/rockchip/vpu/rockchip_vpu_drv.c
index b94ff97451db..ad17e04e701a 100644
--- a/drivers/staging/media/rockchip/vpu/rockchip_vpu_drv.c
+++ b/drivers/staging/media/rockchip/vpu/rockchip_vpu_drv.c
@@ -45,12 +45,15 @@ void *rockchip_vpu_get_ctrl(struct rockchip_vpu_ctx *ctx, u32 id)
 
 dma_addr_t rockchip_vpu_get_ref(struct vb2_queue *q, u64 ts)
 {
+	struct vb2_buffer *buf;
 	int index;
 
 	index = vb2_find_timestamp(q, ts, 0);
-	if (index >= 0)
-		return vb2_dma_contig_plane_dma_addr(q->bufs[index], 0);
-	return 0;
+	if (index < 0)
+		return 0;
+
+	buf = vb2_get_buffer(q, index);
+	return vb2_dma_contig_plane_dma_addr(buf, 0);
 }
 
 static int
-- 
2.20.1

Re: [PATCH 0/5] media: Access videobuf2 buffers via an accessor

[Boris Brezillon]

On Thu,  6 Jun 2019 12:44:21 -0300
Ezequiel Garcia <ezequiel@collabora.com> wrote:

> Hi,
> 
> This patchset introduces a new vb2_get_buffer accessor and then
> uses it on all the drivers that are accessing videobuf2
> private buffer array directly.

Just curious, how did you find all occurrences of direct q->bufs[]
accesses? If you used a cocci script it might be worth submitting it so
we don't end up with new offenders of the "don't access q->bufs[]
directly" rule.

> 
> I'm skipping Intel IPU3 driver here, since the code goes beyond
> just accessing the buffer. It also modifies the buffer queue
> directly. I believe this driver would need some more cleanup
> and love from its maintainers.
> 
> Note that OMAP2/OMAP3 display driver is videobuf1 and so not
> affected by this change.
> 
> Lastly, note that I'm doing the minimum changes to drivers I can't test,
> only using the new accessor and avoiding any further changes.

Can you also add a patch to remove the private buf pointers array in the
cedrus driver?

> `
> Thanks,
> Ezequiel
> 
> Ezequiel Garcia (5):
>   media: vb2: Introduce a vb2_get_buffer accessor
>   media: mtk-jpeg: Use vb2_get_buffer
>   media: mtk-vcodec: Use vb2_get_buffer
>   media: sti: Use vb2_get_buffer
>   media: rockchip: Use vb2_get_buffer
> 
>  .../media/platform/mtk-jpeg/mtk_jpeg_core.c    |  2 +-
>  .../media/platform/mtk-vcodec/mtk_vcodec_enc.c | 12 +++++++++---
>  drivers/media/platform/sti/hva/hva-v4l2.c      |  4 +++-
>  .../media/rockchip/vpu/rockchip_vpu_drv.c      |  9 ++++++---
>  include/media/videobuf2-core.h                 | 18 ++++++++++++++++++
>  5 files changed, 37 insertions(+), 8 deletions(-)
> 

Re: [PATCH 0/5] media: Access videobuf2 buffers via an accessor

[Ezequiel Garcia]

On Thu, 2019-06-06 at 19:43 +0200, Boris Brezillon wrote:
> On Thu,  6 Jun 2019 12:44:21 -0300
> Ezequiel Garcia <ezequiel@collabora.com> wrote:
> 
> > Hi,
> > 
> > This patchset introduces a new vb2_get_buffer accessor and then
> > uses it on all the drivers that are accessing videobuf2
> > private buffer array directly.
> 
> Just curious, how did you find all occurrences of direct q->bufs[]
> accesses? If you used a cocci script it might be worth submitting it so
> we don't end up with new offenders of the "don't access q->bufs[]
> directly" rule.
> 

No, I just inspected the code and tried a few grep variants.

Hopefully, I haven't missed any!

> > I'm skipping Intel IPU3 driver here, since the code goes beyond
> > just accessing the buffer. It also modifies the buffer queue
> > directly. I believe this driver would need some more cleanup
> > and love from its maintainers.
> > 
> > Note that OMAP2/OMAP3 display driver is videobuf1 and so not
> > affected by this change.
> > 
> > Lastly, note that I'm doing the minimum changes to drivers I can't test,
> > only using the new accessor and avoiding any further changes.
> 
> Can you also add a patch to remove the private buf pointers array in the
> cedrus driver?
> 

You mean removing the dst_bufs field?

I can but it's not part of this series, is it?

And I'd rather someone else test it, as my cedrus boards
are not wired at the moment.
 
> > `
> > Thanks,
> > Ezequiel
> > 
> > Ezequiel Garcia (5):
> >   media: vb2: Introduce a vb2_get_buffer accessor
> >   media: mtk-jpeg: Use vb2_get_buffer
> >   media: mtk-vcodec: Use vb2_get_buffer
> >   media: sti: Use vb2_get_buffer
> >   media: rockchip: Use vb2_get_buffer
> > 
> >  .../media/platform/mtk-jpeg/mtk_jpeg_core.c    |  2 +-
> >  .../media/platform/mtk-vcodec/mtk_vcodec_enc.c | 12 +++++++++---
> >  drivers/media/platform/sti/hva/hva-v4l2.c      |  4 +++-
> >  .../media/rockchip/vpu/rockchip_vpu_drv.c      |  9 ++++++---
> >  include/media/videobuf2-core.h                 | 18 ++++++++++++++++++
> >  5 files changed, 37 insertions(+), 8 deletions(-)
> > 

Re: [PATCH 0/5] media: Access videobuf2 buffers via an accessor

[Boris Brezillon]

On Thu, 06 Jun 2019 15:13:00 -0300
Ezequiel Garcia <ezequiel@collabora.com> wrote:

> On Thu, 2019-06-06 at 19:43 +0200, Boris Brezillon wrote:
> > On Thu,  6 Jun 2019 12:44:21 -0300
> > Ezequiel Garcia <ezequiel@collabora.com> wrote:
> >   
> > > Hi,
> > > 
> > > This patchset introduces a new vb2_get_buffer accessor and then
> > > uses it on all the drivers that are accessing videobuf2
> > > private buffer array directly.  
> > 
> > Just curious, how did you find all occurrences of direct q->bufs[]
> > accesses? If you used a cocci script it might be worth submitting it so
> > we don't end up with new offenders of the "don't access q->bufs[]
> > directly" rule.
> >   
> 
> No, I just inspected the code and tried a few grep variants.

Okay.

> 
> Hopefully, I haven't missed any!
> 
> > > I'm skipping Intel IPU3 driver here, since the code goes beyond
> > > just accessing the buffer. It also modifies the buffer queue
> > > directly. I believe this driver would need some more cleanup
> > > and love from its maintainers.
> > > 
> > > Note that OMAP2/OMAP3 display driver is videobuf1 and so not
> > > affected by this change.
> > > 
> > > Lastly, note that I'm doing the minimum changes to drivers I can't test,
> > > only using the new accessor and avoiding any further changes.  
> > 
> > Can you also add a patch to remove the private buf pointers array in the
> > cedrus driver?
> >   
> 
> You mean removing the dst_bufs field?

Yes.

> 
> I can but it's not part of this series, is it?

Fair enough.

> 
> And I'd rather someone else test it, as my cedrus boards
> are not wired at the moment.

I guess we can ask Jernej, Jonas, Paul or Maxime if they can test.