Hi Simon On Thu, 9 Nov 2023 at 17:46, Simon Ser <contact@xxxxxxxxxxx> wrote: > > On Thursday, November 9th, 2023 at 16:42, Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx> 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? > > 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
