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.
Signed-off-by: Tomeu Vizoso <[email protected]>
Cc: Oded Gabbay <[email protected]>
---
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
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 <[email protected]>
> Cc: Oded Gabbay <[email protected]>
Reviewed-by: Christian Gmeiner <[email protected]>
> ---
> 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
On 4/24/2024 12:37 AM, Tomeu Vizoso wrote:
> 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.
>
> Signed-off-by: Tomeu Vizoso <[email protected]>
> Cc: Oded Gabbay <[email protected]>
I hope Oded chimes in as Accel maintainer. I think Airlie/Vetter had
also previously mentioned they'd have opinions on what is Accel vs DRM.
This gets a nack from me in its current state. This is not a strong
nack, and I don't want to discourage you. I think there is a path forward.
The Accel subsystem documentation says that accel drivers will reside in
drivers/accel/ but this does not.
Also, the commit text for "accel: add dedicated minor for accelerator
devices" mentions -
"for drivers that
declare they handle compute accelerator, using a new driver feature
flag called DRIVER_COMPUTE_ACCEL. It is important to note that this
driver feature is mutually exclusive with DRIVER_RENDER. Devices that
want to expose both graphics and compute device char files should be
handled by two drivers that are connected using the auxiliary bus
framework."
I don't see any of that happening here (two drivers connected by aux
bus, one in drivers/accel).
I think this is the first case we've had of a combo DRM/Accel usecase,
and so there isn't an existing example to refer you to on how to
structure things. I think you are going to be the first example where
we figure all of this out.
On a more implementation note, ioctls for Accel devices should not be
marked DRM_RENDER_ALLOW. Seems like your attempt to reuse as much of
the code as possible trips over this.
-Jeff
On Thu, Apr 25, 2024 at 1:32 PM Christian Gmeiner
<[email protected]> 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 <[email protected]>
> > Cc: Oded Gabbay <[email protected]>
>
> Reviewed-by: Christian Gmeiner <[email protected]>
>
> > ---
> > 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
On Thu, Apr 25, 2024 at 8:59 PM Jeffrey Hugo <[email protected]> wrote:
>
> On 4/24/2024 12:37 AM, Tomeu Vizoso wrote:
> > 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.
> >
> > Signed-off-by: Tomeu Vizoso <[email protected]>
> > Cc: Oded Gabbay <[email protected]>
>
> I hope Oded chimes in as Accel maintainer. I think Airlie/Vetter had
> also previously mentioned they'd have opinions on what is Accel vs DRM.
>
> This gets a nack from me in its current state. This is not a strong
> nack, and I don't want to discourage you. I think there is a path forward.
>
> The Accel subsystem documentation says that accel drivers will reside in
> drivers/accel/ but this does not.
Indeed, there is that code organization aspect.
> Also, the commit text for "accel: add dedicated minor for accelerator
> devices" mentions -
>
> "for drivers that
> declare they handle compute accelerator, using a new driver feature
> flag called DRIVER_COMPUTE_ACCEL. It is important to note that this
> driver feature is mutually exclusive with DRIVER_RENDER. Devices that
> want to expose both graphics and compute device char files should be
> handled by two drivers that are connected using the auxiliary bus
> framework."
>
> I don't see any of that happening here (two drivers connected by aux
> bus, one in drivers/accel).
Well, the text refers to devices, not drivers. The case we are talking
about is a driver that wants to sometimes expose an accel node, and
sometimes a render node, depending on the hardware it is dealing with.
So there would either be a device exposing a single render node, or a
device exposing a single accel node.
Though by using the auxiliary bus we could in theory solve the code
organization problem mentioned above, I'm not quite seeing how to do
this in a clean way. The driver in /drivers/gpu/drm would have to be a
DRM driver that doesn't register a DRM device, but registers a device
in the auxiliary bus for the driver in /drivers/accel to bind to? Or
are you seeing some possibility that would fit better in the current
DRM framework?
> I think this is the first case we've had of a combo DRM/Accel usecase,
> and so there isn't an existing example to refer you to on how to
> structure things. I think you are going to be the first example where
> we figure all of this out.
Yep, I will be grateful for any ideas on how to structure this.
> On a more implementation note, ioctls for Accel devices should not be
> marked DRM_RENDER_ALLOW. Seems like your attempt to reuse as much of
> the code as possible trips over this.
Indeed, thanks.
Cheers,
Tomeu
> -Jeff
Oded, Dave,
Do you have an opinion on this?
Thanks,
Tomeu
On Fri, Apr 26, 2024 at 8:10 AM Tomeu Vizoso <[email protected]> wrote:
>
> On Thu, Apr 25, 2024 at 8:59 PM Jeffrey Hugo <[email protected]> wrote:
> >
> > On 4/24/2024 12:37 AM, Tomeu Vizoso wrote:
> > > 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.
> > >
> > > Signed-off-by: Tomeu Vizoso <[email protected]>
> > > Cc: Oded Gabbay <[email protected]>
> >
> > I hope Oded chimes in as Accel maintainer. I think Airlie/Vetter had
> > also previously mentioned they'd have opinions on what is Accel vs DRM.
> >
> > This gets a nack from me in its current state. This is not a strong
> > nack, and I don't want to discourage you. I think there is a path forward.
> >
> > The Accel subsystem documentation says that accel drivers will reside in
> > drivers/accel/ but this does not.
>
> Indeed, there is that code organization aspect.
>
> > Also, the commit text for "accel: add dedicated minor for accelerator
> > devices" mentions -
> >
> > "for drivers that
> > declare they handle compute accelerator, using a new driver feature
> > flag called DRIVER_COMPUTE_ACCEL. It is important to note that this
> > driver feature is mutually exclusive with DRIVER_RENDER. Devices that
> > want to expose both graphics and compute device char files should be
> > handled by two drivers that are connected using the auxiliary bus
> > framework."
> >
> > I don't see any of that happening here (two drivers connected by aux
> > bus, one in drivers/accel).
>
> Well, the text refers to devices, not drivers. The case we are talking
> about is a driver that wants to sometimes expose an accel node, and
> sometimes a render node, depending on the hardware it is dealing with.
> So there would either be a device exposing a single render node, or a
> device exposing a single accel node.
>
> Though by using the auxiliary bus we could in theory solve the code
> organization problem mentioned above, I'm not quite seeing how to do
> this in a clean way. The driver in /drivers/gpu/drm would have to be a
> DRM driver that doesn't register a DRM device, but registers a device
> in the auxiliary bus for the driver in /drivers/accel to bind to? Or
> are you seeing some possibility that would fit better in the current
> DRM framework?
>
> > I think this is the first case we've had of a combo DRM/Accel usecase,
> > and so there isn't an existing example to refer you to on how to
> > structure things. I think you are going to be the first example where
> > we figure all of this out.
>
> Yep, I will be grateful for any ideas on how to structure this.
>
> > On a more implementation note, ioctls for Accel devices should not be
> > marked DRM_RENDER_ALLOW. Seems like your attempt to reuse as much of
> > the code as possible trips over this.
>
> Indeed, thanks.
>
> Cheers,
>
> Tomeu
>
> > -Jeff
On Thu, May 09, 2024 at 03:53:01PM +0200, Tomeu Vizoso wrote:
> Oded, Dave,
>
> Do you have an opinion on this?
>
> Thanks,
>
> Tomeu
Hi Tomeu,
Sorry for not replying earlier, I was down with Covid (again...).
To your question, I don't have an objection to what you are
suggesting. My personal view of accel is that it is an integral part of
DRM and therefore, if there is an *existing* drm driver that wants to
create an accel node, I'm not against it.
There is the question of why you want to expose an accel node, and
here I would like to hear Dave's and Sima's opinion on your suggested
solution as it may affect the direction of other drm drivers.
Thanks,
Oded.
p.s.
Please only use bottom-posting when replying, thanks :)
>
> On Fri, Apr 26, 2024 at 8:10 AM Tomeu Vizoso <[email protected]> wrote:
> >
> > On Thu, Apr 25, 2024 at 8:59 PM Jeffrey Hugo <[email protected]> wrote:
> > >
> > > On 4/24/2024 12:37 AM, Tomeu Vizoso wrote:
> > > > 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.
> > > >
> > > > Signed-off-by: Tomeu Vizoso <[email protected]>
> > > > Cc: Oded Gabbay <[email protected]>
> > >
> > > I hope Oded chimes in as Accel maintainer. I think Airlie/Vetter had
> > > also previously mentioned they'd have opinions on what is Accel vs DRM.
> > >
> > > This gets a nack from me in its current state. This is not a strong
> > > nack, and I don't want to discourage you. I think there is a path forward.
> > >
> > > The Accel subsystem documentation says that accel drivers will reside in
> > > drivers/accel/ but this does not.
> >
> > Indeed, there is that code organization aspect.
> >
> > > Also, the commit text for "accel: add dedicated minor for accelerator
> > > devices" mentions -
> > >
> > > "for drivers that
> > > declare they handle compute accelerator, using a new driver feature
> > > flag called DRIVER_COMPUTE_ACCEL. It is important to note that this
> > > driver feature is mutually exclusive with DRIVER_RENDER. Devices that
> > > want to expose both graphics and compute device char files should be
> > > handled by two drivers that are connected using the auxiliary bus
> > > framework."
> > >
> > > I don't see any of that happening here (two drivers connected by aux
> > > bus, one in drivers/accel).
> >
> > Well, the text refers to devices, not drivers. The case we are talking
> > about is a driver that wants to sometimes expose an accel node, and
> > sometimes a render node, depending on the hardware it is dealing with.
> > So there would either be a device exposing a single render node, or a
> > device exposing a single accel node.
> >
> > Though by using the auxiliary bus we could in theory solve the code
> > organization problem mentioned above, I'm not quite seeing how to do
> > this in a clean way. The driver in /drivers/gpu/drm would have to be a
> > DRM driver that doesn't register a DRM device, but registers a device
> > in the auxiliary bus for the driver in /drivers/accel to bind to? Or
> > are you seeing some possibility that would fit better in the current
> > DRM framework?
> >
> > > I think this is the first case we've had of a combo DRM/Accel usecase,
> > > and so there isn't an existing example to refer you to on how to
> > > structure things. I think you are going to be the first example where
> > > we figure all of this out.
> >
> > Yep, I will be grateful for any ideas on how to structure this.
> >
> > > On a more implementation note, ioctls for Accel devices should not be
> > > marked DRM_RENDER_ALLOW. Seems like your attempt to reuse as much of
> > > the code as possible trips over this.
> >
> > Indeed, thanks.
> >
> > Cheers,
> >
> > Tomeu
> >
> > > -Jeff