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 <[email protected]> > Reported-by: Tomasz Figa <[email protected]> > Cc: <[email protected]> # 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
