Re: [REVIEW PATCH 3/3] saa7134: convert to vb2
On 04/17/2014 03:13 PM, Mauro Carvalho Chehab wrote: Em Thu, 17 Apr 2014 11:49:51 +0200 Hans Verkuil hverk...@xs4all.nl escreveu: On 04/17/2014 04:17 AM, Mauro Carvalho Chehab wrote: Em Thu, 17 Apr 2014 00:33:55 +0200 Hans Verkuil hverk...@xs4all.nl escreveu: On 04/17/2014 12:23 AM, Mauro Carvalho Chehab wrote: Em Mon, 10 Mar 2014 13:20:49 +0100 Hans Verkuil hverk...@xs4all.nl escreveu: From: Hans Verkuil hans.verk...@cisco.com Convert the saa7134 driver to vb2. Note that while this uses the vb2-dma-sg version, the VB2_USERPTR mode is disabled. The DMA hardware only supports DMAing full pages, and in the USERPTR memory model the first and last scatter-gather buffer is almost never a full page. In practice this means that we can't use the VB2_USERPTR mode. Why not? Provided that the buffer is equal or bigger than the number of pages required by saa7134, that should be OK. All the driver needs to do is to check if the USERPTR buffer condition is met, returning an error otherwise (and likely printing a msg at dmesg). Yuck. Well, I'll take a look at this. It has in my view the same problem as abusing USERPTR to pass pointers to physically contiguous memory: yes, it 'supports' USERPTR, but it has additional requirements which userspace has no way of knowing or detecting. It's really not USERPTR at all, it is PAGE_ALIGNED_USERPTR. 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. Regards, Hans Quite different. Hmm... If I remember well, mmapped memory (being userptr or not) are always page aligned, at least on systems with MMU. Not malloc()ed memory. That's what userptr is about. Take a look at videobuf_dma_init_user_locked at drivers/media/v4l2-core/videobuf-dma-sg.c: first = (data PAGE_MASK) PAGE_SHIFT; last = ((data+size-1) PAGE_MASK) PAGE_SHIFT; dma-offset = data ~PAGE_MASK; dma-size = size; dma-nr_pages = last-first+1; dma-pages = kmalloc(dma-nr_pages * sizeof(struct page *), GFP_KERNEL); The physical memory is always page aligned, even if VM memory isn't. The offset there is actually used just to subtract the size, at videobuf_pages_to_sg(). So, with VB1, USERPTR works fine, and no special care is needed on userspace to align the offset. Btw, it seems that VB2 also does the same. Take a look at vb2_dma_sg_get_userptr(). -- 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
Re: [REVIEW PATCH 3/3] saa7134: convert to vb2
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 + (-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; -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- 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
Re: [REVIEW PATCH 3/3] saa7134: convert to vb2
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 + (-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
Re: [REVIEW PATCH 3/3] saa7134: convert to vb2
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 -- Devin J. Heitmueller - Kernel Labs http://www.kernellabs.com -- 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
Re: [REVIEW PATCH 3/3] saa7134: convert to vb2
On 04/17/2014 04:17 AM, Mauro Carvalho Chehab wrote: Em Thu, 17 Apr 2014 00:33:55 +0200 Hans Verkuil hverk...@xs4all.nl escreveu: On 04/17/2014 12:23 AM, Mauro Carvalho Chehab wrote: Em Mon, 10 Mar 2014 13:20:49 +0100 Hans Verkuil hverk...@xs4all.nl escreveu: From: Hans Verkuil hans.verk...@cisco.com Convert the saa7134 driver to vb2. Note that while this uses the vb2-dma-sg version, the VB2_USERPTR mode is disabled. The DMA hardware only supports DMAing full pages, and in the USERPTR memory model the first and last scatter-gather buffer is almost never a full page. In practice this means that we can't use the VB2_USERPTR mode. Why not? Provided that the buffer is equal or bigger than the number of pages required by saa7134, that should be OK. All the driver needs to do is to check if the USERPTR buffer condition is met, returning an error otherwise (and likely printing a msg at dmesg). Yuck. Well, I'll take a look at this. It has in my view the same problem as abusing USERPTR to pass pointers to physically contiguous memory: yes, it 'supports' USERPTR, but it has additional requirements which userspace has no way of knowing or detecting. It's really not USERPTR at all, it is PAGE_ALIGNED_USERPTR. Quite different. Hmm... If I remember well, mmapped memory (being userptr or not) are always page aligned, at least on systems with MMU. Not malloc()ed memory. That's what userptr is about. I would prefer that you have to enable it explicitly through e.g. a module option. That way you can still do it, but you really have to know what you are doing. I suspect that this change will break some userspace programs used for video surveillance equipment. This has been tested with raw video, compressed video, VBI, radio, DVB and video overlays. Unfortunately, a vb2 conversion is one of those things you cannot split up in smaller patches, it's all or nothing. This patch switches the whole driver over to vb2, using the vb2 ioctl and fop helper functions. Not quite true. This patch contains lots of non-vb2 stuff, like: - Coding Style fixes; - Removal of res_get/res_set/res_free; - Functions got moved from one place to another one. I will see if there is anything sensible that I can split up. I'm not aware of any particular coding style issues, but I'll review it. There are several, like: - dprintk(buffer_finish %p\n,q-curr); + dprintk(buffer_finish %p\n, q-curr); Also, it seems that you moved some functions, like: ts_reset_encoder(struct saa7134_dev* dev) that was moved to some other part of the code and renamed as stop_streaming(). There are several of such cases, with makes hard to really see the VB2 changes, and what it might be some code dropped by mistake. The removal of the resource functions is not something I can split up. It is replaced by the resource handling that's built into the vb2 helper functions. Well, currently, it is really hard to see that all the checks between empress and normal video streams are still done right, as the patch become big and messy. The original checks were never correct. This driver was buggy as hell once you tried to use multiple streams at the same time. I have split it up some more, but the actual vb2 conversion remains a big patch. Regards, Hans Please try to break it into a more granular set of patches that would help to check if everything is there. Thanks, Mauro Regards, Hans It is really hard to review it, as is, as the real changes are mixed with the above code cleanups/changes. Please split this patch in a way that it allows reviewing the changes there. -- 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
Re: [REVIEW PATCH 3/3] saa7134: convert to vb2
Em Thu, 17 Apr 2014 11:49:51 +0200 Hans Verkuil hverk...@xs4all.nl escreveu: On 04/17/2014 04:17 AM, Mauro Carvalho Chehab wrote: Em Thu, 17 Apr 2014 00:33:55 +0200 Hans Verkuil hverk...@xs4all.nl escreveu: On 04/17/2014 12:23 AM, Mauro Carvalho Chehab wrote: Em Mon, 10 Mar 2014 13:20:49 +0100 Hans Verkuil hverk...@xs4all.nl escreveu: From: Hans Verkuil hans.verk...@cisco.com Convert the saa7134 driver to vb2. Note that while this uses the vb2-dma-sg version, the VB2_USERPTR mode is disabled. The DMA hardware only supports DMAing full pages, and in the USERPTR memory model the first and last scatter-gather buffer is almost never a full page. In practice this means that we can't use the VB2_USERPTR mode. Why not? Provided that the buffer is equal or bigger than the number of pages required by saa7134, that should be OK. All the driver needs to do is to check if the USERPTR buffer condition is met, returning an error otherwise (and likely printing a msg at dmesg). Yuck. Well, I'll take a look at this. It has in my view the same problem as abusing USERPTR to pass pointers to physically contiguous memory: yes, it 'supports' USERPTR, but it has additional requirements which userspace has no way of knowing or detecting. It's really not USERPTR at all, it is PAGE_ALIGNED_USERPTR. Quite different. Hmm... If I remember well, mmapped memory (being userptr or not) are always page aligned, at least on systems with MMU. Not malloc()ed memory. That's what userptr is about. Take a look at videobuf_dma_init_user_locked at drivers/media/v4l2-core/videobuf-dma-sg.c: first = (data PAGE_MASK) PAGE_SHIFT; last = ((data+size-1) PAGE_MASK) PAGE_SHIFT; dma-offset = data ~PAGE_MASK; dma-size = size; dma-nr_pages = last-first+1; dma-pages = kmalloc(dma-nr_pages * sizeof(struct page *), GFP_KERNEL); The physical memory is always page aligned, even if VM memory isn't. The offset there is actually used just to subtract the size, at videobuf_pages_to_sg(). So, with VB1, USERPTR works fine, and no special care is needed on userspace to align the offset. Btw, it seems that VB2 also does the same. Take a look at vb2_dma_sg_get_userptr(). I would prefer that you have to enable it explicitly through e.g. a module option. That way you can still do it, but you really have to know what you are doing. I suspect that this change will break some userspace programs used for video surveillance equipment. This has been tested with raw video, compressed video, VBI, radio, DVB and video overlays. Unfortunately, a vb2 conversion is one of those things you cannot split up in smaller patches, it's all or nothing. This patch switches the whole driver over to vb2, using the vb2 ioctl and fop helper functions. Not quite true. This patch contains lots of non-vb2 stuff, like: - Coding Style fixes; - Removal of res_get/res_set/res_free; - Functions got moved from one place to another one. I will see if there is anything sensible that I can split up. I'm not aware of any particular coding style issues, but I'll review it. There are several, like: - dprintk(buffer_finish %p\n,q-curr); + dprintk(buffer_finish %p\n, q-curr); Also, it seems that you moved some functions, like: ts_reset_encoder(struct saa7134_dev* dev) that was moved to some other part of the code and renamed as stop_streaming(). There are several of such cases, with makes hard to really see the VB2 changes, and what it might be some code dropped by mistake. The removal of the resource functions is not something I can split up. It is replaced by the resource handling that's built into the vb2 helper functions. Well, currently, it is really hard to see that all the checks between empress and normal video streams are still done right, as the patch become big and messy. The original checks were never correct. This driver was buggy as hell once you tried to use multiple streams at the same time. I have split it up some more, but the actual vb2 conversion remains a big patch. Ok. Regards, Hans Please try to break it into a more granular set of patches that would help to check if everything is there. Thanks, Mauro Regards, Hans It is really hard to review it, as is, as the real changes are mixed with the above code cleanups/changes. Please split this patch in a way that it allows reviewing the changes there. -- Regards, Mauro -- 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
Re: [REVIEW PATCH 3/3] saa7134: convert to vb2
On 04/17/2014 12:23 AM, Mauro Carvalho Chehab wrote: Em Mon, 10 Mar 2014 13:20:49 +0100 Hans Verkuil hverk...@xs4all.nl escreveu: From: Hans Verkuil hans.verk...@cisco.com Convert the saa7134 driver to vb2. Note that while this uses the vb2-dma-sg version, the VB2_USERPTR mode is disabled. The DMA hardware only supports DMAing full pages, and in the USERPTR memory model the first and last scatter-gather buffer is almost never a full page. In practice this means that we can't use the VB2_USERPTR mode. Why not? Provided that the buffer is equal or bigger than the number of pages required by saa7134, that should be OK. All the driver needs to do is to check if the USERPTR buffer condition is met, returning an error otherwise (and likely printing a msg at dmesg). Yuck. Well, I'll take a look at this. It has in my view the same problem as abusing USERPTR to pass pointers to physically contiguous memory: yes, it 'supports' USERPTR, but it has additional requirements which userspace has no way of knowing or detecting. It's really not USERPTR at all, it is PAGE_ALIGNED_USERPTR. Quite different. I would prefer that you have to enable it explicitly through e.g. a module option. That way you can still do it, but you really have to know what you are doing. I suspect that this change will break some userspace programs used for video surveillance equipment. This has been tested with raw video, compressed video, VBI, radio, DVB and video overlays. Unfortunately, a vb2 conversion is one of those things you cannot split up in smaller patches, it's all or nothing. This patch switches the whole driver over to vb2, using the vb2 ioctl and fop helper functions. Not quite true. This patch contains lots of non-vb2 stuff, like: - Coding Style fixes; - Removal of res_get/res_set/res_free; - Functions got moved from one place to another one. I will see if there is anything sensible that I can split up. I'm not aware of any particular coding style issues, but I'll review it. The removal of the resource functions is not something I can split up. It is replaced by the resource handling that's built into the vb2 helper functions. Regards, Hans It is really hard to review it, as is, as the real changes are mixed with the above code cleanups/changes. Please split this patch in a way that it allows reviewing the changes there. -- 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
Re: [REVIEW PATCH 3/3] saa7134: convert to vb2
Em Thu, 17 Apr 2014 00:33:55 +0200 Hans Verkuil hverk...@xs4all.nl escreveu: On 04/17/2014 12:23 AM, Mauro Carvalho Chehab wrote: Em Mon, 10 Mar 2014 13:20:49 +0100 Hans Verkuil hverk...@xs4all.nl escreveu: From: Hans Verkuil hans.verk...@cisco.com Convert the saa7134 driver to vb2. Note that while this uses the vb2-dma-sg version, the VB2_USERPTR mode is disabled. The DMA hardware only supports DMAing full pages, and in the USERPTR memory model the first and last scatter-gather buffer is almost never a full page. In practice this means that we can't use the VB2_USERPTR mode. Why not? Provided that the buffer is equal or bigger than the number of pages required by saa7134, that should be OK. All the driver needs to do is to check if the USERPTR buffer condition is met, returning an error otherwise (and likely printing a msg at dmesg). Yuck. Well, I'll take a look at this. It has in my view the same problem as abusing USERPTR to pass pointers to physically contiguous memory: yes, it 'supports' USERPTR, but it has additional requirements which userspace has no way of knowing or detecting. It's really not USERPTR at all, it is PAGE_ALIGNED_USERPTR. Quite different. Hmm... If I remember well, mmapped memory (being userptr or not) are always page aligned, at least on systems with MMU. I would prefer that you have to enable it explicitly through e.g. a module option. That way you can still do it, but you really have to know what you are doing. I suspect that this change will break some userspace programs used for video surveillance equipment. This has been tested with raw video, compressed video, VBI, radio, DVB and video overlays. Unfortunately, a vb2 conversion is one of those things you cannot split up in smaller patches, it's all or nothing. This patch switches the whole driver over to vb2, using the vb2 ioctl and fop helper functions. Not quite true. This patch contains lots of non-vb2 stuff, like: - Coding Style fixes; - Removal of res_get/res_set/res_free; - Functions got moved from one place to another one. I will see if there is anything sensible that I can split up. I'm not aware of any particular coding style issues, but I'll review it. There are several, like: - dprintk(buffer_finish %p\n,q-curr); + dprintk(buffer_finish %p\n, q-curr); Also, it seems that you moved some functions, like: ts_reset_encoder(struct saa7134_dev* dev) that was moved to some other part of the code and renamed as stop_streaming(). There are several of such cases, with makes hard to really see the VB2 changes, and what it might be some code dropped by mistake. The removal of the resource functions is not something I can split up. It is replaced by the resource handling that's built into the vb2 helper functions. Well, currently, it is really hard to see that all the checks between empress and normal video streams are still done right, as the patch become big and messy. Please try to break it into a more granular set of patches that would help to check if everything is there. Thanks, Mauro Regards, Hans It is really hard to review it, as is, as the real changes are mixed with the above code cleanups/changes. Please split this patch in a way that it allows reviewing the changes there. -- Regards, Mauro -- 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
