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

[Eric Anholt]

On Mon, Apr 27, 2020 at 1:05 PM Peter Collingbourne <pcc at google.com> 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.
>
> Because all drivers that currently support render nodes also support
> buffer sharing, the DRIVER_RENDER flag is no longer necessary to mark
> devices as supporting render nodes, so remove it and just rely on
> the presence of a prime_handle_to_fd function pointer to determine
> whether buffer sharing is supported.

I'm definitely interested in seeing a patch like this land, as I think
the current state is an ugly historical artifact.  We just have to be
careful.

Were there any instances of drivers with render engines exposing PRIME
but not RENDER?  We should be careful to make sure that we're not
exposing new privileges for those through adding render nodes.

There's a UAPI risk I see here.  Right now, on a system with a single
renderer GPU, we can just open /dev/dri/renderD128 and get the GPU for
rendering, and various things are relying on that (such as libwaffle,
used in piglit among other things)   Adding render nodes for kms-only
drivers could displace the actual GPU to 129, and the question is
whether this will be disruptive.  For Mesa, I think this works out,
because kmsro should load on the kms device's node and then share
buffers over to the real GPU that it digs around to find at init time.
Just saying, I'm not sure I know all of the userspace well enough to
say "this should be safe despite that"

(And, maybe, if we decide that it's not safe enough, we could punt
kms-only drivers to a higher starting number?)

> @@ -260,12 +258,6 @@ static int vc4_drm_bind(struct device *dev)
>         if (!vc4)
>                 return -ENOMEM;
>
> -       /* If VC4 V3D is missing, don't advertise render nodes. */
> -       node = of_find_matching_node_and_match(NULL, vc4_v3d_dt_match, NULL);
> -       if (!node || !of_device_is_available(node))
> -               vc4_drm_driver.driver_features &= ~DRIVER_RENDER;
> -       of_node_put(node);
> -
>         drm = drm_dev_alloc(&vc4_drm_driver, dev);
>         if (IS_ERR(drm))
>                 return PTR_ERR(drm);

Looks like dropping this code from vc4 should be fine -- kmsro looks
for a render node from each driver name it supports in turn, so even
if v3d moves from renderD128 to renderD129, things should still work.