| Message ID | CAGoCfix1j8kLQQe3yMDj+bqi=Pyj_K+en31a-h32+HMzVU1arQ@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <dheitmueller@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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans, > 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. Don't misunderstand, I acknowledge the very real possibility that I don't fully understand the underlying problem. And to be clear I wasn't intending to send the patch to this mailing list expecting it to be merged. That said, I reproduced it on the ti81xx platform on both em28xx and uvcvideo, so I was comfortable it wasn't an issue with my em28xx VB2 conversion. > 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. Entirely possible. I hadn't followed that thread previously but will take a look. Devin
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;