Re: [RFC PATCH v2 2/3] accel: add dedicated minor for accelerator devices
On Sun, Nov 6, 2022 at 12:54 PM Oded Gabbay wrote:
>
> On Thu, Nov 3, 2022 at 7:26 AM Jiho Chu wrote:
> >
> > On Wed, 2 Nov 2022 22:34:04 +0200
> > Oded Gabbay wrote:
> >
> > > +/**
> > > + * accel_open - open method for ACCEL file
> > > + * @inode: device inode
> > > + * @filp: file pointer.
> > > + *
> > > + * This function must be used by drivers as their _operations.open
> > > method.
> > > + * It looks up the correct ACCEL device and instantiates all the per-file
> > > + * resources for it. It also calls the _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(>open_count);
> > > +
> >
> > Hi,
> > It needs to consider drm_global_mutex to access open_count.
> > please check doxy of open_count.
> Now that I'm changing the code back to be part of drm.ko, I can return
> all the code that is in drm_copy which I removed for this to compile.
I take it back. All the code that I omitted was for legacy drivers.
If you look inside drm_dev_needs_global_mutex(), you will see 3 cases
where you need to take the global mutex, and all 3 are only relevant
for legacy drivers and/or drivers that use deprecated features.
So, I disagree with your original comment here.
Moreover, open_count is atomic, so I don't need to take the mutex to
increment it, and as you can see in drm_open(), the function
increments it regardless of whether it takes
drm_dev_needs_global_mutex.
Oded
>
> >
> >
> > > + /* 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(>open_count);
> > > + accel_minor_release(minor);
> > > + return retcode;
> > > +}
> > > +EXPORT_SYMBOL_GPL(accel_open);
> > > +
> > > static int accel_stub_open(struct inode *inode, struct file *filp)
> > > {
> > > - DRM_DEBUG("Operation not supported");
> > > + const struct file_operations *new_fops;
> > > + struct drm_minor *minor;
> > > + int err;
> > > +
> > > + DRM_DEBUG("\n");
> >
> > It seems useless.
> Correct, I removed it in v3.
> Thanks,
> Oded
> >
> > Thanks.
> > Jiho Chu
Re: [RFC PATCH v2 2/3] accel: add dedicated minor for accelerator devices
On Thu, Nov 3, 2022 at 7:26 AM Jiho Chu wrote:
>
> On Wed, 2 Nov 2022 22:34:04 +0200
> Oded Gabbay wrote:
>
> > +/**
> > + * accel_open - open method for ACCEL file
> > + * @inode: device inode
> > + * @filp: file pointer.
> > + *
> > + * This function must be used by drivers as their _operations.open
> > method.
> > + * It looks up the correct ACCEL device and instantiates all the per-file
> > + * resources for it. It also calls the _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(>open_count);
> > +
>
> Hi,
> It needs to consider drm_global_mutex to access open_count.
> please check doxy of open_count.
Now that I'm changing the code back to be part of drm.ko, I can return
all the code that is in drm_copy which I removed for this to compile.
>
>
> > + /* 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(>open_count);
> > + accel_minor_release(minor);
> > + return retcode;
> > +}
> > +EXPORT_SYMBOL_GPL(accel_open);
> > +
> > static int accel_stub_open(struct inode *inode, struct file *filp)
> > {
> > - DRM_DEBUG("Operation not supported");
> > + const struct file_operations *new_fops;
> > + struct drm_minor *minor;
> > + int err;
> > +
> > + DRM_DEBUG("\n");
>
> It seems useless.
Correct, I removed it in v3.
Thanks,
Oded
>
> Thanks.
> Jiho Chu
Re: [RFC PATCH v2 2/3] accel: add dedicated minor for accelerator devices
On Wed, Nov 2, 2022 at 11:17 PM Jeffrey Hugo wrote:
>
> On 11/2/2022 2:34 PM, Oded Gabbay wrote:
> > @@ -24,16 +33,6 @@ static char *accel_devnode(struct device *dev, umode_t
> > *mode)
> >
> > 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.
> > - */
>
> Why are we removing this?
It should have been removed at the first patch, and will be fixed in v3.
I'm removing it as it is a static function. We don't document every
static function.
>
> > static int accel_sysfs_init(void)
> > {
> > int err;
> > @@ -54,11 +53,6 @@ static int accel_sysfs_init(void)
> > return 0;
> > }
> >
> > -/**
> > - * accel_sysfs_destroy - destroys ACCEL class
> > - *
> > - * Destroy the ACCEL device class.
> > - */
>
> Again, why remove this? Adding it in one patch than immediately
> removing it in the next patch seems wasteful.
Correct, will be removed from the first patch in the next version.
>
> > static void accel_sysfs_destroy(void)
> > {
> > if (IS_ERR_OR_NULL(accel_class))
> > @@ -68,11 +62,185 @@ static void accel_sysfs_destroy(void)
> > accel_class = NULL;
> > }
> >
> > +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 _operations.open
> > method.
>
> Feels like it would be helpful to have an accel version of
> DEFINE_DRM_GEM_FOPS() which helps accel drivers to get this right
Yeah, I also thought about it. I'll add it.
thanks,
oded
>
> > + * It looks up the correct ACCEL device and instantiates all the per-file
> > + * resources for it. It also calls the _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(>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(>open_count);
> > + accel_minor_release(minor);
> > + return retcode;
> > +}
> > +EXPORT_SYMBOL_GPL(accel_open);
Re: [RFC PATCH v2 2/3] accel: add dedicated minor for accelerator devices
On Wed, 2 Nov 2022 22:34:04 +0200
Oded Gabbay wrote:
> +/**
> + * accel_open - open method for ACCEL file
> + * @inode: device inode
> + * @filp: file pointer.
> + *
> + * This function must be used by drivers as their _operations.open
> method.
> + * It looks up the correct ACCEL device and instantiates all the per-file
> + * resources for it. It also calls the _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(>open_count);
> +
Hi,
It needs to consider drm_global_mutex to access open_count.
please check doxy of 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(>open_count);
> + accel_minor_release(minor);
> + return retcode;
> +}
> +EXPORT_SYMBOL_GPL(accel_open);
> +
> static int accel_stub_open(struct inode *inode, struct file *filp)
> {
> - DRM_DEBUG("Operation not supported");
> + const struct file_operations *new_fops;
> + struct drm_minor *minor;
> + int err;
> +
> + DRM_DEBUG("\n");
It seems useless.
Thanks.
Jiho Chu
Re: [RFC PATCH v2 2/3] accel: add dedicated minor for accelerator devices
On 11/2/2022 2:34 PM, Oded Gabbay wrote:
@@ -24,16 +33,6 @@ static char *accel_devnode(struct device *dev, umode_t *mode)
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.
- */
Why are we removing this?
static int accel_sysfs_init(void)
{
int err;
@@ -54,11 +53,6 @@ static int accel_sysfs_init(void)
return 0;
}
-/**
- * accel_sysfs_destroy - destroys ACCEL class
- *
- * Destroy the ACCEL device class.
- */
Again, why remove this? Adding it in one patch than immediately
removing it in the next patch seems wasteful.
static void accel_sysfs_destroy(void)
{
if (IS_ERR_OR_NULL(accel_class))
@@ -68,11 +62,185 @@ static void accel_sysfs_destroy(void)
accel_class = NULL;
}
+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 _operations.open method.
Feels like it would be helpful to have an accel version of
DEFINE_DRM_GEM_FOPS() which helps accel drivers to get this right
+ * It looks up the correct ACCEL device and instantiates all the per-file
+ * resources for it. It also calls the _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(>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(>open_count);
+ accel_minor_release(minor);
+ return retcode;
+}
+EXPORT_SYMBOL_GPL(accel_open);
[RFC PATCH v2 2/3] accel: add dedicated minor for accelerator devices
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 xarray 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 xarray is done solely
by functions in accel_drv.c, as the xarray 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() partially duplicates drm_open as I removed some code from
it that handles legacy devices.
Signed-off-by: Oded Gabbay
---
Changes in v2:
- Moved all accel minor handling code to accel_drv.c
- Replaced deprecated idr with xarray
drivers/accel/accel_drv.c | 205 +
drivers/gpu/drm/drm_file.c | 2 +-
include/drm/drm_accel.h| 29 +-
include/drm/drm_device.h | 3 +
include/drm/drm_drv.h | 8 ++
include/drm/drm_file.h | 21 +++-
6 files changed, 247 insertions(+), 21 deletions(-)
diff --git a/drivers/accel/accel_drv.c b/drivers/accel/accel_drv.c
index 6132765ea054..964a93799936 100644
--- a/drivers/accel/accel_drv.c
+++ b/drivers/accel/accel_drv.c
@@ -9,13 +9,22 @@
#include
#include
#include
+#include
#include
+#include
+#include
#include
#include
+static DEFINE_XARRAY_ALLOC(accel_minors_xa);
+
static struct dentry *accel_debugfs_root;
-struct class *accel_class;
+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)
{
@@ -24,16 +33,6 @@ static char *accel_devnode(struct device *dev, umode_t *mode)
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.
- */
static int accel_sysfs_init(void)
{
int err;
@@ -54,11 +53,6 @@ static int accel_sysfs_init(void)
return 0;
}
-/**
- * accel_sysfs_destroy - destroys ACCEL class
- *
- * Destroy the ACCEL device class.
- */
static void accel_sysfs_destroy(void)
{
if (IS_ERR_OR_NULL(accel_class))
@@ -68,11 +62,185 @@ static void accel_sysfs_destroy(void)
accel_class = NULL;
}
+/**
+ * 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 = _sysfs_device_minor;
+}
+
+/**
+ * accel_minor_alloc() - Allocates a new accel minor
+ *
+ * This function access the accel minors xarray and allocates from it
+ * a new id to represent a new accel minor
+ *
+ * Return: A new id on success or error code in case xa_alloc failed
+ */
+int accel_minor_alloc(void)
+{
+ int rc, index;
+
+ rc = xa_alloc(_minors_xa, , NULL,
+ XA_LIMIT(0, ACCEL_MAX_MINORS - 1), GFP_KERNEL);
+ if (rc < 0)
+ return rc;
+
+ return index;
+}
+
+/**
+ * accel_minor_remove() - Remove an accel minor
+ * @index: The minor id to remove.
+ *
+ * This function access the accel minors xarray and removes from
+ * it the member with the id that
