Hi Yunke,
On Wed, Apr 03, 2024 at 06:13:04PM +0900, Yunke Cao wrote:
> 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>
> ---
> .../media/common/videobuf2/videobuf2-core.c | 64 ++++++++++---------
> 1 file changed, 34 insertions(+), 30 deletions(-)
>
Thanks for the second revision and sorry for the delay. Please check my
comments inline.
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index b6bf8f232f48..702f7b6f783a 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -1341,11 +1341,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_dbuf;
nit: Maybe err_put_planes, since we're cleaning up the planes[] array?
> }
>
> /* use DMABUF size if length is not provided */
> @@ -1356,17 +1358,14 @@ 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_dbuf;
> }
>
> /* 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);
>
> @@ -1375,29 +1374,30 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
> vb->copied_timestamp = 0;
> call_void_vb_qop(vb, buf_cleanup, vb);
Would it make sense to also move these two to the if (reacquired) part
below, since they are done once for the entire vb?
> }
> + }
>
> - /* Release previously acquired memory if present */
> - __vb2_plane_dmabuf_put(vb, &vb->planes[plane]);
> - vb->planes[plane].bytesused = 0;
> - vb->planes[plane].length = 0;
> - vb->planes[plane].m.fd = 0;
> - vb->planes[plane].data_offset = 0;
I don't see the code below setting the 4 fields above to zero. Is it
intended?
> + if (reacquired) {
> + __vb2_buf_dmabuf_put(vb);
> +
> + 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_dbuf;
Hmm, I think in this case we need to also clean up the partially acquired
planes of vb.
> + }
>
> - /* 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 = planes[plane].dbuf;
> + vb->planes[plane].mem_priv = mem_priv;
> }
> -
> - vb->planes[plane].dbuf = dbuf;
> - vb->planes[plane].mem_priv = mem_priv;
> + } else {
> + for (plane = 0; plane < vb->num_planes; ++plane)
> + dma_buf_put(planes[plane].dbuf);
> }
>
> /*
> @@ -1413,7 +1413,7 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
> if (ret) {
> dprintk(q, 1, "failed to map dmabuf for plane %d\n",
> plane);
> - goto err;
> + goto err_put_vb2_buf;
> }
> vb->planes[plane].dbuf_mapped = 1;
> }
I think this entire loop can also go under the (reacquired) case, since
(!reacquired) means that all the planes were identical (and thus are
alreday mapped). Given that now we release all the planes in one go, we
could even simplify it by dropping the dbuf_mapped check from the loop.
> @@ -1437,7 +1437,7 @@ 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;
> }
> }
Same for this block.
>
> @@ -1445,11 +1445,15 @@ static int __prepare_dmabuf(struct vb2_buffer *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_dbuf:
> + for (plane = 0; plane < vb->num_planes; ++plane)
dma_buf_put() will throw a warning if the dmabuf pointer is NULL and just
plain crash if IS_ERR(), so we shouldn't call it on array elements that we
didn't succeed for.
> + 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);
Actually, would it make sense to invert the order of clean-up steps here?
In case if only the first loop fails, we don't really need to do anything with
vb. Or am I missing something?
Best regards,
Tomasz
