| Message ID | 20240814020643.2229637-3-yunkec@chromium.org |
|---|---|
| State | Accepted |
| Commit | 95af7c00f35b9cb0873afb8f191f42e04714ed2a |
| Headers | show |
| Series | media: videobuf2-core: attach once if multiple planes share the same dbuf | expand |
+linux-samsung-soc@vger.kernel.org Hi, Yunke, Tomasz, Hans, On 8/14/24 3:06 AM, Yunke Cao wrote: > 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> > --- > 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) cut > + > + if (reacquired) { cut > - /* > - * 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; > + } I'm running into an issue on my Pixel 6 device with this change. I see that this chunk of code was moved only for the `reacquired` case. > - 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); But then the above method is called, were the pixel downstream driver [1] tries to: bufcon_dmabuf[i] = dma_buf_get(vb->planes[i].m.fd); This fails with -EBADF as the core driver did not set vb->planes[plane].m.fd for `!reacquired`. The following diff makes the Pixel 6 downstream driver work as before this change. Shall we set the relevant data copied from userspace to vb->planes in the `!reacquired` case again? Thanks, ta [1] https://android.googlesource.com/kernel/gs/+/refs/tags/android-15.0.0_r0.14/drivers/media/platform/exynos/mfc/mfc_enc_vb2.c#215 diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c index 02fe81b9be28..0acaf8deaf78 100644 --- a/drivers/media/common/videobuf2/videobuf2-core.c +++ b/drivers/media/common/videobuf2/videobuf2-core.c @@ -1365,6 +1365,18 @@ static int __prepare_userptr(struct vb2_buffer *vb) return ret; } +static void __v2buf_set_planes(struct vb2_buffer *vb, struct vb2_plane *planes) +{ + unsigned int plane; + + 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; + } +} + /* * __prepare_dmabuf() - prepare a DMABUF buffer */ @@ -1459,12 +1471,7 @@ static int __prepare_dmabuf(struct vb2_buffer *vb) * 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; - } + __v2buf_set_planes(vb, planes); /* * Call driver-specific initialization on the newly acquired buffer, @@ -1476,6 +1483,8 @@ static int __prepare_dmabuf(struct vb2_buffer *vb) goto err_put_vb2_buf; } } else { + __v2buf_set_planes(vb, planes); + for (plane = 0; plane < vb->num_planes; ++plane) dma_buf_put(planes[plane].dbuf); }
Hi, Tomasz, On 11/5/24 5:24 AM, Tomasz Figa wrote: > I think it should be fine to just move the following parts outside of > this if block and then call the buf_init op conditionally if > (reacquired), like it was in the old code. > > Would you mind sending a fix patch (with a Fixes: tag)? Not at all, thanks for the suggestion. Will test and send it today. Thanks, ta
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);