Re: [RFC PATCH v2 4/8] media: videobuf2: introduce VIDEOBUF2_PAGE memops
Hi, On Tue, Jan 10, 2012 at 6:20 PM, Marek Szyprowski wrote: >> Sorry, could you describe the abuse problem in a bit detail? > > Videobuf2 requires memory module handlers to provide vaddr method to provide > a pointer in > kernel virtual address space to video data (buffer content). It is used for > example by Yes, this is what the patch is doing, __get_free_pages just returns the kernel virtual address which will be passed to driver. > read()/write() io method emulator. Memory allocator/handler should not be > specific to any > particular use case in the device driver. That's the design. Simple. Most of the patch is copied from videobuf-vmalloc.c, and the interfaces are totally same with videobuf-vmalloc.c. > > I your case you want to give pointer to struct page from the memory allocator > to the In my case, the pointer to struct page is not required to the driver at all, so I think you have misunderstood the patch, don't I? > driver. The cookie method has been introduced exactly for this purpose. > Memory allocator > also provides a simple inline function to convert generic 'void *' return > type from cookie > method to allocator specific structure/pointer. > vb2_dma_contig_plane_dma_addr() and > vb2_dma_sg_plane_desc() were examples how does passing allocator specific > type though the > cookie method works. thanks, -- Ming Lei -- 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: [RFC PATCH v2 4/8] media: videobuf2: introduce VIDEOBUF2_PAGE memops
Hello, On Friday, December 23, 2011 1:21 PM Ming Lei wrote: > >> >> > Your current implementation also abuses the design and api of > >> >> > videobuf2 memory > >> >> > allocators. If the allocator needs to return a custom structure to > >> >> > the driver > >> >> > >> >> I think returning vaddr is enough. > >> >> > >> >> > you should use cookie method. vaddr is intended to provide only a > >> >> > pointer to > >> >> > kernel virtual mapping, but you pass a struct page * there. > >> >> > >> >> No, __get_free_pages returns virtual address instead of 'struct page *'. > >> > > >> > Then you MUST use cookie for it. vaddr method should return kernel > >> > virtual address > >> > to the buffer video data. Some parts of videobuf2 relies on this - it is > >> > used by file > >> > io emulator (read(), write() calls) and mmap equivalent for non-mmu > >> > systems. > >> > > >> > Manual casting in the driver is also a bad idea, that's why there are > >> > helper functions > >> > >> I don't see any casts are needed. The dma address can be got from vaddr > >> with > >> dma_map_* easily in drivers, see the usage on patch 8/8(media: video: > >> introduce > >> omap4 face detection module driver). > > > > Sorry, but I won't accept such driver/allocator which abuses the defined > > API. I've already > > pointed what vaddr method is used for. > > Sorry, could you describe the abuse problem in a bit detail? Videobuf2 requires memory module handlers to provide vaddr method to provide a pointer in kernel virtual address space to video data (buffer content). It is used for example by read()/write() io method emulator. Memory allocator/handler should not be specific to any particular use case in the device driver. That's the design. Simple. I your case you want to give pointer to struct page from the memory allocator to the driver. The cookie method has been introduced exactly for this purpose. Memory allocator also provides a simple inline function to convert generic 'void *' return type from cookie method to allocator specific structure/pointer. vb2_dma_contig_plane_dma_addr() and vb2_dma_sg_plane_desc() were examples how does passing allocator specific type though the cookie method works. Best regards -- Marek Szyprowski Samsung Poland R&D Center -- 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: [RFC PATCH v2 4/8] media: videobuf2: introduce VIDEOBUF2_PAGE memops
Hi, On Fri, Dec 23, 2011 at 6:38 PM, Marek Szyprowski wrote: > Hello, > > On Friday, December 23, 2011 10:51 AM Ming Lei wrote: > >> On Fri, Dec 23, 2011 at 5:34 PM, Marek Szyprowski >> wrote: >> >> >> For example, on ARM, there is very limited kernel virtual address space >> >> reserved >> >> for DMA coherent buffer mapping, the default size is about 2M if I >> >> don't remember mistakenly. >> > >> > It can be easily increased for particular boards, there is no problem with >> > this. >> >> It is not easily to increase it because there is very limited space reserved >> for >> this purpose, see Documentation/arm/memory.txt. Also looks like it is >> not configurable. > > It is really not a big issue to increase it by a few MBytes. IMO, the extra few MBytes are not helpful for the issue at all, considered the kernel virtual memory address fragmentation problem, you know there is only several MBytes DMA coherent virtual address space on ARM. > >> >> > I understand that there might be some speed issues with coherent >> >> > (uncached) >> >> > userspace mappings, but I would solve it in completely different way. >> >> > The interface >> >> >> >> Also there is poor performance inside kernel space, see [1] >> > >> > Your driver doesn't access video data inside kernel space, so this is also >> > not an issue. >> >> Why not introduce it so that other drivers(include face detection) can >> benefit with it? :-) > > We can get back into this once a driver which really benefits from comes. In fact, streaming DMA is more widespread used than coherent DMA in kernel, so why can't videobuf2 support streaming dma? :-) You know under some situation, coherent DMA buffer is very slower than other buffer to be accessed by CPU. > >> >> > >> >> > Your current implementation also abuses the design and api of videobuf2 >> >> > memory >> >> > allocators. If the allocator needs to return a custom structure to the >> >> > driver >> >> >> >> I think returning vaddr is enough. >> >> >> >> > you should use cookie method. vaddr is intended to provide only a >> >> > pointer to >> >> > kernel virtual mapping, but you pass a struct page * there. >> >> >> >> No, __get_free_pages returns virtual address instead of 'struct page *'. >> > >> > Then you MUST use cookie for it. vaddr method should return kernel virtual >> > address >> > to the buffer video data. Some parts of videobuf2 relies on this - it is >> > used by file >> > io emulator (read(), write() calls) and mmap equivalent for non-mmu >> > systems. >> > >> > Manual casting in the driver is also a bad idea, that's why there are >> > helper functions >> >> I don't see any casts are needed. The dma address can be got from vaddr with >> dma_map_* easily in drivers, see the usage on patch 8/8(media: video: >> introduce >> omap4 face detection module driver). > > Sorry, but I won't accept such driver/allocator which abuses the defined API. > I've already > pointed what vaddr method is used for. Sorry, could you describe the abuse problem in a bit detail? > >> > defined for both dma_contig and dma_sg allocators: >> > vb2_dma_contig_plane_dma_addr() and >> > vb2_dma_sg_plane_desc(). >> >> These two helpers are not needed and won't be provided by VIDEOBUF2_PAGE >> memops. > > I gave the example. Your allocator should have something similar. I don't think the two helper are required for the VIDEOBUF2_PAGE memops, why can't driver handle DMA mapping directly? thanks, -- Ming Lei -- 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: [RFC PATCH v2 4/8] media: videobuf2: introduce VIDEOBUF2_PAGE memops
Hello, On Friday, December 23, 2011 10:51 AM Ming Lei wrote: > On Fri, Dec 23, 2011 at 5:34 PM, Marek Szyprowski > wrote: > > >> For example, on ARM, there is very limited kernel virtual address space > >> reserved > >> for DMA coherent buffer mapping, the default size is about 2M if I > >> don't remember mistakenly. > > > > It can be easily increased for particular boards, there is no problem with > > this. > > It is not easily to increase it because there is very limited space reserved > for > this purpose, see Documentation/arm/memory.txt. Also looks like it is > not configurable. It is really not a big issue to increase it by a few MBytes. > >> > I understand that there might be some speed issues with coherent > >> > (uncached) > >> > userspace mappings, but I would solve it in completely different way. > >> > The interface > >> > >> Also there is poor performance inside kernel space, see [1] > > > > Your driver doesn't access video data inside kernel space, so this is also > > not an issue. > > Why not introduce it so that other drivers(include face detection) can > benefit with it? :-) We can get back into this once a driver which really benefits from comes. > >> > > >> > Your current implementation also abuses the design and api of videobuf2 > >> > memory > >> > allocators. If the allocator needs to return a custom structure to the > >> > driver > >> > >> I think returning vaddr is enough. > >> > >> > you should use cookie method. vaddr is intended to provide only a > >> > pointer to > >> > kernel virtual mapping, but you pass a struct page * there. > >> > >> No, __get_free_pages returns virtual address instead of 'struct page *'. > > > > Then you MUST use cookie for it. vaddr method should return kernel virtual > > address > > to the buffer video data. Some parts of videobuf2 relies on this - it is > > used by file > > io emulator (read(), write() calls) and mmap equivalent for non-mmu systems. > > > > Manual casting in the driver is also a bad idea, that's why there are > > helper functions > > I don't see any casts are needed. The dma address can be got from vaddr with > dma_map_* easily in drivers, see the usage on patch 8/8(media: video: > introduce > omap4 face detection module driver). Sorry, but I won't accept such driver/allocator which abuses the defined API. I've already pointed what vaddr method is used for. > > defined for both dma_contig and dma_sg allocators: > > vb2_dma_contig_plane_dma_addr() and > > vb2_dma_sg_plane_desc(). > > These two helpers are not needed and won't be provided by VIDEOBUF2_PAGE > memops. I gave the example. Your allocator should have something similar. Best regards -- Marek Szyprowski Samsung Poland R&D Center -- 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: [RFC PATCH v2 4/8] media: videobuf2: introduce VIDEOBUF2_PAGE memops
Hi, On Fri, Dec 23, 2011 at 5:34 PM, Marek Szyprowski wrote: >> For example, on ARM, there is very limited kernel virtual address space >> reserved >> for DMA coherent buffer mapping, the default size is about 2M if I >> don't remember mistakenly. > > It can be easily increased for particular boards, there is no problem with > this. It is not easily to increase it because there is very limited space reserved for this purpose, see Documentation/arm/memory.txt. Also looks like it is not configurable. > >> > I understand that there might be some speed issues with coherent (uncached) >> > userspace mappings, but I would solve it in completely different way. The >> > interface >> >> Also there is poor performance inside kernel space, see [1] > > Your driver doesn't access video data inside kernel space, so this is also > not an issue. Why not introduce it so that other drivers(include face detection) can benefit with it? :-) >> > >> > Your current implementation also abuses the design and api of videobuf2 >> > memory >> > allocators. If the allocator needs to return a custom structure to the >> > driver >> >> I think returning vaddr is enough. >> >> > you should use cookie method. vaddr is intended to provide only a pointer >> > to >> > kernel virtual mapping, but you pass a struct page * there. >> >> No, __get_free_pages returns virtual address instead of 'struct page *'. > > Then you MUST use cookie for it. vaddr method should return kernel virtual > address > to the buffer video data. Some parts of videobuf2 relies on this - it is used > by file > io emulator (read(), write() calls) and mmap equivalent for non-mmu systems. > > Manual casting in the driver is also a bad idea, that's why there are helper > functions I don't see any casts are needed. The dma address can be got from vaddr with dma_map_* easily in drivers, see the usage on patch 8/8(media: video: introduce omap4 face detection module driver). > defined for both dma_contig and dma_sg allocators: > vb2_dma_contig_plane_dma_addr() and > vb2_dma_sg_plane_desc(). These two helpers are not needed and won't be provided by VIDEOBUF2_PAGE memops. thanks, -- Ming Lei -- 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: [RFC PATCH v2 4/8] media: videobuf2: introduce VIDEOBUF2_PAGE memops
Hello, On Friday, December 23, 2011 10:22 AM Ming Lei wrote: > On Thu, Dec 22, 2011 at 5:28 PM, Marek Szyprowski > wrote: > >> DMA contig memory resource is very limited and precious, also > >> accessing to it from CPU is very slow on some platform. > >> > >> For some cases(such as the comming face detection driver), DMA Streaming > >> buffer is enough, so introduce VIDEOBUF2_PAGE to allocate continuous > >> physical memory but letting video device driver to handle DMA buffer > >> mapping > >> and unmapping things. > >> > >> Signed-off-by: Ming Lei > > > > Could you elaborate a bit why do you think that DMA contig memory resource > > is so limited? If dma_alloc_coherent fails because of the memory > > fragmentation, > > the alloc_pages() call with order > 0 will also fail. > > For example, on ARM, there is very limited kernel virtual address space > reserved > for DMA coherent buffer mapping, the default size is about 2M if I > don't remember mistakenly. It can be easily increased for particular boards, there is no problem with this. > > I understand that there might be some speed issues with coherent (uncached) > > userspace mappings, but I would solve it in completely different way. The > > interface > > Also there is poor performance inside kernel space, see [1] Your driver doesn't access video data inside kernel space, so this is also not an issue. > > for both coherent/uncached and non-coherent/cached contig allocator should > > be the > > same, so exchanging them is easy and will not require changes in the driver. > > I'm planning to introduce some design changes in memory allocator api and > > introduce > > prepare and finish callbacks in allocator ops. I hope to post the rfc after > > Christmas. For your face detection driver using standard dma-contig > > allocator > > shouldn't be a big issue. > > > > Your current implementation also abuses the design and api of videobuf2 > > memory > > allocators. If the allocator needs to return a custom structure to the > > driver > > I think returning vaddr is enough. > > > you should use cookie method. vaddr is intended to provide only a pointer to > > kernel virtual mapping, but you pass a struct page * there. > > No, __get_free_pages returns virtual address instead of 'struct page *'. Then you MUST use cookie for it. vaddr method should return kernel virtual address to the buffer video data. Some parts of videobuf2 relies on this - it is used by file io emulator (read(), write() calls) and mmap equivalent for non-mmu systems. Manual casting in the driver is also a bad idea, that's why there are helper functions defined for both dma_contig and dma_sg allocators: vb2_dma_contig_plane_dma_addr() and vb2_dma_sg_plane_desc(). Best regards -- Marek Szyprowski Samsung Poland R&D Center -- 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: [RFC PATCH v2 4/8] media: videobuf2: introduce VIDEOBUF2_PAGE memops
On Thu, Dec 22, 2011 at 5:28 PM, Marek Szyprowski wrote: > Hello, > > On Wednesday, December 14, 2011 3:00 PM Ming Lei wrote: > >> DMA contig memory resource is very limited and precious, also >> accessing to it from CPU is very slow on some platform. >> >> For some cases(such as the comming face detection driver), DMA Streaming >> buffer is enough, so introduce VIDEOBUF2_PAGE to allocate continuous >> physical memory but letting video device driver to handle DMA buffer mapping >> and unmapping things. >> >> Signed-off-by: Ming Lei > > Could you elaborate a bit why do you think that DMA contig memory resource > is so limited? If dma_alloc_coherent fails because of the memory > fragmentation, > the alloc_pages() call with order > 0 will also fail. For example, on ARM, there is very limited kernel virtual address space reserved for DMA coherent buffer mapping, the default size is about 2M if I don't remember mistakenly. > > I understand that there might be some speed issues with coherent (uncached) > userspace mappings, but I would solve it in completely different way. The > interface Also there is poor performance inside kernel space, see [1] > for both coherent/uncached and non-coherent/cached contig allocator should be > the > same, so exchanging them is easy and will not require changes in the driver. > I'm planning to introduce some design changes in memory allocator api and > introduce > prepare and finish callbacks in allocator ops. I hope to post the rfc after > Christmas. For your face detection driver using standard dma-contig allocator > shouldn't be a big issue. > > Your current implementation also abuses the design and api of videobuf2 memory > allocators. If the allocator needs to return a custom structure to the driver I think returning vaddr is enough. > you should use cookie method. vaddr is intended to provide only a pointer to > kernel virtual mapping, but you pass a struct page * there. No, __get_free_pages returns virtual address instead of 'struct page *'. thanks, -- Ming Lei [1], http://marc.info/?t=13119814851&r=1&w=2 -- 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: [RFC PATCH v2 4/8] media: videobuf2: introduce VIDEOBUF2_PAGE memops
Hello, On Wednesday, December 14, 2011 3:00 PM Ming Lei wrote: > DMA contig memory resource is very limited and precious, also > accessing to it from CPU is very slow on some platform. > > For some cases(such as the comming face detection driver), DMA Streaming > buffer is enough, so introduce VIDEOBUF2_PAGE to allocate continuous > physical memory but letting video device driver to handle DMA buffer mapping > and unmapping things. > > Signed-off-by: Ming Lei Could you elaborate a bit why do you think that DMA contig memory resource is so limited? If dma_alloc_coherent fails because of the memory fragmentation, the alloc_pages() call with order > 0 will also fail. I understand that there might be some speed issues with coherent (uncached) userspace mappings, but I would solve it in completely different way. The interface for both coherent/uncached and non-coherent/cached contig allocator should be the same, so exchanging them is easy and will not require changes in the driver. I'm planning to introduce some design changes in memory allocator api and introduce prepare and finish callbacks in allocator ops. I hope to post the rfc after Christmas. For your face detection driver using standard dma-contig allocator shouldn't be a big issue. Your current implementation also abuses the design and api of videobuf2 memory allocators. If the allocator needs to return a custom structure to the driver you should use cookie method. vaddr is intended to provide only a pointer to kernel virtual mapping, but you pass a struct page * there. (snipped) Best regards -- Marek Szyprowski Samsung Poland R&D Center -- 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
