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
