In the existing implementation, validating planes, checking if the planes
changed, releasing previous planes and reaquiring new planes all happens in
the same for loop.
Split the for loop into 3 parts
1. In the first for loop, validate planes and check if planes changed.
2. Call __vb2_buf_dmabuf_put() to release all planes.
3. In the second for loop, reaquire new planes.
Signed-off-by: Yunke Cao <yunkec@xxxxxxxxxxxx>
Acked-by: Tomasz Figa <tfiga@xxxxxxxxxxxx>
---
v3:
- Applied Tomasz's review comment:
- Rename err_put_dbuf as err_put_planes.
- Move code that only executed once into if (reacquired) to simply it.
- In error handling, only call dma_buf_put() for valid pointers.
---
.../media/common/videobuf2/videobuf2-core.c | 115 +++++++++---------
1 file changed, 59 insertions(+), 56 deletions(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index 4d232b08f950..b53d94659e30 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -1387,11 +1387,13 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
for (plane = 0; plane < vb->num_planes; ++plane) {
struct dma_buf *dbuf = dma_buf_get(planes[plane].m.fd);
+ planes[plane].dbuf = dbuf;
+
if (IS_ERR_OR_NULL(dbuf)) {
dprintk(q, 1, "invalid dmabuf fd for plane %d\n",
plane);
ret = -EINVAL;
- goto err;
+ goto err_put_planes;
}
/* use DMABUF size if length is not provided */
@@ -1402,76 +1404,68 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
dprintk(q, 1, "invalid dmabuf length %u for plane %d, minimum length %u\n",
planes[plane].length, plane,
vb->planes[plane].min_length);
- dma_buf_put(dbuf);
ret = -EINVAL;
- goto err;
+ goto err_put_planes;
}
/* Skip the plane if already verified */
if (dbuf == vb->planes[plane].dbuf &&
- vb->planes[plane].length == planes[plane].length) {
- dma_buf_put(dbuf);
+ vb->planes[plane].length == planes[plane].length)
continue;
- }
dprintk(q, 3, "buffer for plane %d changed\n", plane);
- if (!reacquired) {
- reacquired = true;
+ reacquired = true;
+ }
+
+ if (reacquired) {
+ if (vb->planes[0].mem_priv) {
vb->copied_timestamp = 0;
call_void_vb_qop(vb, buf_cleanup, vb);
+ __vb2_buf_dmabuf_put(vb);
}
- /* Release previously acquired memory if present */
- __vb2_plane_dmabuf_put(vb, &vb->planes[plane]);
-
- /* Acquire each plane's memory */
- mem_priv = call_ptr_memop(attach_dmabuf,
- vb,
- q->alloc_devs[plane] ? : q->dev,
- dbuf,
- planes[plane].length);
- if (IS_ERR(mem_priv)) {
- dprintk(q, 1, "failed to attach dmabuf\n");
- ret = PTR_ERR(mem_priv);
- dma_buf_put(dbuf);
- goto err;
- }
-
- vb->planes[plane].dbuf = dbuf;
- vb->planes[plane].mem_priv = mem_priv;
- }
+ for (plane = 0; plane < vb->num_planes; ++plane) {
+ /* Acquire each plane's memory */
+ mem_priv = call_ptr_memop(attach_dmabuf,
+ vb,
+ q->alloc_devs[plane] ? : q->dev,
+ planes[plane].dbuf,
+ planes[plane].length);
+ if (IS_ERR(mem_priv)) {
+ dprintk(q, 1, "failed to attach dmabuf\n");
+ ret = PTR_ERR(mem_priv);
+ goto err_put_vb2_buf;
+ }
- /*
- * This pins the buffer(s) with dma_buf_map_attachment()). It's done
- * here instead just before the DMA, while queueing the buffer(s) so
- * userspace knows sooner rather than later if the dma-buf map fails.
- */
- for (plane = 0; plane < vb->num_planes; ++plane) {
- if (vb->planes[plane].dbuf_mapped)
- continue;
+ vb->planes[plane].dbuf = planes[plane].dbuf;
+ vb->planes[plane].mem_priv = mem_priv;
- ret = call_memop(vb, map_dmabuf, vb->planes[plane].mem_priv);
- if (ret) {
- dprintk(q, 1, "failed to map dmabuf for plane %d\n",
- plane);
- goto err;
+ /*
+ * This pins the buffer(s) with dma_buf_map_attachment()). It's done
+ * here instead just before the DMA, while queueing the buffer(s) so
+ * userspace knows sooner rather than later if the dma-buf map fails.
+ */
+ ret = call_memop(vb, map_dmabuf, vb->planes[plane].mem_priv);
+ if (ret) {
+ dprintk(q, 1, "failed to map dmabuf for plane %d\n",
+ plane);
+ goto err_put_vb2_buf;
+ }
+ vb->planes[plane].dbuf_mapped = 1;
}
- vb->planes[plane].dbuf_mapped = 1;
- }
- /*
- * Now that everything is in order, copy relevant information
- * provided by userspace.
- */
- for (plane = 0; plane < vb->num_planes; ++plane) {
- vb->planes[plane].bytesused = planes[plane].bytesused;
- vb->planes[plane].length = planes[plane].length;
- vb->planes[plane].m.fd = planes[plane].m.fd;
- vb->planes[plane].data_offset = planes[plane].data_offset;
- }
+ /*
+ * Now that everything is in order, copy relevant information
+ * provided by userspace.
+ */
+ for (plane = 0; plane < vb->num_planes; ++plane) {
+ vb->planes[plane].bytesused = planes[plane].bytesused;
+ vb->planes[plane].length = planes[plane].length;
+ vb->planes[plane].m.fd = planes[plane].m.fd;
+ vb->planes[plane].data_offset = planes[plane].data_offset;
+ }
- if (reacquired) {
/*
* Call driver-specific initialization on the newly acquired buffer,
* if provided.
@@ -1479,19 +1473,28 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
ret = call_vb_qop(vb, buf_init, vb);
if (ret) {
dprintk(q, 1, "buffer initialization failed\n");
- goto err;
+ goto err_put_vb2_buf;
}
+ } else {
+ for (plane = 0; plane < vb->num_planes; ++plane)
+ dma_buf_put(planes[plane].dbuf);
}
ret = call_vb_qop(vb, buf_prepare, vb);
if (ret) {
dprintk(q, 1, "buffer preparation failed\n");
call_void_vb_qop(vb, buf_cleanup, vb);
- goto err;
+ goto err_put_vb2_buf;
}
return 0;
-err:
+
+err_put_planes:
+ for (plane = 0; plane < vb->num_planes; ++plane) {
+ if (!IS_ERR_OR_NULL(planes[plane].dbuf))
+ dma_buf_put(planes[plane].dbuf);
+ }
+err_put_vb2_buf:
/* In case of errors, release planes that were already acquired */
__vb2_buf_dmabuf_put(vb);
--
2.46.0.184.g6999bdac58-goog
