On Mon, Mar 4, 2019 at 6:53 AM Andrew F. Davis <a...@ti.com> wrote: > On 3/1/19 6:06 AM, Brian Starkey wrote: > > On Mon, Feb 25, 2019 at 08:36:04AM -0600, Andrew F. Davis wrote: > >> +static long dma_heap_ioctl(struct file *filp, unsigned int cmd, unsigned > >> long arg) > >> +{ > >> + switch (cmd) { > >> + case DMA_HEAP_IOC_ALLOC: > >> + { > >> + struct dma_heap_allocation_data heap_allocation; > >> + struct dma_heap *heap = filp->private_data; > >> + int fd; > >> + > >> + if (copy_from_user(&heap_allocation, (void __user *)arg, > >> _IOC_SIZE(cmd))) > >> + return -EFAULT; > > > > Am I right in thinking "cmd" comes from userspace? It might be a good > > idea to not trust _IOC_SIZE(cmd) and use our own. I'm looking at > > Documentation/ioctl/botching-up-ioctls.txt and drm_ioctl() > > > > Yes cmd is from userspace, but we are in a switch-case that has already > matched cmd == DMA_HEAP_IOC_ALLOC which is sized correctly. >
Well, even so, I went through and made this cleanup over the weekend, as sizeof(heap_allocation) is probably more straight forward. The current patchset against v5.0 (with hikey960 patches), which includes the flags and other suggested changes is here: https://git.linaro.org/people/john.stultz/android-dev.git/log/?h=dev/dma-buf-heap W/ userland support here: https://android-review.googlesource.com/c/device/linaro/hikey/+/909436 I'm hoping to send this out for a real RFC in the next few days. So Andrew, if you can check it out and make sure it suits you ok I'd appreciate it! > >> + heap->num_of_buffers = 0; > >> + heap->num_of_alloc_bytes = 0; > >> + heap->alloc_bytes_wm = 0; > >> + spin_lock_init(&heap->stat_lock); > >> + heap_root = debugfs_create_dir(heap->name, dma_heap_debug_root); > >> + debugfs_create_u64("num_of_buffers", 0444, heap_root, > >> &heap->num_of_buffers); > >> + debugfs_create_u64("num_of_alloc_bytes", 0444, heap_root, > >> &heap->num_of_alloc_bytes); > >> + debugfs_create_u64("alloc_bytes_wm", 0444, heap_root, > >> &heap->alloc_bytes_wm); > > > > I can see it being useful to be able to reset this one. > > > > Agree, looks like these will be pulled into the heaps themselves in the > next rev John is working on, so shouldn't matter here. Yea. Sort of half-way done on this. I yanked the stats, but haven't re-added them back to the heaps yet. > What we are moving to is a better (I think) ownership model. 'DMA-heaps' > only tracks 'heaps', 'heaps' track their 'buffers'. In the above we have > 'DMA-heaps' tracking info on 'buffers', bypassing the 'heaps' layer, so > in the next rev will be the 'DMA-heaps' core asks the 'heaps' to report > back stats. Yea. This matches my plan. > >> +/* > >> + * mappings of this buffer should be un-cached, otherwise dmabuf heaps > >> will > >> + * need cache maintenance using DMA_BUF_SYNC_* when the buffer is mapped > >> for dma > >> + */ > >> +#define DMA_HEAP_FLAG_COHERENT 1 > > > > I'm not really clear on the intention of this flag, and I do think > > that "COHERENT" is a confusing/overloaded term. > > > > It is, I wanted to use the term as used by the other DMA frameworks, but > I don't really like it either. > > > To me, if the buffer is actually coherent with DMA masters, then > > there's no need for any cache maintenance - which is the opposite of > > what the comment says. > > > > Buffers themselves can't be (non)coherent, they are just memory, masters > on a bus can have coherent interactions depending on which masters are > talking and how the buffer in question is managed. So really the term > isn't right almost anywhere as it only applies from the perspective of > the local core (running Linux) and only if simply not locally caching > the buffer is enough to make it "coherent". But that is a rant best > saved for another time. > > For us lets drop that flag, if you want to allocate from a > non-locally-cached buffer then it can be its own heap that only provides > that type of allocation. Or even the same heap exporter providing both > types, just registers itself twice with the DMA-heaps core, once for an > interface that gives out cached buffers and one for uncached. So I've not removed this yet. My only concern is that if its a reasonable common attribute for heaps to implement, we probably should keep it, rather then pushing for new heaps for coherent/non-coherent. This comes from my experience creating the CLOCK_REALTIME_ALARM, CLOCK_BOOTTIME_ALARM clockids, then later realizing ALARM should have been just a attribute flag on the REALTIME/BOOTTIME clockids. I'd rather not rework the heaps to have system and system-coherent and cma and cma-coherent, if its a general thing. That said, I did find the flag's meaning confusing myself initially, so maybe holding off on it for now (if we don't have a clear user) is a good idea? thanks -john _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel