Re: [RFC PATCH 2/2] vc4: introduce DMA-BUF heap

[Sebastian Wick <sebastian.wick@redhat.com>]

On Thu, Nov 09, 2023 at 06:38:20PM +0000, Dave Stevenson wrote:
> Hi Simon
> 
> On Thu, 9 Nov 2023 at 17:46, Simon Ser <contact@emersion.fr> wrote:
> >
> > On Thursday, November 9th, 2023 at 16:42, Dave Stevenson <dave.stevenson@raspberrypi.com> wrote:
> >
> > > > > - What would be a good name for the heap? "vc4" is maybe a bit naive and
> > > > >   not precise enough. Something with "cma"? Do we need to plan a naming
> > > > >   scheme to accomodate for multiple vc4 devices?
> > > >
> > > > That's a general issue though that happens with pretty much all devices
> > > > with a separate node for modesetting and rendering, so I don't think
> > > > addressing it only for vc4 make sense, we should make it generic.
> > > >
> > > > So maybe something like "scanout"?
> > > >
> > > > One thing we need to consider too is that the Pi5 will have multiple
> > > > display nodes (4(?) iirc) with different capabilities, vc4 being only
> > > > one of them. This will impact that solution too.
> > >
> > > It does need to scale.
> > >
> > > Pi5 adds 4 additional DRM devices (2xDSI, 1xDPI, and 1xComposite
> > > Video), and just this last week I've been running Wayfire with TinyDRM
> > > drivers for SPI displays and UDL (DisplayLink) outputs as well.
> > > Presumably all of those drivers need to have appropriate hooks added
> > > so they each expose a dma-heap to enable scanout buffers to be
> > > allocated.
> >
> > I'm not sure this makes sense necessarily for all of these devices. For vc4 and
> > the 4 additional RPi 5 DRM devices, probably. For the rest, e.g. UDL, I'm not
> > sure it makes sense to expose scanout memory allocation via DMA heaps: AFAIK
> > UDL needs CPU access to the buffers, it can't "scanout", and thus directly
> > rendering via v3d to a "scanout-capable" buffer doesn't make sense. There will
> > be a copy (CPU download) either way, and allocating via v3d wouldn't make a
> > difference.
> 
> You as a developer may know that UDL is going to need CPU access, but
> how does a generic userspace app know? Is it a case of falling back to
> allocating via the renderer if there is no suitably named heap?

If we're going with the idea of using sysfs to link a device to a heap
(which I think we should), then those devices all should be linked to
the system memory heap, no?

> > > Can we add another function pointer to the struct drm_driver (or
> > > similar) to do the allocation, and move the actual dmabuf handling
> > > code into the core?
> >
> > Do you mean that the new logic introduced in this patch should be in DRM core
> > to allow other drivers to more easily re-use it? Or do you mean something else?
> 
> Yes, make it easy to reuse between drivers.
> 
> > Indeed, there is nothing vc4-specific in this patch, the only requirement is
> > that the driver uses drm_gem_dma_helper. So this code could be moved into (or
> > alongside) that helper in DRM core. However, maybe it would be best to wait to
> > have a second user for this infrastructure before we move into core.
> 
> Upstreaming of the DSI / DPI / composite drivers for Pi5 should only
> be a few months away, and they can all directly scanout.
> 
> I expect the Rockchip platforms to also fall into the same category as
> the Pi, with Mali as the 3D IP, and the VOP block as the scanout
> engine. Have I missed some detail that means that they aren't a second
> user for this?
> 
> > > > > - Need to add !vc5 support.
> > > >
> > > > If by !vc5 you mean RPi0-3, then it's probably not needed there at all
> > > > since it has a single node for both modesetting and rendering?
> > >
> > > That is true, but potentially vc4 could be rendering for scanout via UDL or SPI.
> > > Is it easier to always have the dma-heap allocator for every DRM card
> > > rather than making userspace mix and match depending on whether it is
> > > all in one vs split?
> >
> > I don't think it's realistic to try to always make DMA heaps available for each
> > and every driver which might need it from day one. It's too big of a task. And
> > it's an even bigger task to try to design a fully generic heap compatibility
> > uAPI from day one. I'd much rather add the heaps one by one, if and when we
> > figure that it makes sense, and incrementally work our way through.
> 
> Is it really that massive a task? We have the dma heap UAPI for
> handling the allocations, so what new UAPI is required?
> 
> If it's a new function pointer in struct drm_driver, then the heap is
> only created by the core if that function is defined using the driver
> name. The function returns a struct dma_buf *.
> Any driver using drm_gem_dma_helper can use the new helper function
> that is basically your vc4_dma_heap_allocate. The "if
> (WARN_ON_ONCE(!vc4->is_vc5))" could be handled by not setting the
> function pointer on those platforms.
> 
> Sorry, I feel I must be missing some critical piece of information here.
> 
>   Dave
>