Re: [RESEND][PATCH v8 0/5] DMA-BUF Heaps (destaging ION)

From: "Andrew F. Davis" <afd@ti.com>
To: John Stultz <john.stultz@linaro.org>
Cc: Brian Starkey <Brian.Starkey@arm.com>, nd <nd@arm.com>,
	Sudipto Paul <Sudipto.Paul@arm.com>,
	Vincent Donnefort <Vincent.Donnefort@arm.com>,
	Chenbo Feng <fengc@google.com>,
	Alistair Strachan <astrachan@google.com>,
	Liam Mark <lmark@codeaurora.org>,
	lkml <linux-kernel@vger.kernel.org>,
	Christoph Hellwig <hch@infradead.org>,
	DRI mailing list <dri-devel@lists.freedesktop.org>,
	Hridya Valsaraju <hridya@google.com>,
	Ayan Halder <Ayan.Halder@arm.com>,
	Pratik Patel <pratikp@codeaurora.org>
Subject: Re: [RESEND][PATCH v8 0/5] DMA-BUF Heaps (destaging ION)
Date: Thu, 17 Oct 2019 15:29:05 -0400	[thread overview]
Message-ID: <a3c66479-7433-ec29-fbec-81aef60cb063@ti.com> (raw)
In-Reply-To: <CALAqxLV6EBHKPEaEkyfhEYyw0TXayTeY_4AWXfuASLLyxZh5+Q@mail.gmail.com>

On 10/17/19 3:14 PM, John Stultz wrote:
> On Wed, Oct 16, 2019 at 10:41 AM Andrew F. Davis <afd@ti.com> wrote:
>> On 10/14/19 5:07 AM, Brian Starkey wrote:
>>> Hi Andrew,
>>>
>>> On Wed, Oct 09, 2019 at 02:27:15PM -0400, Andrew F. Davis wrote:
>>>> The CMA driver that registers these nodes will have to be expanded to
>>>> export them using this framework as needed. We do something similar to
>>>> export SRAM nodes:
>>>>
>>>> https://lkml.org/lkml/2019/3/21/575
>>>>
>>>> Unlike the system/default-cma driver which can be centralized in the
>>>> tree, these extra exporters will probably live out in other subsystems
>>>> and so are added in later steps.
>>>>
>>>> Andrew
>>>
>>> I was under the impression that the "cma_for_each_area" loop in patch
>>> 4 would do that (add_cma_heaps). Is it not the case?
>>>
>>
>> For these cma nodes yes, I thought you meant reserved memory areas in
>> general.
> 
> Ok, sorry I didn't see this earlier, not only was I still dropped from
> the To list, but the copy I got from dri-devel ended up marked as
> spam.
> 
>> Just as a side note, I'm not a huge fan of the cma_for_each_area() to
>> begin with, it seems a bit out of place when they could be selectively
>> added as heaps as needed. Not sure how that will work with cma nodes
>> specifically assigned to devices, seems like we could just steal their
>> memory space from userspace with this..
> 
> So this would be a concern with ION as well, since it does the same
> thing because being able to allocate from multiple CMA heaps for
> device specific purpose is really useful.
> And at least with dmabuf heaps each heap can be given its own
> permissions so there's less likelihood for any abuse as you describe.
> 

Yes it was a problem with ION also, having individual files per heap
does help with some permissions, but my issue is what if I don't want my
CMA exported at all, cma_for_each_area() just grabs them all anyway.

> And it also allows various device cma nodes to still be allocated from
> using the same interface (rather then having to use a custom driver
> ioctl for each device).
> 

This is definitely the way to go, it's the implementation of how we get
the CMAs to export in the first place that is a bit odd.

> But if the objection stands, do you have a proposal for an alternative
> way to enumerate a subset of CMA heaps?
> 

When in staging ION had to reach into the CMA framework as the other
direction would not be allowed, so cma_for_each_area() was added. If
DMA-BUF heaps is not in staging then we can do the opposite, and have
the CMA framework register heaps itself using our framework. That way
the CMA system could decide what areas to export or not (maybe based on
a DT property or similar).

The end result is the same so we can make this change later (it has to
come after DMA-BUF heaps is in anyway).

Andrew

> thanks
> -john
> 