linux-kernel - Re: [PATCH] drm/etnaviv: Create an accel device node if compute-only

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAAObsKDFDNV48XF1DobvVv04Q+DfBY=weBJPKbBewJPbPibRVA@mail.gmail.com>
Date: Mon, 20 May 2024 11:24:26 +0200
From: Tomeu Vizoso <tomeu@...euvizoso.net>
To: Lucas Stach <l.stach@...gutronix.de>
Cc: linux-kernel@...r.kernel.org, Oded Gabbay <ogabbay@...nel.org>, 
	Russell King <linux+etnaviv@...linux.org.uk>, 
	Christian Gmeiner <christian.gmeiner@...il.com>, David Airlie <airlied@...il.com>, 
	Daniel Vetter <daniel@...ll.ch>, etnaviv@...ts.freedesktop.org, 
	dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH] drm/etnaviv: Create an accel device node if compute-only

On Fri, May 10, 2024 at 10:34 AM Lucas Stach <l.stach@...gutronix.de> wrote:
>
> Hi Tomeu,
>
> Am Mittwoch, dem 24.04.2024 um 08:37 +0200 schrieb Tomeu Vizoso:
> > If we expose a render node for NPUs without rendering capabilities, the
> > userspace stack will offer it to compositors and applications for
> > rendering, which of course won't work.
> >
> > Userspace is probably right in not questioning whether a render node
> > might not be capable of supporting rendering, so change it in the kernel
> > instead by exposing a /dev/accel node.
> >
> > Before we bring the device up we don't know whether it is capable of
> > rendering or not (depends on the features of its blocks), so first try
> > to probe a rendering node, and if we find out that there is no rendering
> > hardware, abort and retry with an accel node.
> >
> I thought about this for a while. My opinion is that this is the wrong
> approach. We are adding another path to the kernel driver, potentially
> complicating the userspace side, as now the NPU backend needs to look
> for both render and accel nodes.

Forgot to mention in my earlier reply today that with the proposed
solution no changes are needed in the Gallium drivers, only in the
pipeloader component in Mesa, and in the Gallium frontends.

But those changes are needed anyway to support the upcoming
compute-only NPUs, such as Rockchip's.

These are the changes I needed to make to the userspace to go with
this kernel patch:

https://gitlab.freedesktop.org/tomeu/mesa/-/commit/6b0db4cce406c574d2b7710208df9c8bd1ab6345

Cheers,

Tomeu

> While currently accel and drm are
> pretty closely related and we can share most of the driver, it might
> still be a maintenance hassle in the long run.
>
> On the other hand we already have precedence of compute only DRM
> devices exposing a render node: there are AMD GPUs that don't expose a
> graphics queue and are thus not able to actually render graphics. Mesa
> already handles this in part via the PIPE_CAP_GRAPHICS and I think we
> should simply extend this to not offer a EGL display on screens without
> that capability.
>
> Regards,
> Lucas
>
> > Signed-off-by: Tomeu Vizoso <tomeu@...euvizoso.net>
> > Cc: Oded Gabbay <ogabbay@...nel.org>
> > ---
> >  drivers/gpu/drm/etnaviv/etnaviv_drv.c | 46 ++++++++++++++++++++++-----
> >  1 file changed, 38 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> > index 6500f3999c5f..8e7dd23115f4 100644
> > --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> > +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c
> > @@ -11,6 +11,7 @@
> >  #include <linux/platform_device.h>
> >  #include <linux/uaccess.h>
> >
> > +#include <drm/drm_accel.h>
> >  #include <drm/drm_debugfs.h>
> >  #include <drm/drm_drv.h>
> >  #include <drm/drm_file.h>
> > @@ -488,10 +489,10 @@ static const struct drm_ioctl_desc etnaviv_ioctls[] = {
> >       ETNA_IOCTL(PM_QUERY_SIG, pm_query_sig, DRM_RENDER_ALLOW),
> >  };
> >
> > -DEFINE_DRM_GEM_FOPS(fops);
> > +DEFINE_DRM_GEM_FOPS(render_fops);
> > +DEFINE_DRM_ACCEL_FOPS(accel_fops);
> >
> > -static const struct drm_driver etnaviv_drm_driver = {
> > -     .driver_features    = DRIVER_GEM | DRIVER_RENDER,
> > +static struct drm_driver etnaviv_drm_driver = {
> >       .open               = etnaviv_open,
> >       .postclose           = etnaviv_postclose,
> >       .gem_prime_import_sg_table = etnaviv_gem_prime_import_sg_table,
> > @@ -500,7 +501,6 @@ static const struct drm_driver etnaviv_drm_driver = {
> >  #endif
> >       .ioctls             = etnaviv_ioctls,
> >       .num_ioctls         = DRM_ETNAVIV_NUM_IOCTLS,
> > -     .fops               = &fops,
> >       .name               = "etnaviv",
> >       .desc               = "etnaviv DRM",
> >       .date               = "20151214",
> > @@ -508,15 +508,20 @@ static const struct drm_driver etnaviv_drm_driver = {
> >       .minor              = 4,
> >  };
> >
> > -/*
> > - * Platform driver:
> > - */
> > -static int etnaviv_bind(struct device *dev)
> > +static int etnaviv_bind_with_type(struct device *dev, u32 type)
> >  {
> >       struct etnaviv_drm_private *priv;
> >       struct drm_device *drm;
> > +     bool is_compute_only = true;
> >       int ret;
> >
> > +     etnaviv_drm_driver.driver_features = DRIVER_GEM | type;
> > +
> > +     if (type == DRIVER_RENDER)
> > +             etnaviv_drm_driver.fops = &render_fops;
> > +     else
> > +             etnaviv_drm_driver.fops = &accel_fops;
> > +
> >       drm = drm_dev_alloc(&etnaviv_drm_driver, dev);
> >       if (IS_ERR(drm))
> >               return PTR_ERR(drm);
> > @@ -553,6 +558,18 @@ static int etnaviv_bind(struct device *dev)
> >
> >       load_gpu(drm);
> >
> > +     for (unsigned int i = 0; i < ETNA_MAX_PIPES; i++) {
> > +             struct etnaviv_gpu *g = priv->gpu[i];
> > +
> > +             if (g && (g->identity.minor_features8 & chipMinorFeatures8_COMPUTE_ONLY) == 0)
> > +                     is_compute_only = false;
> > +     }
> > +
> > +     if (type == DRIVER_RENDER && is_compute_only) {
> > +             ret = -EINVAL;
> > +             goto out_unbind;
> > +     }
> > +
> >       ret = drm_dev_register(drm, 0);
> >       if (ret)
> >               goto out_unbind;
> > @@ -571,6 +588,19 @@ static int etnaviv_bind(struct device *dev)
> >       return ret;
> >  }
> >
> > +/*
> > + * Platform driver:
> > + */
> > +static int etnaviv_bind(struct device *dev)
> > +{
> > +     int ret = etnaviv_bind_with_type(dev, DRIVER_RENDER);
> > +
> > +     if (ret == -EINVAL)
> > +             return etnaviv_bind_with_type(dev, DRIVER_COMPUTE_ACCEL);
> > +
> > +     return ret;
> > +}
> > +
> >  static void etnaviv_unbind(struct device *dev)
> >  {
> >       struct drm_device *drm = dev_get_drvdata(dev);
>