@@ -1388,11 +1388,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 */
@@ -1403,76 +1405,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.
@@ -1480,19 +1474,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);
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@chromium.org> --- 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(-)