Hi Devin, On 04/18/2014 03:49 PM, Devin Heitmueller wrote: >> This was a bit confusing following the previous paragraph. I meant to say >> that the >> *saa7134* userptr implementation is not USERPTR at all but >> PAGE_ALIGNED_USERPTR. >> >> A proper USERPTR implementation (like in bttv) can use any malloc()ed >> pointer as >> it should, but saa7134 can't as it requires the application to align the >> pointer >> to a page boundary, which is non-standard. > > It's probably worth mentioning at this point that there's a bug in > videobuf2_vmalloc() where it forces the buffer provided by userptr > mode to be page aligned. This causes issues if you hand it a buffer > that isn't actually page aligned, as it writes to an arbitrary offset > into the buffer instead of the start of the buffer you provided. > > I've had the following patch in my private tree for months, but had > been hesitant to submit it since I didn't know if it would effect > other implementations. I wasn't sure if USERPTR mode required the > buffers to be page aligned and I was violating the spec. > > Devin > > From: Devin Heitmueller <dheitmuel...@kernellabs.com> > Date: Fri, 17 May 2013 19:53:02 +0000 (-0400) > Subject: Fix alignment bug when using VB2 with userptr mode > > Fix alignment bug when using VB2 with userptr mode > > If we pass in a USERPTR buffer that isn't page aligned, the driver > will align it (and not tell userland). The result is that the driver > thinks the video starts in one place while userland thinks it starts > in another. This manifests itself as the video being horizontally > shifted and there being garbage from the start of the field up to > that point. > > This problem was seen with both the em28xx and uvc drivers when > testing on the Davinci platform (where the buffers allocated are > not necessarily page aligned). > --- > > diff --git a/linux/drivers/media/v4l2-core/videobuf2-vmalloc.c > b/linux/drivers/media/v4l2-core/videobuf2-vmalloc.c > index 94efa04..ad7ce7c 100644 > --- a/linux/drivers/media/v4l2-core/videobuf2-vmalloc.c > +++ b/linux/drivers/media/v4l2-core/videobuf2-vmalloc.c > @@ -82,7 +82,7 @@ static void *vb2_vmalloc_get_userptr(void > *alloc_ctx, unsigned long vaddr, > return NULL; > > buf->write = write; > - offset = vaddr & ~PAGE_MASK; > + offset = 0; > buf->size = size; >
This makes no sense. The vivi driver uses vb2-vmalloc as well, and that works perfectly fine in userptr mode. Applying this patch breaks vivi userptr mode, so this is a NACK for this patch. I wonder though if this is related to this thread: http://www.spinics.net/lists/linux-media/msg75815.html I suspect that in your case the vb2_get_contig_userptr() function is called which as far as I can tell is the wrong function to call for the vmalloc case since there is absolutely no requirement that user pointers should be physically contiguous for vmalloc drivers. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
