Re: [RFC PATCH 2/3] drm: define new accel major and register it

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 10/24/2022 1:52 AM, Dave Airlie wrote:
On Mon, 24 Oct 2022 at 17:23, Oded Gabbay <ogabbay@xxxxxxxxxx> wrote:

On Sun, Oct 23, 2022 at 3:40 PM Greg Kroah-Hartman
<gregkh@xxxxxxxxxxxxxxxxxxx> wrote:

On Sun, Oct 23, 2022 at 12:46:21AM +0300, Oded Gabbay wrote:
The accelerator devices will be exposed to the user space with a new,
dedicated major number - 261.

The drm core registers the new major number as a char device and create
corresponding sysfs and debugfs root entries, same as for the drm major.

In case CONFIG_ACCEL is not selected, this code is not compiled in.

Signed-off-by: Oded Gabbay <ogabbay@xxxxxxxxxx>
---
  Documentation/admin-guide/devices.txt |  5 +++
  drivers/gpu/drm/drm_drv.c             | 45 +++++++++++++++++++++++
  drivers/gpu/drm/drm_internal.h        |  3 ++
  drivers/gpu/drm/drm_sysfs.c           | 52 +++++++++++++++++++++++++++
  include/drm/drm_ioctl.h               |  1 +
  5 files changed, 106 insertions(+)

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/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 8214a0b1ab7f..b58ffb1433d6 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -67,6 +67,10 @@ static bool drm_core_init_complete;

  static struct dentry *drm_debugfs_root;

+#ifdef CONFIG_ACCEL
+static struct dentry *accel_debugfs_root;
+#endif
+
  DEFINE_STATIC_SRCU(drm_unplug_srcu);

  /*
@@ -1031,9 +1035,19 @@ static const struct file_operations drm_stub_fops = {
       .llseek = noop_llseek,
  };

+static void accel_core_exit(void)
+{
+#ifdef CONFIG_ACCEL
+     unregister_chrdev(ACCEL_MAJOR, "accel");
+     debugfs_remove(accel_debugfs_root);
+     accel_sysfs_destroy();
+#endif
+}

Why is all of this in drm_drv.c?

Why not put it in drm/accel/accel.c or something like that?  Then put
the proper stuff into a .h file and then you have no #ifdef in the .c
files.
I thought about that, adding an accel.c in drivers/accel/ and putting
this code there.
Eventually I thought that for two functions it's not worth it, but I
guess that in addition to the reason you gave, one can argue that
there will probably be more code in that file anyway, so why not open
it now.
I'll change this if no one else thinks otherwise.

Seems like a good idea to start doing it now, might make things easier
to keep separated.

I agree. I was a bit confused going through this patch, and envisioning how an accel driver would use the interface. I think an accel_internal.h would be clearer.

-Jeff



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux