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

[PATCH v2 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.

Changes from v1:

* Address feedback from Boris, and drop redundant check.

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

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
---
Changes from v1:
* Drop redundant num_buffers > 0 check.
---
 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..640aabe69450 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 (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

[PATCH v2 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 v2 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 67e8a023ef41..5198912a1996 100644
--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_enc.c
@@ -860,11 +860,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 v2 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 v2 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 v2 1/5] media: vb2: Introduce a vb2_get_buffer accessor

[Marek Szyprowski]

On 2019-06-10 22:55, Ezequiel Garcia 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.
>
> Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>
> Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>

Acked-by: Marek Szyprowski <m.szyprowski@samsung.com>

> ---
> Changes from v1:
> * Drop redundant num_buffers > 0 check.
> ---
>   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..640aabe69450 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 (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-*.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

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

[Hans Verkuil]

On 6/10/19 10:55 PM, Ezequiel Garcia wrote:
> Use the newly introduced vb2_get_buffer API and avoid
> accessing buffers in the queue directly.

Can you rebase this patch? It no longer applies after the hantro rename
was merged.

Thanks!

	Hans

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

[PATCH] media: hantro: 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/hantro/hantro_drv.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/media/hantro/hantro_drv.c b/drivers/staging/media/hantro/hantro_drv.c
index 1d3af881d088..c3665f0e87a2 100644
--- a/drivers/staging/media/hantro/hantro_drv.c
+++ b/drivers/staging/media/hantro/hantro_drv.c
@@ -45,12 +45,14 @@ void *hantro_get_ctrl(struct hantro_ctx *ctx, u32 id)
 
 dma_addr_t hantro_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] media: hantro: Use vb2_get_buffer

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2042 bytes --]

Hi Ezequiel,

I love your patch! Yet something to improve:

[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on next-20190621]
[cannot apply to v5.2-rc5]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Ezequiel-Garcia/media-hantro-Use-vb2_get_buffer/20190622-022557
base:   git://linuxtv.org/media_tree.git master
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 7.4.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.4.0 make.cross ARCH=arm 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All error/warnings (new ones prefixed by >>):

   drivers/staging//media/hantro/hantro_drv.c: In function 'hantro_get_ref':
>> drivers/staging//media/hantro/hantro_drv.c:54:8: error: implicit declaration of function 'vb2_get_buffer'; did you mean 'vb2_expbuf'? [-Werror=implicit-function-declaration]
     buf = vb2_get_buffer(q, index);
           ^~~~~~~~~~~~~~
           vb2_expbuf
>> drivers/staging//media/hantro/hantro_drv.c:54:6: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
     buf = vb2_get_buffer(q, index);
         ^
   cc1: some warnings being treated as errors

vim +54 drivers/staging//media/hantro/hantro_drv.c

    45	
    46	dma_addr_t hantro_get_ref(struct vb2_queue *q, u64 ts)
    47	{
    48		struct vb2_buffer *buf;
    49		int index;
    50	
    51		index = vb2_find_timestamp(q, ts, 0);
    52		if (index < 0)
    53			return 0;
  > 54		buf = vb2_get_buffer(q, index);
    55		return vb2_dma_contig_plane_dma_addr(buf, 0);
    56	}
    57	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 70710 bytes --]

Re: [PATCH] media: hantro: Use vb2_get_buffer

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2076 bytes --]

Hi Ezequiel,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on next-20190621]
[cannot apply to v5.2-rc5]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Ezequiel-Garcia/media-hantro-Use-vb2_get_buffer/20190622-022557
base:   git://linuxtv.org/media_tree.git master
config: nds32-allmodconfig (attached as .config)
compiler: nds32le-linux-gcc (GCC) 8.1.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=8.1.0 make.cross ARCH=nds32 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/staging/media/hantro/hantro_drv.c: In function 'hantro_get_ref':
   drivers/staging/media/hantro/hantro_drv.c:54:8: error: implicit declaration of function 'vb2_get_buffer'; did you mean 'vb2_expbuf'? [-Werror=implicit-function-declaration]
     buf = vb2_get_buffer(q, index);
           ^~~~~~~~~~~~~~
           vb2_expbuf
>> drivers/staging/media/hantro/hantro_drv.c:54:6: warning: assignment to 'struct vb2_buffer *' from 'int' makes pointer from integer without a cast [-Wint-conversion]
     buf = vb2_get_buffer(q, index);
         ^
   cc1: some warnings being treated as errors

vim +54 drivers/staging/media/hantro/hantro_drv.c

    45	
    46	dma_addr_t hantro_get_ref(struct vb2_queue *q, u64 ts)
    47	{
    48		struct vb2_buffer *buf;
    49		int index;
    50	
    51		index = vb2_find_timestamp(q, ts, 0);
    52		if (index < 0)
    53			return 0;
  > 54		buf = vb2_get_buffer(q, index);
    55		return vb2_dma_contig_plane_dma_addr(buf, 0);
    56	}
    57	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 52162 bytes --]