| Message ID | 20241106121802.2939237-1-tudor.ambarus@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: videobuf2-core: copy vb planes unconditionally | expand |
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-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 >
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 >>
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);
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(-)