media: videobuf2-core: release all planes first in __prepare_dmabuf()

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>
Acked-by: Tomasz Figa <tfiga@chromium.org>
Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>

diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index 4d232b08f9505474aee234423a607db6848b8363..b53d94659e3068b0c00f6f75dd51723f41e6e3e8 100644 (file)
--- 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);