Message ID | CAB-zwWjb+2ExjNDB3OtHmRmgaHMnO-VgEe9VZk_wU=ryrq_AGw@mail.gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Hello, On Thursday, July 28, 2011 11:10 PM Ramirez Luna, Omar wrote: > I know it is very early but here it is a tryout of the dma_map_sg and > dma_unmap_sg with iommu, I made it to roughly understand what is needed to > remove drivers/omap-iovmm.c (which is a virtual memory manager > implementation on top of omap iommu driver). > > This patch is placed on top of Marek Szyprowsk initial work: > > ARM: DMA-mapping & IOMMU integration > http://thread.gmane.org/gmane.linux.kernel.mm/63727/ > > It was tested on an OMAP zoom3 platform and tidspbridge driver. The patch > is used to map user space buffers to dsp's iommu, get_user_pages is used to > form the sg list that will be passed to dma_map_sg. > > While at it, I bumped into some issues that I would like to get some > feedback or know if they are being considered: > > 1. There is no way to keep track of what virtual address are being mapped > in the scatterlist, which we need to propagate to the dsp, in order that it > knows where does the buffers start and end on its virtual address space. > I ended up adding an iov_address to scatterlist which if accepted should be > toggled/affected by the selection of CONFIG_IOMMU_API. Sorry, but your patch is completely wrong. You should not add any additional entries to scatterlist. dma_addr IS the virtual address in the device's io address space, so the dma_addr is a value that your device should put into it's own registers to start dma transfer to provided memory pages. > 2. tidspbridge driver sometimes needs to map a physical address into a > fixed virtual address (i.e. the start of a firmware section is expected to > be at dsp va 0x20000000), there is no straight forward way to do this with > the dma api given that it only expects to receive a cpu_addr, a sg or a > page, by adding iov_address I could pass phys and iov addresses in a sg > and overcome this limitation, but, these addresses belong to: We also encountered the problem of fixed firmware address. We addressed is by setting io address space start to this address and letting device driver to rely on the fact that the first call to dma_alloc() will match this address. > 2a. Shared memory between ARM and DSP: this memory is allocated through > memblock API which takes it out of kernel control to be later > ioremap'd and iommu map'd to the dsp (this because a non-cacheable > requirement), so, these physical addresses doesn't have a linear > virtual address translation, which is what dma api expects. I hope that the issue with page cache attributes can be resolved if we always allocate memory from CMA (see the latest CMAv12 patches: http://www.spinics.net/lists/linux-media/msg35674.html ) > 2b. Bus addresses: of dsp peripherals which are also ioremap'd and > affected by the same thing. Right now I have no idea how to handle ioremapped areas in dma-mapping framework, but do we really need to support them? Best regards
On Fri, Jul 29, 2011 at 09:50:32AM +0200, Marek Szyprowski wrote: > On Thursday, July 28, 2011 11:10 PM Ramirez Luna, Omar wrote: > > 2. tidspbridge driver sometimes needs to map a physical address into a > > fixed virtual address (i.e. the start of a firmware section is expected to > > be at dsp va 0x20000000), there is no straight forward way to do this with > > the dma api given that it only expects to receive a cpu_addr, a sg or a > > page, by adding iov_address I could pass phys and iov addresses in a sg > > and overcome this limitation, but, these addresses belong to: > > We also encountered the problem of fixed firmware address. We addressed is by > setting io address space start to this address and letting device driver to > rely on the fact that the first call to dma_alloc() will match this address. This sounds rather hacky. How about partitioning the address space for the device and give the dma-api only a part of it. The other parts can be directly mapped using the iommu-api then. Regards, Joerg 1
Hello, On Friday, July 29, 2011 11:36 AM Joerg Roedel wrote: > On Fri, Jul 29, 2011 at 09:50:32AM +0200, Marek Szyprowski wrote: > > On Thursday, July 28, 2011 11:10 PM Ramirez Luna, Omar wrote: > > > > 2. tidspbridge driver sometimes needs to map a physical address into a > > > fixed virtual address (i.e. the start of a firmware section is expected to > > > be at dsp va 0x20000000), there is no straight forward way to do this with > > > the dma api given that it only expects to receive a cpu_addr, a sg or a > > > page, by adding iov_address I could pass phys and iov addresses in a sg > > > and overcome this limitation, but, these addresses belong to: > > > > We also encountered the problem of fixed firmware address. We addressed is by > > setting io address space start to this address and letting device driver to > > rely on the fact that the first call to dma_alloc() will match this address. > > This sounds rather hacky. How about partitioning the address space for > the device and give the dma-api only a part of it. The other parts can > be directly mapped using the iommu-api then. Well, I'm not convinced that iommu-api should be used by the device drivers directly. If possible we should rather extend dma-mapping than use such hacks. Best regards
On Fri, Jul 29, 2011 at 12:14:25PM +0200, Marek Szyprowski wrote: > > This sounds rather hacky. How about partitioning the address space for > > the device and give the dma-api only a part of it. The other parts can > > be directly mapped using the iommu-api then. > > Well, I'm not convinced that iommu-api should be used by the device drivers > directly. If possible we should rather extend dma-mapping than use such hacks. Building this into dma-api would turn it into an iommu-api. The line between the apis are clear. The iommu-api provides direct mapping of bus-addresses to system-addresses while the dma-api puts a memory manager on-top which deals with bus-address allocation itself. So if you want to map bus-addresses directly the iommu-api is the way to go. This is in no way a hack. Regards, Joerg
Hello, On Friday, July 29, 2011 12:54 PM Joerg Roedel wrote: > On Fri, Jul 29, 2011 at 12:14:25PM +0200, Marek Szyprowski wrote: > > > This sounds rather hacky. How about partitioning the address space for > > > the device and give the dma-api only a part of it. The other parts can > > > be directly mapped using the iommu-api then. > > > > Well, I'm not convinced that iommu-api should be used by the device drivers > > directly. If possible we should rather extend dma-mapping than use such hacks. > > Building this into dma-api would turn it into an iommu-api. The line > between the apis are clear. The iommu-api provides direct mapping > of bus-addresses to system-addresses while the dma-api puts a memory > manager on-top which deals with bus-address allocation itself. > So if you want to map bus-addresses directly the iommu-api is the way to > go. This is in no way a hack. The problem starts when you want to use the same driver on two different systems: one with iommu and one without. Our driver depends only on dma-mapping and the fact that the first allocation starts from the right address. On systems without iommu, board code calls bootmem_reserve() and dma_declare_coherent() for the required memory range. Systems with IOMMU just sets up device io address space to start at the specified address. This works fine, because in our system each device has its own, private iommu controller and private address space. Right now I have no idea how to handle this better. Perhaps with should be possible to specify somehow the target dma_address when doing memory allocation, but I'm not really convinced yet if this is really required. Best regards
Hi. On Fri, Jul 29, 2011 at 11:24 PM, Marek Szyprowski <m.szyprowski@samsung.com> wrote: > Hello, > > On Friday, July 29, 2011 12:54 PM Joerg Roedel wrote: > >> On Fri, Jul 29, 2011 at 12:14:25PM +0200, Marek Szyprowski wrote: >> > > This sounds rather hacky. How about partitioning the address space for >> > > the device and give the dma-api only a part of it. The other parts can >> > > be directly mapped using the iommu-api then. >> > >> > Well, I'm not convinced that iommu-api should be used by the device drivers >> > directly. If possible we should rather extend dma-mapping than use such > hacks. >> >> Building this into dma-api would turn it into an iommu-api. The line >> between the apis are clear. The iommu-api provides direct mapping >> of bus-addresses to system-addresses while the dma-api puts a memory >> manager on-top which deals with bus-address allocation itself. >> So if you want to map bus-addresses directly the iommu-api is the way to >> go. This is in no way a hack. > > The problem starts when you want to use the same driver on two different > systems: > one with iommu and one without. Our driver depends only on dma-mapping and the > fact > that the first allocation starts from the right address. On systems without > iommu, > board code calls bootmem_reserve() and dma_declare_coherent() for the required > memory range. Systems with IOMMU just sets up device io address space to start > at the specified address. This works fine, because in our system each device has > its own, private iommu controller and private address space. > > Right now I have no idea how to handle this better. Perhaps with should be > possible > to specify somehow the target dma_address when doing memory allocation, but I'm > not > really convinced yet if this is really required. > What about using 'dma_handle' argument of alloc_coherent callback of dma_map_ops? Although it is an output argument, I think we can convey a hint or start address to map to the IO memory manager that resides behind dma API. Of course, it is unable to map a specific physical address with the dma address with the idea. I think the problem can be solved for some application with overriding alloc_coherent callback in the machine initialization code. Still the above idea cannot answer when a physical address is needed to be mapped to a specific dma address with 'dma_map_*()'. DMA API is so abstract that it cannot cover all requirements by various device drivers;; Regards, Cho KyongHo.
Hi, On Fri, Jul 29, 2011 at 2:50 AM, Marek Szyprowski <m.szyprowski@samsung.com> wrote: >> 1. There is no way to keep track of what virtual address are being mapped >> in the scatterlist, which we need to propagate to the dsp, in order that it >> knows where does the buffers start and end on its virtual address space. >> I ended up adding an iov_address to scatterlist which if accepted should be >> toggled/affected by the selection of CONFIG_IOMMU_API. > > Sorry, but your patch is completely wrong. You should not add any additional > entries to scatterlist. At the time it was the easiest way for me to keep track of both virtual and physical addresses, without doing a page_to_phys every time on unmap. I understand that it might fall out of the scope of the scatterlist struct. > dma_addr IS the virtual address in the device's io > address space, so the dma_addr is a value that your device should put into > it's own registers to start dma transfer to provided memory pages. I also wanted to keep the same part as the original arm_dma_map_sg: s->dma_address = __dma_map_page... Where the dma_address was the "clean" (from cache) physical address. But if desired, I guess this value can be replaced for the iommu va. >> 2. tidspbridge driver sometimes needs to map a physical address into a >> fixed virtual address (i.e. the start of a firmware section is expected to >> be at dsp va 0x20000000), there is no straight forward way to do this with >> the dma api given that it only expects to receive a cpu_addr, a sg or a >> page, by adding iov_address I could pass phys and iov addresses in a sg >> and overcome this limitation, but, these addresses belong to: > > We also encountered the problem of fixed firmware address. We addressed is by > setting io address space start to this address and letting device driver to > rely on the fact that the first call to dma_alloc() will match this address. Indeed, however in my case, I need sections at (I might have approximated the numbers to the real ones): 0x11000000 for dsp shared memory 0x11800000 for peripherals 0x20000000 for dsp external code 0x21000000 for mapped buffers The end of a section and start of the other usually have a gap, so the exact address needs to be specified by the firmware. So, this won't work with just letting the pool manager to provide the virtual address. >> 2a. Shared memory between ARM and DSP: this memory is allocated through >> memblock API which takes it out of kernel control to be later >> ioremap'd and iommu map'd to the dsp (this because a non-cacheable >> requirement), so, these physical addresses doesn't have a linear >> virtual address translation, which is what dma api expects. > > I hope that the issue with page cache attributes can be resolved if we always > allocate memory from CMA (see the latest CMAv12 patches: > http://www.spinics.net/lists/linux-media/msg35674.html ) I'll take a look more closely and will be trying them if possible. >> 2b. Bus addresses: of dsp peripherals which are also ioremap'd and >> affected by the same thing. > > Right now I have no idea how to handle ioremapped areas in dma-mapping > framework, but do we really need to support them? It would be good to know if this is expected to be handled by dma-mapping, otherwise it can be dealt by iommu api as Joerg pointed out. Thanks for your comments, Omar
Hi, On Sun, Jul 31, 2011 at 7:57 PM, KyongHo Cho <pullip.cho@samsung.com> wrote: > On Fri, Jul 29, 2011 at 11:24 PM, Marek Szyprowski ... >> Right now I have no idea how to handle this better. Perhaps with should be >> possible >> to specify somehow the target dma_address when doing memory allocation, but I'm >> not >> really convinced yet if this is really required. >> > What about using 'dma_handle' argument of alloc_coherent callback of > dma_map_ops? > Although it is an output argument, I think we can convey a hint or > start address to map > to the IO memory manager that resides behind dma API. I also thought on this one, even dma_map_single receives a void *ptr which could be casted into a struct with both physical and virtual addresses to be mapped, but IMHO, this starts to add twists into the dma map parameters which might create confusion. > DMA API is so abstract that it cannot cover all requirements by > various device drivers;; Agree. Regards, Omar
On Mon, Aug 08, 2011 at 10:21:46AM -0500, Ramirez Luna, Omar wrote: > Hi, > > On Sun, Jul 31, 2011 at 7:57 PM, KyongHo Cho <pullip.cho@samsung.com> wrote: > > On Fri, Jul 29, 2011 at 11:24 PM, Marek Szyprowski > ... > >> Right now I have no idea how to handle this better. Perhaps with should be > >> possible > >> to specify somehow the target dma_address when doing memory allocation, but I'm > >> not > >> really convinced yet if this is really required. > >> > > What about using 'dma_handle' argument of alloc_coherent callback of > > dma_map_ops? > > Although it is an output argument, I think we can convey a hint or > > start address to map > > to the IO memory manager that resides behind dma API. > > I also thought on this one, even dma_map_single receives a void *ptr > which could be casted into a struct with both physical and virtual > addresses to be mapped, but IMHO, this starts to add twists into the > dma map parameters which might create confusion. No - don't even consider that. That's highly non-standard usage and it'll break all existing drivers to do so.
Hello, On Monday, August 08, 2011 5:05 PM Ramirez Luna, Omar wrote: > On Fri, Jul 29, 2011 at 2:50 AM, Marek Szyprowski > <m.szyprowski@samsung.com> wrote: > >> 1. There is no way to keep track of what virtual address are being mapped > >> in the scatterlist, which we need to propagate to the dsp, in order that it > >> knows where does the buffers start and end on its virtual address space. > >> I ended up adding an iov_address to scatterlist which if accepted should be > >> toggled/affected by the selection of CONFIG_IOMMU_API. > > > > Sorry, but your patch is completely wrong. You should not add any additional > > entries to scatterlist. > > At the time it was the easiest way for me to keep track of both > virtual and physical addresses, without doing a page_to_phys every > time on unmap. I understand that it might fall out of the scope of the > scatterlist struct. > > > dma_addr IS the virtual address in the device's io > > address space, so the dma_addr is a value that your device should put into > > it's own registers to start dma transfer to provided memory pages. > > I also wanted to keep the same part as the original arm_dma_map_sg: > > s->dma_address = __dma_map_page... > > Where the dma_address was the "clean" (from cache) physical address. Nope, DMA-mapping API defines dma_address as a value that should be written to device registers to start DMA transfer. Physical address of the page should never be used by the driver directly. > But if desired, I guess this value can be replaced for the iommu va. > > >> 2. tidspbridge driver sometimes needs to map a physical address into a > >> fixed virtual address (i.e. the start of a firmware section is expected to > >> be at dsp va 0x20000000), there is no straight forward way to do this with > >> the dma api given that it only expects to receive a cpu_addr, a sg or a > >> page, by adding iov_address I could pass phys and iov addresses in a sg > >> and overcome this limitation, but, these addresses belong to: > > > > We also encountered the problem of fixed firmware address. We addressed is by > > setting io address space start to this address and letting device driver to > > rely on the fact that the first call to dma_alloc() will match this address. > > Indeed, however in my case, I need sections at (I might have > approximated the numbers to the real ones): > > 0x11000000 for dsp shared memory > 0x11800000 for peripherals > 0x20000000 for dsp external code > 0x21000000 for mapped buffers > > The end of a section and start of the other usually have a gap, so the > exact address needs to be specified by the firmware. So, this won't > work with just letting the pool manager to provide the virtual > address. Are all of these regions used by the same single device driver? It looks that you might need to create separate struct device entries for each 'memory' region and attach them as the children to your main device structure. Each such child device can have different iommu/memory configuration and the main driver can easily gather them with device_find_child() function. We have such solution working very well for our video codec. Please refer to the following patches merged to v3.1-rc1: 1. MFC driver: af935746781088f28904601469671d244d2f653b - drivers/media/video/s5p-mfc/s5p_mfc.c, function s5p_mfc_probe() 2. platform device definitions: 0f75a96bc0c4611dea0c7207533f822315120054 Best regards
Hello, On Monday, August 08, 2011 5:22 PM Ramirez Luna, Omar wrote: > On Sun, Jul 31, 2011 at 7:57 PM, KyongHo Cho <pullip.cho@samsung.com> wrote: > > On Fri, Jul 29, 2011 at 11:24 PM, Marek Szyprowski > ... > >> Right now I have no idea how to handle this better. Perhaps with should be > >> possible > >> to specify somehow the target dma_address when doing memory allocation, but I'm > >> not > >> really convinced yet if this is really required. > >> > > What about using 'dma_handle' argument of alloc_coherent callback of > > dma_map_ops? > > Although it is an output argument, I think we can convey a hint or > > start address to map > > to the IO memory manager that resides behind dma API. > > I also thought on this one, even dma_map_single receives a void *ptr > which could be casted into a struct with both physical and virtual > addresses to be mapped, but IMHO, this starts to add twists into the > dma map parameters which might create confusion. Nope, this is completely wrong approach. DMA-mapping is kernel wide, architecture independent API and you should not define any exceptions from it. > > DMA API is so abstract that it cannot cover all requirements by > > various device drivers;; > > Agree. From my perspective DMA API is quite well designed as cross-architecture API. The only problem is the lack of documentation how to use it correctly in the embedded world. Best regards
On Tue, Aug 9, 2011 at 1:51 AM, Marek Szyprowski <m.szyprowski@samsung.com> wrote: > Hello, > > On Monday, August 08, 2011 5:05 PM Ramirez Luna, Omar wrote: > >> On Fri, Jul 29, 2011 at 2:50 AM, Marek Szyprowski >> <m.szyprowski@samsung.com> wrote: >> > dma_addr IS the virtual address in the device's io >> > address space, so the dma_addr is a value that your device should put into >> > it's own registers to start dma transfer to provided memory pages. >> >> I also wanted to keep the same part as the original arm_dma_map_sg: >> >> s->dma_address = __dma_map_page... >> >> Where the dma_address was the "clean" (from cache) physical address. > > Nope, DMA-mapping API defines dma_address as a value that should be written to > device registers to start DMA transfer. Yes, but in the standard context of DMA, the dma_address is the place in memory where the transference is going to take place, e.g.: you don't fill dma_address right now because dma_map_sg overrides with the previous assignment (and I'm not saying that I'm using this value for anything). OTOH, on iommu context, this will be filled with the virtual address that the device will be accessing for DMA, I'm OK with that, what I was trying to say, is that you need the "clean" physical address after mapping the page to the mmu even if the return value of __dma_map_page is not going to be stored. > Are all of these regions used by the same single device driver? Yes they are part of the same firmware that controls the dsp. > It looks > that you might need to create separate struct device entries for each 'memory' > region and attach them as the children to your main device structure. Each > such child device can have different iommu/memory configuration and the main > driver can easily gather them with device_find_child() function. We have such > solution working very well for our video codec. Please refer to the following > patches merged to v3.1-rc1: > > 1. MFC driver: af935746781088f28904601469671d244d2f653b - > drivers/media/video/s5p-mfc/s5p_mfc.c, function s5p_mfc_probe() > > 2. platform device definitions: 0f75a96bc0c4611dea0c7207533f822315120054 I took a quick look, it seems like you only need 2 memory regions and for that only define 2 devices, I'll consider it to see how it looks for me defining a bunch of devices (5 or less) for these memory sections. Thanks, Omar
diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c index b6397c1..2cc4853 100644 --- a/arch/arm/mm/dma-mapping.c +++ b/arch/arm/mm/dma-mapping.c @@ -1318,10 +1318,78 @@ void arm_iommu_free_attrs(struct device *dev, size_t size, void *cpu_addr, mutex_unlock(&mapping->lock); } +int arm_iommu_map_sg(struct device *dev, struct scatterlist *sg, int nents, + enum dma_data_direction dir) +{ + struct dma_iommu_mapping *mapping = dev->archdata.mapping; + struct scatterlist *s; + dma_addr_t iova; + size_t size = 0; + int i, j; + + BUG_ON(!valid_dma_direction(dir)); + + /* XXX do not assume al ents of PAGE_SIZE */ + size = nents * PAGE_SIZE; + iova = gen_pool_alloc(mapping->pool, size); + if (iova == 0) + return 0; + + for_each_sg(sg, s, nents, i) { + int ret; + unsigned int phys = page_to_phys(sg_page(s)); + + /* XXX Add arch flags */ + ret = iommu_map(mapping->domain, iova, phys, 0, 0); + if (ret < 0) + goto bad_mapping; + + s->iov_address = iova; + iova += PAGE_SIZE; + + /* XXX do something on error to clean iommu map*/ + s->dma_address = __dma_map_page(dev, sg_page(s), s->offset, + s->length, dir); + if (dma_mapping_error(dev, s->dma_address)) + goto bad_mapping; + } + debug_dma_map_sg(dev, sg, nents, nents, dir); + return nents; + + bad_mapping: + for_each_sg(sg, s, i, j) + __dma_unmap_page(dev, sg_dma_address(s), sg_dma_len(s), dir); + return 0; + +} + +void arm_iommu_unmap_sg(struct device *dev, struct scatterlist *sg, int nents, + enum dma_data_direction dir) +{ + struct dma_iommu_mapping *mapping = dev->archdata.mapping; + dma_addr_t iova = sg_iov_address(sg); + struct scatterlist *s; + size_t size = 0; + int i; + + debug_dma_unmap_sg(dev, sg, nents, dir); + + for_each_sg(sg, s, nents, i) { + __dma_unmap_page(dev, sg_dma_address(s), sg_dma_len(s), dir); + iommu_unmap(mapping->domain, sg_iov_address(s), 0); + } + + size = nents * PAGE_SIZE; + gen_pool_free(mapping->pool, iova, size); +} + + struct arm_dma_map_ops iommu_ops = { .alloc_attrs = arm_iommu_alloc_attrs, .free_attrs = arm_iommu_free_attrs, .mmap_attrs = arm_iommu_mmap_attrs, + .map_sg = arm_iommu_map_sg, + .unmap_sg = arm_iommu_unmap_sg, }; EXPORT_SYMBOL_GPL(iommu_ops); diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c index 9b21b80..6b2a3e1 100644 --- a/drivers/iommu/omap-iommu.c +++ b/drivers/iommu/omap-iommu.c @@ -22,6 +22,7 @@ #include <linux/mutex.h> #include <asm/cacheflush.h> +#include <asm/dma-iommu.h> #include <plat/iommu.h> #include <plat/iopgtable.h> @@ -879,9 +880,15 @@ EXPORT_SYMBOL_GPL(iommu_set_da_range); */ struct device *omap_find_iommu_device(const char *name) { - return driver_find_device(&omap_iommu_driver.driver, NULL, + struct device *dev; + + dev = driver_find_device(&omap_iommu_driver.driver, NULL, (void *)name, device_match_by_alias); + + arm_iommu_assign_device(dev, 0x204f0000, 0x304f0000); + + return dev; } EXPORT_SYMBOL_GPL(omap_find_iommu_device); diff --git a/include/asm-generic/scatterlist.h b/include/asm-generic/scatterlist.h index 5de0735..831d626 100644 --- a/include/asm-generic/scatterlist.h +++ b/include/asm-generic/scatterlist.h @@ -11,6 +11,7 @@ struct scatterlist { unsigned int offset; unsigned int length; dma_addr_t dma_address; + dma_addr_t iov_address; #ifdef CONFIG_NEED_SG_DMA_LENGTH unsigned int dma_length; #endif @@ -25,6 +26,8 @@ struct scatterlist { */ #define sg_dma_address(sg) ((sg)->dma_address) +#define sg_iov_address(sg) ((sg)->iov_address) + #ifdef CONFIG_NEED_SG_DMA_LENGTH #define sg_dma_len(sg) ((sg)->dma_length) #else