[RFC,2/3] drm: define new accel major and register it
Commit Message
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@kernel.org>
---
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(+)
Comments
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@kernel.org>
> ---
> 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.
Keeping #ifdef out of C files is key, please do not do things like you
have here. Especially as it ends up with this kind of mess:
> +static int __init accel_core_init(void)
> +{
> +#ifdef CONFIG_ACCEL
> + 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", &drm_stub_fops);
> + if (ret < 0)
> + goto error;
> +
> +error:
> + /* Any cleanup will be done in drm_core_exit() that will call
> + * to accel_core_exit()
> + */
> + return ret;
> +#else
> + return 0;
> +#endif
> +}
That's just a mess.
thanks,
greg k-h
On Sun, Oct 23, 2022 at 3:40 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> 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@kernel.org>
> > ---
> > 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.
Oded
>
> Keeping #ifdef out of C files is key, please do not do things like you
> have here. Especially as it ends up with this kind of mess:
>
> > +static int __init accel_core_init(void)
> > +{
> > +#ifdef CONFIG_ACCEL
> > + 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", &drm_stub_fops);
> > + if (ret < 0)
> > + goto error;
> > +
> > +error:
> > + /* Any cleanup will be done in drm_core_exit() that will call
> > + * to accel_core_exit()
> > + */
> > + return ret;
> > +#else
> > + return 0;
> > +#endif
> > +}
>
>
> That's just a mess.
>
> thanks,
>
> greg k-h
On Mon, 24 Oct 2022 at 17:23, Oded Gabbay <ogabbay@kernel.org> wrote:
>
> On Sun, Oct 23, 2022 at 3:40 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> 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@kernel.org>
> > > ---
> > > 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.
Dave.
On 10/24/2022 1:52 AM, Dave Airlie wrote:
> On Mon, 24 Oct 2022 at 17:23, Oded Gabbay <ogabbay@kernel.org> wrote:
>>
>> On Sun, Oct 23, 2022 at 3:40 PM Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> 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@kernel.org>
>>>> ---
>>>> 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
@@ -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,
@@ -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
+}
+
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();
@@ -1041,6 +1055,33 @@ static void drm_core_exit(void)
drm_connector_ida_destroy();
}
+static int __init accel_core_init(void)
+{
+#ifdef CONFIG_ACCEL
+ 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", &drm_stub_fops);
+ if (ret < 0)
+ goto error;
+
+error:
+ /* Any cleanup will be done in drm_core_exit() that will call
+ * to accel_core_exit()
+ */
+ return ret;
+#else
+ return 0;
+#endif
+}
+
static int __init drm_core_init(void)
{
int ret;
@@ -1061,6 +1102,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;
@@ -147,9 +147,12 @@ void drm_master_internal_release(struct drm_device *dev);
/* drm_sysfs.c */
extern struct class *drm_class;
+extern struct class *accel_class;
int drm_sysfs_init(void);
void drm_sysfs_destroy(void);
+int accel_sysfs_init(void);
+void accel_sysfs_destroy(void);
struct device *drm_sysfs_minor_alloc(struct drm_minor *minor);
int drm_sysfs_connector_add(struct drm_connector *connector);
void drm_sysfs_connector_remove(struct drm_connector *connector);
@@ -56,6 +56,7 @@ static struct device_type drm_sysfs_device_connector = {
};
struct class *drm_class;
+struct class *accel_class;
#ifdef CONFIG_ACPI
static bool drm_connector_acpi_bus_match(struct device *dev)
@@ -148,6 +149,57 @@ static void drm_sysfs_release(struct device *dev)
kfree(dev);
}
+static char *accel_devnode(struct device *dev, umode_t *mode)
+{
+ return kasprintf(GFP_KERNEL, "accel/%s", dev_name(dev));
+}
+
+static CLASS_ATTR_STRING(accel_version, 0444, "accel 1.0.0 20221018");
+
+/**
+ * accel_sysfs_init - initialize sysfs helpers
+ *
+ * This is used to create the ACCEL class, which is the implicit parent of any
+ * other top-level ACCEL sysfs objects.
+ *
+ * You must call accel_sysfs_destroy() to release the allocated resources.
+ *
+ * Return: 0 on success, negative error code on failure.
+ */
+int accel_sysfs_init(void)
+{
+ int err;
+
+ accel_class = class_create(THIS_MODULE, "accel");
+ if (IS_ERR(accel_class))
+ return PTR_ERR(accel_class);
+
+ err = class_create_file(accel_class, &class_attr_accel_version.attr);
+ if (err) {
+ class_destroy(accel_class);
+ accel_class = NULL;
+ return err;
+ }
+
+ accel_class->devnode = accel_devnode;
+
+ return 0;
+}
+
+/**
+ * accel_sysfs_destroy - destroys ACCEL class
+ *
+ * Destroy the ACCEL device class.
+ */
+void accel_sysfs_destroy(void)
+{
+ if (IS_ERR_OR_NULL(accel_class))
+ return;
+ class_remove_file(accel_class, &class_attr_accel_version.attr);
+ class_destroy(accel_class);
+ accel_class = NULL;
+}
+
/*
* Connector properties
*/
@@ -70,6 +70,7 @@ typedef int drm_ioctl_compat_t(struct file *filp, unsigned int cmd,
#define DRM_IOCTL_NR(n) _IOC_NR(n)
#define DRM_IOCTL_TYPE(n) _IOC_TYPE(n)
#define DRM_MAJOR 226
+#define ACCEL_MAJOR 261
/**
* enum drm_ioctl_flags - DRM ioctl flags