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: <CAAObsKBjytOtVrDL5TKWqrBO3OuhQtRVk5Mn3MA801xyRczo+w@mail.gmail.com>
Date: Fri, 26 Apr 2024 07:36:09 +0200
From: Tomeu Vizoso <tomeu@...euvizoso.net>
To: Christian Gmeiner <christian.gmeiner@...il.com>
Cc: linux-kernel@...r.kernel.org, Oded Gabbay <ogabbay@...nel.org>, 
	Lucas Stach <l.stach@...gutronix.de>, Russell King <linux+etnaviv@...linux.org.uk>, 
	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 Thu, Apr 25, 2024 at 1:32 PM Christian Gmeiner
<christian.gmeiner@...il.com> wrote:
>
> Hi Tomeu,
>
> >
> > 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 really love this idea of moving away from a render node. What needs to be done
> on the userspace side?

Doesn't seem that bad, here is a proof of concept:

https://gitlab.freedesktop.org/tomeu/mesa/-/tree/teflon-accel

Thanks for taking a look.

Tomeu

> > Signed-off-by: Tomeu Vizoso <tomeu@...euvizoso.net>
> > Cc: Oded Gabbay <ogabbay@...nel.org>
>
> Reviewed-by: Christian Gmeiner <cgmeiner@...lia.com>
>
> > ---
> >  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);
> > --
> > 2.44.0
> >
>
>
> --
> greets
> --
> Christian Gmeiner, MSc
>
> https://christian-gmeiner.info/privacypolicy