From: Tomasz Figa <tfiga@chromium.org>
To: Yunke Cao <yunkec@chromium.org>
Cc: Marek Szyprowski <m.szyprowski@samsung.com>,
Mauro Carvalho Chehab <mchehab@kernel.org>,
linux-media@vger.kernel.org
Subject: Re: [PATCH v2 1/3] media: videobuf2-core: release all planes first in __prepare_dmabuf()
Date: Fri, 17 May 2024 20:11:39 +0900 [thread overview]
Message-ID: <idyreh3tm33dappbaoek43urhr75jhcu44l6d67qszos42yhcp@mqxlfmdmryto> (raw)
In-Reply-To: <20240403091306.1308878-2-yunkec@chromium.org>
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@chromium.org>
> ---
> .../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
next prev parent reply other threads:[~2024-05-17 11:11 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-03 9:13 [PATCH v2 0/3] media: videobuf2-core: attach once if multiple planes share the same dbuf Yunke Cao
2024-04-03 9:13 ` [PATCH v2 1/3] media: videobuf2-core: release all planes first in __prepare_dmabuf() Yunke Cao
2024-05-17 11:11 ` Tomasz Figa [this message]
2024-05-30 4:13 ` Yunke Cao
2024-04-03 9:13 ` [PATCH v2 2/3] media: videobuf2-core: reverse the iteration order in __vb2_buf_dmabuf_put Yunke Cao
2024-04-24 10:24 ` Hans Verkuil
2024-05-17 11:14 ` Tomasz Figa
2024-04-03 9:13 ` [PATCH v2 3/3] media: videobuf2-core: attach once if multiple planes share the same dbuf Yunke Cao
2024-05-17 11:23 ` Tomasz Figa
2024-05-20 2:19 ` Yunke Cao
2024-04-24 10:25 ` [PATCH v2 0/3] " Hans Verkuil
2024-05-08 16:02 ` Tomasz Figa
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=idyreh3tm33dappbaoek43urhr75jhcu44l6d67qszos42yhcp@mqxlfmdmryto \
--to=tfiga@chromium.org \
--cc=linux-media@vger.kernel.org \
--cc=m.szyprowski@samsung.com \
--cc=mchehab@kernel.org \
--cc=yunkec@chromium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).