* [PATCH] media: videobuf2-core: copy vb planes unconditionally
@ 2024-11-06 12:18 Tudor Ambarus
2024-11-06 17:04 ` William McVicker
2024-11-06 23:41 ` Tomasz Figa
0 siblings, 2 replies; 4+ messages in thread
From: Tudor Ambarus @ 2024-11-06 12:18 UTC (permalink / raw)
To: tfiga
Cc: m.szyprowski, mchehab, yunkec, hverkuil, linux-media,
linux-kernel, linux-samsung-soc, andre.draszik, kernel-team,
willmcvicker, Tudor Ambarus, stable
Copy the relevant data from userspace to the vb->planes unconditionally
as it's possible some of the fields may have changed after the buffer
has been validated.
Keep the dma_buf_put(planes[plane].dbuf) calls in the first
`if (!reacquired)` case, in order to be close to the plane validation code
where the buffers were got in the first place.
Cc: stable@vger.kernel.org
Fixes: 95af7c00f35b ("media: videobuf2-core: release all planes first in __prepare_dmabuf()")
Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
---
.../media/common/videobuf2/videobuf2-core.c | 28 ++++++++++---------
1 file changed, 15 insertions(+), 13 deletions(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index f07dc53a9d06..c0cc441b5164 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -1482,18 +1482,23 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
}
vb->planes[plane].dbuf_mapped = 1;
}
+ } else {
+ for (plane = 0; plane < vb->num_planes; ++plane)
+ dma_buf_put(planes[plane].dbuf);
+ }
- /*
- * 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.
@@ -1503,9 +1508,6 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
dprintk(q, 1, "buffer initialization failed\n");
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);
--
2.47.0.199.ga7371fff76-goog
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] media: videobuf2-core: copy vb planes unconditionally
2024-11-06 12:18 [PATCH] media: videobuf2-core: copy vb planes unconditionally Tudor Ambarus
@ 2024-11-06 17:04 ` William McVicker
2024-11-06 23:41 ` Tomasz Figa
1 sibling, 0 replies; 4+ messages in thread
From: William McVicker @ 2024-11-06 17:04 UTC (permalink / raw)
To: Tudor Ambarus
Cc: tfiga, m.szyprowski, mchehab, yunkec, hverkuil, linux-media,
linux-kernel, linux-samsung-soc, andre.draszik, kernel-team,
stable
Hi Tudor,
On 11/06/2024, Tudor Ambarus wrote:
> Copy the relevant data from userspace to the vb->planes unconditionally
> as it's possible some of the fields may have changed after the buffer
> has been validated.
>
> Keep the dma_buf_put(planes[plane].dbuf) calls in the first
> `if (!reacquired)` case, in order to be close to the plane validation code
> where the buffers were got in the first place.
>
> Cc: stable@vger.kernel.org
> Fixes: 95af7c00f35b ("media: videobuf2-core: release all planes first in __prepare_dmabuf()")
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
Thanks for sending this fix! I have tested that this fixes the video playback
issues I was seeing on my Pixel 6 device. Feel free to include my
Tested-by: Will McVicker <willmcvicker@google.com>
Thanks!
--Will
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] media: videobuf2-core: copy vb planes unconditionally
2024-11-06 12:18 [PATCH] media: videobuf2-core: copy vb planes unconditionally Tudor Ambarus
2024-11-06 17:04 ` William McVicker
@ 2024-11-06 23:41 ` Tomasz Figa
2024-11-07 9:17 ` Hans Verkuil
1 sibling, 1 reply; 4+ messages in thread
From: Tomasz Figa @ 2024-11-06 23:41 UTC (permalink / raw)
To: Tudor Ambarus
Cc: m.szyprowski, mchehab, yunkec, hverkuil, linux-media,
linux-kernel, linux-samsung-soc, andre.draszik, kernel-team,
willmcvicker, stable
On Wed, Nov 6, 2024 at 9:18 PM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>
> Copy the relevant data from userspace to the vb->planes unconditionally
> as it's possible some of the fields may have changed after the buffer
> has been validated.
>
> Keep the dma_buf_put(planes[plane].dbuf) calls in the first
> `if (!reacquired)` case, in order to be close to the plane validation code
> where the buffers were got in the first place.
>
> Cc: stable@vger.kernel.org
> Fixes: 95af7c00f35b ("media: videobuf2-core: release all planes first in __prepare_dmabuf()")
> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
> ---
> .../media/common/videobuf2/videobuf2-core.c | 28 ++++++++++---------
> 1 file changed, 15 insertions(+), 13 deletions(-)
>
Thanks for the fix.
Acked-by: Tomasz Figa <tfiga@chromium.org>
(We probably need some tests to verify this behavior... It seems like
the way v4l2-compliance is implemented [1] would only trigger the
!reacquired case on most drivers.)
[1] https://git.linuxtv.org/v4l-utils.git/tree/utils/v4l2-compliance/v4l2-test-buffers.cpp#n2071
(just queuing all imported buffers in order and re-queuing them
exactly as they are dequeued [2])
[2] https://git.linuxtv.org/v4l-utils.git/tree/utils/v4l2-compliance/v4l2-test-buffers.cpp#n1299
Best regards,
Tomasz
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index f07dc53a9d06..c0cc441b5164 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -1482,18 +1482,23 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
> }
> vb->planes[plane].dbuf_mapped = 1;
> }
> + } else {
> + for (plane = 0; plane < vb->num_planes; ++plane)
> + dma_buf_put(planes[plane].dbuf);
> + }
>
> - /*
> - * 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.
> @@ -1503,9 +1508,6 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
> dprintk(q, 1, "buffer initialization failed\n");
> 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);
> --
> 2.47.0.199.ga7371fff76-goog
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] media: videobuf2-core: copy vb planes unconditionally
2024-11-06 23:41 ` Tomasz Figa
@ 2024-11-07 9:17 ` Hans Verkuil
0 siblings, 0 replies; 4+ messages in thread
From: Hans Verkuil @ 2024-11-07 9:17 UTC (permalink / raw)
To: Tomasz Figa, Tudor Ambarus
Cc: m.szyprowski, mchehab, yunkec, linux-media, linux-kernel,
linux-samsung-soc, andre.draszik, kernel-team, willmcvicker,
stable
On 07/11/2024 00:41, Tomasz Figa wrote:
> On Wed, Nov 6, 2024 at 9:18 PM Tudor Ambarus <tudor.ambarus@linaro.org> wrote:
>>
>> Copy the relevant data from userspace to the vb->planes unconditionally
>> as it's possible some of the fields may have changed after the buffer
>> has been validated.
>>
>> Keep the dma_buf_put(planes[plane].dbuf) calls in the first
>> `if (!reacquired)` case, in order to be close to the plane validation code
>> where the buffers were got in the first place.
>>
>> Cc: stable@vger.kernel.org
>> Fixes: 95af7c00f35b ("media: videobuf2-core: release all planes first in __prepare_dmabuf()")
>> Signed-off-by: Tudor Ambarus <tudor.ambarus@linaro.org>
>> ---
>> .../media/common/videobuf2/videobuf2-core.c | 28 ++++++++++---------
>> 1 file changed, 15 insertions(+), 13 deletions(-)
>>
>
> Thanks for the fix.
>
> Acked-by: Tomasz Figa <tfiga@chromium.org>
>
> (We probably need some tests to verify this behavior... It seems like
> the way v4l2-compliance is implemented [1] would only trigger the
> !reacquired case on most drivers.)
>
> [1] https://git.linuxtv.org/v4l-utils.git/tree/utils/v4l2-compliance/v4l2-test-buffers.cpp#n2071
> (just queuing all imported buffers in order and re-queuing them
> exactly as they are dequeued [2])
> [2] https://git.linuxtv.org/v4l-utils.git/tree/utils/v4l2-compliance/v4l2-test-buffers.cpp#n1299
I'll see if I can improve that test.
Regards,
Hans
>
> Best regards,
> Tomasz
>
>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
>> index f07dc53a9d06..c0cc441b5164 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
>> @@ -1482,18 +1482,23 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
>> }
>> vb->planes[plane].dbuf_mapped = 1;
>> }
>> + } else {
>> + for (plane = 0; plane < vb->num_planes; ++plane)
>> + dma_buf_put(planes[plane].dbuf);
>> + }
>>
>> - /*
>> - * 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.
>> @@ -1503,9 +1508,6 @@ static int __prepare_dmabuf(struct vb2_buffer *vb)
>> dprintk(q, 1, "buffer initialization failed\n");
>> 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);
>> --
>> 2.47.0.199.ga7371fff76-goog
>>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-11-07 9:17 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-11-06 12:18 [PATCH] media: videobuf2-core: copy vb planes unconditionally Tudor Ambarus
2024-11-06 17:04 ` William McVicker
2024-11-06 23:41 ` Tomasz Figa
2024-11-07 9:17 ` Hans Verkuil
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 NNTP newsgroup(s).