| Message ID | 20240814020643.2229637-3-yunkec@chromium.org (mailing list archive) |
|---|---|
| State | New |
| 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 Tudor, On Tue, Nov 5, 2024 at 2:40 AM Tudor Ambarus <tudor.ambarus@linaro.org> wrote: > > +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. Thanks for the report! If I remember correctly the idea was that if the buffer was validated to be the same as given previously, there is no need to update the fields, because they should already be the same. However, I can see that this may not be true for bytesused, m.fd and data_offset in some scenarios. > > > - 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; > - } 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)? Best regards, Tomasz > + __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); I > + > 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);