media: Access videobuf2 buffers via an accessor

PATCH Superseded Ezequiel Garcia 2 participants 10 messages Jun 6, 2019

Subsystems (5)

Media (V4L/DVB) Staging Arm MediaTek Arm Rockchip +1 more subsystems

lore

Share & tools

lore

Ezequiel Garcia

author

2019-06-06 15:45 UTC #0 lore raw

cover media: Access videobuf2 buffers via an accessor

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(-)

Signature · 1 line

2.20.1

Ezequiel Garcia

author

2019-06-06 15:45 UTC #1 lore raw

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

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 <[email protected]>

---
 include/media/videobuf2-core.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

include/media/videobuf2-core.h +18 -0

index c03ef7cc5071..7b1729320b16 100644
@@ -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-*.

Signature · 1 line

2.20.1

Boris Brezillon

Reviewed-by on v1 1/5

2019-06-06 17:39 UTC #2 lore raw

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

Quoted 21 lines · On Thu, 6 Jun 2019 12:44:22 -0300 Ezequiel Garcia <[email protected]>…

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 <[email protected]>
---
 include/media/videobuf2-core.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
include/media/videobuf2-core.h +4 -0
index c03ef7cc5071..7b1729320b16 100644
@@ -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

include/media/videobuf2-core.h static inline void vb2_clear_last_buffer_dequeued(struct vb2_queue *q) · line 1179

Quoted 10 lines

include/media/videobuf2-core.h +10 -0

+ *
+ * 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 <[email protected]>

Quoted 7 lines

include/media/videobuf2-core.h +4 -0

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

Ezequiel Garcia

author

2019-06-06 15:45 UTC #3 lore raw

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

In reply to Ezequiel Garcia

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

Cc Rick Chang <[email protected]>

Cc Bin Liu <[email protected]>

Signed-off-by Ezequiel Garcia <[email protected]>

---
 drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/media/platform/mtk-jpeg/mtk_jpeg_core.c +1 -1

index 3b199662cb34..f8f808abada1 100644
@@ -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;

Signature · 1 line

2.20.1

Ezequiel Garcia

author

2019-06-06 15:45 UTC #4 lore raw

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

In reply to Ezequiel Garcia

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

Cc Tiffany Lin <[email protected]>

Cc Andrew-CT Chen <[email protected]>

Signed-off-by Ezequiel Garcia <[email protected]>

---
 drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c +9 -3

index 2c92ee4f0c8c..a26b7269d6f1 100644
@@ -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);
 		}
 	}

Signature · 1 line

2.20.1

Ezequiel Garcia

author

2019-06-06 15:45 UTC #5 lore raw

PATCH 4/5 media: sti: Use vb2_get_buffer

In reply to Ezequiel Garcia

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

Cc Jean-Christophe Trotin <[email protected]>

Signed-off-by Ezequiel Garcia <[email protected]>

---
 drivers/media/platform/sti/hva/hva-v4l2.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

drivers/media/platform/sti/hva/hva-v4l2.c +3 -1

index c42623dccfd6..64004d15a9c9 100644
@@ -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;
 	}

Signature · 1 line

2.20.1

Ezequiel Garcia

author

2019-06-06 15:45 UTC #6 lore raw

PATCH 5/5 media: rockchip: Use vb2_get_buffer

In reply to Ezequiel Garcia

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

Signed-off-by Ezequiel Garcia <[email protected]>

---
 drivers/staging/media/rockchip/vpu/rockchip_vpu_drv.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

drivers/staging/media/rockchip/vpu/rockchip_vpu_drv.c +6 -3

index b94ff97451db..ad17e04e701a 100644
@@ -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

Signature · 1 line

2.20.1

Boris Brezillon

2019-06-06 17:43 UTC #7 lore raw

In reply to Ezequiel Garcia

Quoted 5 lines · On Thu, 6 Jun 2019 12:44:21 -0300 Ezequiel Garcia <[email protected]>…

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.

Quoted 11 lines

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?

Quoted 18 lines

`
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(-)

Ezequiel Garcia

author

2019-06-06 18:13 UTC #8 lore raw

Quoted 5 lines · Boris Brezillon

Quoted 2 lines · On Thu, 6 Jun 2019 12:44:21 -0300 Ezequiel Garcia <[email protected]>…
Hi,

Quoted 9 lines

Quoted 3 lines
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!

Quoted 5 lines

Quoted 5 lines
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.

Quoted 9 lines

Quoted 5 lines
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.

Quoted 18 lines

Quoted 18 lines
`
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(-)

Boris Brezillon

2019-06-06 19:08 UTC #9 lore raw

Quoted 10 lines · On Thu, 06 Jun 2019 15:13:00 -0300 Ezequiel Garcia <[email protected]>…

Quoted 9 lines · Boris Brezillon
Quoted 5 lines · On Thu, 6 Jun 2019 12:44:21 -0300 Ezequiel Garcia <[email protected]>…
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.

Quoted 7 lines

Quoted 5 lines
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.

Quoted 11 lines

Hopefully, I haven't missed any!
Quoted 8 lines
Quoted 8 lines
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.

Quoted 8 lines

Quoted 6 lines
Quoted 2 lines
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.

Quoted 2 lines

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

Fair enough.

Quoted 3 lines

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.