[PATCH] drm: enable render nodes wherever buffer sharing is supported

[Brian Starkey]

On Wed, Apr 29, 2020 at 06:31:01PM +0100, Liviu Dudau wrote:
> On Wed, Apr 29, 2020 at 09:51:19AM -0700, Peter Collingbourne wrote:
> > On Wed, Apr 29, 2020 at 9:17 AM Brian Starkey <brian.starkey at arm.com> wrote:
> > >
> > > Hi Peter,
> > >
> > > On Mon, Apr 27, 2020 at 01:05:13PM -0700, Peter Collingbourne wrote:
> > > > Render nodes are not just useful for devices supporting GPU hardware
> > > > acceleration. Even on devices that only support dumb frame buffers,
> > > > they are useful in situations where composition (using software
> > > > rasterization) and KMS are done in different processes with buffer
> > > > sharing being used to send frame buffers between them. This is the
> > > > situation on Android, where surfaceflinger is the compositor and the
> > > > composer HAL uses KMS to display the buffers. Thus it is beneficial
> > > > to expose render nodes on all devices that support buffer sharing.
> > >
> > > If I understand your problem right, the issue is that you're getting
> > > your buffers in minigbm via pl111, which means you want a render node
> > > just for buffer allocation? Then HWComposer will import those on the
> > > primary node for displaying.
> > 
> > Correct.
> > 
> > > I'm not at all familiar with how minigbm sits in Android, but on our
> > > platforms where the Display and GPU devices are different, we allocate
> > > via ion in Gralloc, and import those into both the GPU and Display.
> > > I think that should work on pl111 too - if you can allocate contiguous
> > > memory from ion (or dma-buf heaps) in minigbm, then you don't need the
> > > render node.
> > 
> > Those contiguous memory regions would not necessarily be compatible
> > with the pl111 device as far as I know -- according to [1], on some
> > devices, a designated memory region must be used for the framebuffer
> > and therefore the memory region allocated in CMA would not be
> > compatible. That being said, FVP doesn't appear to be one of those
> > devices, so in principle that might work for FVP (provided that the
> > user provides a sufficiently large cma=X kernel command line
> > argument), but not for those other devices.

Yeah I'd be surprised if FVP cares about anything other than it being
contiguous.

My understanding of how "most" Android devices implement this is to
have ion heaps representing whatever dedicated memory regions there
are. Gralloc can then pick the appropriate one based on the gralloc
usage flags. So allocations wouldn't go via the DRM driver.

It looks like the checks in pl111 can't support that usage model if
use_device_memory is set (though that wouldn't matter on FVP).

> 
> We have been doing that with mali-dp drivers for years. Because most of
> our dev environments are on FPGAs, we needed to use the local RAM on
> those boards so we've had to assign a memory region to the driver that
> in turn was using CMA. We've made heavy use of 'reserved-memory' and
> 'memory-region' nodes in the DT to link the two together, but things
> worked out quite well. Because the 'reserved-memory' sub-node was marked
> as 'compatible="shared-dma-pool"', gralloc and ION could be used to
> allocate memory from it.

This is a little imperfect because ion doesn't have a way to access
the `dev` pointer needed to allocate from device-attached CMA regions,
so some hackery is required.

I think dma-heaps would let you expose device-specific CMA regions.

Even if you did that and allocated from the right place, the check
in pl111 would reject any attempt to import it if it's set to
use_device_memory.

I don't know if there's a better way to tell if the memory is
compatible, but that check seems like a bit of a sledgehammer. It
looks like it not only forces the memory to be from the right place,
it also forces it to have been allocated via pl111.

On FVP though, I reckon any old CMA memory should be fine.

Cheers,
-Brian

> 
> Best regards,
> Liviu
> 
> > 
> > Peter
> > 
> > [1] https://www.kernel.org/doc/Documentation/devicetree/bindings/display/arm%2Cpl11x.txt
> 
> -- 
> ====================
> | I would like to |
> | fix the world,  |
> | but they're not |
> | giving me the   |
>  \ source code!  /
>   ---------------
>     ¯\_(ツ)_/¯