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);
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
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-b... (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-b...
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
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-b... (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-b...
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
linux-stable-mirror@lists.linaro.org
-
Hans Verkuil -
Tomasz Figa -
Tudor Ambarus -
William McVicker