| Message ID | 1488491084-17252-11-git-send-email-labbott@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Laura, Thank you for the patch. On Thursday 02 Mar 2017 13:44:42 Laura Abbott wrote: > When CMA was first introduced, its primary use was for DMA allocation > and the only way to get CMA memory was to call dma_alloc_coherent. This > put Ion in an awkward position since there was no device structure > readily available and setting one up messed up the coherency model. > These days, CMA can be allocated directly from the APIs. Switch to using > this model to avoid needing a dummy device. This also avoids awkward > caching questions. If the DMA mapping API isn't suitable for today's requirements anymore, I believe that's what needs to be fixed, instead of working around the problem by introducing another use-case-specific API. > Signed-off-by: Laura Abbott <labbott@redhat.com> > --- > drivers/staging/android/ion/ion_cma_heap.c | 97 +++++++-------------------- > 1 file changed, 26 insertions(+), 71 deletions(-) > > diff --git a/drivers/staging/android/ion/ion_cma_heap.c > b/drivers/staging/android/ion/ion_cma_heap.c index d562fd7..6838825 100644 > --- a/drivers/staging/android/ion/ion_cma_heap.c > +++ b/drivers/staging/android/ion/ion_cma_heap.c > @@ -19,24 +19,19 @@ > #include <linux/slab.h> > #include <linux/errno.h> > #include <linux/err.h> > -#include <linux/dma-mapping.h> > +#include <linux/cma.h> > +#include <linux/scatterlist.h> > > #include "ion.h" > #include "ion_priv.h" > > struct ion_cma_heap { > struct ion_heap heap; > - struct device *dev; > + struct cma *cma; > }; > > #define to_cma_heap(x) container_of(x, struct ion_cma_heap, heap) > > -struct ion_cma_buffer_info { > - void *cpu_addr; > - dma_addr_t handle; > - struct sg_table *table; > -}; > - > > /* ION CMA heap operations functions */ > static int ion_cma_allocate(struct ion_heap *heap, struct ion_buffer > *buffer, @@ -44,93 +39,53 @@ static int ion_cma_allocate(struct ion_heap > *heap, struct ion_buffer *buffer, unsigned long flags) > { > struct ion_cma_heap *cma_heap = to_cma_heap(heap); > - struct device *dev = cma_heap->dev; > - struct ion_cma_buffer_info *info; > - > - dev_dbg(dev, "Request buffer allocation len %ld\n", len); > - > - if (buffer->flags & ION_FLAG_CACHED) > - return -EINVAL; > + struct sg_table *table; > + struct page *pages; > + int ret; > > - info = kzalloc(sizeof(*info), GFP_KERNEL); > - if (!info) > + pages = cma_alloc(cma_heap->cma, len, 0); > + if (!pages) > return -ENOMEM; > > - info->cpu_addr = dma_alloc_coherent(dev, len, &(info->handle), > - GFP_HIGHUSER | __GFP_ZERO); > - > - if (!info->cpu_addr) { > - dev_err(dev, "Fail to allocate buffer\n"); > + table = kmalloc(sizeof(struct sg_table), GFP_KERNEL); > + if (!table) > goto err; > - } > > - info->table = kmalloc(sizeof(*info->table), GFP_KERNEL); > - if (!info->table) > + ret = sg_alloc_table(table, 1, GFP_KERNEL); > + if (ret) > goto free_mem; > > - if (dma_get_sgtable(dev, info->table, info->cpu_addr, info->handle, > - len)) > - goto free_table; > - /* keep this for memory release */ > - buffer->priv_virt = info; > - buffer->sg_table = info->table; > - dev_dbg(dev, "Allocate buffer %p\n", buffer); > + sg_set_page(table->sgl, pages, len, 0); > + > + buffer->priv_virt = pages; > + buffer->sg_table = table; > return 0; > > -free_table: > - kfree(info->table); > free_mem: > - dma_free_coherent(dev, len, info->cpu_addr, info->handle); > + kfree(table); > err: > - kfree(info); > + cma_release(cma_heap->cma, pages, buffer->size); > return -ENOMEM; > } > > static void ion_cma_free(struct ion_buffer *buffer) > { > struct ion_cma_heap *cma_heap = to_cma_heap(buffer->heap); > - struct device *dev = cma_heap->dev; > - struct ion_cma_buffer_info *info = buffer->priv_virt; > + struct page *pages = buffer->priv_virt; > > - dev_dbg(dev, "Release buffer %p\n", buffer); > /* release memory */ > - dma_free_coherent(dev, buffer->size, info->cpu_addr, info->handle); > + cma_release(cma_heap->cma, pages, buffer->size); > /* release sg table */ > - sg_free_table(info->table); > - kfree(info->table); > - kfree(info); > -} > - > -static int ion_cma_mmap(struct ion_heap *mapper, struct ion_buffer *buffer, > - struct vm_area_struct *vma) > -{ > - struct ion_cma_heap *cma_heap = to_cma_heap(buffer->heap); > - struct device *dev = cma_heap->dev; > - struct ion_cma_buffer_info *info = buffer->priv_virt; > - > - return dma_mmap_coherent(dev, vma, info->cpu_addr, info->handle, > - buffer->size); > -} > - > -static void *ion_cma_map_kernel(struct ion_heap *heap, > - struct ion_buffer *buffer) > -{ > - struct ion_cma_buffer_info *info = buffer->priv_virt; > - /* kernel memory mapping has been done at allocation time */ > - return info->cpu_addr; > -} > - > -static void ion_cma_unmap_kernel(struct ion_heap *heap, > - struct ion_buffer *buffer) > -{ > + sg_free_table(buffer->sg_table); > + kfree(buffer->sg_table); > } > > static struct ion_heap_ops ion_cma_ops = { > .allocate = ion_cma_allocate, > .free = ion_cma_free, > - .map_user = ion_cma_mmap, > - .map_kernel = ion_cma_map_kernel, > - .unmap_kernel = ion_cma_unmap_kernel, > + .map_user = ion_heap_map_user, > + .map_kernel = ion_heap_map_kernel, > + .unmap_kernel = ion_heap_unmap_kernel, > }; > > struct ion_heap *ion_cma_heap_create(struct ion_platform_heap *data) > @@ -147,7 +102,7 @@ struct ion_heap *ion_cma_heap_create(struct > ion_platform_heap *data) * get device from private heaps data, later it > will be > * used to make the link with reserved CMA memory > */ > - cma_heap->dev = data->priv; > + cma_heap->cma = data->priv; > cma_heap->heap.type = ION_HEAP_TYPE_DMA; > return &cma_heap->heap; > }
On 03/03/2017 08:41 AM, Laurent Pinchart wrote: > Hi Laura, > > Thank you for the patch. > > On Thursday 02 Mar 2017 13:44:42 Laura Abbott wrote: >> When CMA was first introduced, its primary use was for DMA allocation >> and the only way to get CMA memory was to call dma_alloc_coherent. This >> put Ion in an awkward position since there was no device structure >> readily available and setting one up messed up the coherency model. >> These days, CMA can be allocated directly from the APIs. Switch to using >> this model to avoid needing a dummy device. This also avoids awkward >> caching questions. > > If the DMA mapping API isn't suitable for today's requirements anymore, I > believe that's what needs to be fixed, instead of working around the problem > by introducing another use-case-specific API. > I don't think this is a usecase specific API. CMA has been decoupled from DMA already because it's used in other places. The trying to go through DMA was just another layer of abstraction, especially since there isn't a device available for allocation. Thanks, Laura
On Fri, Mar 03, 2017 at 10:50:20AM -0800, Laura Abbott wrote: > On 03/03/2017 08:41 AM, Laurent Pinchart wrote: > > Hi Laura, > > > > Thank you for the patch. > > > > On Thursday 02 Mar 2017 13:44:42 Laura Abbott wrote: > >> When CMA was first introduced, its primary use was for DMA allocation > >> and the only way to get CMA memory was to call dma_alloc_coherent. This > >> put Ion in an awkward position since there was no device structure > >> readily available and setting one up messed up the coherency model. > >> These days, CMA can be allocated directly from the APIs. Switch to using > >> this model to avoid needing a dummy device. This also avoids awkward > >> caching questions. > > > > If the DMA mapping API isn't suitable for today's requirements anymore, I > > believe that's what needs to be fixed, instead of working around the problem > > by introducing another use-case-specific API. > > > > I don't think this is a usecase specific API. CMA has been decoupled from > DMA already because it's used in other places. The trying to go through > DMA was just another layer of abstraction, especially since there isn't > a device available for allocation. Also, we've had separation of allocation and dma-mapping since forever, that's how it works almost everywhere. Not exactly sure why/how arm-soc ecosystem ended up focused so much on dma_alloc_coherent. I think separating allocation from dma mapping/coherency is perfectly fine, and the way to go. -Daniel
Hi Daniel, On Monday 06 Mar 2017 11:32:04 Daniel Vetter wrote: > On Fri, Mar 03, 2017 at 10:50:20AM -0800, Laura Abbott wrote: > > On 03/03/2017 08:41 AM, Laurent Pinchart wrote: > >> On Thursday 02 Mar 2017 13:44:42 Laura Abbott wrote: > >>> When CMA was first introduced, its primary use was for DMA allocation > >>> and the only way to get CMA memory was to call dma_alloc_coherent. This > >>> put Ion in an awkward position since there was no device structure > >>> readily available and setting one up messed up the coherency model. > >>> These days, CMA can be allocated directly from the APIs. Switch to > >>> using this model to avoid needing a dummy device. This also avoids > >>> awkward caching questions. > >> > >> If the DMA mapping API isn't suitable for today's requirements anymore, > >> I believe that's what needs to be fixed, instead of working around the > >> problem by introducing another use-case-specific API. > > > > I don't think this is a usecase specific API. CMA has been decoupled from > > DMA already because it's used in other places. The trying to go through > > DMA was just another layer of abstraction, especially since there isn't > > a device available for allocation. > > Also, we've had separation of allocation and dma-mapping since forever, > that's how it works almost everywhere. Not exactly sure why/how arm-soc > ecosystem ended up focused so much on dma_alloc_coherent. I believe because that was the easy way to specify memory constraints. The API receives a device pointer and will allocate memory suitable for DMA for that device. The fact that it maps it to the device is a side-effect in my opinion. > I think separating allocation from dma mapping/coherency is perfectly > fine, and the way to go. Especially given that in many cases we'll want to share buffers between multiple devices, so we'll need to map them multiple times. My point still stands though, if we want to move towards a model where allocation and mapping are decoupled, we need an allocation function that takes constraints (possibly implemented with two layers, a constraint resolution layer on top of a pool/heap/type/foo-based allocator), and a mapping API. IOMMU handling being integrated in the DMA mapping API we're currently stuck with it, which might call for brushing up that API.
On Mon, Mar 06, 2017 at 03:43:53PM +0200, Laurent Pinchart wrote: > Hi Daniel, > > On Monday 06 Mar 2017 11:32:04 Daniel Vetter wrote: > > On Fri, Mar 03, 2017 at 10:50:20AM -0800, Laura Abbott wrote: > > > On 03/03/2017 08:41 AM, Laurent Pinchart wrote: > > >> On Thursday 02 Mar 2017 13:44:42 Laura Abbott wrote: > > >>> When CMA was first introduced, its primary use was for DMA allocation > > >>> and the only way to get CMA memory was to call dma_alloc_coherent. This > > >>> put Ion in an awkward position since there was no device structure > > >>> readily available and setting one up messed up the coherency model. > > >>> These days, CMA can be allocated directly from the APIs. Switch to > > >>> using this model to avoid needing a dummy device. This also avoids > > >>> awkward caching questions. > > >> > > >> If the DMA mapping API isn't suitable for today's requirements anymore, > > >> I believe that's what needs to be fixed, instead of working around the > > >> problem by introducing another use-case-specific API. > > > > > > I don't think this is a usecase specific API. CMA has been decoupled from > > > DMA already because it's used in other places. The trying to go through > > > DMA was just another layer of abstraction, especially since there isn't > > > a device available for allocation. > > > > Also, we've had separation of allocation and dma-mapping since forever, > > that's how it works almost everywhere. Not exactly sure why/how arm-soc > > ecosystem ended up focused so much on dma_alloc_coherent. > > I believe because that was the easy way to specify memory constraints. The API > receives a device pointer and will allocate memory suitable for DMA for that > device. The fact that it maps it to the device is a side-effect in my opinion. > > > I think separating allocation from dma mapping/coherency is perfectly > > fine, and the way to go. > > Especially given that in many cases we'll want to share buffers between > multiple devices, so we'll need to map them multiple times. > > My point still stands though, if we want to move towards a model where > allocation and mapping are decoupled, we need an allocation function that > takes constraints (possibly implemented with two layers, a constraint > resolution layer on top of a pool/heap/type/foo-based allocator), and a > mapping API. IOMMU handling being integrated in the DMA mapping API we're > currently stuck with it, which might call for brushing up that API. Hm, maybe I wasn't clear, but that's exactly what I assume will happen: The constraint resolver is the unix device memory allocation thing, which happens entirely in userspace. There's a lot more than just "where to allocate" to negotiate, e.g. pixel format, stride/size limits/requirements, tiling formats. A lot of it the kernel doesn't even know. Allocation then needs to happen through the kernel ofc, but that doesn't mean we need to have all the constraint resolving in the kernel. As long as the kernel exposes the device /dev node -> ion heap stuff, userspace can figure this out. Or an alternative way would be to have a cascade of ion heaps to keep things a notch more opaque. Either way, no actaul constraint resolving in the kernel itself, and except for a bunch more stuff in sysfs maybe, also no other uapi changes. Once we have a place to allocate stuff which isn't the device driver at least, aka ION. And then once allocated you use the dma apis to instantiate the iommus mappings. Anyway, at least from my understanding I think there's 0 risk with merging ION wrt the constraint resolving side (at least as discussed around XDC last year), and for setups that need cma, it might finally enable to get things moving forward. Or do I miss something big here? -Daniel
On 03/06/2017 07:52 AM, Daniel Vetter wrote: > On Mon, Mar 06, 2017 at 03:43:53PM +0200, Laurent Pinchart wrote: >> Hi Daniel, >> >> On Monday 06 Mar 2017 11:32:04 Daniel Vetter wrote: >>> On Fri, Mar 03, 2017 at 10:50:20AM -0800, Laura Abbott wrote: >>>> On 03/03/2017 08:41 AM, Laurent Pinchart wrote: >>>>> On Thursday 02 Mar 2017 13:44:42 Laura Abbott wrote: >>>>>> When CMA was first introduced, its primary use was for DMA allocation >>>>>> and the only way to get CMA memory was to call dma_alloc_coherent. This >>>>>> put Ion in an awkward position since there was no device structure >>>>>> readily available and setting one up messed up the coherency model. >>>>>> These days, CMA can be allocated directly from the APIs. Switch to >>>>>> using this model to avoid needing a dummy device. This also avoids >>>>>> awkward caching questions. >>>>> >>>>> If the DMA mapping API isn't suitable for today's requirements anymore, >>>>> I believe that's what needs to be fixed, instead of working around the >>>>> problem by introducing another use-case-specific API. >>>> >>>> I don't think this is a usecase specific API. CMA has been decoupled from >>>> DMA already because it's used in other places. The trying to go through >>>> DMA was just another layer of abstraction, especially since there isn't >>>> a device available for allocation. >>> >>> Also, we've had separation of allocation and dma-mapping since forever, >>> that's how it works almost everywhere. Not exactly sure why/how arm-soc >>> ecosystem ended up focused so much on dma_alloc_coherent. >> >> I believe because that was the easy way to specify memory constraints. The API >> receives a device pointer and will allocate memory suitable for DMA for that >> device. The fact that it maps it to the device is a side-effect in my opinion. >> Agreed. The device Ion wanted to use was never a real device though so any constraints it satisfied were making assumptions about what memory would be allocated. >>> I think separating allocation from dma mapping/coherency is perfectly >>> fine, and the way to go. >> >> Especially given that in many cases we'll want to share buffers between >> multiple devices, so we'll need to map them multiple times. >> >> My point still stands though, if we want to move towards a model where >> allocation and mapping are decoupled, we need an allocation function that >> takes constraints (possibly implemented with two layers, a constraint >> resolution layer on top of a pool/heap/type/foo-based allocator), and a >> mapping API. IOMMU handling being integrated in the DMA mapping API we're >> currently stuck with it, which might call for brushing up that API. > > Hm, maybe I wasn't clear, but that's exactly what I assume will happen: > > The constraint resolver is the unix device memory allocation thing, which > happens entirely in userspace. There's a lot more than just "where to > allocate" to negotiate, e.g. pixel format, stride/size > limits/requirements, tiling formats. A lot of it the kernel doesn't even > know. > > Allocation then needs to happen through the kernel ofc, but that doesn't > mean we need to have all the constraint resolving in the kernel. As long > as the kernel exposes the device /dev node -> ion heap stuff, userspace > can figure this out. Or an alternative way would be to have a cascade of > ion heaps to keep things a notch more opaque. Either way, no actaul > constraint resolving in the kernel itself, and except for a bunch more > stuff in sysfs maybe, also no other uapi changes. Once we have a place to > allocate stuff which isn't the device driver at least, aka ION. > > And then once allocated you use the dma apis to instantiate the iommus > mappings. > > Anyway, at least from my understanding I think there's 0 risk with merging > ION wrt the constraint resolving side (at least as discussed around XDC > last year), and for setups that need cma, it might finally enable to get > things moving forward. > > Or do I miss something big here? > -Daniel > This all sounds like what I was thinking. I think some of the concerns may be that the details of constraint solving are mostly handwaving right now. Thanks, Laura
diff --git a/drivers/staging/android/ion/ion_cma_heap.c b/drivers/staging/android/ion/ion_cma_heap.c index d562fd7..6838825 100644 --- a/drivers/staging/android/ion/ion_cma_heap.c +++ b/drivers/staging/android/ion/ion_cma_heap.c @@ -19,24 +19,19 @@ #include <linux/slab.h> #include <linux/errno.h> #include <linux/err.h> -#include <linux/dma-mapping.h> +#include <linux/cma.h> +#include <linux/scatterlist.h> #include "ion.h" #include "ion_priv.h" struct ion_cma_heap { struct ion_heap heap; - struct device *dev; + struct cma *cma; }; #define to_cma_heap(x) container_of(x, struct ion_cma_heap, heap) -struct ion_cma_buffer_info { - void *cpu_addr; - dma_addr_t handle; - struct sg_table *table; -}; - /* ION CMA heap operations functions */ static int ion_cma_allocate(struct ion_heap *heap, struct ion_buffer *buffer, @@ -44,93 +39,53 @@ static int ion_cma_allocate(struct ion_heap *heap, struct ion_buffer *buffer, unsigned long flags) { struct ion_cma_heap *cma_heap = to_cma_heap(heap); - struct device *dev = cma_heap->dev; - struct ion_cma_buffer_info *info; - - dev_dbg(dev, "Request buffer allocation len %ld\n", len); - - if (buffer->flags & ION_FLAG_CACHED) - return -EINVAL; + struct sg_table *table; + struct page *pages; + int ret; - info = kzalloc(sizeof(*info), GFP_KERNEL); - if (!info) + pages = cma_alloc(cma_heap->cma, len, 0); + if (!pages) return -ENOMEM; - info->cpu_addr = dma_alloc_coherent(dev, len, &(info->handle), - GFP_HIGHUSER | __GFP_ZERO); - - if (!info->cpu_addr) { - dev_err(dev, "Fail to allocate buffer\n"); + table = kmalloc(sizeof(struct sg_table), GFP_KERNEL); + if (!table) goto err; - } - info->table = kmalloc(sizeof(*info->table), GFP_KERNEL); - if (!info->table) + ret = sg_alloc_table(table, 1, GFP_KERNEL); + if (ret) goto free_mem; - if (dma_get_sgtable(dev, info->table, info->cpu_addr, info->handle, - len)) - goto free_table; - /* keep this for memory release */ - buffer->priv_virt = info; - buffer->sg_table = info->table; - dev_dbg(dev, "Allocate buffer %p\n", buffer); + sg_set_page(table->sgl, pages, len, 0); + + buffer->priv_virt = pages; + buffer->sg_table = table; return 0; -free_table: - kfree(info->table); free_mem: - dma_free_coherent(dev, len, info->cpu_addr, info->handle); + kfree(table); err: - kfree(info); + cma_release(cma_heap->cma, pages, buffer->size); return -ENOMEM; } static void ion_cma_free(struct ion_buffer *buffer) { struct ion_cma_heap *cma_heap = to_cma_heap(buffer->heap); - struct device *dev = cma_heap->dev; - struct ion_cma_buffer_info *info = buffer->priv_virt; + struct page *pages = buffer->priv_virt; - dev_dbg(dev, "Release buffer %p\n", buffer); /* release memory */ - dma_free_coherent(dev, buffer->size, info->cpu_addr, info->handle); + cma_release(cma_heap->cma, pages, buffer->size); /* release sg table */ - sg_free_table(info->table); - kfree(info->table); - kfree(info); -} - -static int ion_cma_mmap(struct ion_heap *mapper, struct ion_buffer *buffer, - struct vm_area_struct *vma) -{ - struct ion_cma_heap *cma_heap = to_cma_heap(buffer->heap); - struct device *dev = cma_heap->dev; - struct ion_cma_buffer_info *info = buffer->priv_virt; - - return dma_mmap_coherent(dev, vma, info->cpu_addr, info->handle, - buffer->size); -} - -static void *ion_cma_map_kernel(struct ion_heap *heap, - struct ion_buffer *buffer) -{ - struct ion_cma_buffer_info *info = buffer->priv_virt; - /* kernel memory mapping has been done at allocation time */ - return info->cpu_addr; -} - -static void ion_cma_unmap_kernel(struct ion_heap *heap, - struct ion_buffer *buffer) -{ + sg_free_table(buffer->sg_table); + kfree(buffer->sg_table); } static struct ion_heap_ops ion_cma_ops = { .allocate = ion_cma_allocate, .free = ion_cma_free, - .map_user = ion_cma_mmap, - .map_kernel = ion_cma_map_kernel, - .unmap_kernel = ion_cma_unmap_kernel, + .map_user = ion_heap_map_user, + .map_kernel = ion_heap_map_kernel, + .unmap_kernel = ion_heap_unmap_kernel, }; struct ion_heap *ion_cma_heap_create(struct ion_platform_heap *data) @@ -147,7 +102,7 @@ struct ion_heap *ion_cma_heap_create(struct ion_platform_heap *data) * get device from private heaps data, later it will be * used to make the link with reserved CMA memory */ - cma_heap->dev = data->priv; + cma_heap->cma = data->priv; cma_heap->heap.type = ION_HEAP_TYPE_DMA; return &cma_heap->heap; }
When CMA was first introduced, its primary use was for DMA allocation and the only way to get CMA memory was to call dma_alloc_coherent. This put Ion in an awkward position since there was no device structure readily available and setting one up messed up the coherency model. These days, CMA can be allocated directly from the APIs. Switch to using this model to avoid needing a dummy device. This also avoids awkward caching questions. Signed-off-by: Laura Abbott <labbott@redhat.com> --- drivers/staging/android/ion/ion_cma_heap.c | 97 ++++++++---------------------- 1 file changed, 26 insertions(+), 71 deletions(-)