On Tue, Mar 19, 2019 at 5:08 AM Brian Starkey <brian.star...@arm.com> wrote: > > Hi John, > > On Tue, Mar 05, 2019 at 12:54:29PM -0800, John Stultz wrote: > > From: "Andrew F. Davis" <a...@ti.com> > > [snip] > > > + > > +#define NUM_HEAP_MINORS 128 > > +static DEFINE_IDR(dma_heap_idr); > > +static DEFINE_MUTEX(minor_lock); /* Protect idr accesses */ > > I saw that Matthew Wilcox is trying to nuke idr: > https://patchwork.freedesktop.org/series/57073/ > > Perhaps a different data structure could be considered? (I don't have > an informed opinion on which).
Thanks for pointing this out! I've just switched to using the Xarray implementation in my tree. > > +static int dma_heap_buffer_alloc(struct dma_heap *heap, size_t len, > > + unsigned int flags) > > +{ > > + len = PAGE_ALIGN(len); > > + if (!len) > > + return -EINVAL; > > I think aligning len to pages only makes sense if heaps are going to > allocate aligned to pages too. Perhaps that's an implicit assumption? > If so, lets document it. I've added a comment as such (or do you have more thoughts on where it should be documented?), and for consistency removed the PAGE_ALIGN usage in the heap allocator hooks. > Why not let the heaps take care of aligning len however they want > though? As Andrew already said, It seems page granularity would have to be the finest allocation granularity for dmabufs. If heaps want to implement their own larger granularity alignment, I don't see any reason they would be limited there. And for me, its mostly because I stubbed my toe implementing the heap code w/ the first patch that didn't have the page alignment in the generic code. :) > > + /* Create device */ > > + heap->heap_devt = MKDEV(MAJOR(dma_heap_devt), heap->minor); > > + dev_ret = device_create(dma_heap_class, > > + NULL, > > + heap->heap_devt, > > + NULL, > > + heap->name); > > + if (IS_ERR(dev_ret)) { > > + pr_err("dma_heap: Unable to create char device\n"); > > + return PTR_ERR(dev_ret); > > + } > > + > > + /* Add device */ > > + cdev_init(&heap->heap_cdev, &dma_heap_fops); > > + ret = cdev_add(&heap->heap_cdev, dma_heap_devt, NUM_HEAP_MINORS); > > Shouldn't this be s/dma_heap_devt/heap->heap_devt/ and a count of 1? > > Also would it be better to have cdev_add/device_create the other way > around? First create the char device, then once it's all set up > register it with sysfs. Thanks for catching that! Much appreciated! Reworked as suggested. Though I realized last week I have not figured out a consistent way to have the heaps show up in /dev/dma_heaps/<device> on both Android and classic Linux environments. I need to go stare at the /dev/input/ setup code some more. > > + if (ret < 0) { > > + device_destroy(dma_heap_class, heap->heap_devt); > > + pr_err("dma_heap: Unable to add char device\n"); > > + return ret; > > + } > > + > > + return 0; > > +} > > +EXPORT_SYMBOL(dma_heap_add); > > Until we've figured out how modules are going to work, I still think > it would be a good idea to not export this. Done! thanks -john _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel