| Message ID | 1308556213-24970-8-git-send-email-m.szyprowski@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi. On Mon, Jun 20, 2011 at 4:50 PM, Marek Szyprowski <m.szyprowski@samsung.com> wrote: > struct dma_map_ops { > - void* (*alloc_coherent)(struct device *dev, size_t size, > - dma_addr_t *dma_handle, gfp_t gfp); > - void (*free_coherent)(struct device *dev, size_t size, > - void *vaddr, dma_addr_t dma_handle); > + void* (*alloc)(struct device *dev, size_t size, > + dma_addr_t *dma_handle, gfp_t gfp, > + struct dma_attrs *attrs); > + void (*free)(struct device *dev, size_t size, > + void *vaddr, dma_addr_t dma_handle, > + struct dma_attrs *attrs); > + int (*mmap)(struct device *, struct vm_area_struct *, > + void *, dma_addr_t, size_t, struct dma_attrs *attrs); > + > dma_addr_t (*map_page)(struct device *dev, struct page *page, > unsigned long offset, size_t size, > enum dma_data_direction dir, I still don't agree with your idea that change alloc_coherent() with alloc(). As I said before, we actually do not need dma_alloc_writecombine() anymore because it is not different from dma_alloc_coherent() in ARM. Most of other architectures do not have dma_alloc_writecombine(). If you want dma_alloc_coherent() to allocate user virtual address, I believe that it is also available with mmap() you introduced. Regards, Cho KyongHo.
On Mon, Jun 20, 2011 at 11:45:41PM +0900, KyongHo Cho wrote: > I still don't agree with your idea that change alloc_coherent() with alloc(). > As I said before, we actually do not need dma_alloc_writecombine() anymore > because it is not different from dma_alloc_coherent() in ARM. Wrong - there is a difference. For pre-ARMv6 CPUs, it returns memory with different attributes from DMA coherent memory. And we're not going to sweep away pre-ARMv6 CPUs any time soon. So you can't ignore dma_alloc_writecombine() which must remain to sanely support framebuffers.
On Tue, Jun 21, 2011 at 12:06 AM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > Wrong - there is a difference. For pre-ARMv6 CPUs, it returns memory > with different attributes from DMA coherent memory. > > And we're not going to sweep away pre-ARMv6 CPUs any time soon. So > you can't ignore dma_alloc_writecombine() which must remain to sanely > support framebuffers. > OK. Thanks. Then, I think we can implement dma_alloc_writecombine() away from dma_map_ops. IMHO, those devices that use dma_alloc_writecombine() are enough with the default dma_map_ops. Removing a member from dma_map_ops is too heavy work.
Hello, On Monday, June 20, 2011 4:46 PM KyongHo Cho wrote: > On Mon, Jun 20, 2011 at 4:50 PM, Marek Szyprowski > <m.szyprowski@samsung.com> wrote: > > > struct dma_map_ops { > > - void* (*alloc_coherent)(struct device *dev, size_t size, > > - dma_addr_t *dma_handle, gfp_t gfp); > > - void (*free_coherent)(struct device *dev, size_t size, > > - void *vaddr, dma_addr_t dma_handle); > > + void* (*alloc)(struct device *dev, size_t size, > > + dma_addr_t *dma_handle, gfp_t gfp, > > + struct dma_attrs *attrs); > > + void (*free)(struct device *dev, size_t size, > > + void *vaddr, dma_addr_t dma_handle, > > + struct dma_attrs *attrs); > > + int (*mmap)(struct device *, struct vm_area_struct *, > > + void *, dma_addr_t, size_t, struct dma_attrs > *attrs); > > + > > dma_addr_t (*map_page)(struct device *dev, struct page *page, > > unsigned long offset, size_t size, > > enum dma_data_direction dir, > > I still don't agree with your idea that change alloc_coherent() with > alloc(). > As I said before, we actually do not need dma_alloc_writecombine() anymore > because it is not different from dma_alloc_coherent() in ARM. You already got a reply that dropping dma_alloc_writecombine() is no go on ARM. > Most of other architectures do not have dma_alloc_writecombine(). That's not a problem. Once we agree on dma_alloc_attrs(), the drivers can be changed to use DMA_ATTR_WRITE_COMBINE attribute. If the platform doesn't support the attribute, it is just ignored. That's the whole point of the attributes extension. Once a driver is converted to dma_alloc_attrs(), it can be used without any changes either on platforms that supports some specific attributes or the one that doesn't implement support for any of them. > If you want dma_alloc_coherent() to allocate user virtual address, > I believe that it is also available with mmap() you introduced. Allocation is a separate operation from mapping to userspace. Mmap operation should just map the buffer (represented by a cookie of type dma_addr_t) to user address space. Note that some drivers (like framebuffer drivers for example) also needs to have both types of mapping - one for user space and one for kernel virtual space. Best regards
2011/6/21 Marek Szyprowski <m.szyprowski@samsung.com>: > > You already got a reply that dropping dma_alloc_writecombine() is no > go on ARM. > Yes. > That's not a problem. Once we agree on dma_alloc_attrs(), the drivers > can be changed to use DMA_ATTR_WRITE_COMBINE attribute. If the platform > doesn't support the attribute, it is just ignored. That's the whole > point of the attributes extension. Once a driver is converted to > dma_alloc_attrs(), it can be used without any changes either on platforms > that supports some specific attributes or the one that doesn't implement > support for any of them. > I just worried that removing an existing construct is not a simple job. You introduced 3 attributes: DMA_ATTR_WRITE_COMBINE, ***COHERENT and ***NO_KERNEL_VADDR I replied earlier, I also indicated that write combining option for dma_alloc_*() is required. But it does not mean dma_map_ops must support that. I think dma_alloc_writecombine() can be implemented regardless of dma_map_ops. > Allocation is a separate operation from mapping to userspace. Mmap > operation should just map the buffer (represented by a cookie of type > dma_addr_t) to user address space. > > Note that some drivers (like framebuffer drivers for example) also > needs to have both types of mapping - one for user space and one for > kernel virtual space. I meant that I think DMA_ATTR_NOKERNELVADDR is not required because you also introduced mmap callback.
Hello, On Wednesday, June 22, 2011 2:01 AM KyongHo Cho wrote: > 2011/6/21 Marek Szyprowski <m.szyprowski@samsung.com>: > > > > You already got a reply that dropping dma_alloc_writecombine() is no > > go on ARM. > > > Yes. > > > That's not a problem. Once we agree on dma_alloc_attrs(), the drivers > > can be changed to use DMA_ATTR_WRITE_COMBINE attribute. If the platform > > doesn't support the attribute, it is just ignored. That's the whole > > point of the attributes extension. Once a driver is converted to > > dma_alloc_attrs(), it can be used without any changes either on platforms > > that supports some specific attributes or the one that doesn't implement > > support for any of them. > > > I just worried that removing an existing construct is not a simple job. > You introduced 3 attributes: DMA_ATTR_WRITE_COMBINE, ***COHERENT and > ***NO_KERNEL_VADDR COHERENT is the default behavior when no attribute is provided. I also don't want to remove existing calls to dma_alloc_coherent() and dma_alloc_writecombine() from the drivers. This can be done later, once dma_alloc_attrs() API will stabilize. > I replied earlier, I also indicated that write combining option for > dma_alloc_*() is required. > But it does not mean dma_map_ops must support that. > I think dma_alloc_writecombine() can be implemented regardless of > dma_map_ops. The discussion about the need of dma_alloc_attrs() has been performed on Memory Management Summit at Linaro Meeting in Budapest. It simplifies the API and provides full backward compatibility for existing drivers. > > Allocation is a separate operation from mapping to userspace. Mmap > > operation should just map the buffer (represented by a cookie of type > > dma_addr_t) to user address space. > > > > Note that some drivers (like framebuffer drivers for example) also > > needs to have both types of mapping - one for user space and one for > > kernel virtual space. > > I meant that I think DMA_ATTR_NOKERNELVADDR is not required because > you also introduced mmap callback. I've already said that mmap callback is not related to the fact that the buffer has been allocated with or without respective kernel virtual space mapping. I really don't get what do you mean here. Best regards
On Monday 20 June 2011, Marek Szyprowski wrote: > Introduce new alloc/free/mmap methods that take attributes argument. > alloc/free_coherent can be implemented on top of the new alloc/free > calls with NULL attributes. dma_alloc_non_coherent can be implemented > using DMA_ATTR_NONCOHERENT attribute, dma_alloc_writecombine can also > use separate DMA_ATTR_WRITECOMBINE attribute. This way the drivers will > get more generic, platform independent way of allocating dma memory > buffers with specific parameters. > > One more attribute can be usefull: DMA_ATTR_NOKERNELVADDR. Buffers with > such attribute will not have valid kernel virtual address. They might be > usefull for drivers that only exports the DMA buffers to userspace (like > for example V4L2 or ALSA). > > mmap method is introduced to let the drivers create a user space mapping > for a DMA buffer in generic, architecture independent way. > > TODO: update all dma_map_ops clients for all architectures > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> Yes, I think that is good, but the change needs to be done atomically across all architectures. This should be easy enough as I believe all other architectures that use dma_map_ops don't even require dma_alloc_noncoherent but just define it to dma_alloc_coherent because they have only coherent memory in regular device drivers. On a related note, do you plan to make the CMA work use this transparently, or do you want to have a DMA_ATTR_LARGE or DMA_ATTR_CONTIGUOUS for CMA? Arnd
On Monday 20 June 2011, Marek Szyprowski wrote: > mmap method is introduced to let the drivers create a user space mapping > for a DMA buffer in generic, architecture independent way. One more thing: please split out the mmap change into a separate patch. I sense that there might be some objections to that, and it's better to let people know about it early on than having them complain later when that has already been merged. Arnd
On Fri, 2011-06-24 at 17:51 +0200, Arnd Bergmann wrote: > On Monday 20 June 2011, Marek Szyprowski wrote: > > Introduce new alloc/free/mmap methods that take attributes argument. > > alloc/free_coherent can be implemented on top of the new alloc/free > > calls with NULL attributes. dma_alloc_non_coherent can be implemented > > using DMA_ATTR_NONCOHERENT attribute, dma_alloc_writecombine can also > > use separate DMA_ATTR_WRITECOMBINE attribute. This way the drivers will > > get more generic, platform independent way of allocating dma memory > > buffers with specific parameters. > > > > One more attribute can be usefull: DMA_ATTR_NOKERNELVADDR. Buffers with > > such attribute will not have valid kernel virtual address. They might be > > usefull for drivers that only exports the DMA buffers to userspace (like > > for example V4L2 or ALSA). > > > > mmap method is introduced to let the drivers create a user space mapping > > for a DMA buffer in generic, architecture independent way. > > > > TODO: update all dma_map_ops clients for all architectures > > > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > > Yes, I think that is good, but the change needs to be done atomically > across all architectures. This should be easy enough as I believe > all other architectures that use dma_map_ops don't even require > dma_alloc_noncoherent This statement is definitely not true of parisc, and also, I believe, not true of sh, so that would have to figure in the conversion work too. James > but just define it to dma_alloc_coherent > because they have only coherent memory in regular device drivers. > > On a related note, do you plan to make the CMA work use this > transparently, or do you want to have a DMA_ATTR_LARGE or > DMA_ATTR_CONTIGUOUS for CMA? > > Arnd > -- > To unsubscribe from this list: send the line "unsubscribe linux-arch" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday 24 June 2011, James Bottomley wrote: > On Fri, 2011-06-24 at 17:51 +0200, Arnd Bergmann wrote: > > Yes, I think that is good, but the change needs to be done atomically > > across all architectures. This should be easy enough as I believe > > all other architectures that use dma_map_ops don't even require > > dma_alloc_noncoherent > > This statement is definitely not true of parisc, and also, I believe, > not true of sh, so that would have to figure in the conversion work too. As far as I can tell, parisc uses its own hppa_dma_ops, not dma_map_ops, and arch/sh/include/asm/dma-mapping.h contains an unconditional #define dma_alloc_noncoherent(d, s, h, f) dma_alloc_coherent(d, s, h, f) If you want to change parisc to use dma_map_ops then I would suggest adding another attribute for alloc_noncoherent. Arnd
Hello, On Friday, June 24, 2011 5:52 PM Arnd Bergmann wrote: > On Monday 20 June 2011, Marek Szyprowski wrote: > > Introduce new alloc/free/mmap methods that take attributes argument. > > alloc/free_coherent can be implemented on top of the new alloc/free > > calls with NULL attributes. dma_alloc_non_coherent can be implemented > > using DMA_ATTR_NONCOHERENT attribute, dma_alloc_writecombine can also > > use separate DMA_ATTR_WRITECOMBINE attribute. This way the drivers will > > get more generic, platform independent way of allocating dma memory > > buffers with specific parameters. > > > > One more attribute can be usefull: DMA_ATTR_NOKERNELVADDR. Buffers with > > such attribute will not have valid kernel virtual address. They might be > > usefull for drivers that only exports the DMA buffers to userspace (like > > for example V4L2 or ALSA). > > > > mmap method is introduced to let the drivers create a user space mapping > > for a DMA buffer in generic, architecture independent way. > > > > TODO: update all dma_map_ops clients for all architectures > > > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > > Yes, I think that is good, but the change needs to be done atomically > across all architectures. Yes, I'm aware of this and I will include such changes in the next version of my patches. > This should be easy enough as I believe > all other architectures that use dma_map_ops don't even require > dma_alloc_noncoherent but just define it to dma_alloc_coherent > because they have only coherent memory in regular device drivers. Right, this should be quite simple. I will also add DMA_ATTR_NON_COHERENT attribute for implementing dma_alloc_noncoherent() call. > On a related note, do you plan to make the CMA work use this > transparently, or do you want to have a DMA_ATTR_LARGE or > DMA_ATTR_CONTIGUOUS for CMA? IMHO it will be better to hide the CMA from the drivers. Memory allocated from CMA doesn't really differ from the one allocated by dma_alloc_coherent() (which internally use alloc_pages()), so I really see no reason for adding additional attribute for it. Best regards
On Monday 27 June 2011, Marek Szyprowski wrote: > > On a related note, do you plan to make the CMA work use this > > transparently, or do you want to have a DMA_ATTR_LARGE or > > DMA_ATTR_CONTIGUOUS for CMA? > > IMHO it will be better to hide the CMA from the drivers. Memory allocated > from CMA doesn't really differ from the one allocated by dma_alloc_coherent() > (which internally use alloc_pages()), so I really see no reason for adding > additional attribute for it. Ok, fair enough. On a semi-related topic, IIRC we still need to make sure that dma_alloc_coherent() pages are unmapped from the linear mapping. I hope this is independent of both CMA and this patch. Arnd
Hello, On Monday, June 27, 2011 3:22 PM Arnd Bergmann wrote: > On Monday 27 June 2011, Marek Szyprowski wrote: > > > On a related note, do you plan to make the CMA work use this > > > transparently, or do you want to have a DMA_ATTR_LARGE or > > > DMA_ATTR_CONTIGUOUS for CMA? > > > > IMHO it will be better to hide the CMA from the drivers. Memory allocated > > from CMA doesn't really differ from the one allocated by > dma_alloc_coherent() > > (which internally use alloc_pages()), so I really see no reason for > adding > > additional attribute for it. > > Ok, fair enough. On a semi-related topic, IIRC we still need to make sure > that dma_alloc_coherent() pages are unmapped from the linear mapping. I > hope > this is independent of both CMA and this patch. Right, that's one more big item that is still on the TODO list. Best regards
Hello, On Friday, June 24, 2011 5:54 PM Arnd Bergmann wrote: > On Monday 20 June 2011, Marek Szyprowski wrote: > > mmap method is introduced to let the drivers create a user space mapping > > for a DMA buffer in generic, architecture independent way. > > One more thing: please split out the mmap change into a separate patch. Ok, no problem. > I sense that there might be some objections to that, and it's better > to let people know about it early on than having them complain > later when that has already been merged. Ok, I will also prepare much more detailed description for mmap patch. Best regards
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h index ba8319a..08a6fa8 100644 --- a/include/linux/dma-mapping.h +++ b/include/linux/dma-mapping.h @@ -16,10 +16,15 @@ enum dma_data_direction { }; struct dma_map_ops { - void* (*alloc_coherent)(struct device *dev, size_t size, - dma_addr_t *dma_handle, gfp_t gfp); - void (*free_coherent)(struct device *dev, size_t size, - void *vaddr, dma_addr_t dma_handle); + void* (*alloc)(struct device *dev, size_t size, + dma_addr_t *dma_handle, gfp_t gfp, + struct dma_attrs *attrs); + void (*free)(struct device *dev, size_t size, + void *vaddr, dma_addr_t dma_handle, + struct dma_attrs *attrs); + int (*mmap)(struct device *, struct vm_area_struct *, + void *, dma_addr_t, size_t, struct dma_attrs *attrs); + dma_addr_t (*map_page)(struct device *dev, struct page *page, unsigned long offset, size_t size, enum dma_data_direction dir,