[RFC PATCH v3 0/3] new subsystem for compute accelerator devices

[RFC PATCH v3 0/3] new subsystem for compute accelerator devices

[Oded Gabbay]

This is the third version of the RFC following the comments given on the
second version, but more importantly, following testing done by the VPU
driver people and myself. We found out that there is a circular dependency
between DRM and accel. DRM calls accel exported symbols during init and when
accel devices are registering (all the minor handling), then accel calls DRM
exported symbols. Therefore, if the two components are compiled as modules,
there is a circular dependency.

To overcome this, I have decided to compile the accel core code as part of
the DRM kernel module (drm.ko). IMO, this is inline with the spirit of the
design choice to have accel reuse the DRM core code and avoid code
duplication.

Another important change is that I have reverted back to use IDR for minor
handling instead of xarray. This is because I have found that xarray doesn't
handle well the scenario where you allocate a NULL entry and then exchange it
with a real pointer. It appears xarray still considers that entry a "zero"
entry. This is unfortunate because DRM works that way (first allocates a NULL
entry and then replaces the entry with a real pointer).

I decided to revert to IDR because I don't want to hold up these patches,
as many people are blocked until the support for accel is merged. The xarray
issue should be fixed as a separate patch by either fixing the xarray code or
changing how DRM + ACCEL do minor id handling.

The patches are in the following repo:
https://git.kernel.org/pub/scm/linux/kernel/git/ogabbay/accel.git/log/?h=accel_v3

As in v2, The HEAD of that branch is a commit adding a dummy driver that
registers an accel device using the new framework. This can be served
as a simple reference. I have checked inserting and removing the dummy driver,
and opening and closing /dev/accel/accel0 and nothing got broken :)

v1 cover letter:
https://lkml.org/lkml/2022/10/22/544

v2 cover letter:
https://lore.kernel.org/lkml/20221102203405.1797491-1-ogabbay@kernel.org/T/

Thanks,
Oded.

Oded Gabbay (3):
  drivers/accel: define kconfig and register a new major
  accel: add dedicated minor for accelerator devices
  drm: initialize accel framework

 Documentation/admin-guide/devices.txt |   5 +
 MAINTAINERS                           |   8 +
 drivers/Kconfig                       |   2 +
 drivers/accel/Kconfig                 |  24 ++
 drivers/accel/drm_accel.c             | 322 ++++++++++++++++++++++++++
 drivers/gpu/drm/Makefile              |   1 +
 drivers/gpu/drm/drm_drv.c             | 102 +++++---
 drivers/gpu/drm/drm_file.c            |   2 +-
 drivers/gpu/drm/drm_sysfs.c           |  24 +-
 include/drm/drm_accel.h               |  97 ++++++++
 include/drm/drm_device.h              |   3 +
 include/drm/drm_drv.h                 |   8 +
 include/drm/drm_file.h                |  21 +-
 13 files changed, 582 insertions(+), 37 deletions(-)
 create mode 100644 drivers/accel/Kconfig
 create mode 100644 drivers/accel/drm_accel.c
 create mode 100644 include/drm/drm_accel.h

--
2.25.1

[RFC PATCH v3 1/3] drivers/accel: define kconfig and register a new major

[Oded Gabbay]

Add a new Kconfig for the accel subsystem. The Kconfig currently
contains only the basic CONFIG_DRM_ACCEL option that will be used to
decide whether to compile the accel registration code. Therefore, the
kconfig option is defined as bool.

The accel code will be compiled as part of drm.ko and will be called
directly from the DRM core code. The reason we compile it as part of
drm.ko and not as a separate module is because of cyclic dependency
between drm.ko and the separate module (if it would have existed).
This is due to the fact that DRM core code calls accel functions and
vice-versa.

The accelerator devices will be exposed to the user space with a new,
dedicated major number - 261.

The accel init function registers the new major number as a char device
and create corresponding sysfs and debugfs root entries, similar to
what is done in DRM init function.

I added a new header called drm_accel.h to include/drm/, that will hold
the prototypes of the drm_accel.c functions. In case CONFIG_DRM_ACCEL
is set to 'N', that header will contain empty inline implementations of
those functions, to allow DRM core code to compile successfully
without dependency on CONFIG_DRM_ACCEL.

I Updated the MAINTAINERS file accordingly with the newly added folder
and I have taken the liberty to appropriate the dri-devel mailing list
and the dri-devel IRC channel for the accel subsystem.

Signed-off-by: Oded Gabbay <ogabbay@kernel.org>
---
Changes in v3:
 - Remove accel kernel module and build the code as part of drm.ko.
 - Rename CONFIG_ACCEL to CONFIG_DRM_ACCEL as the code is built as part of
   drm.ko.
 - Rename accel_drv.c to drm_accel.c
 - Change kconfig option to be bool instead of tristate
 - Add . at end of help text in kconfig

 Documentation/admin-guide/devices.txt |  5 ++
 MAINTAINERS                           |  8 +++
 drivers/Kconfig                       |  2 +
 drivers/accel/Kconfig                 | 24 ++++++++
 drivers/accel/drm_accel.c             | 82 +++++++++++++++++++++++++++
 drivers/gpu/drm/Makefile              |  1 +
 include/drm/drm_accel.h               | 31 ++++++++++
 7 files changed, 153 insertions(+)
 create mode 100644 drivers/accel/Kconfig
 create mode 100644 drivers/accel/drm_accel.c
 create mode 100644 include/drm/drm_accel.h

diff --git a/Documentation/admin-guide/devices.txt b/Documentation/admin-guide/devices.txt
index 9764d6edb189..06c525e01ea5 100644
--- a/Documentation/admin-guide/devices.txt
+++ b/Documentation/admin-guide/devices.txt
@@ -3080,6 +3080,11 @@
 		  ...
 		  255 = /dev/osd255	256th OSD Device

+ 261 char	Compute Acceleration Devices
+		  0 = /dev/accel/accel0	First acceleration device
+		  1 = /dev/accel/accel1	Second acceleration device
+		    ...
+
  384-511 char	RESERVED FOR DYNAMIC ASSIGNMENT
 		Character devices that request a dynamic allocation of major
 		number will take numbers starting from 511 and downward,
diff --git a/MAINTAINERS b/MAINTAINERS
index 30e3df70daec..5b07d81c985e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6825,6 +6825,14 @@ F:	include/drm/drm*
 F:	include/linux/vga*
 F:	include/uapi/drm/drm*

+DRM COMPUTE ACCELERATORS DRIVERS AND FRAMEWORK
+M:	Oded Gabbay <ogabbay@kernel.org>
+L:	dri-devel@lists.freedesktop.org
+S:	Maintained
+C:	irc://irc.oftc.net/dri-devel
+T:	git https://git.kernel.org/pub/scm/linux/kernel/git/ogabbay/accel.git
+F:	drivers/accel/
+
 DRM DRIVERS FOR ALLWINNER A10
 M:	Maxime Ripard <mripard@kernel.org>
 M:	Chen-Yu Tsai <wens@csie.org>
diff --git a/drivers/Kconfig b/drivers/Kconfig
index 19ee995bd0ae..968bd0a6fd78 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -99,6 +99,8 @@ source "drivers/media/Kconfig"

 source "drivers/video/Kconfig"

+source "drivers/accel/Kconfig"
+
 source "sound/Kconfig"

 source "drivers/hid/Kconfig"
diff --git a/drivers/accel/Kconfig b/drivers/accel/Kconfig
new file mode 100644
index 000000000000..c9ce849b2984
--- /dev/null
+++ b/drivers/accel/Kconfig
@@ -0,0 +1,24 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# Compute Acceleration device configuration
+#
+# This framework provides support for compute acceleration devices, such
+# as, but not limited to, Machine-Learning and Deep-Learning acceleration
+# devices
+#
+menuconfig DRM_ACCEL
+	bool "Compute Acceleration Framework"
+	depends on DRM
+	help
+	  Framework for device drivers of compute acceleration devices, such
+	  as, but not limited to, Machine-Learning and Deep-Learning
+	  acceleration devices.
+	  If you say Y here, you need to select the module that's right for
+	  your acceleration device from the list below.
+	  This framework is integrated with the DRM subsystem as compute
+	  accelerators and GPUs share a lot in common and can use almost the
+	  same infrastructure code.
+	  Having said that, acceleration devices will have a different
+	  major number than GPUs, and will be exposed to user-space using
+	  different device files, called accel/accel* (in /dev, sysfs
+	  and debugfs).
diff --git a/drivers/accel/drm_accel.c b/drivers/accel/drm_accel.c
new file mode 100644
index 000000000000..943d960ddefc
--- /dev/null
+++ b/drivers/accel/drm_accel.c
@@ -0,0 +1,82 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * Copyright 2022 HabanaLabs, Ltd.
+ * All Rights Reserved.
+ *
+ */
+
+#include <linux/debugfs.h>
+#include <linux/device.h>
+
+#include <drm/drm_accel.h>
+#include <drm/drm_ioctl.h>
+#include <drm/drm_print.h>
+
+static struct dentry *accel_debugfs_root;
+static struct class *accel_class;
+
+static char *accel_devnode(struct device *dev, umode_t *mode)
+{
+	return kasprintf(GFP_KERNEL, "accel/%s", dev_name(dev));
+}
+
+static int accel_sysfs_init(void)
+{
+	accel_class = class_create(THIS_MODULE, "accel");
+	if (IS_ERR(accel_class))
+		return PTR_ERR(accel_class);
+
+	accel_class->devnode = accel_devnode;
+
+	return 0;
+}
+
+static void accel_sysfs_destroy(void)
+{
+	if (IS_ERR_OR_NULL(accel_class))
+		return;
+	class_destroy(accel_class);
+	accel_class = NULL;
+}
+
+static int accel_stub_open(struct inode *inode, struct file *filp)
+{
+	return -EOPNOTSUPP;
+}
+
+static const struct file_operations accel_stub_fops = {
+	.owner = THIS_MODULE,
+	.open = accel_stub_open,
+	.llseek = noop_llseek,
+};
+
+void accel_core_exit(void)
+{
+	unregister_chrdev(ACCEL_MAJOR, "accel");
+	debugfs_remove(accel_debugfs_root);
+	accel_sysfs_destroy();
+}
+
+int __init accel_core_init(void)
+{
+	int ret;
+
+	ret = accel_sysfs_init();
+	if (ret < 0) {
+		DRM_ERROR("Cannot create ACCEL class: %d\n", ret);
+		goto error;
+	}
+
+	accel_debugfs_root = debugfs_create_dir("accel", NULL);
+
+	ret = register_chrdev(ACCEL_MAJOR, "accel", &accel_stub_fops);
+	if (ret < 0)
+		goto error;
+
+error:
+	/* Any cleanup will be done in drm_core_exit() that will call
+	 * to accel_core_exit()
+	 */
+	return ret;
+}
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 6e55c47288e4..f51aa5eca7e7 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -70,6 +70,7 @@ drm-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
 drm-$(CONFIG_DRM_PRIVACY_SCREEN) += \
 	drm_privacy_screen.o \
 	drm_privacy_screen_x86.o
+drm-$(CONFIG_DRM_ACCEL) += ../../accel/drm_accel.o
 obj-$(CONFIG_DRM)	+= drm.o

 obj-$(CONFIG_DRM_NOMODESET) += drm_nomodeset.o
diff --git a/include/drm/drm_accel.h b/include/drm/drm_accel.h
new file mode 100644
index 000000000000..31b42d3d6a15
--- /dev/null
+++ b/include/drm/drm_accel.h
@@ -0,0 +1,31 @@
+/* SPDX-License-Identifier: GPL-2.0
+ *
+ * Copyright 2022 HabanaLabs, Ltd.
+ * All Rights Reserved.
+ *
+ */
+
+#ifndef DRM_ACCEL_H_
+#define DRM_ACCEL_H_
+
+#define ACCEL_MAJOR     261
+
+#if IS_ENABLED(CONFIG_DRM_ACCEL)
+
+void accel_core_exit(void);
+int accel_core_init(void);
+
+#else
+
+static inline void accel_core_exit(void)
+{
+}
+
+static inline int __init accel_core_init(void)
+{
+	return 0;
+}
+
+#endif /* IS_ENABLED(CONFIG_DRM_ACCEL) */
+
+#endif /* DRM_ACCEL_H_ */
--
2.25.1

Re: [RFC PATCH v3 1/3] drivers/accel: define kconfig and register a new major

[Jeffrey Hugo]

On 11/6/2022 2:02 PM, Oded Gabbay wrote:
> +int __init accel_core_init(void)
> +{
> +	int ret;
> +
> +	ret = accel_sysfs_init();
> +	if (ret < 0) {
> +		DRM_ERROR("Cannot create ACCEL class: %d\n", ret);
> +		goto error;
> +	}
> +
> +	accel_debugfs_root = debugfs_create_dir("accel", NULL);
> +
> +	ret = register_chrdev(ACCEL_MAJOR, "accel", &accel_stub_fops);
> +	if (ret < 0)
> +		goto error;

We are not jumping over anything here.  Seems like this whole if block 
could just be removed.

> +
> +error:
> +	/* Any cleanup will be done in drm_core_exit() that will call
> +	 * to accel_core_exit()
> +	 */

This doesn't look like the standard multi-line comment style.  Are we 
going to say that the accel subsystem follows net and differs from the 
kernel standard?

> +	return ret;
> +}

Re: [RFC PATCH v3 1/3] drivers/accel: define kconfig and register a new major

[Oded Gabbay]

On Mon, Nov 7, 2022 at 6:12 PM Jeffrey Hugo <quic_jhugo@quicinc.com> wrote:
>
> On 11/6/2022 2:02 PM, Oded Gabbay wrote:
> > +int __init accel_core_init(void)
> > +{
> > +     int ret;
> > +
> > +     ret = accel_sysfs_init();
> > +     if (ret < 0) {
> > +             DRM_ERROR("Cannot create ACCEL class: %d\n", ret);
> > +             goto error;
> > +     }
> > +
> > +     accel_debugfs_root = debugfs_create_dir("accel", NULL);
> > +
> > +     ret = register_chrdev(ACCEL_MAJOR, "accel", &accel_stub_fops);
> > +     if (ret < 0)
> > +             goto error;
>
> We are not jumping over anything here.  Seems like this whole if block
> could just be removed.
correct, will be fixed.
>
> > +
> > +error:
> > +     /* Any cleanup will be done in drm_core_exit() that will call
> > +      * to accel_core_exit()
> > +      */
>
> This doesn't look like the standard multi-line comment style.  Are we
> going to say that the accel subsystem follows net and differs from the
> kernel standard?
I'll change it to the kernel standard.
Thx,
Oded
>
> > +     return ret;
> > +}

Re: [RFC PATCH v3 1/3] drivers/accel: define kconfig and register a new major

[Stanislaw Gruszka]

On Sun, Nov 06, 2022 at 11:02:23PM +0200, Oded Gabbay wrote:
> Add a new Kconfig for the accel subsystem. The Kconfig currently
> contains only the basic CONFIG_DRM_ACCEL option that will be used to
> decide whether to compile the accel registration code. Therefore, the
> kconfig option is defined as bool.
> 
> The accel code will be compiled as part of drm.ko and will be called
> directly from the DRM core code. The reason we compile it as part of
> drm.ko and not as a separate module is because of cyclic dependency
> between drm.ko and the separate module (if it would have existed).
> This is due to the fact that DRM core code calls accel functions and
> vice-versa.
> 
> The accelerator devices will be exposed to the user space with a new,
> dedicated major number - 261.
> 
> The accel init function registers the new major number as a char device
> and create corresponding sysfs and debugfs root entries, similar to
> what is done in DRM init function.
> 
> I added a new header called drm_accel.h to include/drm/, that will hold
> the prototypes of the drm_accel.c functions. In case CONFIG_DRM_ACCEL
> is set to 'N', that header will contain empty inline implementations of
> those functions, to allow DRM core code to compile successfully
> without dependency on CONFIG_DRM_ACCEL.
> 
> I Updated the MAINTAINERS file accordingly with the newly added folder
> and I have taken the liberty to appropriate the dri-devel mailing list
> and the dri-devel IRC channel for the accel subsystem.
> 
> Signed-off-by: Oded Gabbay <ogabbay@kernel.org>

Tested-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>

I tested those patches with intel_vpu driver. After initial troubles,
I got things worked with our driver and user mode components.

Regards
Stanislaw

Re: [RFC PATCH v3 1/3] drivers/accel: define kconfig and register a new major

[Oded Gabbay]

On Tue, Nov 8, 2022 at 2:46 PM Stanislaw Gruszka
<stanislaw.gruszka@linux.intel.com> wrote:
>
> On Sun, Nov 06, 2022 at 11:02:23PM +0200, Oded Gabbay wrote:
> > Add a new Kconfig for the accel subsystem. The Kconfig currently
> > contains only the basic CONFIG_DRM_ACCEL option that will be used to
> > decide whether to compile the accel registration code. Therefore, the
> > kconfig option is defined as bool.
> >
> > The accel code will be compiled as part of drm.ko and will be called
> > directly from the DRM core code. The reason we compile it as part of
> > drm.ko and not as a separate module is because of cyclic dependency
> > between drm.ko and the separate module (if it would have existed).
> > This is due to the fact that DRM core code calls accel functions and
> > vice-versa.
> >
> > The accelerator devices will be exposed to the user space with a new,
> > dedicated major number - 261.
> >
> > The accel init function registers the new major number as a char device
> > and create corresponding sysfs and debugfs root entries, similar to
> > what is done in DRM init function.
> >
> > I added a new header called drm_accel.h to include/drm/, that will hold
> > the prototypes of the drm_accel.c functions. In case CONFIG_DRM_ACCEL
> > is set to 'N', that header will contain empty inline implementations of
> > those functions, to allow DRM core code to compile successfully
> > without dependency on CONFIG_DRM_ACCEL.
> >
> > I Updated the MAINTAINERS file accordingly with the newly added folder
> > and I have taken the liberty to appropriate the dri-devel mailing list
> > and the dri-devel IRC channel for the accel subsystem.
> >
> > Signed-off-by: Oded Gabbay <ogabbay@kernel.org>
>
> Tested-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
>
> I tested those patches with intel_vpu driver. After initial troubles,
> I got things worked with our driver and user mode components.
>
> Regards
> Stanislaw

Thanks!
Great to hear that.
Oded

[RFC PATCH v3 2/3] accel: add dedicated minor for accelerator devices

[Oded Gabbay]

The accelerator devices are exposed to user-space using a dedicated
major. In addition, they are represented in /dev with new, dedicated
device char names: /dev/accel/accel*. This is done to make sure any
user-space software that tries to open a graphic card won't open
the accelerator device by mistake.

The above implies that the minor numbering should be separated from
the rest of the DRM devices. However, to avoid code duplication, we
want the drm_minor structure to be able to represent the accelerator
device.

To achieve this, we add a new drm_minor* to drm_device that represents
the accelerator device. This pointer is initialized 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.

In addition, we define a different IDR to handle the accelerators
minors. This is done to make the minor's index be identical to the
device index in /dev/. Any access to the IDR is done solely
by functions in accel_drv.c, as the IDR is define as static. The
DRM core functions call those functions in case they detect the minor's
type is DRM_MINOR_ACCEL.

We define a separate accel_open function (from drm_open) that the
accel drivers should set as their open callback function. Both these
functions eventually call the same drm_open_helper(), which had to be
changed to be non-static so it can be called from accel_drv.c.
accel_open() only partially duplicates drm_open as I removed some code
from it that handles legacy devices.

To help new drivers, I defined DEFINE_DRM_ACCEL_FOPS macro to easily
set the required function operations pointers structure.

Signed-off-by: Oded Gabbay <ogabbay@kernel.org>
---
Changes in v3:
 - Remove useless DRM_DEBUG("\n") at accel_stub_open()
 - Add function of accel_debugfs_init() as accel_debugfs_root is static
   member in drm_accel.c
 - Add DRM_ACCEL_FOPS and DEFINE_DRM_ACCEL_FOPS macros
 - Replace minor handling from xarray back to idr, as xarray doesn't handle
   well exchanging content of a NULL entry to non-NULL. This should be handled
   in a different patch that will either fix xarray code or change DRM minor
   init flow.
 - Make accel_minor_replace() to return void.

 drivers/accel/drm_accel.c  | 242 ++++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/drm_file.c |   2 +-
 include/drm/drm_accel.h    |  68 ++++++++++-
 include/drm/drm_device.h   |   3 +
 include/drm/drm_drv.h      |   8 ++
 include/drm/drm_file.h     |  21 +++-
 6 files changed, 340 insertions(+), 4 deletions(-)

diff --git a/drivers/accel/drm_accel.c b/drivers/accel/drm_accel.c
index 943d960ddefc..05167c929866 100644
--- a/drivers/accel/drm_accel.c
+++ b/drivers/accel/drm_accel.c
@@ -8,14 +8,25 @@

 #include <linux/debugfs.h>
 #include <linux/device.h>
+#include <linux/xarray.h>

 #include <drm/drm_accel.h>
+#include <drm/drm_debugfs.h>
+#include <drm/drm_drv.h>
+#include <drm/drm_file.h>
 #include <drm/drm_ioctl.h>
 #include <drm/drm_print.h>

+static DEFINE_SPINLOCK(accel_minor_lock);
+static struct idr accel_minors_idr;
+
 static struct dentry *accel_debugfs_root;
 static struct class *accel_class;

+static struct device_type accel_sysfs_device_minor = {
+	.name = "accel_minor"
+};
+
 static char *accel_devnode(struct device *dev, umode_t *mode)
 {
 	return kasprintf(GFP_KERNEL, "accel/%s", dev_name(dev));
@@ -40,9 +51,235 @@ static void accel_sysfs_destroy(void)
 	accel_class = NULL;
 }

+static int accel_name_info(struct seq_file *m, void *data)
+{
+	struct drm_info_node *node = (struct drm_info_node *) m->private;
+	struct drm_minor *minor = node->minor;
+	struct drm_device *dev = minor->dev;
+	struct drm_master *master;
+
+	mutex_lock(&dev->master_mutex);
+	master = dev->master;
+	seq_printf(m, "%s", dev->driver->name);
+	if (dev->dev)
+		seq_printf(m, " dev=%s", dev_name(dev->dev));
+	if (master && master->unique)
+		seq_printf(m, " master=%s", master->unique);
+	if (dev->unique)
+		seq_printf(m, " unique=%s", dev->unique);
+	seq_puts(m, "\n");
+	mutex_unlock(&dev->master_mutex);
+
+	return 0;
+}
+
+static const struct drm_info_list accel_debugfs_list[] = {
+	{"name", accel_name_info, 0}
+};
+#define ACCEL_DEBUGFS_ENTRIES ARRAY_SIZE(accel_debugfs_list)
+
+/**
+ * accel_debugfs_init() - Initialize debugfs for accel minor
+ * @minor: Pointer to the drm_minor instance.
+ * @minor_id: The minor's id
+ *
+ * This function initializes the drm minor's debugfs members and creates
+ * a root directory for the minor in debugfs. It also creates common files
+ * for accelerators and calls the driver's debugfs init callback.
+ */
+void accel_debugfs_init(struct drm_minor *minor, int minor_id)
+{
+	struct drm_device *dev = minor->dev;
+	char name[64];
+
+	INIT_LIST_HEAD(&minor->debugfs_list);
+	mutex_init(&minor->debugfs_lock);
+	sprintf(name, "%d", minor_id);
+	minor->debugfs_root = debugfs_create_dir(name, accel_debugfs_root);
+
+	drm_debugfs_create_files(accel_debugfs_list, ACCEL_DEBUGFS_ENTRIES,
+				 minor->debugfs_root, minor);
+
+	if (dev->driver->debugfs_init)
+		dev->driver->debugfs_init(minor);
+}
+
+/**
+ * accel_set_device_instance_params() - Set some device parameters for accel device
+ * @kdev: Pointer to the device instance.
+ * @index: The minor's index
+ *
+ * This function creates the dev_t of the device using the accel major and
+ * the device's minor number. In addition, it sets the class and type of the
+ * device instance to the accel sysfs class and device type, respectively.
+ */
+void accel_set_device_instance_params(struct device *kdev, int index)
+{
+	kdev->devt = MKDEV(ACCEL_MAJOR, index);
+	kdev->class = accel_class;
+	kdev->type = &accel_sysfs_device_minor;
+}
+
+/**
+ * accel_minor_alloc() - Allocates a new accel minor
+ *
+ * This function access the accel minors idr and allocates from it
+ * a new id to represent a new accel minor
+ *
+ * Return: A new id on success or error code in case idr_alloc failed
+ */
+int accel_minor_alloc(void)
+{
+	unsigned long flags;
+	int r;
+
+	spin_lock_irqsave(&accel_minor_lock, flags);
+	r = idr_alloc(&accel_minors_idr, NULL, 0, ACCEL_MAX_MINORS, GFP_NOWAIT);
+	spin_unlock_irqrestore(&accel_minor_lock, flags);
+
+	return r;
+}
+
+/**
+ * accel_minor_remove() - Remove an accel minor
+ * @index: The minor id to remove.
+ *
+ * This function access the accel minors idr and removes from
+ * it the member with the id that is passed to this function.
+ */
+void accel_minor_remove(int index)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&accel_minor_lock, flags);
+	idr_remove(&accel_minors_idr, index);
+	spin_unlock_irqrestore(&accel_minor_lock, flags);
+}
+
+/**
+ * accel_minor_replace() - Replace minor pointer in accel minors idr.
+ * @minor: Pointer to the new minor.
+ * @index: The minor id to replace.
+ *
+ * This function access the accel minors idr structure and replaces the pointer
+ * that is associated with an existing id. Because the minor pointer can be
+ * NULL, we need to explicitly pass the index.
+ *
+ * Return: 0 for success, negative value for error
+ */
+void accel_minor_replace(struct drm_minor *minor, int index)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&accel_minor_lock, flags);
+	idr_replace(&accel_minors_idr, minor, index);
+	spin_unlock_irqrestore(&accel_minor_lock, flags);
+}
+
+/*
+ * Looks up the given minor-ID and returns the respective DRM-minor object. The
+ * refence-count of the underlying device is increased so you must release this
+ * object with accel_minor_release().
+ *
+ * The object can be only a drm_minor that represents an accel device.
+ *
+ * As long as you hold this minor, it is guaranteed that the object and the
+ * minor->dev pointer will stay valid! However, the device may get unplugged and
+ * unregistered while you hold the minor.
+ */
+static struct drm_minor *accel_minor_acquire(unsigned int minor_id)
+{
+	struct drm_minor *minor;
+	unsigned long flags;
+
+	spin_lock_irqsave(&accel_minor_lock, flags);
+	minor = idr_find(&accel_minors_idr, minor_id);
+	if (minor)
+		drm_dev_get(minor->dev);
+	spin_unlock_irqrestore(&accel_minor_lock, flags);
+
+	if (!minor) {
+		return ERR_PTR(-ENODEV);
+	} else if (drm_dev_is_unplugged(minor->dev)) {
+		drm_dev_put(minor->dev);
+		return ERR_PTR(-ENODEV);
+	}
+
+	return minor;
+}
+
+static void accel_minor_release(struct drm_minor *minor)
+{
+	drm_dev_put(minor->dev);
+}
+
+/**
+ * accel_open - open method for ACCEL file
+ * @inode: device inode
+ * @filp: file pointer.
+ *
+ * This function must be used by drivers as their &file_operations.open method.
+ * It looks up the correct ACCEL device and instantiates all the per-file
+ * resources for it. It also calls the &drm_driver.open driver callback.
+ *
+ * Return: 0 on success or negative errno value on failure.
+ */
+int accel_open(struct inode *inode, struct file *filp)
+{
+	struct drm_device *dev;
+	struct drm_minor *minor;
+	int retcode;
+
+	minor = accel_minor_acquire(iminor(inode));
+	if (IS_ERR(minor))
+		return PTR_ERR(minor);
+
+	dev = minor->dev;
+
+	atomic_fetch_inc(&dev->open_count);
+
+	/* share address_space across all char-devs of a single device */
+	filp->f_mapping = dev->anon_inode->i_mapping;
+
+	retcode = drm_open_helper(filp, minor);
+	if (retcode)
+		goto err_undo;
+
+	return 0;
+
+err_undo:
+	atomic_dec(&dev->open_count);
+	accel_minor_release(minor);
+	return retcode;
+}
+EXPORT_SYMBOL_GPL(accel_open);
+
 static int accel_stub_open(struct inode *inode, struct file *filp)
 {
-	return -EOPNOTSUPP;
+	const struct file_operations *new_fops;
+	struct drm_minor *minor;
+	int err;
+
+	minor = accel_minor_acquire(iminor(inode));
+	if (IS_ERR(minor))
+		return PTR_ERR(minor);
+
+	new_fops = fops_get(minor->dev->driver->fops);
+	if (!new_fops) {
+		err = -ENODEV;
+		goto out;
+	}
+
+	replace_fops(filp, new_fops);
+	if (filp->f_op->open)
+		err = filp->f_op->open(inode, filp);
+	else
+		err = 0;
+
+out:
+	accel_minor_release(minor);
+
+	return err;
 }

 static const struct file_operations accel_stub_fops = {
@@ -56,12 +293,15 @@ void accel_core_exit(void)
 	unregister_chrdev(ACCEL_MAJOR, "accel");
 	debugfs_remove(accel_debugfs_root);
 	accel_sysfs_destroy();
+	idr_destroy(&accel_minors_idr);
 }

 int __init accel_core_init(void)
 {
 	int ret;

+	idr_init(&accel_minors_idr);
+
 	ret = accel_sysfs_init();
 	if (ret < 0) {
 		DRM_ERROR("Cannot create ACCEL class: %d\n", ret);
diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index a8b4d918e9a3..64b4a3a87fbb 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -326,7 +326,7 @@ static int drm_cpu_valid(void)
  * Creates and initializes a drm_file structure for the file private data in \p
  * filp and add it into the double linked list in \p dev.
  */
-static int drm_open_helper(struct file *filp, struct drm_minor *minor)
+int drm_open_helper(struct file *filp, struct drm_minor *minor)
 {
 	struct drm_device *dev = minor->dev;
 	struct drm_file *priv;
diff --git a/include/drm/drm_accel.h b/include/drm/drm_accel.h
index 31b42d3d6a15..b0c20367faad 100644
--- a/include/drm/drm_accel.h
+++ b/include/drm/drm_accel.h
@@ -8,12 +8,56 @@
 #ifndef DRM_ACCEL_H_
 #define DRM_ACCEL_H_

-#define ACCEL_MAJOR     261
+#include <drm/drm_file.h>
+
+#define ACCEL_MAJOR		261
+#define ACCEL_MAX_MINORS	256
+
+/**
+ * DRM_ACCEL_FOPS - Default drm accelerators file operations
+ *
+ * This macro provides a shorthand for setting the accelerator file ops in the
+ * &file_operations structure.  If all you need are the default ops, use
+ * DEFINE_DRM_ACCEL_FOPS instead.
+ */
+#define DRM_ACCEL_FOPS \
+	.open		= accel_open,\
+	.release	= drm_release,\
+	.unlocked_ioctl	= drm_ioctl,\
+	.compat_ioctl	= drm_compat_ioctl,\
+	.poll		= drm_poll,\
+	.read		= drm_read,\
+	.llseek		= noop_llseek
+
+/**
+ * DEFINE_DRM_ACCEL_FOPS() - macro to generate file operations for accelerators drivers
+ * @name: name for the generated structure
+ *
+ * This macro autogenerates a suitable &struct file_operations for accelerators based
+ * drivers, which can be assigned to &drm_driver.fops. Note that this structure
+ * cannot be shared between drivers, because it contains a reference to the
+ * current module using THIS_MODULE.
+ *
+ * Note that the declaration is already marked as static - if you need a
+ * non-static version of this you're probably doing it wrong and will break the
+ * THIS_MODULE reference by accident.
+ */
+#define DEFINE_DRM_ACCEL_FOPS(name) \
+	static const struct file_operations name = {\
+		.owner		= THIS_MODULE,\
+		DRM_ACCEL_FOPS,\
+	}

 #if IS_ENABLED(CONFIG_DRM_ACCEL)

 void accel_core_exit(void);
 int accel_core_init(void);
+void accel_minor_remove(int index);
+int accel_minor_alloc(void);
+void accel_minor_replace(struct drm_minor *minor, int index);
+void accel_set_device_instance_params(struct device *kdev, int index);
+int accel_open(struct inode *inode, struct file *filp);
+void accel_debugfs_init(struct drm_minor *minor, int minor_id);

 #else

@@ -23,9 +67,31 @@ static inline void accel_core_exit(void)

 static inline int __init accel_core_init(void)
 {
+	/* Return 0 to allow drm_core_init to complete successfully */
 	return 0;
 }

+static inline void accel_minor_remove(int index)
+{
+}
+
+static inline int accel_minor_alloc(void)
+{
+	return -EOPNOTSUPP;
+}
+
+static inline void accel_minor_replace(struct drm_minor *minor, int index)
+{
+}
+
+static inline void accel_set_device_instance_params(struct device *kdev, int index)
+{
+}
+
+static inline void accel_debugfs_init(struct drm_minor *minor, int minor_id)
+{
+
+
 #endif /* IS_ENABLED(CONFIG_DRM_ACCEL) */

 #endif /* DRM_ACCEL_H_ */
diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
index 9923c7a6885e..933ce2048e20 100644
--- a/include/drm/drm_device.h
+++ b/include/drm/drm_device.h
@@ -93,6 +93,9 @@ struct drm_device {
 	/** @render: Render node */
 	struct drm_minor *render;

+	/** @accel: Compute Acceleration node */
+	struct drm_minor *accel;
+
 	/**
 	 * @registered:
 	 *
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index f6159acb8856..706e68ca5116 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -94,6 +94,14 @@ enum drm_driver_feature {
 	 * synchronization of command submission.
 	 */
 	DRIVER_SYNCOBJ_TIMELINE         = BIT(6),
+	/**
+	 * @DRIVER_COMPUTE_ACCEL:
+	 *
+	 * Driver supports compute acceleration devices. This flag is mutually exclusive with
+	 * @DRIVER_RENDER and @DRIVER_MODESET. Devices that support both graphics and compute
+	 * acceleration should be handled by two drivers that are connected using auxiliry bus.
+	 */
+	DRIVER_COMPUTE_ACCEL            = BIT(7),

 	/* IMPORTANT: Below are all the legacy flags, add new ones above. */

diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
index d780fd151789..0d1f853092ab 100644
--- a/include/drm/drm_file.h
+++ b/include/drm/drm_file.h
@@ -51,11 +51,15 @@ struct file;

 /* Note that the order of this enum is ABI (it determines
  * /dev/dri/renderD* numbers).
+ *
+ * Setting DRM_MINOR_ACCEL to 32 gives enough space for more drm minors to
+ * be implemented before we hit any future
  */
 enum drm_minor_type {
 	DRM_MINOR_PRIMARY,
 	DRM_MINOR_CONTROL,
 	DRM_MINOR_RENDER,
+	DRM_MINOR_ACCEL = 32,
 };

 /**
@@ -70,7 +74,7 @@ enum drm_minor_type {
 struct drm_minor {
 	/* private: */
 	int index;			/* Minor device number */
-	int type;                       /* Control or render */
+	int type;                       /* Control or render or accel */
 	struct device *kdev;		/* Linux device */
 	struct drm_device *dev;

@@ -397,7 +401,22 @@ static inline bool drm_is_render_client(const struct drm_file *file_priv)
 	return file_priv->minor->type == DRM_MINOR_RENDER;
 }

+/**
+ * drm_is_accel_client - is this an open file of the compute acceleration node
+ * @file_priv: DRM file
+ *
+ * Returns true if this is an open file of the compute acceleration node, i.e.
+ * &drm_file.minor of @file_priv is a accel minor.
+ *
+ * See also the :ref:`section on accel nodes <drm_accel_node>`.
+ */
+static inline bool drm_is_accel_client(const struct drm_file *file_priv)
+{
+	return file_priv->minor->type == DRM_MINOR_ACCEL;
+}
+
 int drm_open(struct inode *inode, struct file *filp);
+int drm_open_helper(struct file *filp, struct drm_minor *minor);
 ssize_t drm_read(struct file *filp, char __user *buffer,
 		 size_t count, loff_t *offset);
 int drm_release(struct inode *inode, struct file *filp);
--
2.25.1

Re: [RFC PATCH v3 2/3] accel: add dedicated minor for accelerator devices

[Jeffrey Hugo]

On 11/6/2022 2:02 PM, Oded Gabbay wrote:
> --- a/drivers/accel/drm_accel.c
> +++ b/drivers/accel/drm_accel.c
> @@ -8,14 +8,25 @@
> 
>   #include <linux/debugfs.h>
>   #include <linux/device.h>
> +#include <linux/xarray.h>

If we are not using xarray at this time, do we still need this include?

> 
>   #include <drm/drm_accel.h>
> +#include <drm/drm_debugfs.h>
> +#include <drm/drm_drv.h>
> +#include <drm/drm_file.h>
>   #include <drm/drm_ioctl.h>
>   #include <drm/drm_print.h>
> 
> +static DEFINE_SPINLOCK(accel_minor_lock);
> +static struct idr accel_minors_idr;

I beleive we should have an explicit include for the IDR header.

> --- a/include/drm/drm_accel.h
> +++ b/include/drm/drm_accel.h
> @@ -8,12 +8,56 @@
>   #ifndef DRM_ACCEL_H_
>   #define DRM_ACCEL_H_
> 
> -#define ACCEL_MAJOR     261
> +#include <drm/drm_file.h>
> +
> +#define ACCEL_MAJOR		261
> +#define ACCEL_MAX_MINORS	256

This diff seems really weird.  The changes to the ACCEL_MAJOR define 
could get pushed to the previous patch, no?

> @@ -23,9 +67,31 @@ static inline void accel_core_exit(void)
> 
>   static inline int __init accel_core_init(void)
>   {
> +	/* Return 0 to allow drm_core_init to complete successfully */

Move to previous patch?

> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -94,6 +94,14 @@ enum drm_driver_feature {
>   	 * synchronization of command submission.
>   	 */
>   	DRIVER_SYNCOBJ_TIMELINE         = BIT(6),
> +	/**
> +	 * @DRIVER_COMPUTE_ACCEL:
> +	 *
> +	 * Driver supports compute acceleration devices. This flag is mutually exclusive with
> +	 * @DRIVER_RENDER and @DRIVER_MODESET. Devices that support both graphics and compute
> +	 * acceleration should be handled by two drivers that are connected using auxiliry bus.

auxiliry -> auxiliary

Re: [RFC PATCH v3 2/3] accel: add dedicated minor for accelerator devices

[Oded Gabbay]

On Mon, Nov 7, 2022 at 6:20 PM Jeffrey Hugo <quic_jhugo@quicinc.com> wrote:
>
> On 11/6/2022 2:02 PM, Oded Gabbay wrote:
> > --- a/drivers/accel/drm_accel.c
> > +++ b/drivers/accel/drm_accel.c
> > @@ -8,14 +8,25 @@
> >
> >   #include <linux/debugfs.h>
> >   #include <linux/device.h>
> > +#include <linux/xarray.h>
>
> If we are not using xarray at this time, do we still need this include?
>
> >
> >   #include <drm/drm_accel.h>
> > +#include <drm/drm_debugfs.h>
> > +#include <drm/drm_drv.h>
> > +#include <drm/drm_file.h>
> >   #include <drm/drm_ioctl.h>
> >   #include <drm/drm_print.h>
> >
> > +static DEFINE_SPINLOCK(accel_minor_lock);
> > +static struct idr accel_minors_idr;
>
> I beleive we should have an explicit include for the IDR header.
>
> > --- a/include/drm/drm_accel.h
> > +++ b/include/drm/drm_accel.h
> > @@ -8,12 +8,56 @@
> >   #ifndef DRM_ACCEL_H_
> >   #define DRM_ACCEL_H_
> >
> > -#define ACCEL_MAJOR     261
> > +#include <drm/drm_file.h>
> > +
> > +#define ACCEL_MAJOR          261
> > +#define ACCEL_MAX_MINORS     256
>
> This diff seems really weird.  The changes to the ACCEL_MAJOR define
> could get pushed to the previous patch, no?
>
> > @@ -23,9 +67,31 @@ static inline void accel_core_exit(void)
> >
> >   static inline int __init accel_core_init(void)
> >   {
> > +     /* Return 0 to allow drm_core_init to complete successfully */
>
> Move to previous patch?
>
> > --- a/include/drm/drm_drv.h
> > +++ b/include/drm/drm_drv.h
> > @@ -94,6 +94,14 @@ enum drm_driver_feature {
> >        * synchronization of command submission.
> >        */
> >       DRIVER_SYNCOBJ_TIMELINE         = BIT(6),
> > +     /**
> > +      * @DRIVER_COMPUTE_ACCEL:
> > +      *
> > +      * Driver supports compute acceleration devices. This flag is mutually exclusive with
> > +      * @DRIVER_RENDER and @DRIVER_MODESET. Devices that support both graphics and compute
> > +      * acceleration should be handled by two drivers that are connected using auxiliry bus.
>
> auxiliry -> auxiliary
>
All comments will be fixed.
Thanks,
Oded

Re: [RFC PATCH v3 2/3] accel: add dedicated minor for accelerator devices

[Tvrtko Ursulin]

On 06/11/2022 21:02, Oded Gabbay wrote:
> The accelerator devices are exposed to user-space using a dedicated
> major. In addition, they are represented in /dev with new, dedicated
> device char names: /dev/accel/accel*. This is done to make sure any
> user-space software that tries to open a graphic card won't open
> the accelerator device by mistake.
> 
> The above implies that the minor numbering should be separated from
> the rest of the DRM devices. However, to avoid code duplication, we
> want the drm_minor structure to be able to represent the accelerator
> device.
> 
> To achieve this, we add a new drm_minor* to drm_device that represents
> the accelerator device. This pointer is initialized 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.
> 
> In addition, we define a different IDR to handle the accelerators
> minors. This is done to make the minor's index be identical to the
> device index in /dev/. Any access to the IDR is done solely
> by functions in accel_drv.c, as the IDR is define as static. The
> DRM core functions call those functions in case they detect the minor's
> type is DRM_MINOR_ACCEL.
> 
> We define a separate accel_open function (from drm_open) that the
> accel drivers should set as their open callback function. Both these
> functions eventually call the same drm_open_helper(), which had to be
> changed to be non-static so it can be called from accel_drv.c.
> accel_open() only partially duplicates drm_open as I removed some code
> from it that handles legacy devices.
> 
> To help new drivers, I defined DEFINE_DRM_ACCEL_FOPS macro to easily
> set the required function operations pointers structure.
> 
> Signed-off-by: Oded Gabbay <ogabbay@kernel.org>
> ---
> Changes in v3:
>   - Remove useless DRM_DEBUG("\n") at accel_stub_open()
>   - Add function of accel_debugfs_init() as accel_debugfs_root is static
>     member in drm_accel.c
>   - Add DRM_ACCEL_FOPS and DEFINE_DRM_ACCEL_FOPS macros
>   - Replace minor handling from xarray back to idr, as xarray doesn't handle
>     well exchanging content of a NULL entry to non-NULL. This should be handled
>     in a different patch that will either fix xarray code or change DRM minor
>     init flow.
>   - Make accel_minor_replace() to return void.
> 
>   drivers/accel/drm_accel.c  | 242 ++++++++++++++++++++++++++++++++++++-
>   drivers/gpu/drm/drm_file.c |   2 +-
>   include/drm/drm_accel.h    |  68 ++++++++++-
>   include/drm/drm_device.h   |   3 +
>   include/drm/drm_drv.h      |   8 ++
>   include/drm/drm_file.h     |  21 +++-
>   6 files changed, 340 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/accel/drm_accel.c b/drivers/accel/drm_accel.c
> index 943d960ddefc..05167c929866 100644
> --- a/drivers/accel/drm_accel.c
> +++ b/drivers/accel/drm_accel.c
> @@ -8,14 +8,25 @@
> 
>   #include <linux/debugfs.h>
>   #include <linux/device.h>
> +#include <linux/xarray.h>
> 
>   #include <drm/drm_accel.h>
> +#include <drm/drm_debugfs.h>
> +#include <drm/drm_drv.h>
> +#include <drm/drm_file.h>
>   #include <drm/drm_ioctl.h>
>   #include <drm/drm_print.h>
> 
> +static DEFINE_SPINLOCK(accel_minor_lock);
> +static struct idr accel_minors_idr;
> +
>   static struct dentry *accel_debugfs_root;
>   static struct class *accel_class;
> 
> +static struct device_type accel_sysfs_device_minor = {
> +	.name = "accel_minor"
> +};
> +
>   static char *accel_devnode(struct device *dev, umode_t *mode)
>   {
>   	return kasprintf(GFP_KERNEL, "accel/%s", dev_name(dev));
> @@ -40,9 +51,235 @@ static void accel_sysfs_destroy(void)
>   	accel_class = NULL;
>   }
> 
> +static int accel_name_info(struct seq_file *m, void *data)
> +{
> +	struct drm_info_node *node = (struct drm_info_node *) m->private;
> +	struct drm_minor *minor = node->minor;
> +	struct drm_device *dev = minor->dev;
> +	struct drm_master *master;
> +
> +	mutex_lock(&dev->master_mutex);
> +	master = dev->master;
> +	seq_printf(m, "%s", dev->driver->name);
> +	if (dev->dev)
> +		seq_printf(m, " dev=%s", dev_name(dev->dev));
> +	if (master && master->unique)
> +		seq_printf(m, " master=%s", master->unique);

Does the all drm_master business apply with accel?

> +	if (dev->unique)
> +		seq_printf(m, " unique=%s", dev->unique);
> +	seq_puts(m, "\n");
> +	mutex_unlock(&dev->master_mutex);
> +
> +	return 0;
> +}
> +
> +static const struct drm_info_list accel_debugfs_list[] = {
> +	{"name", accel_name_info, 0}
> +};
> +#define ACCEL_DEBUGFS_ENTRIES ARRAY_SIZE(accel_debugfs_list)
> +
> +/**
> + * accel_debugfs_init() - Initialize debugfs for accel minor
> + * @minor: Pointer to the drm_minor instance.
> + * @minor_id: The minor's id
> + *
> + * This function initializes the drm minor's debugfs members and creates
> + * a root directory for the minor in debugfs. It also creates common files
> + * for accelerators and calls the driver's debugfs init callback.
> + */
> +void accel_debugfs_init(struct drm_minor *minor, int minor_id)
> +{
> +	struct drm_device *dev = minor->dev;
> +	char name[64];
> +
> +	INIT_LIST_HEAD(&minor->debugfs_list);
> +	mutex_init(&minor->debugfs_lock);
> +	sprintf(name, "%d", minor_id);
> +	minor->debugfs_root = debugfs_create_dir(name, accel_debugfs_root);
> +
> +	drm_debugfs_create_files(accel_debugfs_list, ACCEL_DEBUGFS_ENTRIES,
> +				 minor->debugfs_root, minor);
> +
> +	if (dev->driver->debugfs_init)
> +		dev->driver->debugfs_init(minor);
> +}
> +
> +/**
> + * accel_set_device_instance_params() - Set some device parameters for accel device
> + * @kdev: Pointer to the device instance.
> + * @index: The minor's index
> + *
> + * This function creates the dev_t of the device using the accel major and
> + * the device's minor number. In addition, it sets the class and type of the
> + * device instance to the accel sysfs class and device type, respectively.
> + */
> +void accel_set_device_instance_params(struct device *kdev, int index)
> +{
> +	kdev->devt = MKDEV(ACCEL_MAJOR, index);
> +	kdev->class = accel_class;
> +	kdev->type = &accel_sysfs_device_minor;
> +}
> +
> +/**
> + * accel_minor_alloc() - Allocates a new accel minor
> + *
> + * This function access the accel minors idr and allocates from it
> + * a new id to represent a new accel minor
> + *
> + * Return: A new id on success or error code in case idr_alloc failed
> + */
> +int accel_minor_alloc(void)
> +{
> +	unsigned long flags;
> +	int r;
> +
> +	spin_lock_irqsave(&accel_minor_lock, flags);
> +	r = idr_alloc(&accel_minors_idr, NULL, 0, ACCEL_MAX_MINORS, GFP_NOWAIT);
> +	spin_unlock_irqrestore(&accel_minor_lock, flags);
> +
> +	return r;
> +}
> +
> +/**
> + * accel_minor_remove() - Remove an accel minor
> + * @index: The minor id to remove.
> + *
> + * This function access the accel minors idr and removes from
> + * it the member with the id that is passed to this function.
> + */
> +void accel_minor_remove(int index)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&accel_minor_lock, flags);
> +	idr_remove(&accel_minors_idr, index);
> +	spin_unlock_irqrestore(&accel_minor_lock, flags);
> +}
> +
> +/**
> + * accel_minor_replace() - Replace minor pointer in accel minors idr.
> + * @minor: Pointer to the new minor.
> + * @index: The minor id to replace.
> + *
> + * This function access the accel minors idr structure and replaces the pointer
> + * that is associated with an existing id. Because the minor pointer can be
> + * NULL, we need to explicitly pass the index.
> + *
> + * Return: 0 for success, negative value for error
> + */
> +void accel_minor_replace(struct drm_minor *minor, int index)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&accel_minor_lock, flags);
> +	idr_replace(&accel_minors_idr, minor, index);
> +	spin_unlock_irqrestore(&accel_minor_lock, flags);
> +}
> +
> +/*
> + * Looks up the given minor-ID and returns the respective DRM-minor object. The
> + * refence-count of the underlying device is increased so you must release this
> + * object with accel_minor_release().
> + *
> + * The object can be only a drm_minor that represents an accel device.
> + *
> + * As long as you hold this minor, it is guaranteed that the object and the
> + * minor->dev pointer will stay valid! However, the device may get unplugged and
> + * unregistered while you hold the minor.
> + */
> +static struct drm_minor *accel_minor_acquire(unsigned int minor_id)
> +{
> +	struct drm_minor *minor;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&accel_minor_lock, flags);
> +	minor = idr_find(&accel_minors_idr, minor_id);
> +	if (minor)
> +		drm_dev_get(minor->dev);
> +	spin_unlock_irqrestore(&accel_minor_lock, flags);
> +
> +	if (!minor) {
> +		return ERR_PTR(-ENODEV);
> +	} else if (drm_dev_is_unplugged(minor->dev)) {
> +		drm_dev_put(minor->dev);
> +		return ERR_PTR(-ENODEV);
> +	}
> +
> +	return minor;
> +}
> +
> +static void accel_minor_release(struct drm_minor *minor)
> +{
> +	drm_dev_put(minor->dev);
> +}
> +
> +/**
> + * accel_open - open method for ACCEL file
> + * @inode: device inode
> + * @filp: file pointer.
> + *
> + * This function must be used by drivers as their &file_operations.open method.
> + * It looks up the correct ACCEL device and instantiates all the per-file
> + * resources for it. It also calls the &drm_driver.open driver callback.
> + *
> + * Return: 0 on success or negative errno value on failure.
> + */
> +int accel_open(struct inode *inode, struct file *filp)
> +{
> +	struct drm_device *dev;
> +	struct drm_minor *minor;
> +	int retcode;
> +
> +	minor = accel_minor_acquire(iminor(inode));
> +	if (IS_ERR(minor))
> +		return PTR_ERR(minor);
> +
> +	dev = minor->dev;
> +
> +	atomic_fetch_inc(&dev->open_count);

Why fetch and could you even only increment once everything worked (to 
avoid having to decrement on the error unwind)?

> +
> +	/* share address_space across all char-devs of a single device */
> +	filp->f_mapping = dev->anon_inode->i_mapping;
> +
> +	retcode = drm_open_helper(filp, minor);
> +	if (retcode)
> +		goto err_undo;
> +
> +	return 0;
> +
> +err_undo:
> +	atomic_dec(&dev->open_count);
> +	accel_minor_release(minor);
> +	return retcode;
> +}
> +EXPORT_SYMBOL_GPL(accel_open);
> +
>   static int accel_stub_open(struct inode *inode, struct file *filp)
>   {
> -	return -EOPNOTSUPP;
> +	const struct file_operations *new_fops;
> +	struct drm_minor *minor;
> +	int err;
> +
> +	minor = accel_minor_acquire(iminor(inode));
> +	if (IS_ERR(minor))
> +		return PTR_ERR(minor);
> +
> +	new_fops = fops_get(minor->dev->driver->fops);
> +	if (!new_fops) {
> +		err = -ENODEV;
> +		goto out;
> +	}
> +
> +	replace_fops(filp, new_fops);
> +	if (filp->f_op->open)
> +		err = filp->f_op->open(inode, filp);
> +	else
> +		err = 0;
> +
> +out:
> +	accel_minor_release(minor);
> +
> +	return err;
>   }
> 
>   static const struct file_operations accel_stub_fops = {
> @@ -56,12 +293,15 @@ void accel_core_exit(void)
>   	unregister_chrdev(ACCEL_MAJOR, "accel");
>   	debugfs_remove(accel_debugfs_root);
>   	accel_sysfs_destroy();
> +	idr_destroy(&accel_minors_idr);
>   }
> 
>   int __init accel_core_init(void)
>   {
>   	int ret;
> 
> +	idr_init(&accel_minors_idr);
> +
>   	ret = accel_sysfs_init();
>   	if (ret < 0) {
>   		DRM_ERROR("Cannot create ACCEL class: %d\n", ret);
> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> index a8b4d918e9a3..64b4a3a87fbb 100644
> --- a/drivers/gpu/drm/drm_file.c
> +++ b/drivers/gpu/drm/drm_file.c
> @@ -326,7 +326,7 @@ static int drm_cpu_valid(void)
>    * Creates and initializes a drm_file structure for the file private data in \p
>    * filp and add it into the double linked list in \p dev.
>    */
> -static int drm_open_helper(struct file *filp, struct drm_minor *minor)
> +int drm_open_helper(struct file *filp, struct drm_minor *minor)
>   {
>   	struct drm_device *dev = minor->dev;
>   	struct drm_file *priv;
> diff --git a/include/drm/drm_accel.h b/include/drm/drm_accel.h
> index 31b42d3d6a15..b0c20367faad 100644
> --- a/include/drm/drm_accel.h
> +++ b/include/drm/drm_accel.h
> @@ -8,12 +8,56 @@
>   #ifndef DRM_ACCEL_H_
>   #define DRM_ACCEL_H_
> 
> -#define ACCEL_MAJOR     261
> +#include <drm/drm_file.h>
> +
> +#define ACCEL_MAJOR		261
> +#define ACCEL_MAX_MINORS	256
> +
> +/**
> + * DRM_ACCEL_FOPS - Default drm accelerators file operations
> + *
> + * This macro provides a shorthand for setting the accelerator file ops in the
> + * &file_operations structure.  If all you need are the default ops, use
> + * DEFINE_DRM_ACCEL_FOPS instead.
> + */
> +#define DRM_ACCEL_FOPS \
> +	.open		= accel_open,\
> +	.release	= drm_release,\
> +	.unlocked_ioctl	= drm_ioctl,\
> +	.compat_ioctl	= drm_compat_ioctl,\
> +	.poll		= drm_poll,\
> +	.read		= drm_read,\
> +	.llseek		= noop_llseek
> +
> +/**
> + * DEFINE_DRM_ACCEL_FOPS() - macro to generate file operations for accelerators drivers
> + * @name: name for the generated structure
> + *
> + * This macro autogenerates a suitable &struct file_operations for accelerators based
> + * drivers, which can be assigned to &drm_driver.fops. Note that this structure
> + * cannot be shared between drivers, because it contains a reference to the
> + * current module using THIS_MODULE.
> + *
> + * Note that the declaration is already marked as static - if you need a
> + * non-static version of this you're probably doing it wrong and will break the
> + * THIS_MODULE reference by accident.
> + */
> +#define DEFINE_DRM_ACCEL_FOPS(name) \
> +	static const struct file_operations name = {\
> +		.owner		= THIS_MODULE,\
> +		DRM_ACCEL_FOPS,\
> +	}
> 
>   #if IS_ENABLED(CONFIG_DRM_ACCEL)
> 
>   void accel_core_exit(void);
>   int accel_core_init(void);
> +void accel_minor_remove(int index);
> +int accel_minor_alloc(void);
> +void accel_minor_replace(struct drm_minor *minor, int index);
> +void accel_set_device_instance_params(struct device *kdev, int index);
> +int accel_open(struct inode *inode, struct file *filp);
> +void accel_debugfs_init(struct drm_minor *minor, int minor_id);
> 
>   #else
> 
> @@ -23,9 +67,31 @@ static inline void accel_core_exit(void)
> 
>   static inline int __init accel_core_init(void)
>   {
> +	/* Return 0 to allow drm_core_init to complete successfully */
>   	return 0;
>   }
> 
> +static inline void accel_minor_remove(int index)
> +{
> +}
> +
> +static inline int accel_minor_alloc(void)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> +static inline void accel_minor_replace(struct drm_minor *minor, int index)
> +{
> +}
> +
> +static inline void accel_set_device_instance_params(struct device *kdev, int index)
> +{
> +}
> +
> +static inline void accel_debugfs_init(struct drm_minor *minor, int minor_id)
> +{
> +
> +
>   #endif /* IS_ENABLED(CONFIG_DRM_ACCEL) */
> 
>   #endif /* DRM_ACCEL_H_ */
> diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
> index 9923c7a6885e..933ce2048e20 100644
> --- a/include/drm/drm_device.h
> +++ b/include/drm/drm_device.h
> @@ -93,6 +93,9 @@ struct drm_device {
>   	/** @render: Render node */
>   	struct drm_minor *render;
> 
> +	/** @accel: Compute Acceleration node */
> +	struct drm_minor *accel;
> +
>   	/**
>   	 * @registered:
>   	 *
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index f6159acb8856..706e68ca5116 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -94,6 +94,14 @@ enum drm_driver_feature {
>   	 * synchronization of command submission.
>   	 */
>   	DRIVER_SYNCOBJ_TIMELINE         = BIT(6),
> +	/**
> +	 * @DRIVER_COMPUTE_ACCEL:
> +	 *
> +	 * Driver supports compute acceleration devices. This flag is mutually exclusive with
> +	 * @DRIVER_RENDER and @DRIVER_MODESET. Devices that support both graphics and compute
> +	 * acceleration should be handled by two drivers that are connected using auxiliry bus.
> +	 */
> +	DRIVER_COMPUTE_ACCEL            = BIT(7),
> 
>   	/* IMPORTANT: Below are all the legacy flags, add new ones above. */
> 
> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> index d780fd151789..0d1f853092ab 100644
> --- a/include/drm/drm_file.h
> +++ b/include/drm/drm_file.h
> @@ -51,11 +51,15 @@ struct file;
> 
>   /* Note that the order of this enum is ABI (it determines
>    * /dev/dri/renderD* numbers).
> + *
> + * Setting DRM_MINOR_ACCEL to 32 gives enough space for more drm minors to
> + * be implemented before we hit any future
>    */
>   enum drm_minor_type {
>   	DRM_MINOR_PRIMARY,
>   	DRM_MINOR_CONTROL,
>   	DRM_MINOR_RENDER,
> +	DRM_MINOR_ACCEL = 32,

Didn't patch 1/3 say it's a standalone character device major? Are there 
two ways to open the same thing?

>   };
> 
>   /**
> @@ -70,7 +74,7 @@ enum drm_minor_type {
>   struct drm_minor {
>   	/* private: */
>   	int index;			/* Minor device number */
> -	int type;                       /* Control or render */
> +	int type;                       /* Control or render or accel */

Could this be self documenting if type was enum drm_minor_type?

>   	struct device *kdev;		/* Linux device */
>   	struct drm_device *dev;
> 
> @@ -397,7 +401,22 @@ static inline bool drm_is_render_client(const struct drm_file *file_priv)
>   	return file_priv->minor->type == DRM_MINOR_RENDER;
>   }
> 
> +/**
> + * drm_is_accel_client - is this an open file of the compute acceleration node
> + * @file_priv: DRM file
> + *
> + * Returns true if this is an open file of the compute acceleration node, i.e.
> + * &drm_file.minor of @file_priv is a accel minor.
> + *
> + * See also the :ref:`section on accel nodes <drm_accel_node>`.
> + */
> +static inline bool drm_is_accel_client(const struct drm_file *file_priv)
> +{
> +	return file_priv->minor->type == DRM_MINOR_ACCEL;
> +}
> +
>   int drm_open(struct inode *inode, struct file *filp);
> +int drm_open_helper(struct file *filp, struct drm_minor *minor);
>   ssize_t drm_read(struct file *filp, char __user *buffer,
>   		 size_t count, loff_t *offset);
>   int drm_release(struct inode *inode, struct file *filp);
> --
> 2.25.1
> 

Regards,

Tvrtko

Re: [RFC PATCH v3 2/3] accel: add dedicated minor for accelerator devices

[Oded Gabbay]

On Tue, Nov 8, 2022 at 3:13 PM Tvrtko Ursulin
<tvrtko.ursulin@linux.intel.com> wrote:
>
>
> On 06/11/2022 21:02, Oded Gabbay wrote:
> > The accelerator devices are exposed to user-space using a dedicated
> > major. In addition, they are represented in /dev with new, dedicated
> > device char names: /dev/accel/accel*. This is done to make sure any
> > user-space software that tries to open a graphic card won't open
> > the accelerator device by mistake.
> >
> > The above implies that the minor numbering should be separated from
> > the rest of the DRM devices. However, to avoid code duplication, we
> > want the drm_minor structure to be able to represent the accelerator
> > device.
> >
> > To achieve this, we add a new drm_minor* to drm_device that represents
> > the accelerator device. This pointer is initialized 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.
> >
> > In addition, we define a different IDR to handle the accelerators
> > minors. This is done to make the minor's index be identical to the
> > device index in /dev/. Any access to the IDR is done solely
> > by functions in accel_drv.c, as the IDR is define as static. The
> > DRM core functions call those functions in case they detect the minor's
> > type is DRM_MINOR_ACCEL.
> >
> > We define a separate accel_open function (from drm_open) that the
> > accel drivers should set as their open callback function. Both these
> > functions eventually call the same drm_open_helper(), which had to be
> > changed to be non-static so it can be called from accel_drv.c.
> > accel_open() only partially duplicates drm_open as I removed some code
> > from it that handles legacy devices.
> >
> > To help new drivers, I defined DEFINE_DRM_ACCEL_FOPS macro to easily
> > set the required function operations pointers structure.
> >
> > Signed-off-by: Oded Gabbay <ogabbay@kernel.org>
> > ---
> > Changes in v3:
> >   - Remove useless DRM_DEBUG("\n") at accel_stub_open()
> >   - Add function of accel_debugfs_init() as accel_debugfs_root is static
> >     member in drm_accel.c
> >   - Add DRM_ACCEL_FOPS and DEFINE_DRM_ACCEL_FOPS macros
> >   - Replace minor handling from xarray back to idr, as xarray doesn't handle
> >     well exchanging content of a NULL entry to non-NULL. This should be handled
> >     in a different patch that will either fix xarray code or change DRM minor
> >     init flow.
> >   - Make accel_minor_replace() to return void.
> >
> >   drivers/accel/drm_accel.c  | 242 ++++++++++++++++++++++++++++++++++++-
> >   drivers/gpu/drm/drm_file.c |   2 +-
> >   include/drm/drm_accel.h    |  68 ++++++++++-
> >   include/drm/drm_device.h   |   3 +
> >   include/drm/drm_drv.h      |   8 ++
> >   include/drm/drm_file.h     |  21 +++-
> >   6 files changed, 340 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/accel/drm_accel.c b/drivers/accel/drm_accel.c
> > index 943d960ddefc..05167c929866 100644
> > --- a/drivers/accel/drm_accel.c
> > +++ b/drivers/accel/drm_accel.c
> > @@ -8,14 +8,25 @@
> >
> >   #include <linux/debugfs.h>
> >   #include <linux/device.h>
> > +#include <linux/xarray.h>
> >
> >   #include <drm/drm_accel.h>
> > +#include <drm/drm_debugfs.h>
> > +#include <drm/drm_drv.h>
> > +#include <drm/drm_file.h>
> >   #include <drm/drm_ioctl.h>
> >   #include <drm/drm_print.h>
> >
> > +static DEFINE_SPINLOCK(accel_minor_lock);
> > +static struct idr accel_minors_idr;
> > +
> >   static struct dentry *accel_debugfs_root;
> >   static struct class *accel_class;
> >
> > +static struct device_type accel_sysfs_device_minor = {
> > +     .name = "accel_minor"
> > +};
> > +
> >   static char *accel_devnode(struct device *dev, umode_t *mode)
> >   {
> >       return kasprintf(GFP_KERNEL, "accel/%s", dev_name(dev));
> > @@ -40,9 +51,235 @@ static void accel_sysfs_destroy(void)
> >       accel_class = NULL;
> >   }
> >
> > +static int accel_name_info(struct seq_file *m, void *data)
> > +{
> > +     struct drm_info_node *node = (struct drm_info_node *) m->private;
> > +     struct drm_minor *minor = node->minor;
> > +     struct drm_device *dev = minor->dev;
> > +     struct drm_master *master;
> > +
> > +     mutex_lock(&dev->master_mutex);
> > +     master = dev->master;
> > +     seq_printf(m, "%s", dev->driver->name);
> > +     if (dev->dev)
> > +             seq_printf(m, " dev=%s", dev_name(dev->dev));
> > +     if (master && master->unique)
> > +             seq_printf(m, " master=%s", master->unique);
>
> Does the all drm_master business apply with accel?
No, it was left by mistake, I'll remove it.

>
> > +     if (dev->unique)
> > +             seq_printf(m, " unique=%s", dev->unique);
> > +     seq_puts(m, "\n");
> > +     mutex_unlock(&dev->master_mutex);
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct drm_info_list accel_debugfs_list[] = {
> > +     {"name", accel_name_info, 0}
> > +};
> > +#define ACCEL_DEBUGFS_ENTRIES ARRAY_SIZE(accel_debugfs_list)
> > +
> > +/**
> > + * accel_debugfs_init() - Initialize debugfs for accel minor
> > + * @minor: Pointer to the drm_minor instance.
> > + * @minor_id: The minor's id
> > + *
> > + * This function initializes the drm minor's debugfs members and creates
> > + * a root directory for the minor in debugfs. It also creates common files
> > + * for accelerators and calls the driver's debugfs init callback.
> > + */
> > +void accel_debugfs_init(struct drm_minor *minor, int minor_id)
> > +{
> > +     struct drm_device *dev = minor->dev;
> > +     char name[64];
> > +
> > +     INIT_LIST_HEAD(&minor->debugfs_list);
> > +     mutex_init(&minor->debugfs_lock);
> > +     sprintf(name, "%d", minor_id);
> > +     minor->debugfs_root = debugfs_create_dir(name, accel_debugfs_root);
> > +
> > +     drm_debugfs_create_files(accel_debugfs_list, ACCEL_DEBUGFS_ENTRIES,
> > +                              minor->debugfs_root, minor);
> > +
> > +     if (dev->driver->debugfs_init)
> > +             dev->driver->debugfs_init(minor);
> > +}
> > +
> > +/**
> > + * accel_set_device_instance_params() - Set some device parameters for accel device
> > + * @kdev: Pointer to the device instance.
> > + * @index: The minor's index
> > + *
> > + * This function creates the dev_t of the device using the accel major and
> > + * the device's minor number. In addition, it sets the class and type of the
> > + * device instance to the accel sysfs class and device type, respectively.
> > + */
> > +void accel_set_device_instance_params(struct device *kdev, int index)
> > +{
> > +     kdev->devt = MKDEV(ACCEL_MAJOR, index);
> > +     kdev->class = accel_class;
> > +     kdev->type = &accel_sysfs_device_minor;
> > +}
> > +
> > +/**
> > + * accel_minor_alloc() - Allocates a new accel minor
> > + *
> > + * This function access the accel minors idr and allocates from it
> > + * a new id to represent a new accel minor
> > + *
> > + * Return: A new id on success or error code in case idr_alloc failed
> > + */
> > +int accel_minor_alloc(void)
> > +{
> > +     unsigned long flags;
> > +     int r;
> > +
> > +     spin_lock_irqsave(&accel_minor_lock, flags);
> > +     r = idr_alloc(&accel_minors_idr, NULL, 0, ACCEL_MAX_MINORS, GFP_NOWAIT);
> > +     spin_unlock_irqrestore(&accel_minor_lock, flags);
> > +
> > +     return r;
> > +}
> > +
> > +/**
> > + * accel_minor_remove() - Remove an accel minor
> > + * @index: The minor id to remove.
> > + *
> > + * This function access the accel minors idr and removes from
> > + * it the member with the id that is passed to this function.
> > + */
> > +void accel_minor_remove(int index)
> > +{
> > +     unsigned long flags;
> > +
> > +     spin_lock_irqsave(&accel_minor_lock, flags);
> > +     idr_remove(&accel_minors_idr, index);
> > +     spin_unlock_irqrestore(&accel_minor_lock, flags);
> > +}
> > +
> > +/**
> > + * accel_minor_replace() - Replace minor pointer in accel minors idr.
> > + * @minor: Pointer to the new minor.
> > + * @index: The minor id to replace.
> > + *
> > + * This function access the accel minors idr structure and replaces the pointer
> > + * that is associated with an existing id. Because the minor pointer can be
> > + * NULL, we need to explicitly pass the index.
> > + *
> > + * Return: 0 for success, negative value for error
> > + */
> > +void accel_minor_replace(struct drm_minor *minor, int index)
> > +{
> > +     unsigned long flags;
> > +
> > +     spin_lock_irqsave(&accel_minor_lock, flags);
> > +     idr_replace(&accel_minors_idr, minor, index);
> > +     spin_unlock_irqrestore(&accel_minor_lock, flags);
> > +}
> > +
> > +/*
> > + * Looks up the given minor-ID and returns the respective DRM-minor object. The
> > + * refence-count of the underlying device is increased so you must release this
> > + * object with accel_minor_release().
> > + *
> > + * The object can be only a drm_minor that represents an accel device.
> > + *
> > + * As long as you hold this minor, it is guaranteed that the object and the
> > + * minor->dev pointer will stay valid! However, the device may get unplugged and
> > + * unregistered while you hold the minor.
> > + */
> > +static struct drm_minor *accel_minor_acquire(unsigned int minor_id)
> > +{
> > +     struct drm_minor *minor;
> > +     unsigned long flags;
> > +
> > +     spin_lock_irqsave(&accel_minor_lock, flags);
> > +     minor = idr_find(&accel_minors_idr, minor_id);
> > +     if (minor)
> > +             drm_dev_get(minor->dev);
> > +     spin_unlock_irqrestore(&accel_minor_lock, flags);
> > +
> > +     if (!minor) {
> > +             return ERR_PTR(-ENODEV);
> > +     } else if (drm_dev_is_unplugged(minor->dev)) {
> > +             drm_dev_put(minor->dev);
> > +             return ERR_PTR(-ENODEV);
> > +     }
> > +
> > +     return minor;
> > +}
> > +
> > +static void accel_minor_release(struct drm_minor *minor)
> > +{
> > +     drm_dev_put(minor->dev);
> > +}
> > +
> > +/**
> > + * accel_open - open method for ACCEL file
> > + * @inode: device inode
> > + * @filp: file pointer.
> > + *
> > + * This function must be used by drivers as their &file_operations.open method.
> > + * It looks up the correct ACCEL device and instantiates all the per-file
> > + * resources for it. It also calls the &drm_driver.open driver callback.
> > + *
> > + * Return: 0 on success or negative errno value on failure.
> > + */
> > +int accel_open(struct inode *inode, struct file *filp)
> > +{
> > +     struct drm_device *dev;
> > +     struct drm_minor *minor;
> > +     int retcode;
> > +
> > +     minor = accel_minor_acquire(iminor(inode));
> > +     if (IS_ERR(minor))
> > +             return PTR_ERR(minor);
> > +
> > +     dev = minor->dev;
> > +
> > +     atomic_fetch_inc(&dev->open_count);
>
> Why fetch and could you even only increment once everything worked (to
> avoid having to decrement on the error unwind)?
It was copied from drm_open. I tried to not do any changes that
weren't mandatory to make it work, to minimize the chance of me
breaking things.
I guess I can do what you suggested. From the drm code, doesn't seem
moving it will cause any harm.
>
> > +
> > +     /* share address_space across all char-devs of a single device */
> > +     filp->f_mapping = dev->anon_inode->i_mapping;
> > +
> > +     retcode = drm_open_helper(filp, minor);
> > +     if (retcode)
> > +             goto err_undo;
> > +
> > +     return 0;
> > +
> > +err_undo:
> > +     atomic_dec(&dev->open_count);
> > +     accel_minor_release(minor);
> > +     return retcode;
> > +}
> > +EXPORT_SYMBOL_GPL(accel_open);
> > +
> >   static int accel_stub_open(struct inode *inode, struct file *filp)
> >   {
> > -     return -EOPNOTSUPP;
> > +     const struct file_operations *new_fops;
> > +     struct drm_minor *minor;
> > +     int err;
> > +
> > +     minor = accel_minor_acquire(iminor(inode));
> > +     if (IS_ERR(minor))
> > +             return PTR_ERR(minor);
> > +
> > +     new_fops = fops_get(minor->dev->driver->fops);
> > +     if (!new_fops) {
> > +             err = -ENODEV;
> > +             goto out;
> > +     }
> > +
> > +     replace_fops(filp, new_fops);
> > +     if (filp->f_op->open)
> > +             err = filp->f_op->open(inode, filp);
> > +     else
> > +             err = 0;
> > +
> > +out:
> > +     accel_minor_release(minor);
> > +
> > +     return err;
> >   }
> >
> >   static const struct file_operations accel_stub_fops = {
> > @@ -56,12 +293,15 @@ void accel_core_exit(void)
> >       unregister_chrdev(ACCEL_MAJOR, "accel");
> >       debugfs_remove(accel_debugfs_root);
> >       accel_sysfs_destroy();
> > +     idr_destroy(&accel_minors_idr);
> >   }
> >
> >   int __init accel_core_init(void)
> >   {
> >       int ret;
> >
> > +     idr_init(&accel_minors_idr);
> > +
> >       ret = accel_sysfs_init();
> >       if (ret < 0) {
> >               DRM_ERROR("Cannot create ACCEL class: %d\n", ret);
> > diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> > index a8b4d918e9a3..64b4a3a87fbb 100644
> > --- a/drivers/gpu/drm/drm_file.c
> > +++ b/drivers/gpu/drm/drm_file.c
> > @@ -326,7 +326,7 @@ static int drm_cpu_valid(void)
> >    * Creates and initializes a drm_file structure for the file private data in \p
> >    * filp and add it into the double linked list in \p dev.
> >    */
> > -static int drm_open_helper(struct file *filp, struct drm_minor *minor)
> > +int drm_open_helper(struct file *filp, struct drm_minor *minor)
> >   {
> >       struct drm_device *dev = minor->dev;
> >       struct drm_file *priv;
> > diff --git a/include/drm/drm_accel.h b/include/drm/drm_accel.h
> > index 31b42d3d6a15..b0c20367faad 100644
> > --- a/include/drm/drm_accel.h
> > +++ b/include/drm/drm_accel.h
> > @@ -8,12 +8,56 @@
> >   #ifndef DRM_ACCEL_H_
> >   #define DRM_ACCEL_H_
> >
> > -#define ACCEL_MAJOR     261
> > +#include <drm/drm_file.h>
> > +
> > +#define ACCEL_MAJOR          261
> > +#define ACCEL_MAX_MINORS     256
> > +
> > +/**
> > + * DRM_ACCEL_FOPS - Default drm accelerators file operations
> > + *
> > + * This macro provides a shorthand for setting the accelerator file ops in the
> > + * &file_operations structure.  If all you need are the default ops, use
> > + * DEFINE_DRM_ACCEL_FOPS instead.
> > + */
> > +#define DRM_ACCEL_FOPS \
> > +     .open           = accel_open,\
> > +     .release        = drm_release,\
> > +     .unlocked_ioctl = drm_ioctl,\
> > +     .compat_ioctl   = drm_compat_ioctl,\
> > +     .poll           = drm_poll,\
> > +     .read           = drm_read,\
> > +     .llseek         = noop_llseek
> > +
> > +/**
> > + * DEFINE_DRM_ACCEL_FOPS() - macro to generate file operations for accelerators drivers
> > + * @name: name for the generated structure
> > + *
> > + * This macro autogenerates a suitable &struct file_operations for accelerators based
> > + * drivers, which can be assigned to &drm_driver.fops. Note that this structure
> > + * cannot be shared between drivers, because it contains a reference to the
> > + * current module using THIS_MODULE.
> > + *
> > + * Note that the declaration is already marked as static - if you need a
> > + * non-static version of this you're probably doing it wrong and will break the
> > + * THIS_MODULE reference by accident.
> > + */
> > +#define DEFINE_DRM_ACCEL_FOPS(name) \
> > +     static const struct file_operations name = {\
> > +             .owner          = THIS_MODULE,\
> > +             DRM_ACCEL_FOPS,\
> > +     }
> >
> >   #if IS_ENABLED(CONFIG_DRM_ACCEL)
> >
> >   void accel_core_exit(void);
> >   int accel_core_init(void);
> > +void accel_minor_remove(int index);
> > +int accel_minor_alloc(void);
> > +void accel_minor_replace(struct drm_minor *minor, int index);
> > +void accel_set_device_instance_params(struct device *kdev, int index);
> > +int accel_open(struct inode *inode, struct file *filp);
> > +void accel_debugfs_init(struct drm_minor *minor, int minor_id);
> >
> >   #else
> >
> > @@ -23,9 +67,31 @@ static inline void accel_core_exit(void)
> >
> >   static inline int __init accel_core_init(void)
> >   {
> > +     /* Return 0 to allow drm_core_init to complete successfully */
> >       return 0;
> >   }
> >
> > +static inline void accel_minor_remove(int index)
> > +{
> > +}
> > +
> > +static inline int accel_minor_alloc(void)
> > +{
> > +     return -EOPNOTSUPP;
> > +}
> > +
> > +static inline void accel_minor_replace(struct drm_minor *minor, int index)
> > +{
> > +}
> > +
> > +static inline void accel_set_device_instance_params(struct device *kdev, int index)
> > +{
> > +}
> > +
> > +static inline void accel_debugfs_init(struct drm_minor *minor, int minor_id)
> > +{
> > +
> > +
> >   #endif /* IS_ENABLED(CONFIG_DRM_ACCEL) */
> >
> >   #endif /* DRM_ACCEL_H_ */
> > diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h
> > index 9923c7a6885e..933ce2048e20 100644
> > --- a/include/drm/drm_device.h
> > +++ b/include/drm/drm_device.h
> > @@ -93,6 +93,9 @@ struct drm_device {
> >       /** @render: Render node */
> >       struct drm_minor *render;
> >
> > +     /** @accel: Compute Acceleration node */
> > +     struct drm_minor *accel;
> > +
> >       /**
> >        * @registered:
> >        *
> > diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> > index f6159acb8856..706e68ca5116 100644
> > --- a/include/drm/drm_drv.h
> > +++ b/include/drm/drm_drv.h
> > @@ -94,6 +94,14 @@ enum drm_driver_feature {
> >        * synchronization of command submission.
> >        */
> >       DRIVER_SYNCOBJ_TIMELINE         = BIT(6),
> > +     /**
> > +      * @DRIVER_COMPUTE_ACCEL:
> > +      *
> > +      * Driver supports compute acceleration devices. This flag is mutually exclusive with
> > +      * @DRIVER_RENDER and @DRIVER_MODESET. Devices that support both graphics and compute
> > +      * acceleration should be handled by two drivers that are connected using auxiliry bus.
> > +      */
> > +     DRIVER_COMPUTE_ACCEL            = BIT(7),
> >
> >       /* IMPORTANT: Below are all the legacy flags, add new ones above. */
> >
> > diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> > index d780fd151789..0d1f853092ab 100644
> > --- a/include/drm/drm_file.h
> > +++ b/include/drm/drm_file.h
> > @@ -51,11 +51,15 @@ struct file;
> >
> >   /* Note that the order of this enum is ABI (it determines
> >    * /dev/dri/renderD* numbers).
> > + *
> > + * Setting DRM_MINOR_ACCEL to 32 gives enough space for more drm minors to
> > + * be implemented before we hit any future
> >    */
> >   enum drm_minor_type {
> >       DRM_MINOR_PRIMARY,
> >       DRM_MINOR_CONTROL,
> >       DRM_MINOR_RENDER,
> > +     DRM_MINOR_ACCEL = 32,
>
> Didn't patch 1/3 say it's a standalone character device major? Are there
> two ways to open the same thing?
>
We do use a new, dedicated major number. But one of the main points
that was agreed was we should leverage the drm code.
And from how I look at things, the best way to do that was to decide
that the accel device is just a new drm minor type.
This way, accel code will seamlessly integrate into the current drm core code.

> >   };
> >
> >   /**
> > @@ -70,7 +74,7 @@ enum drm_minor_type {
> >   struct drm_minor {
> >       /* private: */
> >       int index;                      /* Minor device number */
> > -     int type;                       /* Control or render */
> > +     int type;                       /* Control or render or accel */
>
> Could this be self documenting if type was enum drm_minor_type?
Again, I preferred to change as little as possible existing code in
drm, to make sure I don't do any regressions in this basic patch-set.
I really want to keep drm changes to an absolute minimum and any such
change should come in a separate patch.

Thanks,
Oded
>
> >       struct device *kdev;            /* Linux device */
> >       struct drm_device *dev;
> >
> > @@ -397,7 +401,22 @@ static inline bool drm_is_render_client(const struct drm_file *file_priv)
> >       return file_priv->minor->type == DRM_MINOR_RENDER;
> >   }
> >
> > +/**
> > + * drm_is_accel_client - is this an open file of the compute acceleration node
> > + * @file_priv: DRM file
> > + *
> > + * Returns true if this is an open file of the compute acceleration node, i.e.
> > + * &drm_file.minor of @file_priv is a accel minor.
> > + *
> > + * See also the :ref:`section on accel nodes <drm_accel_node>`.
> > + */
> > +static inline bool drm_is_accel_client(const struct drm_file *file_priv)
> > +{
> > +     return file_priv->minor->type == DRM_MINOR_ACCEL;
> > +}
> > +
> >   int drm_open(struct inode *inode, struct file *filp);
> > +int drm_open_helper(struct file *filp, struct drm_minor *minor);
> >   ssize_t drm_read(struct file *filp, char __user *buffer,
> >                size_t count, loff_t *offset);
> >   int drm_release(struct inode *inode, struct file *filp);
> > --
> > 2.25.1
> >
>
> Regards,
>
> Tvrtko

[RFC PATCH v3 3/3] drm: initialize accel framework

[Oded Gabbay]

Now that we have the accel framework code ready, let's call the
accel functions from all the appropriate places. These places are the
drm module init/exit functions, and all the drm_minor handling
functions.

Signed-off-by: Oded Gabbay <ogabbay@kernel.org>
---
Changes in v3:
 - Call the new accel_debugfs_init() to initalize debugfs per minor.
 - accel_minor_replace() is void so no need to check return value.

 drivers/gpu/drm/drm_drv.c   | 102 ++++++++++++++++++++++++++----------
 drivers/gpu/drm/drm_sysfs.c |  24 ++++++---
 2 files changed, 91 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 8214a0b1ab7f..1aec019f6389 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -35,6 +35,7 @@
 #include <linux/slab.h>
 #include <linux/srcu.h>

+#include <drm/drm_accel.h>
 #include <drm/drm_cache.h>
 #include <drm/drm_client.h>
 #include <drm/drm_color_mgmt.h>
@@ -90,6 +91,8 @@ static struct drm_minor **drm_minor_get_slot(struct drm_device *dev,
 		return &dev->primary;
 	case DRM_MINOR_RENDER:
 		return &dev->render;
+	case DRM_MINOR_ACCEL:
+		return &dev->accel;
 	default:
 		BUG();
 	}
@@ -104,9 +107,13 @@ static void drm_minor_alloc_release(struct drm_device *dev, void *data)

 	put_device(minor->kdev);

-	spin_lock_irqsave(&drm_minor_lock, flags);
-	idr_remove(&drm_minors_idr, minor->index);
-	spin_unlock_irqrestore(&drm_minor_lock, flags);
+	if (minor->type == DRM_MINOR_ACCEL) {
+		accel_minor_remove(minor->index);
+	} else {
+		spin_lock_irqsave(&drm_minor_lock, flags);
+		idr_remove(&drm_minors_idr, minor->index);
+		spin_unlock_irqrestore(&drm_minor_lock, flags);
+	}
 }

 static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
@@ -123,13 +130,17 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
 	minor->dev = dev;

 	idr_preload(GFP_KERNEL);
-	spin_lock_irqsave(&drm_minor_lock, flags);
-	r = idr_alloc(&drm_minors_idr,
-		      NULL,
-		      64 * type,
-		      64 * (type + 1),
-		      GFP_NOWAIT);
-	spin_unlock_irqrestore(&drm_minor_lock, flags);
+	if (type == DRM_MINOR_ACCEL) {
+		r = accel_minor_alloc();
+	} else {
+		spin_lock_irqsave(&drm_minor_lock, flags);
+		r = idr_alloc(&drm_minors_idr,
+			NULL,
+			64 * type,
+			64 * (type + 1),
+			GFP_NOWAIT);
+		spin_unlock_irqrestore(&drm_minor_lock, flags);
+	}
 	idr_preload_end();

 	if (r < 0)
@@ -161,10 +172,14 @@ static int drm_minor_register(struct drm_device *dev, unsigned int type)
 	if (!minor)
 		return 0;

-	ret = drm_debugfs_init(minor, minor->index, drm_debugfs_root);
-	if (ret) {
-		DRM_ERROR("DRM: Failed to initialize /sys/kernel/debug/dri.\n");
-		goto err_debugfs;
+	if (minor->type == DRM_MINOR_ACCEL) {
+		accel_debugfs_init(minor, minor->index);
+	} else {
+		ret = drm_debugfs_init(minor, minor->index, drm_debugfs_root);
+		if (ret) {
+			DRM_ERROR("DRM: Failed to initialize /sys/kernel/debug/dri.\n");
+			goto err_debugfs;
+		}
 	}

 	ret = device_add(minor->kdev);
@@ -172,9 +187,13 @@ static int drm_minor_register(struct drm_device *dev, unsigned int type)
 		goto err_debugfs;

 	/* replace NULL with @minor so lookups will succeed from now on */
-	spin_lock_irqsave(&drm_minor_lock, flags);
-	idr_replace(&drm_minors_idr, minor, minor->index);
-	spin_unlock_irqrestore(&drm_minor_lock, flags);
+	if (minor->type == DRM_MINOR_ACCEL) {
+		accel_minor_replace(minor, minor->index);
+	} else {
+		spin_lock_irqsave(&drm_minor_lock, flags);
+		idr_replace(&drm_minors_idr, minor, minor->index);
+		spin_unlock_irqrestore(&drm_minor_lock, flags);
+	}

 	DRM_DEBUG("new minor registered %d\n", minor->index);
 	return 0;
@@ -194,9 +213,13 @@ static void drm_minor_unregister(struct drm_device *dev, unsigned int type)
 		return;

 	/* replace @minor with NULL so lookups will fail from now on */
-	spin_lock_irqsave(&drm_minor_lock, flags);
-	idr_replace(&drm_minors_idr, NULL, minor->index);
-	spin_unlock_irqrestore(&drm_minor_lock, flags);
+	if (minor->type == DRM_MINOR_ACCEL) {
+		accel_minor_replace(NULL, minor->index);
+	} else {
+		spin_lock_irqsave(&drm_minor_lock, flags);
+		idr_replace(&drm_minors_idr, NULL, minor->index);
+		spin_unlock_irqrestore(&drm_minor_lock, flags);
+	}

 	device_del(minor->kdev);
 	dev_set_drvdata(minor->kdev, NULL); /* safety belt */
@@ -603,6 +626,14 @@ static int drm_dev_init(struct drm_device *dev,
 	/* no per-device feature limits by default */
 	dev->driver_features = ~0u;

+	if (drm_core_check_feature(dev, DRIVER_COMPUTE_ACCEL) &&
+				(drm_core_check_feature(dev, DRIVER_RENDER) ||
+				drm_core_check_feature(dev, DRIVER_MODESET))) {
+
+		DRM_ERROR("DRM driver can't be both a compute acceleration and graphics driver\n");
+		return -EINVAL;
+	}
+
 	drm_legacy_init_members(dev);
 	INIT_LIST_HEAD(&dev->filelist);
 	INIT_LIST_HEAD(&dev->filelist_internal);
@@ -628,15 +659,21 @@ static int drm_dev_init(struct drm_device *dev,

 	dev->anon_inode = inode;

-	if (drm_core_check_feature(dev, DRIVER_RENDER)) {
-		ret = drm_minor_alloc(dev, DRM_MINOR_RENDER);
+	if (drm_core_check_feature(dev, DRIVER_COMPUTE_ACCEL)) {
+		ret = drm_minor_alloc(dev, DRM_MINOR_ACCEL);
 		if (ret)
 			goto err;
-	}
+	} else {
+		if (drm_core_check_feature(dev, DRIVER_RENDER)) {
+			ret = drm_minor_alloc(dev, DRM_MINOR_RENDER);
+			if (ret)
+				goto err;
+		}

-	ret = drm_minor_alloc(dev, DRM_MINOR_PRIMARY);
-	if (ret)
-		goto err;
+		ret = drm_minor_alloc(dev, DRM_MINOR_PRIMARY);
+		if (ret)
+			goto err;
+	}

 	ret = drm_legacy_create_map_hash(dev);
 	if (ret)
@@ -883,6 +920,10 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
 	if (ret)
 		goto err_minors;

+	ret = drm_minor_register(dev, DRM_MINOR_ACCEL);
+	if (ret)
+		goto err_minors;
+
 	ret = create_compat_control_link(dev);
 	if (ret)
 		goto err_minors;
@@ -902,12 +943,13 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags)
 		 driver->name, driver->major, driver->minor,
 		 driver->patchlevel, driver->date,
 		 dev->dev ? dev_name(dev->dev) : "virtual device",
-		 dev->primary->index);
+		 dev->primary ? dev->primary->index : dev->accel->index);

 	goto out_unlock;

 err_minors:
 	remove_compat_control_link(dev);
+	drm_minor_unregister(dev, DRM_MINOR_ACCEL);
 	drm_minor_unregister(dev, DRM_MINOR_PRIMARY);
 	drm_minor_unregister(dev, DRM_MINOR_RENDER);
 out_unlock:
@@ -950,6 +992,7 @@ void drm_dev_unregister(struct drm_device *dev)
 	drm_legacy_rmmaps(dev);

 	remove_compat_control_link(dev);
+	drm_minor_unregister(dev, DRM_MINOR_ACCEL);
 	drm_minor_unregister(dev, DRM_MINOR_PRIMARY);
 	drm_minor_unregister(dev, DRM_MINOR_RENDER);
 }
@@ -1034,6 +1077,7 @@ static const struct file_operations drm_stub_fops = {
 static void drm_core_exit(void)
 {
 	drm_privacy_screen_lookup_exit();
+	accel_core_exit();
 	unregister_chrdev(DRM_MAJOR, "drm");
 	debugfs_remove(drm_debugfs_root);
 	drm_sysfs_destroy();
@@ -1061,6 +1105,10 @@ static int __init drm_core_init(void)
 	if (ret < 0)
 		goto error;

+	ret = accel_core_init();
+	if (ret < 0)
+		goto error;
+
 	drm_privacy_screen_lookup_init();

 	drm_core_init_complete = true;
diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
index 430e00b16eec..b8da978d85bb 100644
--- a/drivers/gpu/drm/drm_sysfs.c
+++ b/drivers/gpu/drm/drm_sysfs.c
@@ -19,6 +19,7 @@
 #include <linux/kdev_t.h>
 #include <linux/slab.h>

+#include <drm/drm_accel.h>
 #include <drm/drm_connector.h>
 #include <drm/drm_device.h>
 #include <drm/drm_file.h>
@@ -471,19 +472,26 @@ struct device *drm_sysfs_minor_alloc(struct drm_minor *minor)
 	struct device *kdev;
 	int r;

-	if (minor->type == DRM_MINOR_RENDER)
-		minor_str = "renderD%d";
-	else
-		minor_str = "card%d";
-
 	kdev = kzalloc(sizeof(*kdev), GFP_KERNEL);
 	if (!kdev)
 		return ERR_PTR(-ENOMEM);

 	device_initialize(kdev);
-	kdev->devt = MKDEV(DRM_MAJOR, minor->index);
-	kdev->class = drm_class;
-	kdev->type = &drm_sysfs_device_minor;
+
+	if (minor->type == DRM_MINOR_ACCEL) {
+		minor_str = "accel%d";
+		accel_set_device_instance_params(kdev, minor->index);
+	} else {
+		if (minor->type == DRM_MINOR_RENDER)
+			minor_str = "renderD%d";
+		else
+			minor_str = "card%d";
+
+		kdev->devt = MKDEV(DRM_MAJOR, minor->index);
+		kdev->class = drm_class;
+		kdev->type = &drm_sysfs_device_minor;
+	}
+
 	kdev->parent = minor->dev->dev;
 	kdev->release = drm_sysfs_release;
 	dev_set_drvdata(kdev, minor);
--
2.25.1

Re: [RFC PATCH v3 3/3] drm: initialize accel framework

[Jeffrey Hugo]

On 11/6/2022 2:02 PM, Oded Gabbay wrote:

> @@ -603,6 +626,14 @@ static int drm_dev_init(struct drm_device *dev,
>   	/* no per-device feature limits by default */
>   	dev->driver_features = ~0u;
> 
> +	if (drm_core_check_feature(dev, DRIVER_COMPUTE_ACCEL) &&
> +				(drm_core_check_feature(dev, DRIVER_RENDER) ||
> +				drm_core_check_feature(dev, DRIVER_MODESET))) {

Shouldn't the indentation for the 2nd and 3rd line be such that the 
start of the lines is aligned with the "(" on the first line?

Re: [RFC PATCH v3 3/3] drm: initialize accel framework

[Oded Gabbay]

On Mon, Nov 7, 2022 at 6:25 PM Jeffrey Hugo <quic_jhugo@quicinc.com> wrote:
>
> On 11/6/2022 2:02 PM, Oded Gabbay wrote:
>
> > @@ -603,6 +626,14 @@ static int drm_dev_init(struct drm_device *dev,
> >       /* no per-device feature limits by default */
> >       dev->driver_features = ~0u;
> >
> > +     if (drm_core_check_feature(dev, DRIVER_COMPUTE_ACCEL) &&
> > +                             (drm_core_check_feature(dev, DRIVER_RENDER) ||
> > +                             drm_core_check_feature(dev, DRIVER_MODESET))) {
>
> Shouldn't the indentation for the 2nd and 3rd line be such that the
> start of the lines is aligned with the "(" on the first line?
afaik there is no such rule. If there was, checkpatch should have reported that.
Oded

Re: [RFC PATCH v3 0/3] new subsystem for compute accelerator devices

[Jeffrey Hugo]

On 11/6/2022 2:02 PM, Oded Gabbay wrote:
> This is the third version of the RFC following the comments given on the
> second version, but more importantly, following testing done by the VPU
> driver people and myself. We found out that there is a circular dependency
> between DRM and accel. DRM calls accel exported symbols during init and when
> accel devices are registering (all the minor handling), then accel calls DRM
> exported symbols. Therefore, if the two components are compiled as modules,
> there is a circular dependency.
> 
> To overcome this, I have decided to compile the accel core code as part of
> the DRM kernel module (drm.ko). IMO, this is inline with the spirit of the
> design choice to have accel reuse the DRM core code and avoid code
> duplication.
> 
> Another important change is that I have reverted back to use IDR for minor
> handling instead of xarray. This is because I have found that xarray doesn't
> handle well the scenario where you allocate a NULL entry and then exchange it
> with a real pointer. It appears xarray still considers that entry a "zero"
> entry. This is unfortunate because DRM works that way (first allocates a NULL
> entry and then replaces the entry with a real pointer).
> 
> I decided to revert to IDR because I don't want to hold up these patches,
> as many people are blocked until the support for accel is merged. The xarray
> issue should be fixed as a separate patch by either fixing the xarray code or
> changing how DRM + ACCEL do minor id handling.

This sounds sane to me.  However, this appears to be something that 
Matthew Wilcox should be aware of (added for visibility).  Perhaps he 
has a very quick solution.  If not, at-least he might have ideas on how 
to best address in the future.

> The patches are in the following repo:
> https://git.kernel.org/pub/scm/linux/kernel/git/ogabbay/accel.git/log/?h=accel_v3
> 
> As in v2, The HEAD of that branch is a commit adding a dummy driver that
> registers an accel device using the new framework. This can be served
> as a simple reference. I have checked inserting and removing the dummy driver,
> and opening and closing /dev/accel/accel0 and nothing got broken :)
> 
> v1 cover letter:
> https://lkml.org/lkml/2022/10/22/544
> 
> v2 cover letter:
> https://lore.kernel.org/lkml/20221102203405.1797491-1-ogabbay@kernel.org/T/
> 
> Thanks,
> Oded.
> 
> Oded Gabbay (3):
>    drivers/accel: define kconfig and register a new major
>    accel: add dedicated minor for accelerator devices
>    drm: initialize accel framework
> 
>   Documentation/admin-guide/devices.txt |   5 +
>   MAINTAINERS                           |   8 +
>   drivers/Kconfig                       |   2 +
>   drivers/accel/Kconfig                 |  24 ++
>   drivers/accel/drm_accel.c             | 322 ++++++++++++++++++++++++++
>   drivers/gpu/drm/Makefile              |   1 +
>   drivers/gpu/drm/drm_drv.c             | 102 +++++---
>   drivers/gpu/drm/drm_file.c            |   2 +-
>   drivers/gpu/drm/drm_sysfs.c           |  24 +-
>   include/drm/drm_accel.h               |  97 ++++++++
>   include/drm/drm_device.h              |   3 +
>   include/drm/drm_drv.h                 |   8 +
>   include/drm/drm_file.h                |  21 +-
>   13 files changed, 582 insertions(+), 37 deletions(-)
>   create mode 100644 drivers/accel/Kconfig
>   create mode 100644 drivers/accel/drm_accel.c
>   create mode 100644 include/drm/drm_accel.h
> 
> --
> 2.25.1
> 

Re: [RFC PATCH v3 0/3] new subsystem for compute accelerator devices

[Matthew Wilcox]

On Mon, Nov 07, 2022 at 09:07:28AM -0700, Jeffrey Hugo wrote:
> > Another important change is that I have reverted back to use IDR for minor
> > handling instead of xarray. This is because I have found that xarray doesn't
> > handle well the scenario where you allocate a NULL entry and then exchange it
> > with a real pointer. It appears xarray still considers that entry a "zero"
> > entry. This is unfortunate because DRM works that way (first allocates a NULL
> > entry and then replaces the entry with a real pointer).
> > 
> > I decided to revert to IDR because I don't want to hold up these patches,
> > as many people are blocked until the support for accel is merged. The xarray
> > issue should be fixed as a separate patch by either fixing the xarray code or
> > changing how DRM + ACCEL do minor id handling.
> 
> This sounds sane to me.  However, this appears to be something that Matthew
> Wilcox should be aware of (added for visibility).  Perhaps he has a very
> quick solution.  If not, at-least he might have ideas on how to best address
> in the future.

Thanks for cc'ing me.  I wasn't aware of this problem because I hadn't
seen Oded's email yet.  The "problem" is simply a mis-use of the API.

Re: [RFC PATCH v3 0/3] new subsystem for compute accelerator devices

[Jason Gunthorpe]

On Sun, Nov 06, 2022 at 11:02:22PM +0200, Oded Gabbay wrote:
> Another important change is that I have reverted back to use IDR for minor
> handling instead of xarray. This is because I have found that xarray doesn't
> handle well the scenario where you allocate a NULL entry and then exchange it
> with a real pointer. It appears xarray still considers that entry a "zero"
> entry. This is unfortunate because DRM works that way (first allocates a NULL
> entry and then replaces the entry with a real pointer).

This is what XA_ZERO_ENTRY is for.

Some APIs, like xa_alloc automatically promote NULL to XA_ZERO_ENTRY,
others require it to be explicit.

If you use the usual pattern of xa_alloc(NULL), xa_store(!NULL) then
you should be fine, as far as I know. So long as the xarray was tagged
as allocating.

Jason

Re: [RFC PATCH v3 0/3] new subsystem for compute accelerator devices

[Christopher Friedt]

Hi Oded,

On Sun, Nov 6, 2022 at 4:03 PM Oded Gabbay <ogabbay@kernel.org> wrote:
> The patches are in the following repo:
> https://git.kernel.org/pub/scm/linux/kernel/git/ogabbay/accel.git/log/?h=accel_v3
>
> As in v2, The HEAD of that branch is a commit adding a dummy driver that
> registers an accel device using the new framework. This can be served
> as a simple reference. I have checked inserting and removing the dummy driver,
> and opening and closing /dev/accel/accel0 and nothing got broken :)
>
> v1 cover letter:
> https://lkml.org/lkml/2022/10/22/544
>
> v2 cover letter:
> https://lore.kernel.org/lkml/20221102203405.1797491-1-ogabbay@kernel.org/T/

I was in the room at Plumbers when a lot of this was discussed (in
2022 and also 2019), but I haven't really had an opportunity to
provide feedback until now. In general, I think it's great and thanks
for pushing it forward and getting feedback.

The v1 cover letter mentioned RAS (reliability, availability,
serviceability) and Dave also mentioned it here [1]. There was a
suggestion to use Netlink. It's an area that I'm fairly interested in
because I do a lot of development on the firmware side (and
specifically, with Zephyr).

Personally, I think Netlink could be one option for serializing and
deserializing RAS information but it would be helpful for that
interface to be somewhat flexible, like a void * and length, and to
provide userspace the capability of querying which RAS formats are
supported.

For example, AntMicro used OpenAMP + rpmsg in their NVMe accelerator,
and gave a talk on it at ZDS and Plumbers this year [2][3].

In Zephyr, the LGPL license for Netlink might be a non-starter
(although I'm no lawyer). However, Zephyr does already support
OpenAMP, protobufs, json, and will soon support Thrift.

Some companies might prefer to use Netlink. Others might prefer to use
ASN.1. Some companies might prefer to use key-value pairs and limit
the parameters and messages to uint32s. Some might handle all of the
RAS details in-kernel, while others might want the kernel to act more
like a transport to firmware.

Companies already producing accelerators may have a particular
preference for serialization / deserialization in their own
datacenters.

With that, it would be helpful to be able to query RAS capabilities via ioctl.

#define ACCEL_CAP_RAS_KEY_VAL_32 BIT(0)
#define ACCEL_CAP_RAS_NETLINK BIT(1)
#define ACCEL_CAP_RAS_JSON BIT(2)
#define ACCEL_CAP_RAS_PROTOBUF BIT(3)
#define ACCEL_CAP_RAS_GRPC BIT(4)
#define ACCEL_CAP_RAS_THRIFT BIT(5)
#define ACCEL_CAP_RAS_JSON BIT(6)
#define ACCEL_CAP_RAS_ASN1 BIT(7)

or something along those lines. Anyway, just putting the idea out there.

I'm sure there are a lot of opinions on this topic and that there are
a lot of implications of using this or that serialization format.
Obviously there can be security implications as well.

Apologies if I've already missed some of this discussion.

Cheers,

C

[1] https://airlied.blogspot.com/2022/09/accelerators-bof-outcomes-summary.html
[2] https://zephyr2022.sched.com/event/10CFD/open-source-nvme-ai-accelerator-platform-with-zephyr-karol-gugala-antmicro
[3] https://lpc.events/event/16/contributions/1245/

Re: [RFC PATCH v3 0/3] new subsystem for compute accelerator devices

[Oded Gabbay]

On Sat, Nov 12, 2022 at 12:04 AM Christopher Friedt
<chrisfriedt@gmail.com> wrote:
>
> Hi Oded,
>
> On Sun, Nov 6, 2022 at 4:03 PM Oded Gabbay <ogabbay@kernel.org> wrote:
> > The patches are in the following repo:
> > https://git.kernel.org/pub/scm/linux/kernel/git/ogabbay/accel.git/log/?h=accel_v3
> >
> > As in v2, The HEAD of that branch is a commit adding a dummy driver that
> > registers an accel device using the new framework. This can be served
> > as a simple reference. I have checked inserting and removing the dummy driver,
> > and opening and closing /dev/accel/accel0 and nothing got broken :)
> >
> > v1 cover letter:
> > https://lkml.org/lkml/2022/10/22/544
> >
> > v2 cover letter:
> > https://lore.kernel.org/lkml/20221102203405.1797491-1-ogabbay@kernel.org/T/
>
> I was in the room at Plumbers when a lot of this was discussed (in
> 2022 and also 2019), but I haven't really had an opportunity to
> provide feedback until now. In general, I think it's great and thanks
> for pushing it forward and getting feedback.
>
> The v1 cover letter mentioned RAS (reliability, availability,
> serviceability) and Dave also mentioned it here [1]. There was a
> suggestion to use Netlink. It's an area that I'm fairly interested in
> because I do a lot of development on the firmware side (and
> specifically, with Zephyr).
>
> Personally, I think Netlink could be one option for serializing and
> deserializing RAS information but it would be helpful for that
> interface to be somewhat flexible, like a void * and length, and to
> provide userspace the capability of querying which RAS formats are
> supported.
>
> For example, AntMicro used OpenAMP + rpmsg in their NVMe accelerator,
> and gave a talk on it at ZDS and Plumbers this year [2][3].
>
> In Zephyr, the LGPL license for Netlink might be a non-starter
> (although I'm no lawyer). However, Zephyr does already support
> OpenAMP, protobufs, json, and will soon support Thrift.
>
> Some companies might prefer to use Netlink. Others might prefer to use
> ASN.1. Some companies might prefer to use key-value pairs and limit
> the parameters and messages to uint32s. Some might handle all of the
> RAS details in-kernel, while others might want the kernel to act more
> like a transport to firmware.
>
> Companies already producing accelerators may have a particular
> preference for serialization / deserialization in their own
> datacenters.
>
> With that, it would be helpful to be able to query RAS capabilities via ioctl.
>
> #define ACCEL_CAP_RAS_KEY_VAL_32 BIT(0)
> #define ACCEL_CAP_RAS_NETLINK BIT(1)
> #define ACCEL_CAP_RAS_JSON BIT(2)
> #define ACCEL_CAP_RAS_PROTOBUF BIT(3)
> #define ACCEL_CAP_RAS_GRPC BIT(4)
> #define ACCEL_CAP_RAS_THRIFT BIT(5)
> #define ACCEL_CAP_RAS_JSON BIT(6)
> #define ACCEL_CAP_RAS_ASN1 BIT(7)
>
> or something along those lines. Anyway, just putting the idea out there.
>
> I'm sure there are a lot of opinions on this topic and that there are
> a lot of implications of using this or that serialization format.
> Obviously there can be security implications as well.
>
> Apologies if I've already missed some of this discussion.
>
> Cheers,
>
> C
>
> [1] https://airlied.blogspot.com/2022/09/accelerators-bof-outcomes-summary.html
> [2] https://zephyr2022.sched.com/event/10CFD/open-source-nvme-ai-accelerator-platform-with-zephyr-karol-gugala-antmicro
> [3] https://lpc.events/event/16/contributions/1245/

Hi Christopher,
Thanks for all this information.
At this stage, I'm mainly trying to gather information on RAS current
status in the OCP (Open Compute Project) and Linux kernel, so your
email was on point :)
It seems to me that this topic is broader than just accelerators or
GPUs, because there are other device types that are implementing some
kind of RAS (e.g. NIC).
My gut feeling is that the end solution would be some kind of generic
kernel driver/framework that will expose RAS to userspace for any
device type, but it's too early to tell.
I'll update once I have the full picture.

Thanks,
Oded