Received-SPF: pass (google.com: domain of linux-kernel+bounces-158450-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240424063753.3740664-1-tomeu@tomeuvizoso.net>
In-Reply-To: <20240424063753.3740664-1-tomeu@tomeuvizoso.net>
From: Christian Gmeiner <christian.gmeiner@gmail.com>
Date: Thu, 25 Apr 2024 13:32:23 +0200
Message-ID: <CAH9NwWdzeF0=USW_bckDhPSUrgTfnFuAq44m1OhEmTL4JnBzLg@mail.gmail.com>
Subject: Re: [PATCH] drm/etnaviv: Create an accel device node if compute-only
To: Tomeu Vizoso <tomeu@tomeuvizoso.net>
Cc: linux-kernel@vger.kernel.org, Oded Gabbay <ogabbay@kernel.org>, 
	Lucas Stach <l.stach@pengutronix.de>, Russell King <linux+etnaviv@armlinux.org.uk>, 
	David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>, etnaviv@lists.freedesktop.org, 
	dri-devel@lists.freedesktop.org
Content-Type: text/plain; charset="UTF-8"

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?

> Signed-off-by: Tomeu Vizoso <tomeu@tomeuvizoso.net>
> Cc: Oded Gabbay <ogabbay@kernel.org>

Reviewed-by: Christian Gmeiner <cgmeiner@igalia.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