[PATCH v4 0/4] new subsystem for compute accelerator devices

[PATCH v4 0/4] new subsystem for compute accelerator devices

[Oded Gabbay]

This is the fourth (and hopefully last) version of the patch-set to add the
new subsystem for compute accelerators. I removed the RFC headline as
I believe it is now ready for merging.

Compare to v3, this patch-set contains one additional patch that adds
documentation regarding the accel subsystem. I hope it's good enough for
this stage. In addition, there were few very minor fixes according to
comments received on v3.

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

As in v3, 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.

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/

v3 cover letter:
https://lore.kernel.org/lkml/20221106210225.2065371-1-ogabbay@kernel.org/T/

Thanks,
Oded.

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

 Documentation/accel/index.rst         |  17 ++
 Documentation/accel/introduction.rst  | 109 +++++++++
 Documentation/admin-guide/devices.txt |   5 +
 Documentation/subsystem-apis.rst      |   1 +
 MAINTAINERS                           |   9 +
 drivers/Kconfig                       |   2 +
 drivers/accel/Kconfig                 |  24 ++
 drivers/accel/drm_accel.c             | 323 ++++++++++++++++++++++++++
 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 +-
 16 files changed, 711 insertions(+), 37 deletions(-)
 create mode 100644 Documentation/accel/index.rst
 create mode 100644 Documentation/accel/introduction.rst
 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

[PATCH v4 1/4] 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 v4:
 - Fix comment style
 - Remove useless goto
 - Fix indentation of major define
 - Move comment from next patch to this patch

 Documentation/admin-guide/devices.txt |  5 ++
 MAINTAINERS                           |  8 +++
 drivers/Kconfig                       |  2 +
 drivers/accel/Kconfig                 | 24 ++++++++
 drivers/accel/drm_accel.c             | 83 +++++++++++++++++++++++++++
 drivers/gpu/drm/Makefile              |  1 +
 include/drm/drm_accel.h               | 32 +++++++++++
 7 files changed, 155 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 178a7a383b5f..4d752aac3ec0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6831,6 +6831,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..fac6ad6ac28e
--- /dev/null
+++ b/drivers/accel/drm_accel.c
@@ -0,0 +1,83 @@
+// 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)
+		DRM_ERROR("Cannot register ACCEL major: %d\n", ret);
+
+error:
+	/*
+	 * Any cleanup due to errors will be done in drm_core_exit() that
+	 * will call accel_core_exit()
+	 */
+	return ret;
+}
diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index c44a54cadb61..2773e1ad1b6b 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..e1758f484278
--- /dev/null
+++ b/include/drm/drm_accel.h
@@ -0,0 +1,32 @@
+/* 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 to allow drm_core_init to complete successfully */
+	return 0;
+}
+
+#endif /* IS_ENABLED(CONFIG_DRM_ACCEL) */
+
+#endif /* DRM_ACCEL_H_ */
--
2.25.1

[PATCH v4 2/4] 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 v4:
 - Fix spelling
 - Remove comment and move it to previous patch

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

diff --git a/drivers/accel/drm_accel.c b/drivers/accel/drm_accel.c
index fac6ad6ac28e..703d40c4ff45 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 e1758f484278..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_

+#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

@@ -27,6 +71,27 @@ static inline int __init accel_core_init(void)
 	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..cb35db7093c8 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 auxiliary 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

[PATCH v4 3/4] 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>
---
 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

[PATCH v4 4/4] doc: add documentation for accel subsystem

[Oded Gabbay]

Add an introduction section for the accel subsystem. Most of the
relevant data is in the DRM documentation, so the introduction only
presents the why of the new subsystem, how are the compute accelerators
exposed to user-space and what changes need to be done in a standard
DRM driver to register it to the new accel subsystem.

Signed-off-by: Oded Gabbay <ogabbay@kernel.org>
---
 Documentation/accel/index.rst        |  17 +++++
 Documentation/accel/introduction.rst | 109 +++++++++++++++++++++++++++
 Documentation/subsystem-apis.rst     |   1 +
 MAINTAINERS                          |   1 +
 4 files changed, 128 insertions(+)
 create mode 100644 Documentation/accel/index.rst
 create mode 100644 Documentation/accel/introduction.rst

diff --git a/Documentation/accel/index.rst b/Documentation/accel/index.rst
new file mode 100644
index 000000000000..2b43c9a7f67b
--- /dev/null
+++ b/Documentation/accel/index.rst
@@ -0,0 +1,17 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+====================
+Compute Accelerators
+====================
+
+.. toctree::
+   :maxdepth: 1
+
+   introduction
+
+.. only::  subproject and html
+
+   Indices
+   =======
+
+   * :ref:`genindex`
diff --git a/Documentation/accel/introduction.rst b/Documentation/accel/introduction.rst
new file mode 100644
index 000000000000..5a3963eae973
--- /dev/null
+++ b/Documentation/accel/introduction.rst
@@ -0,0 +1,109 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+============
+Introduction
+============
+
+The Linux compute accelerators subsystem is designed to expose compute
+accelerators in a common way to user-space and provide a common set of
+functionality.
+
+These devices can be either stand-alone ASICs or IP blocks inside an SoC/GPU.
+Although these devices are typically designed to accelerate Machine-Learning
+and/or Deep-Learning computations, the accel layer is not limited to handling
+these types of accelerators.
+
+typically, a compute accelerator will belong to one of the following
+categories:
+
+- Edge AI - doing inference at an edge device. It can be an embedded ASIC/FPGA,
+  or an IP inside a SoC (e.g. laptop web camera). These devices
+  are typically configured using registers and can work with or without DMA.
+
+- Inference data-center - single/multi user devices in a large server. This
+  type of device can be stand-alone or an IP inside a SoC or a GPU. It will
+  have on-board DRAM (to hold the DL topology), DMA engines and
+  command submission queues (either kernel or user-space queues).
+  It might also have an MMU to manage multiple users and might also enable
+  virtualization (SR-IOV) to support multiple VMs on the same device. In
+  addition, these devices will usually have some tools, such as profiler and
+  debugger.
+
+- Training data-center - Similar to Inference data-center cards, but typically
+  have more computational power and memory b/w (e.g. HBM) and will likely have
+  a method of scaling-up/out, i.e. connecting to other training cards inside
+  the server or in other servers, respectively.
+
+All these devices typically have different runtime user-space software stacks,
+that are tailored-made to their h/w. In addition, they will also probably
+include a compiler to generate programs to their custom-made computational
+engines. Typically, the common layer in user-space will be the DL frameworks,
+such as PyTorch and TensorFlow.
+
+Sharing code with DRM
+=====================
+
+Because this type of devices can be an IP inside GPUs or have similar
+characteristics as those of GPUs, the accel subsystem will use the
+DRM subsystem's code and functionality. i.e. the accel core code will
+be part of the DRM subsystem and an accel device will be a new type of DRM
+device.
+
+This will allow us to leverage the extensive DRM code-base and
+collaborate with DRM developers that have experience with this type of
+devices. In addition, new features that will be added for the accelerator
+drivers can be of use to GPU drivers as well.
+
+Differentiation from GPUs
+=========================
+
+Because we want to prevent the extensive user-space graphic software stack
+from trying to use an accelerator as a GPU, the compute accelerators will be
+differentiated from GPUs by using a new major number and new device char files.
+
+Furthermore, the drivers will be located in a separate place in the kernel
+tree - drivers/accel/.
+
+The accelerator devices will be exposed to the user space with the dedicated
+261 major number and will have the following convention:
+
+- device char files - /dev/accel/accel*
+- sysfs             - /sys/class/accel/accel*/
+- debugfs           - /sys/kernel/debug/accel/accel*/
+
+Getting Started
+===============
+
+First, read the DRM documentation. Not only it will explain how to write a new
+DRM driver but it will also contain all the information on how to contribute,
+the Code Of Conduct and what is the coding style/documentation. All of that
+is the same for the accel subsystem.
+
+Second, make sure the kernel is configured with CONFIG_DRM_ACCEL.
+
+To expose your device as an accelerator, two changes are needed to
+be done in your driver (as opposed to a standard DRM driver):
+
+- Add the DRIVER_COMPUTE_ACCEL feature flag in your drm_driver's
+  driver_features field. It is important to note that this driver feature is
+  mutually exclusive with DRIVER_RENDER and DRIVER_MODESET. 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.
+
+- Change the open callback in your driver fops structure to accel_open().
+  Alternatively, your driver can use DEFINE_DRM_ACCEL_FOPS macro to easily
+  set the correct function operations pointers structure.
+
+External References
+===================
+
+email threads
+-------------
+
+* `Initial discussion on the New subsystem for acceleration devices <https://lkml.org/lkml/2022/7/31/83>`_ - Oded Gabbay (2022)
+* `patch-set to add the new subsystem <https://lkml.org/lkml/2022/10/22/544>`_ - Oded Gabbay (2022)
+
+Conference talks
+----------------
+
+* `LPC 2022 Accelerators BOF outcomes summary <https://airlied.blogspot.com/2022/09/accelerators-bof-outcomes-summary.html>`_ - Dave Airlie (2022)
diff --git a/Documentation/subsystem-apis.rst b/Documentation/subsystem-apis.rst
index af65004a80aa..b51f38527e14 100644
--- a/Documentation/subsystem-apis.rst
+++ b/Documentation/subsystem-apis.rst
@@ -43,6 +43,7 @@ needed).
    input/index
    hwmon/index
    gpu/index
+   accel/index
    security/index
    sound/index
    crypto/index
diff --git a/MAINTAINERS b/MAINTAINERS
index 4d752aac3ec0..6ba7bb35208a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -6837,6 +6837,7 @@ 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:	Documentation/accel/
 F:	drivers/accel/
 
 DRM DRIVERS FOR ALLWINNER A10
-- 
2.25.1

Re: [PATCH v4 0/4] new subsystem for compute accelerator devices

[Greg Kroah-Hartman]

On Sat, Nov 19, 2022 at 10:44:31PM +0200, Oded Gabbay wrote:
> This is the fourth (and hopefully last) version of the patch-set to add the
> new subsystem for compute accelerators. I removed the RFC headline as
> I believe it is now ready for merging.
> 
> Compare to v3, this patch-set contains one additional patch that adds
> documentation regarding the accel subsystem. I hope it's good enough for
> this stage. In addition, there were few very minor fixes according to
> comments received on v3.
> 
> The patches are in the following repo:
> https://git.kernel.org/pub/scm/linux/kernel/git/ogabbay/accel.git/log/?h=accel_v4
> 
> As in v3, 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.

Looks good, thanks for doing this:

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH v4 2/4] accel: add dedicated minor for accelerator devices

[Jeffrey Hugo]

On 11/19/2022 1:44 PM, Oded Gabbay wrote:
> diff --git a/drivers/accel/drm_accel.c b/drivers/accel/drm_accel.c
> index fac6ad6ac28e..703d40c4ff45 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>

Including xarray, but using idr
This should be linux/idr.h

This seems so minor, I don't think I advise spinning a v5 for it.  If a 
v5 is warranted elsewhere, obviously fix this.  If not, hopefully this 
can be fixed up by whoever applies it, or someone submits a follow up patch.

Hopefully this is the only nit.  I would like to see this merged.

-Jeff

Re: [PATCH v4 4/4] doc: add documentation for accel subsystem

[Jeffrey Hugo]

On 11/19/2022 1:44 PM, Oded Gabbay wrote:
> Add an introduction section for the accel subsystem. Most of the
> relevant data is in the DRM documentation, so the introduction only
> presents the why of the new subsystem, how are the compute accelerators
> exposed to user-space and what changes need to be done in a standard
> DRM driver to register it to the new accel subsystem.
> 
> Signed-off-by: Oded Gabbay <ogabbay@kernel.org>
> ---
>   Documentation/accel/index.rst        |  17 +++++
>   Documentation/accel/introduction.rst | 109 +++++++++++++++++++++++++++
>   Documentation/subsystem-apis.rst     |   1 +
>   MAINTAINERS                          |   1 +
>   4 files changed, 128 insertions(+)
>   create mode 100644 Documentation/accel/index.rst
>   create mode 100644 Documentation/accel/introduction.rst
> 
> diff --git a/Documentation/accel/index.rst b/Documentation/accel/index.rst
> new file mode 100644
> index 000000000000..2b43c9a7f67b
> --- /dev/null
> +++ b/Documentation/accel/index.rst
> @@ -0,0 +1,17 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +====================
> +Compute Accelerators
> +====================
> +
> +.. toctree::
> +   :maxdepth: 1
> +
> +   introduction
> +
> +.. only::  subproject and html
> +
> +   Indices
> +   =======
> +
> +   * :ref:`genindex`
> diff --git a/Documentation/accel/introduction.rst b/Documentation/accel/introduction.rst
> new file mode 100644
> index 000000000000..5a3963eae973
> --- /dev/null
> +++ b/Documentation/accel/introduction.rst
> @@ -0,0 +1,109 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +============
> +Introduction
> +============
> +
> +The Linux compute accelerators subsystem is designed to expose compute
> +accelerators in a common way to user-space and provide a common set of
> +functionality.
> +
> +These devices can be either stand-alone ASICs or IP blocks inside an SoC/GPU.
> +Although these devices are typically designed to accelerate Machine-Learning
> +and/or Deep-Learning computations, the accel layer is not limited to handling

You use "DL" later on as a short form for Deep-Learning.  It would be 
good to introduce that here.

> +these types of accelerators.
> +
> +typically, a compute accelerator will belong to one of the following

Typically

> +categories:
> +
> +- Edge AI - doing inference at an edge device. It can be an embedded ASIC/FPGA,
> +  or an IP inside a SoC (e.g. laptop web camera). These devices
> +  are typically configured using registers and can work with or without DMA.
> +
> +- Inference data-center - single/multi user devices in a large server. This
> +  type of device can be stand-alone or an IP inside a SoC or a GPU. It will
> +  have on-board DRAM (to hold the DL topology), DMA engines and
> +  command submission queues (either kernel or user-space queues).
> +  It might also have an MMU to manage multiple users and might also enable
> +  virtualization (SR-IOV) to support multiple VMs on the same device. In
> +  addition, these devices will usually have some tools, such as profiler and
> +  debugger.
> +
> +- Training data-center - Similar to Inference data-center cards, but typically
> +  have more computational power and memory b/w (e.g. HBM) and will likely have
> +  a method of scaling-up/out, i.e. connecting to other training cards inside
> +  the server or in other servers, respectively.
> +
> +All these devices typically have different runtime user-space software stacks,
> +that are tailored-made to their h/w. In addition, they will also probably
> +include a compiler to generate programs to their custom-made computational
> +engines. Typically, the common layer in user-space will be the DL frameworks,
> +such as PyTorch and TensorFlow.
> +
> +Sharing code with DRM
> +=====================
> +
> +Because this type of devices can be an IP inside GPUs or have similar
> +characteristics as those of GPUs, the accel subsystem will use the
> +DRM subsystem's code and functionality. i.e. the accel core code will
> +be part of the DRM subsystem and an accel device will be a new type of DRM
> +device.
> +
> +This will allow us to leverage the extensive DRM code-base and
> +collaborate with DRM developers that have experience with this type of
> +devices. In addition, new features that will be added for the accelerator
> +drivers can be of use to GPU drivers as well.
> +
> +Differentiation from GPUs
> +=========================
> +
> +Because we want to prevent the extensive user-space graphic software stack
> +from trying to use an accelerator as a GPU, the compute accelerators will be
> +differentiated from GPUs by using a new major number and new device char files.
> +
> +Furthermore, the drivers will be located in a separate place in the kernel
> +tree - drivers/accel/.
> +
> +The accelerator devices will be exposed to the user space with the dedicated
> +261 major number and will have the following convention:
> +
> +- device char files - /dev/accel/accel*
> +- sysfs             - /sys/class/accel/accel*/
> +- debugfs           - /sys/kernel/debug/accel/accel*/
> +
> +Getting Started
> +===============
> +
> +First, read the DRM documentation. Not only it will explain how to write a new

How about a link to the DRM documentation?

> +DRM driver but it will also contain all the information on how to contribute,
> +the Code Of Conduct and what is the coding style/documentation. All of that
> +is the same for the accel subsystem.
> +
> +Second, make sure the kernel is configured with CONFIG_DRM_ACCEL.
> +
> +To expose your device as an accelerator, two changes are needed to
> +be done in your driver (as opposed to a standard DRM driver):
> +
> +- Add the DRIVER_COMPUTE_ACCEL feature flag in your drm_driver's
> +  driver_features field. It is important to note that this driver feature is
> +  mutually exclusive with DRIVER_RENDER and DRIVER_MODESET. Devices that want

I don't remember seeing code that validates a driver with 
DRIVER_COMPUTE_ACCEL does not also have DRIVER_MODESET.  What am I missing?

> +  to expose both graphics and compute device char files should be handled by
> +  two drivers that are connected using the auxiliary bus framework.
> +
> +- Change the open callback in your driver fops structure to accel_open().
> +  Alternatively, your driver can use DEFINE_DRM_ACCEL_FOPS macro to easily
> +  set the correct function operations pointers structure.
> +
> +External References
> +===================
> +
> +email threads
> +-------------
> +
> +* `Initial discussion on the New subsystem for acceleration devices <https://lkml.org/lkml/2022/7/31/83>`_ - Oded Gabbay (2022)
> +* `patch-set to add the new subsystem <https://lkml.org/lkml/2022/10/22/544>`_ - Oded Gabbay (2022)
> +
> +Conference talks
> +----------------
> +
> +* `LPC 2022 Accelerators BOF outcomes summary <https://airlied.blogspot.com/2022/09/accelerators-bof-outcomes-summary.html>`_ - Dave Airlie (2022)
> diff --git a/Documentation/subsystem-apis.rst b/Documentation/subsystem-apis.rst
> index af65004a80aa..b51f38527e14 100644
> --- a/Documentation/subsystem-apis.rst
> +++ b/Documentation/subsystem-apis.rst
> @@ -43,6 +43,7 @@ needed).
>      input/index
>      hwmon/index
>      gpu/index
> +   accel/index
>      security/index
>      sound/index
>      crypto/index
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4d752aac3ec0..6ba7bb35208a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -6837,6 +6837,7 @@ 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:	Documentation/accel/
>   F:	drivers/accel/
>   
>   DRM DRIVERS FOR ALLWINNER A10

Re: [PATCH v4 0/4] new subsystem for compute accelerator devices

[Jeffrey Hugo]

On 11/19/2022 1:44 PM, Oded Gabbay wrote:
> This is the fourth (and hopefully last) version of the patch-set to add the
> new subsystem for compute accelerators. I removed the RFC headline as
> I believe it is now ready for merging.
> 
> Compare to v3, this patch-set contains one additional patch that adds
> documentation regarding the accel subsystem. I hope it's good enough for
> this stage. In addition, there were few very minor fixes according to
> comments received on v3.
> 
> The patches are in the following repo:
> https://git.kernel.org/pub/scm/linux/kernel/git/ogabbay/accel.git/log/?h=accel_v4
> 
> As in v3, 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.
> 
> 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/
> 
> v3 cover letter:
> https://lore.kernel.org/lkml/20221106210225.2065371-1-ogabbay@kernel.org/T/
> 
> Thanks,
> Oded.

Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>

I have some nits.  Nothing that I think should be a blocker for this series.

Re: [PATCH v4 0/4] new subsystem for compute accelerator devices

[Dave Airlie]

On Sun, 20 Nov 2022 at 06:44, Oded Gabbay <ogabbay@kernel.org> wrote:
>
> This is the fourth (and hopefully last) version of the patch-set to add the
> new subsystem for compute accelerators. I removed the RFC headline as
> I believe it is now ready for merging.
>
> Compare to v3, this patch-set contains one additional patch that adds
> documentation regarding the accel subsystem. I hope it's good enough for
> this stage. In addition, there were few very minor fixes according to
> comments received on v3.
>
> The patches are in the following repo:
> https://git.kernel.org/pub/scm/linux/kernel/git/ogabbay/accel.git/log/?h=accel_v4
>
> As in v3, 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.
>

FIx the nits Jeffery raised and the one I brought up and I think we
should be good for this to be in a PR.

Reviewed-by: Dave Airlie <airlied@redhat.com>

Re: [PATCH v4 0/4] new subsystem for compute accelerator devices

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2781 bytes --]


Acked-by: Thomas Zimmermann <tzimmermann@suse.de>

Am 19.11.22 um 21:44 schrieb Oded Gabbay:
> This is the fourth (and hopefully last) version of the patch-set to add the
> new subsystem for compute accelerators. I removed the RFC headline as
> I believe it is now ready for merging.
> 
> Compare to v3, this patch-set contains one additional patch that adds
> documentation regarding the accel subsystem. I hope it's good enough for
> this stage. In addition, there were few very minor fixes according to
> comments received on v3.
> 
> The patches are in the following repo:
> https://git.kernel.org/pub/scm/linux/kernel/git/ogabbay/accel.git/log/?h=accel_v4
> 
> As in v3, 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.
> 
> 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/
> 
> v3 cover letter:
> https://lore.kernel.org/lkml/20221106210225.2065371-1-ogabbay@kernel.org/T/
> 
> Thanks,
> Oded.
> 
> Oded Gabbay (4):
>    drivers/accel: define kconfig and register a new major
>    accel: add dedicated minor for accelerator devices
>    drm: initialize accel framework
>    doc: add documentation for accel subsystem
> 
>   Documentation/accel/index.rst         |  17 ++
>   Documentation/accel/introduction.rst  | 109 +++++++++
>   Documentation/admin-guide/devices.txt |   5 +
>   Documentation/subsystem-apis.rst      |   1 +
>   MAINTAINERS                           |   9 +
>   drivers/Kconfig                       |   2 +
>   drivers/accel/Kconfig                 |  24 ++
>   drivers/accel/drm_accel.c             | 323 ++++++++++++++++++++++++++
>   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 +-
>   16 files changed, 711 insertions(+), 37 deletions(-)
>   create mode 100644 Documentation/accel/index.rst
>   create mode 100644 Documentation/accel/introduction.rst
>   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
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v4 0/4] new subsystem for compute accelerator devices

[Oded Gabbay]

On Mon, Nov 21, 2022 at 8:26 AM Dave Airlie <airlied@gmail.com> wrote:
>
> On Sun, 20 Nov 2022 at 06:44, Oded Gabbay <ogabbay@kernel.org> wrote:
> >
> > This is the fourth (and hopefully last) version of the patch-set to add the
> > new subsystem for compute accelerators. I removed the RFC headline as
> > I believe it is now ready for merging.
> >
> > Compare to v3, this patch-set contains one additional patch that adds
> > documentation regarding the accel subsystem. I hope it's good enough for
> > this stage. In addition, there were few very minor fixes according to
> > comments received on v3.
> >
> > The patches are in the following repo:
> > https://git.kernel.org/pub/scm/linux/kernel/git/ogabbay/accel.git/log/?h=accel_v4
> >
> > As in v3, 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.
> >
>
> FIx the nits Jeffery raised and the one I brought up and I think we
> should be good for this to be in a PR.
>
> Reviewed-by: Dave Airlie <airlied@redhat.com>
Sure, np.
Oded

Re: [PATCH v4 2/4] accel: add dedicated minor for accelerator devices

[Oded Gabbay]

On Sun, Nov 20, 2022 at 11:47 PM Jeffrey Hugo <quic_jhugo@quicinc.com> wrote:
>
> On 11/19/2022 1:44 PM, Oded Gabbay wrote:
> > diff --git a/drivers/accel/drm_accel.c b/drivers/accel/drm_accel.c
> > index fac6ad6ac28e..703d40c4ff45 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>
>
> Including xarray, but using idr
> This should be linux/idr.h
>
> This seems so minor, I don't think I advise spinning a v5 for it.  If a
> v5 is warranted elsewhere, obviously fix this.  If not, hopefully this
> can be fixed up by whoever applies it, or someone submits a follow up patch.
>
> Hopefully this is the only nit.  I would like to see this merged.
>
> -Jeff
Thanks,
I'll update it before sending the PR.
Oded

Re: [PATCH v4 4/4] doc: add documentation for accel subsystem

[Oded Gabbay]

On Mon, Nov 21, 2022 at 12:02 AM Jeffrey Hugo <quic_jhugo@quicinc.com> wrote:
>
> On 11/19/2022 1:44 PM, Oded Gabbay wrote:
> > Add an introduction section for the accel subsystem. Most of the
> > relevant data is in the DRM documentation, so the introduction only
> > presents the why of the new subsystem, how are the compute accelerators
> > exposed to user-space and what changes need to be done in a standard
> > DRM driver to register it to the new accel subsystem.
> >
> > Signed-off-by: Oded Gabbay <ogabbay@kernel.org>
> > ---
> >   Documentation/accel/index.rst        |  17 +++++
> >   Documentation/accel/introduction.rst | 109 +++++++++++++++++++++++++++
> >   Documentation/subsystem-apis.rst     |   1 +
> >   MAINTAINERS                          |   1 +
> >   4 files changed, 128 insertions(+)
> >   create mode 100644 Documentation/accel/index.rst
> >   create mode 100644 Documentation/accel/introduction.rst
> >
> > diff --git a/Documentation/accel/index.rst b/Documentation/accel/index.rst
> > new file mode 100644
> > index 000000000000..2b43c9a7f67b
> > --- /dev/null
> > +++ b/Documentation/accel/index.rst
> > @@ -0,0 +1,17 @@
> > +.. SPDX-License-Identifier: GPL-2.0
> > +
> > +====================
> > +Compute Accelerators
> > +====================
> > +
> > +.. toctree::
> > +   :maxdepth: 1
> > +
> > +   introduction
> > +
> > +.. only::  subproject and html
> > +
> > +   Indices
> > +   =======
> > +
> > +   * :ref:`genindex`
> > diff --git a/Documentation/accel/introduction.rst b/Documentation/accel/introduction.rst
> > new file mode 100644
> > index 000000000000..5a3963eae973
> > --- /dev/null
> > +++ b/Documentation/accel/introduction.rst
> > @@ -0,0 +1,109 @@
> > +.. SPDX-License-Identifier: GPL-2.0
> > +
> > +============
> > +Introduction
> > +============
> > +
> > +The Linux compute accelerators subsystem is designed to expose compute
> > +accelerators in a common way to user-space and provide a common set of
> > +functionality.
> > +
> > +These devices can be either stand-alone ASICs or IP blocks inside an SoC/GPU.
> > +Although these devices are typically designed to accelerate Machine-Learning
> > +and/or Deep-Learning computations, the accel layer is not limited to handling
>
> You use "DL" later on as a short form for Deep-Learning.  It would be
> good to introduce that here.
>
> > +these types of accelerators.
> > +
> > +typically, a compute accelerator will belong to one of the following
>
> Typically
>
> > +categories:
> > +
> > +- Edge AI - doing inference at an edge device. It can be an embedded ASIC/FPGA,
> > +  or an IP inside a SoC (e.g. laptop web camera). These devices
> > +  are typically configured using registers and can work with or without DMA.
> > +
> > +- Inference data-center - single/multi user devices in a large server. This
> > +  type of device can be stand-alone or an IP inside a SoC or a GPU. It will
> > +  have on-board DRAM (to hold the DL topology), DMA engines and
> > +  command submission queues (either kernel or user-space queues).
> > +  It might also have an MMU to manage multiple users and might also enable
> > +  virtualization (SR-IOV) to support multiple VMs on the same device. In
> > +  addition, these devices will usually have some tools, such as profiler and
> > +  debugger.
> > +
> > +- Training data-center - Similar to Inference data-center cards, but typically
> > +  have more computational power and memory b/w (e.g. HBM) and will likely have
> > +  a method of scaling-up/out, i.e. connecting to other training cards inside
> > +  the server or in other servers, respectively.
> > +
> > +All these devices typically have different runtime user-space software stacks,
> > +that are tailored-made to their h/w. In addition, they will also probably
> > +include a compiler to generate programs to their custom-made computational
> > +engines. Typically, the common layer in user-space will be the DL frameworks,
> > +such as PyTorch and TensorFlow.
> > +
> > +Sharing code with DRM
> > +=====================
> > +
> > +Because this type of devices can be an IP inside GPUs or have similar
> > +characteristics as those of GPUs, the accel subsystem will use the
> > +DRM subsystem's code and functionality. i.e. the accel core code will
> > +be part of the DRM subsystem and an accel device will be a new type of DRM
> > +device.
> > +
> > +This will allow us to leverage the extensive DRM code-base and
> > +collaborate with DRM developers that have experience with this type of
> > +devices. In addition, new features that will be added for the accelerator
> > +drivers can be of use to GPU drivers as well.
> > +
> > +Differentiation from GPUs
> > +=========================
> > +
> > +Because we want to prevent the extensive user-space graphic software stack
> > +from trying to use an accelerator as a GPU, the compute accelerators will be
> > +differentiated from GPUs by using a new major number and new device char files.
> > +
> > +Furthermore, the drivers will be located in a separate place in the kernel
> > +tree - drivers/accel/.
> > +
> > +The accelerator devices will be exposed to the user space with the dedicated
> > +261 major number and will have the following convention:
> > +
> > +- device char files - /dev/accel/accel*
> > +- sysfs             - /sys/class/accel/accel*/
> > +- debugfs           - /sys/kernel/debug/accel/accel*/
> > +
> > +Getting Started
> > +===============
> > +
> > +First, read the DRM documentation. Not only it will explain how to write a new
>
> How about a link to the DRM documentation?
>
> > +DRM driver but it will also contain all the information on how to contribute,
> > +the Code Of Conduct and what is the coding style/documentation. All of that
> > +is the same for the accel subsystem.
> > +
> > +Second, make sure the kernel is configured with CONFIG_DRM_ACCEL.
> > +
> > +To expose your device as an accelerator, two changes are needed to
> > +be done in your driver (as opposed to a standard DRM driver):
> > +
> > +- Add the DRIVER_COMPUTE_ACCEL feature flag in your drm_driver's
> > +  driver_features field. It is important to note that this driver feature is
> > +  mutually exclusive with DRIVER_RENDER and DRIVER_MODESET. Devices that want
>
> I don't remember seeing code that validates a driver with
> DRIVER_COMPUTE_ACCEL does not also have DRIVER_MODESET.  What am I missing?

Look at drm_dev_init() (patch 3/4):

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;
}

Thanks for your other comments, I'll fix them before sending the PR.
Oded

>
> > +  to expose both graphics and compute device char files should be handled by
> > +  two drivers that are connected using the auxiliary bus framework.
> > +
> > +- Change the open callback in your driver fops structure to accel_open().
> > +  Alternatively, your driver can use DEFINE_DRM_ACCEL_FOPS macro to easily
> > +  set the correct function operations pointers structure.
> > +
> > +External References
> > +===================
> > +
> > +email threads
> > +-------------
> > +
> > +* `Initial discussion on the New subsystem for acceleration devices <https://lkml.org/lkml/2022/7/31/83>`_ - Oded Gabbay (2022)
> > +* `patch-set to add the new subsystem <https://lkml.org/lkml/2022/10/22/544>`_ - Oded Gabbay (2022)
> > +
> > +Conference talks
> > +----------------
> > +
> > +* `LPC 2022 Accelerators BOF outcomes summary <https://airlied.blogspot.com/2022/09/accelerators-bof-outcomes-summary.html>`_ - Dave Airlie (2022)
> > diff --git a/Documentation/subsystem-apis.rst b/Documentation/subsystem-apis.rst
> > index af65004a80aa..b51f38527e14 100644
> > --- a/Documentation/subsystem-apis.rst
> > +++ b/Documentation/subsystem-apis.rst
> > @@ -43,6 +43,7 @@ needed).
> >      input/index
> >      hwmon/index
> >      gpu/index
> > +   accel/index
> >      security/index
> >      sound/index
> >      crypto/index
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 4d752aac3ec0..6ba7bb35208a 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -6837,6 +6837,7 @@ 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:   Documentation/accel/
> >   F:  drivers/accel/
> >
> >   DRM DRIVERS FOR ALLWINNER A10
>

Re: [PATCH v4 4/4] doc: add documentation for accel subsystem

[Jeffrey Hugo]

On 11/21/2022 8:18 AM, Oded Gabbay wrote:
> On Mon, Nov 21, 2022 at 12:02 AM Jeffrey Hugo <quic_jhugo@quicinc.com> wrote:
>>
>> On 11/19/2022 1:44 PM, Oded Gabbay wrote:
>>> Add an introduction section for the accel subsystem. Most of the
>>> relevant data is in the DRM documentation, so the introduction only
>>> presents the why of the new subsystem, how are the compute accelerators
>>> exposed to user-space and what changes need to be done in a standard
>>> DRM driver to register it to the new accel subsystem.
>>>
>>> Signed-off-by: Oded Gabbay <ogabbay@kernel.org>
>>> ---
>>>    Documentation/accel/index.rst        |  17 +++++
>>>    Documentation/accel/introduction.rst | 109 +++++++++++++++++++++++++++
>>>    Documentation/subsystem-apis.rst     |   1 +
>>>    MAINTAINERS                          |   1 +
>>>    4 files changed, 128 insertions(+)
>>>    create mode 100644 Documentation/accel/index.rst
>>>    create mode 100644 Documentation/accel/introduction.rst
>>>
>>> diff --git a/Documentation/accel/index.rst b/Documentation/accel/index.rst
>>> new file mode 100644
>>> index 000000000000..2b43c9a7f67b
>>> --- /dev/null
>>> +++ b/Documentation/accel/index.rst
>>> @@ -0,0 +1,17 @@
>>> +.. SPDX-License-Identifier: GPL-2.0
>>> +
>>> +====================
>>> +Compute Accelerators
>>> +====================
>>> +
>>> +.. toctree::
>>> +   :maxdepth: 1
>>> +
>>> +   introduction
>>> +
>>> +.. only::  subproject and html
>>> +
>>> +   Indices
>>> +   =======
>>> +
>>> +   * :ref:`genindex`
>>> diff --git a/Documentation/accel/introduction.rst b/Documentation/accel/introduction.rst
>>> new file mode 100644
>>> index 000000000000..5a3963eae973
>>> --- /dev/null
>>> +++ b/Documentation/accel/introduction.rst
>>> @@ -0,0 +1,109 @@
>>> +.. SPDX-License-Identifier: GPL-2.0
>>> +
>>> +============
>>> +Introduction
>>> +============
>>> +
>>> +The Linux compute accelerators subsystem is designed to expose compute
>>> +accelerators in a common way to user-space and provide a common set of
>>> +functionality.
>>> +
>>> +These devices can be either stand-alone ASICs or IP blocks inside an SoC/GPU.
>>> +Although these devices are typically designed to accelerate Machine-Learning
>>> +and/or Deep-Learning computations, the accel layer is not limited to handling
>>
>> You use "DL" later on as a short form for Deep-Learning.  It would be
>> good to introduce that here.
>>
>>> +these types of accelerators.
>>> +
>>> +typically, a compute accelerator will belong to one of the following
>>
>> Typically
>>
>>> +categories:
>>> +
>>> +- Edge AI - doing inference at an edge device. It can be an embedded ASIC/FPGA,
>>> +  or an IP inside a SoC (e.g. laptop web camera). These devices
>>> +  are typically configured using registers and can work with or without DMA.
>>> +
>>> +- Inference data-center - single/multi user devices in a large server. This
>>> +  type of device can be stand-alone or an IP inside a SoC or a GPU. It will
>>> +  have on-board DRAM (to hold the DL topology), DMA engines and
>>> +  command submission queues (either kernel or user-space queues).
>>> +  It might also have an MMU to manage multiple users and might also enable
>>> +  virtualization (SR-IOV) to support multiple VMs on the same device. In
>>> +  addition, these devices will usually have some tools, such as profiler and
>>> +  debugger.
>>> +
>>> +- Training data-center - Similar to Inference data-center cards, but typically
>>> +  have more computational power and memory b/w (e.g. HBM) and will likely have
>>> +  a method of scaling-up/out, i.e. connecting to other training cards inside
>>> +  the server or in other servers, respectively.
>>> +
>>> +All these devices typically have different runtime user-space software stacks,
>>> +that are tailored-made to their h/w. In addition, they will also probably
>>> +include a compiler to generate programs to their custom-made computational
>>> +engines. Typically, the common layer in user-space will be the DL frameworks,
>>> +such as PyTorch and TensorFlow.
>>> +
>>> +Sharing code with DRM
>>> +=====================
>>> +
>>> +Because this type of devices can be an IP inside GPUs or have similar
>>> +characteristics as those of GPUs, the accel subsystem will use the
>>> +DRM subsystem's code and functionality. i.e. the accel core code will
>>> +be part of the DRM subsystem and an accel device will be a new type of DRM
>>> +device.
>>> +
>>> +This will allow us to leverage the extensive DRM code-base and
>>> +collaborate with DRM developers that have experience with this type of
>>> +devices. In addition, new features that will be added for the accelerator
>>> +drivers can be of use to GPU drivers as well.
>>> +
>>> +Differentiation from GPUs
>>> +=========================
>>> +
>>> +Because we want to prevent the extensive user-space graphic software stack
>>> +from trying to use an accelerator as a GPU, the compute accelerators will be
>>> +differentiated from GPUs by using a new major number and new device char files.
>>> +
>>> +Furthermore, the drivers will be located in a separate place in the kernel
>>> +tree - drivers/accel/.
>>> +
>>> +The accelerator devices will be exposed to the user space with the dedicated
>>> +261 major number and will have the following convention:
>>> +
>>> +- device char files - /dev/accel/accel*
>>> +- sysfs             - /sys/class/accel/accel*/
>>> +- debugfs           - /sys/kernel/debug/accel/accel*/
>>> +
>>> +Getting Started
>>> +===============
>>> +
>>> +First, read the DRM documentation. Not only it will explain how to write a new
>>
>> How about a link to the DRM documentation?
>>
>>> +DRM driver but it will also contain all the information on how to contribute,
>>> +the Code Of Conduct and what is the coding style/documentation. All of that
>>> +is the same for the accel subsystem.
>>> +
>>> +Second, make sure the kernel is configured with CONFIG_DRM_ACCEL.
>>> +
>>> +To expose your device as an accelerator, two changes are needed to
>>> +be done in your driver (as opposed to a standard DRM driver):
>>> +
>>> +- Add the DRIVER_COMPUTE_ACCEL feature flag in your drm_driver's
>>> +  driver_features field. It is important to note that this driver feature is
>>> +  mutually exclusive with DRIVER_RENDER and DRIVER_MODESET. Devices that want
>>
>> I don't remember seeing code that validates a driver with
>> DRIVER_COMPUTE_ACCEL does not also have DRIVER_MODESET.  What am I missing?
> 
> Look at drm_dev_init() (patch 3/4):
> 
> 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;
> }

Ah.  I saw "RENDER", but "MODESET" didn't register in my brain.  Thanks 
for pointing it out to me.  All good here.

Re: [PATCH v4 0/4] new subsystem for compute accelerator devices

[Alex Deucher]

On Sat, Nov 19, 2022 at 3:44 PM Oded Gabbay <ogabbay@kernel.org> wrote:
>
> This is the fourth (and hopefully last) version of the patch-set to add the
> new subsystem for compute accelerators. I removed the RFC headline as
> I believe it is now ready for merging.
>
> Compare to v3, this patch-set contains one additional patch that adds
> documentation regarding the accel subsystem. I hope it's good enough for
> this stage. In addition, there were few very minor fixes according to
> comments received on v3.
>
> The patches are in the following repo:
> https://git.kernel.org/pub/scm/linux/kernel/git/ogabbay/accel.git/log/?h=accel_v4
>
> As in v3, 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.
>
> 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/
>
> v3 cover letter:
> https://lore.kernel.org/lkml/20221106210225.2065371-1-ogabbay@kernel.org/T/
>

With the understanding that individual drivers can choose to use
either classic drm or accel, whichever makes the most sense to them,
this series is:
Acked-by: Alex Deucher <alexander.deucer@amd.com>

> Thanks,
> Oded.
>
> Oded Gabbay (4):
>   drivers/accel: define kconfig and register a new major
>   accel: add dedicated minor for accelerator devices
>   drm: initialize accel framework
>   doc: add documentation for accel subsystem
>
>  Documentation/accel/index.rst         |  17 ++
>  Documentation/accel/introduction.rst  | 109 +++++++++
>  Documentation/admin-guide/devices.txt |   5 +
>  Documentation/subsystem-apis.rst      |   1 +
>  MAINTAINERS                           |   9 +
>  drivers/Kconfig                       |   2 +
>  drivers/accel/Kconfig                 |  24 ++
>  drivers/accel/drm_accel.c             | 323 ++++++++++++++++++++++++++
>  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 +-
>  16 files changed, 711 insertions(+), 37 deletions(-)
>  create mode 100644 Documentation/accel/index.rst
>  create mode 100644 Documentation/accel/introduction.rst
>  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: [PATCH v4 0/4] new subsystem for compute accelerator devices

[Alex Deucher]

On Mon, Nov 21, 2022 at 10:57 AM Alex Deucher <alexdeucher@gmail.com> wrote:
>
> On Sat, Nov 19, 2022 at 3:44 PM Oded Gabbay <ogabbay@kernel.org> wrote:
> >
> > This is the fourth (and hopefully last) version of the patch-set to add the
> > new subsystem for compute accelerators. I removed the RFC headline as
> > I believe it is now ready for merging.
> >
> > Compare to v3, this patch-set contains one additional patch that adds
> > documentation regarding the accel subsystem. I hope it's good enough for
> > this stage. In addition, there were few very minor fixes according to
> > comments received on v3.
> >
> > The patches are in the following repo:
> > https://git.kernel.org/pub/scm/linux/kernel/git/ogabbay/accel.git/log/?h=accel_v4
> >
> > As in v3, 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.
> >
> > 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/
> >
> > v3 cover letter:
> > https://lore.kernel.org/lkml/20221106210225.2065371-1-ogabbay@kernel.org/T/
> >
>
> With the understanding that individual drivers can choose to use
> either classic drm or accel, whichever makes the most sense to them,
> this series is:
> Acked-by: Alex Deucher <alexander.deucer@amd.com>

and not typo my email:
Acked-by: Alex Deucher <alexander.deucher@amd.com>

>
> > Thanks,
> > Oded.
> >
> > Oded Gabbay (4):
> >   drivers/accel: define kconfig and register a new major
> >   accel: add dedicated minor for accelerator devices
> >   drm: initialize accel framework
> >   doc: add documentation for accel subsystem
> >
> >  Documentation/accel/index.rst         |  17 ++
> >  Documentation/accel/introduction.rst  | 109 +++++++++
> >  Documentation/admin-guide/devices.txt |   5 +
> >  Documentation/subsystem-apis.rst      |   1 +
> >  MAINTAINERS                           |   9 +
> >  drivers/Kconfig                       |   2 +
> >  drivers/accel/Kconfig                 |  24 ++
> >  drivers/accel/drm_accel.c             | 323 ++++++++++++++++++++++++++
> >  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 +-
> >  16 files changed, 711 insertions(+), 37 deletions(-)
> >  create mode 100644 Documentation/accel/index.rst
> >  create mode 100644 Documentation/accel/introduction.rst
> >  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: [PATCH v4 0/4] new subsystem for compute accelerator devices

[Sonal Santan]

On 11/19/22 12:44, Oded Gabbay wrote:
> This is the fourth (and hopefully last) version of the patch-set to add the
> new subsystem for compute accelerators. I removed the RFC headline as
> I believe it is now ready for merging.
> 
> Compare to v3, this patch-set contains one additional patch that adds
> documentation regarding the accel subsystem. I hope it's good enough for
> this stage. In addition, there were few very minor fixes according to
> comments received on v3.
> 
> The patches are in the following repo:
> https://git.kernel.org/pub/scm/linux/kernel/git/ogabbay/accel.git/log/?h=accel_v4
> 
> As in v3, 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.
> 
> 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/
> 
> v3 cover letter:
> https://lore.kernel.org/lkml/20221106210225.2065371-1-ogabbay@kernel.org/T/

Thanks for defining the new accel subsystem. We are currently working on
DRM based drivers for unannounced acceleration devices. I am fine with
these changes with the assumption that the choice of using classic DRM
or accel is left up to the individual driver.

-Sonal

> 
> Thanks,
> Oded.
> 
> Oded Gabbay (4):
>   drivers/accel: define kconfig and register a new major
>   accel: add dedicated minor for accelerator devices
>   drm: initialize accel framework
>   doc: add documentation for accel subsystem
> 
>  Documentation/accel/index.rst         |  17 ++
>  Documentation/accel/introduction.rst  | 109 +++++++++
>  Documentation/admin-guide/devices.txt |   5 +
>  Documentation/subsystem-apis.rst      |   1 +
>  MAINTAINERS                           |   9 +
>  drivers/Kconfig                       |   2 +
>  drivers/accel/Kconfig                 |  24 ++
>  drivers/accel/drm_accel.c             | 323 ++++++++++++++++++++++++++
>  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 +-
>  16 files changed, 711 insertions(+), 37 deletions(-)
>  create mode 100644 Documentation/accel/index.rst
>  create mode 100644 Documentation/accel/introduction.rst
>  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: [PATCH v4 0/4] new subsystem for compute accelerator devices

[Dave Airlie]

On Tue, 22 Nov 2022 at 09:06, Sonal Santan <sonal.santan@amd.com> wrote:
>
> On 11/19/22 12:44, Oded Gabbay wrote:
> > This is the fourth (and hopefully last) version of the patch-set to add the
> > new subsystem for compute accelerators. I removed the RFC headline as
> > I believe it is now ready for merging.
> >
> > Compare to v3, this patch-set contains one additional patch that adds
> > documentation regarding the accel subsystem. I hope it's good enough for
> > this stage. In addition, there were few very minor fixes according to
> > comments received on v3.
> >
> > The patches are in the following repo:
> > https://git.kernel.org/pub/scm/linux/kernel/git/ogabbay/accel.git/log/?h=accel_v4
> >
> > As in v3, 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.
> >
> > 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/
> >
> > v3 cover letter:
> > https://lore.kernel.org/lkml/20221106210225.2065371-1-ogabbay@kernel.org/T/
>
> Thanks for defining the new accel subsystem. We are currently working on
> DRM based drivers for unannounced acceleration devices. I am fine with
> these changes with the assumption that the choice of using classic DRM
> or accel is left up to the individual driver.

I don't think that decision should be up to any individual driver
author. It will have to be consensus with me/Daniel/Oded and the
driver authors.

Dave.

Re: [PATCH v4 0/4] new subsystem for compute accelerator devices

[Jacek Lawrynowicz]

Hi,

On 11/19/2022 9:44 PM, Oded Gabbay wrote:
> This is the fourth (and hopefully last) version of the patch-set to add the
> new subsystem for compute accelerators. I removed the RFC headline as
> I believe it is now ready for merging.

Looks good and works without issues.
I will rebase next version of Intel VPU patchest on top of this.

Acked-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com>
Tested-by: Jacek Lawrynowicz <jacek.lawrynowicz@linux.intel.com> 

Regards,
Jacek

Re: [PATCH v4 3/4] drm: initialize accel framework

[Melissa Wen]

[-- Attachment #1: Type: text/plain, Size: 9407 bytes --]

On 11/19, Oded Gabbay wrote:
> 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.

Hi Oded,

The proposal overall LGTM, but I didn't manage to compile the kernel
with your patch series when DRM is enabled but accel support doesn't.
Multiple missing link errors...

Am I missing something? Can you take a look at it from this patch 3/4?

Thanks,

Melissa
> 
> Signed-off-by: Oded Gabbay <ogabbay@kernel.org>
> ---
>  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
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 3/4] drm: initialize accel framework

[Oded Gabbay]

On Tue, Nov 22, 2022 at 12:56 PM Melissa Wen <mwen@igalia.com> wrote:
>
> On 11/19, Oded Gabbay wrote:
> > 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.
>
> Hi Oded,
>
> The proposal overall LGTM, but I didn't manage to compile the kernel
> with your patch series when DRM is enabled but accel support doesn't.
> Multiple missing link errors...
>
> Am I missing something? Can you take a look at it from this patch 3/4?
>
> Thanks,
>
> Melissa
Looking at it now, thanks for letting me know.
Oded

> >
> > Signed-off-by: Oded Gabbay <ogabbay@kernel.org>
> > ---
> >  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: [PATCH v4 3/4] drm: initialize accel framework

[Oded Gabbay]

On Tue, Nov 22, 2022 at 12:59 PM Oded Gabbay <ogabbay@kernel.org> wrote:
>
> On Tue, Nov 22, 2022 at 12:56 PM Melissa Wen <mwen@igalia.com> wrote:
> >
> > On 11/19, Oded Gabbay wrote:
> > > 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.
> >
> > Hi Oded,
> >
> > The proposal overall LGTM, but I didn't manage to compile the kernel
> > with your patch series when DRM is enabled but accel support doesn't.
> > Multiple missing link errors...
> >
> > Am I missing something? Can you take a look at it from this patch 3/4?
> >
> > Thanks,
> >
> > Melissa
> Looking at it now, thanks for letting me know.
> Oded
Found the issue, missing } at end of accel_debugfs_init() in
drm_accel.h. Only compiles when accel support isn't compiled in.
I'll fix it before sending the PR to Dave.
Much appreciated :)
Oded

>
> > >
> > > Signed-off-by: Oded Gabbay <ogabbay@kernel.org>
> > > ---
> > >  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: [PATCH v4 3/4] drm: initialize accel framework

[Melissa Wen]

[-- Attachment #1: Type: text/plain, Size: 13137 bytes --]

11/22, Oded Gabbay wrote:
> On Tue, Nov 22, 2022 at 12:59 PM Oded Gabbay <ogabbay@kernel.org> wrote:
> >
> > On Tue, Nov 22, 2022 at 12:56 PM Melissa Wen <mwen@igalia.com> wrote:
> > >
> > > On 11/19, Oded Gabbay wrote:
> > > > 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.
> > >
> > > Hi Oded,
> > >
> > > The proposal overall LGTM, but I didn't manage to compile the kernel
> > > with your patch series when DRM is enabled but accel support doesn't.
> > > Multiple missing link errors...
> > >
> > > Am I missing something? Can you take a look at it from this patch 3/4?
> > >
> > > Thanks,
> > >
> > > Melissa
> > Looking at it now, thanks for letting me know.
> > Oded
> Found the issue, missing } at end of accel_debugfs_init() in
> drm_accel.h. Only compiles when accel support isn't compiled in.
> I'll fix it before sending the PR to Dave.
> Much appreciated :)
> Oded

Oh, great! Just checked here and everything is fine now.
With that, you can add my r-b for the entire series too

Reviewed-by: Melissa Wen <mwen@igalia.com>

Thanks for working on it,

Melissa

> 
> >
> > > >
> > > > Signed-off-by: Oded Gabbay <ogabbay@kernel.org>
> > > > ---
> > > >  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
> > > >

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v4 0/4] new subsystem for compute accelerator devices

[Daniel Vetter]

On Tue, Nov 22, 2022 at 03:46:25PM +1000, Dave Airlie wrote:
> On Tue, 22 Nov 2022 at 09:06, Sonal Santan <sonal.santan@amd.com> wrote:
> >
> > On 11/19/22 12:44, Oded Gabbay wrote:
> > > This is the fourth (and hopefully last) version of the patch-set to add the
> > > new subsystem for compute accelerators. I removed the RFC headline as
> > > I believe it is now ready for merging.
> > >
> > > Compare to v3, this patch-set contains one additional patch that adds
> > > documentation regarding the accel subsystem. I hope it's good enough for
> > > this stage. In addition, there were few very minor fixes according to
> > > comments received on v3.
> > >
> > > The patches are in the following repo:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/ogabbay/accel.git/log/?h=accel_v4
> > >
> > > As in v3, 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.
> > >
> > > 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/
> > >
> > > v3 cover letter:
> > > https://lore.kernel.org/lkml/20221106210225.2065371-1-ogabbay@kernel.org/T/
> >
> > Thanks for defining the new accel subsystem. We are currently working on
> > DRM based drivers for unannounced acceleration devices. I am fine with
> > these changes with the assumption that the choice of using classic DRM
> > or accel is left up to the individual driver.
> 
> I don't think that decision should be up to any individual driver
> author. It will have to be consensus with me/Daniel/Oded and the
> driver authors.

Plus the entire point of this is that it's _still_ a drm based driver. So
aside from changing a flag in the kernel driver and adjusting userspace to
find the right chardev, there should be zero changes need for an existing
drm based driver stack that gets ported to drivers/accel.

And of course if we realize there's issues as we add drivers, we can fix
things up. This is just to kick things off, not something that's going to
be cast in stone for all eternity.

Sonal, with that clarification/explanation, is this entire thing
reasonable in principal and you can drop an Ack onto the series?

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v4 0/4] new subsystem for compute accelerator devices

[Jeffrey Hugo]

On 11/20/2022 3:04 PM, Jeffrey Hugo wrote:
> On 11/19/2022 1:44 PM, Oded Gabbay wrote:
>> This is the fourth (and hopefully last) version of the patch-set to 
>> add the
>> new subsystem for compute accelerators. I removed the RFC headline as
>> I believe it is now ready for merging.
>>
>> Compare to v3, this patch-set contains one additional patch that adds
>> documentation regarding the accel subsystem. I hope it's good enough for
>> this stage. In addition, there were few very minor fixes according to
>> comments received on v3.
>>
>> The patches are in the following repo:
>> https://git.kernel.org/pub/scm/linux/kernel/git/ogabbay/accel.git/log/?h=accel_v4 
>>
>>
>> As in v3, 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.
>>
>> 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/ 
>>
>>
>> v3 cover letter:
>> https://lore.kernel.org/lkml/20221106210225.2065371-1-ogabbay@kernel.org/T/ 
>>
>>
>> Thanks,
>> Oded.
> 
> Reviewed-by: Jeffrey Hugo <quic_jhugo@quicinc.com>
> 
> I have some nits.  Nothing that I think should be a blocker for this 
> series.

I don't recall if I previously mentioned this.  I'm planning on updating 
the QAIC series to be an accel driver.  Therefore there should be 
at-least 1 user (probably several) for this subsystem in short order.

Re: [PATCH v4 0/4] new subsystem for compute accelerator devices

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 705 bytes --]

Hi,

On Sat, Nov 19, 2022 at 10:44:31PM +0200, Oded Gabbay wrote:
> This is the fourth (and hopefully last) version of the patch-set to add the
> new subsystem for compute accelerators. I removed the RFC headline as
> I believe it is now ready for merging.
> 
> Compare to v3, this patch-set contains one additional patch that adds
> documentation regarding the accel subsystem. I hope it's good enough for
> this stage. In addition, there were few very minor fixes according to
> comments received on v3.
> 
> The patches are in the following repo:
> https://git.kernel.org/pub/scm/linux/kernel/git/ogabbay/accel.git/log/?h=accel_v4

Acked-by: Maxime Ripard <maxime@cerno.tech>

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v4 0/4] new subsystem for compute accelerator devices

[Sonal Santan]

On 11/22/22 06:54, Daniel Vetter wrote:
> On Tue, Nov 22, 2022 at 03:46:25PM +1000, Dave Airlie wrote:
>> On Tue, 22 Nov 2022 at 09:06, Sonal Santan <sonal.santan@amd.com> wrote:
>>>
>>> On 11/19/22 12:44, Oded Gabbay wrote:
>>>> This is the fourth (and hopefully last) version of the patch-set to add the
>>>> new subsystem for compute accelerators. I removed the RFC headline as
>>>> I believe it is now ready for merging.
>>>>
>>>> Compare to v3, this patch-set contains one additional patch that adds
>>>> documentation regarding the accel subsystem. I hope it's good enough for
>>>> this stage. In addition, there were few very minor fixes according to
>>>> comments received on v3.
>>>>
>>>> The patches are in the following repo:
>>>> https://git.kernel.org/pub/scm/linux/kernel/git/ogabbay/accel.git/log/?h=accel_v4
>>>>
>>>> As in v3, 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.
>>>>
>>>> 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/
>>>>
>>>> v3 cover letter:
>>>> https://lore.kernel.org/lkml/20221106210225.2065371-1-ogabbay@kernel.org/T/
>>>
>>> Thanks for defining the new accel subsystem. We are currently working on
>>> DRM based drivers for unannounced acceleration devices. I am fine with
>>> these changes with the assumption that the choice of using classic DRM
>>> or accel is left up to the individual driver.
>>
>> I don't think that decision should be up to any individual driver
>> author. It will have to be consensus with me/Daniel/Oded and the
>> driver authors.
> 
> Plus the entire point of this is that it's _still_ a drm based driver. So
> aside from changing a flag in the kernel driver and adjusting userspace to
> find the right chardev, there should be zero changes need for an existing
> drm based driver stack that gets ported to drivers/accel.
> 
> And of course if we realize there's issues as we add drivers, we can fix
> things up. This is just to kick things off, not something that's going to
> be cast in stone for all eternity.
> 
> Sonal, with that clarification/explanation, is this entire thing
> reasonable in principal and you can drop an Ack onto the series?
> 
> Thanks, Daniel


Sounds good. The accel patch series is:
Acked-by: Sonal Santan <sonal.santan@amd.com>

-Sonal

Re: [PATCH v4 0/4] new subsystem for compute accelerator devices

[Daniel Stone]

Hi Oded,

On Sat, 19 Nov 2022 at 20:44, Oded Gabbay <ogabbay@kernel.org> wrote:
> This is the fourth (and hopefully last) version of the patch-set to add the
> new subsystem for compute accelerators. I removed the RFC headline as
> I believe it is now ready for merging.
>
> Compare to v3, this patch-set contains one additional patch that adds
> documentation regarding the accel subsystem. I hope it's good enough for
> this stage. In addition, there were few very minor fixes according to
> comments received on v3.
>
> The patches are in the following repo:
> https://git.kernel.org/pub/scm/linux/kernel/git/ogabbay/accel.git/log/?h=accel_v4

Series is:
Acked-by: Daniel Stone <daniels@collabora.com>

Cheers,
Daniel