+linux-samsung-soc@xxxxxxxxxxxxxxx
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@xxxxxxxxxxxx>
> Acked-by: Tomasz Figa <tfiga@xxxxxxxxxxxx>
> ---
> 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);
}
