media: Access videobuf2 buffers via an accessor
Subsystems (5)
v2 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.
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(-)
Signature · 1 line
2.20.1
PATCH v2 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.
---
Changes from v1:
* Drop redundant num_buffers > 0 check.
--- include/media/videobuf2-core.h | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
include/media/videobuf2-core.h +18 -0
@@ -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-*.
Signature · 1 line
2.20.1
Re: [PATCH v2 1/5] media: vb2: Introduce a vb2_get_buffer accessor
Quoted 6 lines · 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 <[email protected]>Signed-off-by Ezequiel Garcia <[email protected]>
Quoted 26 lines
---
Changes from v1:
* Drop redundantnum_buffers> 0 check.--- include/media/videobuf2-core.h | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)include/media/videobuf2-core.h +11
-0@@ -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,
include/media/videobuf2-core.h static inline void vb2_clear_last_buffer_dequeued(struct vb2_queue *q) · line 1186
Quoted 10 lines
include/media/videobuf2-core.h +7
-0+ 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
Signature · 2 lines
Marek Szyprowski, PhD Samsung R&D Institute Poland
PATCH v2 2/5 media: mtk-jpeg: Use vb2_get_buffer
In reply to Ezequiel GarciaUse the newly introduced vb2_get_buffer API and avoid accessing buffers in the queue directly.
--- 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
@@ -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
PATCH v2 3/5 media: mtk-vcodec: Use vb2_get_buffer
In reply to Ezequiel GarciaUse the newly introduced vb2_get_buffer API and avoid accessing buffers in the queue directly.
--- 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
@@ -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 ); } }
Signature · 1 line
2.20.1
PATCH v2 4/5 media: sti: Use vb2_get_buffer
In reply to Ezequiel GarciaUse the newly introduced vb2_get_buffer API and avoid accessing buffers in the queue directly.
--- 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
@@ -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
PATCH v2 5/5 media: rockchip: Use vb2_get_buffer
In reply to Ezequiel GarciaUse the newly introduced vb2_get_buffer API and avoid accessing buffers in the queue directly.
--- 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
@@ -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
Re: [PATCH v2 5/5] media: rockchip: Use vb2_get_buffer
Quoted 2 lines · Ezequiel Garcia
Use the newly introduced
vb2_get_bufferAPI 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
Quoted 30 lines
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@@ -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 1/1 media: hantro: Use vb2_get_buffer
Use the newly introduced vb2_get_buffer API and avoid accessing buffers in the queue directly.
--- drivers/staging/media/hantro/hantro_drv.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
drivers/staging/media/hantro/hantro_drv.c +5 -3
@@ -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
Signature · 1 line
2.20.1
kbuild test robot
build botRe: [PATCH] media: hantro: Use vb2_get_buffer
I love your patch! Yet something to improve:
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
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
kbuild test robot
build botRe: [PATCH] media: hantro: Use vb2_get_buffer
In reply to Ezequiel GarciaI love your patch! Perhaps something to improve:
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
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