Hi Hans,
Thank you for the patch.
On Fri, Jun 07, 2019 at 10:45:31AM +0200, Hans Verkuil wrote:
> The __prepare_userptr() function made the incorrect assumption that if the
> same user pointer was used as the last one for which memory was acquired, then
> there was no need to re-acquire the memory. This assumption was never properly
> tested, and after doing that it became clear that this was in fact wrong.
Could you explain in the commit message why the assumption is not
correct ?
> Change the behavior to always reacquire memory.
One more reason to not use USERPTR :-)
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
> Reported-by: Tomasz Figa <tfiga@xxxxxxxxxxxx>
> Cc: <stable@xxxxxxxxxxxxxxx> # for v5.1 and up
> ---
> This should be backported to all stable kernels, unfortunately this patch only
> applies cleanly to 5.1, so this has to be backported manually.
> ---
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index 4489744fbbd9..a6400391117f 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -1013,7 +1013,7 @@ static int __prepare_userptr(struct vb2_buffer *vb)
> void *mem_priv;
> unsigned int plane;
> int ret = 0;
> - bool reacquired = vb->planes[0].mem_priv == NULL;
> + bool called_cleanup = false;
>
> memset(planes, 0, sizeof(planes[0]) * vb->num_planes);
> /* Copy relevant information provided by the userspace */
> @@ -1023,15 +1023,6 @@ static int __prepare_userptr(struct vb2_buffer *vb)
> return ret;
>
> for (plane = 0; plane < vb->num_planes; ++plane) {
> - /* Skip the plane if already verified */
> - if (vb->planes[plane].m.userptr &&
> - vb->planes[plane].m.userptr == planes[plane].m.userptr
> - && vb->planes[plane].length == planes[plane].length)
> - continue;
> -
> - dprintk(3, "userspace address for plane %d changed, reacquiring memory\n",
> - plane);
> -
> /* Check if the provided plane buffer is large enough */
> if (planes[plane].length < vb->planes[plane].min_length) {
> dprintk(1, "provided buffer size %u is less than setup size %u for plane %d\n",
> @@ -1044,8 +1035,8 @@ static int __prepare_userptr(struct vb2_buffer *vb)
>
> /* Release previously acquired memory if present */
> if (vb->planes[plane].mem_priv) {
> - if (!reacquired) {
> - reacquired = true;
> + if (!called_cleanup) {
> + called_cleanup = true;
> vb->copied_timestamp = 0;
> call_void_vb_qop(vb, buf_cleanup, vb);
> }
Could we do this unconditionally before the loop ?
> @@ -1083,17 +1074,14 @@ static int __prepare_userptr(struct vb2_buffer *vb)
> vb->planes[plane].data_offset = planes[plane].data_offset;
> }
>
> - if (reacquired) {
> - /*
> - * One or more planes changed, so we must call buf_init to do
> - * the driver-specific initialization on the newly acquired
> - * buffer, if provided.
> - */
> - ret = call_vb_qop(vb, buf_init, vb);
> - if (ret) {
> - dprintk(1, "buffer initialization failed\n");
> - goto err;
> - }
> + /*
> + * Call buf_init to do the driver-specific initialization on
> + * the newly acquired buffer.
> + */
> + ret = call_vb_qop(vb, buf_init, vb);
> + if (ret) {
> + dprintk(1, "buffer initialization failed\n");
> + goto err;
> }
>
> ret = call_vb_qop(vb, buf_prepare, vb);
--
Regards,
Laurent Pinchart
