Hi Tudor,
On Tue, Nov 5, 2024 at 2:40 AM Tudor Ambarus <tudor.ambarus@xxxxxxxxxx> wrote:
>
> +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.
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);
> }
