From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 598BFC4332F for ; Tue, 8 Nov 2022 13:13:54 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 1137F10E44A; Tue, 8 Nov 2022 13:13:53 +0000 (UTC) Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTPS id B6C4110E44A for ; Tue, 8 Nov 2022 13:13:50 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1667913230; x=1699449230; h=message-id:date:mime-version:subject:to:cc:references: from:in-reply-to:content-transfer-encoding; bh=axEwOgywri/ADgdzJs0B1IU8opxe/+0jsh5t2PEr1Vg=; b=PKuPcXiPd+CrgVVhw9iW2PmEsEMMxpmQdffDlOZykTDx3MZb0Pb/L3z6 4IlI+GJpBu0tbhEZJyKOJCVA1Y4bq1fWqEJbxS/FweXqyG2aVSXh2LLcO bFfn5qsEg2+KrYb4LgelstynPlaWELCvPGaNrzRHBSTNZ88nWjjHl6om+ B3WRA/JbqEswVdX0wY+hT5Huhu4EwL0nTYy5KUP9WQBjvzQfRFGg1F41W keK+Vkvigy/8iK5yfKJStEYVXAVm8QwHKOM34EEb3HAwK777p+/xmpZTL B0OmRxCNak0kD+Qgurp1qoCcZqe44PgxjfELc615BFPuGFesp/VObXD/U Q==; X-IronPort-AV: E=McAfee;i="6500,9779,10524"; a="298205500" X-IronPort-AV: E=Sophos;i="5.96,147,1665471600"; d="scan'208";a="298205500" Received: from orsmga007.jf.intel.com ([10.7.209.58]) by orsmga101.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 08 Nov 2022 05:13:50 -0800 X-IronPort-AV: E=McAfee;i="6500,9779,10524"; a="630866885" X-IronPort-AV: E=Sophos;i="5.96,147,1665471600"; d="scan'208";a="630866885" Received: from shylandx-mobl2.ger.corp.intel.com (HELO [10.213.210.50]) ([10.213.210.50]) by orsmga007-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 08 Nov 2022 05:13:45 -0800 Message-ID: <222554f4-c3b0-8375-2ed6-175d4b2c52cb@linux.intel.com> Date: Tue, 8 Nov 2022 13:13:43 +0000 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.3.3 Subject: Re: [RFC PATCH v3 2/3] accel: add dedicated minor for accelerator devices Content-Language: en-US To: Oded Gabbay , David Airlie , Daniel Vetter , Arnd Bergmann , Greg Kroah-Hartman , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, Jason Gunthorpe , John Hubbard , Alex Deucher References: <20221106210225.2065371-1-ogabbay@kernel.org> <20221106210225.2065371-3-ogabbay@kernel.org> From: Tvrtko Ursulin Organization: Intel Corporation UK Plc In-Reply-To: <20221106210225.2065371-3-ogabbay@kernel.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Maciej Kwapulinski , Jeffrey Hugo , Jiho Chu , Randy Dunlap , Christoph Hellwig , Stanislaw Gruszka , Jacek Lawrynowicz , Thomas Zimmermann , Kevin Hilman , Yuji Ishikawa , Jagan Teki Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" On 06/11/2022 21:02, Oded Gabbay wrote: > The accelerator devices are exposed to user-space using a dedicated > major. In addition, they are represented in /dev with new, dedicated > device char names: /dev/accel/accel*. This is done to make sure any > user-space software that tries to open a graphic card won't open > the accelerator device by mistake. > > The above implies that the minor numbering should be separated from > the rest of the DRM devices. However, to avoid code duplication, we > want the drm_minor structure to be able to represent the accelerator > device. > > To achieve this, we add a new drm_minor* to drm_device that represents > the accelerator device. This pointer is initialized for drivers that > declare they handle compute accelerator, using a new driver feature > flag called DRIVER_COMPUTE_ACCEL. It is important to note that this > driver feature is mutually exclusive with DRIVER_RENDER. Devices that > want to expose both graphics and compute device char files should be > handled by two drivers that are connected using the auxiliary bus > framework. > > In addition, we define a different IDR to handle the accelerators > minors. This is done to make the minor's index be identical to the > device index in /dev/. Any access to the IDR is done solely > by functions in accel_drv.c, as the IDR is define as static. The > DRM core functions call those functions in case they detect the minor's > type is DRM_MINOR_ACCEL. > > We define a separate accel_open function (from drm_open) that the > accel drivers should set as their open callback function. Both these > functions eventually call the same drm_open_helper(), which had to be > changed to be non-static so it can be called from accel_drv.c. > accel_open() only partially duplicates drm_open as I removed some code > from it that handles legacy devices. > > To help new drivers, I defined DEFINE_DRM_ACCEL_FOPS macro to easily > set the required function operations pointers structure. > > Signed-off-by: Oded Gabbay > --- > Changes in v3: > - Remove useless DRM_DEBUG("\n") at accel_stub_open() > - Add function of accel_debugfs_init() as accel_debugfs_root is static > member in drm_accel.c > - Add DRM_ACCEL_FOPS and DEFINE_DRM_ACCEL_FOPS macros > - Replace minor handling from xarray back to idr, as xarray doesn't handle > well exchanging content of a NULL entry to non-NULL. This should be handled > in a different patch that will either fix xarray code or change DRM minor > init flow. > - Make accel_minor_replace() to return void. > > drivers/accel/drm_accel.c | 242 ++++++++++++++++++++++++++++++++++++- > drivers/gpu/drm/drm_file.c | 2 +- > include/drm/drm_accel.h | 68 ++++++++++- > include/drm/drm_device.h | 3 + > include/drm/drm_drv.h | 8 ++ > include/drm/drm_file.h | 21 +++- > 6 files changed, 340 insertions(+), 4 deletions(-) > > diff --git a/drivers/accel/drm_accel.c b/drivers/accel/drm_accel.c > index 943d960ddefc..05167c929866 100644 > --- a/drivers/accel/drm_accel.c > +++ b/drivers/accel/drm_accel.c > @@ -8,14 +8,25 @@ > > #include > #include > +#include > > #include > +#include > +#include > +#include > #include > #include > > +static DEFINE_SPINLOCK(accel_minor_lock); > +static struct idr accel_minors_idr; > + > static struct dentry *accel_debugfs_root; > static struct class *accel_class; > > +static struct device_type accel_sysfs_device_minor = { > + .name = "accel_minor" > +}; > + > static char *accel_devnode(struct device *dev, umode_t *mode) > { > return kasprintf(GFP_KERNEL, "accel/%s", dev_name(dev)); > @@ -40,9 +51,235 @@ static void accel_sysfs_destroy(void) > accel_class = NULL; > } > > +static int accel_name_info(struct seq_file *m, void *data) > +{ > + struct drm_info_node *node = (struct drm_info_node *) m->private; > + struct drm_minor *minor = node->minor; > + struct drm_device *dev = minor->dev; > + struct drm_master *master; > + > + mutex_lock(&dev->master_mutex); > + master = dev->master; > + seq_printf(m, "%s", dev->driver->name); > + if (dev->dev) > + seq_printf(m, " dev=%s", dev_name(dev->dev)); > + if (master && master->unique) > + seq_printf(m, " master=%s", master->unique); Does the all drm_master business apply with accel? > + if (dev->unique) > + seq_printf(m, " unique=%s", dev->unique); > + seq_puts(m, "\n"); > + mutex_unlock(&dev->master_mutex); > + > + return 0; > +} > + > +static const struct drm_info_list accel_debugfs_list[] = { > + {"name", accel_name_info, 0} > +}; > +#define ACCEL_DEBUGFS_ENTRIES ARRAY_SIZE(accel_debugfs_list) > + > +/** > + * accel_debugfs_init() - Initialize debugfs for accel minor > + * @minor: Pointer to the drm_minor instance. > + * @minor_id: The minor's id > + * > + * This function initializes the drm minor's debugfs members and creates > + * a root directory for the minor in debugfs. It also creates common files > + * for accelerators and calls the driver's debugfs init callback. > + */ > +void accel_debugfs_init(struct drm_minor *minor, int minor_id) > +{ > + struct drm_device *dev = minor->dev; > + char name[64]; > + > + INIT_LIST_HEAD(&minor->debugfs_list); > + mutex_init(&minor->debugfs_lock); > + sprintf(name, "%d", minor_id); > + minor->debugfs_root = debugfs_create_dir(name, accel_debugfs_root); > + > + drm_debugfs_create_files(accel_debugfs_list, ACCEL_DEBUGFS_ENTRIES, > + minor->debugfs_root, minor); > + > + if (dev->driver->debugfs_init) > + dev->driver->debugfs_init(minor); > +} > + > +/** > + * accel_set_device_instance_params() - Set some device parameters for accel device > + * @kdev: Pointer to the device instance. > + * @index: The minor's index > + * > + * This function creates the dev_t of the device using the accel major and > + * the device's minor number. In addition, it sets the class and type of the > + * device instance to the accel sysfs class and device type, respectively. > + */ > +void accel_set_device_instance_params(struct device *kdev, int index) > +{ > + kdev->devt = MKDEV(ACCEL_MAJOR, index); > + kdev->class = accel_class; > + kdev->type = &accel_sysfs_device_minor; > +} > + > +/** > + * accel_minor_alloc() - Allocates a new accel minor > + * > + * This function access the accel minors idr and allocates from it > + * a new id to represent a new accel minor > + * > + * Return: A new id on success or error code in case idr_alloc failed > + */ > +int accel_minor_alloc(void) > +{ > + unsigned long flags; > + int r; > + > + spin_lock_irqsave(&accel_minor_lock, flags); > + r = idr_alloc(&accel_minors_idr, NULL, 0, ACCEL_MAX_MINORS, GFP_NOWAIT); > + spin_unlock_irqrestore(&accel_minor_lock, flags); > + > + return r; > +} > + > +/** > + * accel_minor_remove() - Remove an accel minor > + * @index: The minor id to remove. > + * > + * This function access the accel minors idr and removes from > + * it the member with the id that is passed to this function. > + */ > +void accel_minor_remove(int index) > +{ > + unsigned long flags; > + > + spin_lock_irqsave(&accel_minor_lock, flags); > + idr_remove(&accel_minors_idr, index); > + spin_unlock_irqrestore(&accel_minor_lock, flags); > +} > + > +/** > + * accel_minor_replace() - Replace minor pointer in accel minors idr. > + * @minor: Pointer to the new minor. > + * @index: The minor id to replace. > + * > + * This function access the accel minors idr structure and replaces the pointer > + * that is associated with an existing id. Because the minor pointer can be > + * NULL, we need to explicitly pass the index. > + * > + * Return: 0 for success, negative value for error > + */ > +void accel_minor_replace(struct drm_minor *minor, int index) > +{ > + unsigned long flags; > + > + spin_lock_irqsave(&accel_minor_lock, flags); > + idr_replace(&accel_minors_idr, minor, index); > + spin_unlock_irqrestore(&accel_minor_lock, flags); > +} > + > +/* > + * Looks up the given minor-ID and returns the respective DRM-minor object. The > + * refence-count of the underlying device is increased so you must release this > + * object with accel_minor_release(). > + * > + * The object can be only a drm_minor that represents an accel device. > + * > + * As long as you hold this minor, it is guaranteed that the object and the > + * minor->dev pointer will stay valid! However, the device may get unplugged and > + * unregistered while you hold the minor. > + */ > +static struct drm_minor *accel_minor_acquire(unsigned int minor_id) > +{ > + struct drm_minor *minor; > + unsigned long flags; > + > + spin_lock_irqsave(&accel_minor_lock, flags); > + minor = idr_find(&accel_minors_idr, minor_id); > + if (minor) > + drm_dev_get(minor->dev); > + spin_unlock_irqrestore(&accel_minor_lock, flags); > + > + if (!minor) { > + return ERR_PTR(-ENODEV); > + } else if (drm_dev_is_unplugged(minor->dev)) { > + drm_dev_put(minor->dev); > + return ERR_PTR(-ENODEV); > + } > + > + return minor; > +} > + > +static void accel_minor_release(struct drm_minor *minor) > +{ > + drm_dev_put(minor->dev); > +} > + > +/** > + * accel_open - open method for ACCEL file > + * @inode: device inode > + * @filp: file pointer. > + * > + * This function must be used by drivers as their &file_operations.open method. > + * It looks up the correct ACCEL device and instantiates all the per-file > + * resources for it. It also calls the &drm_driver.open driver callback. > + * > + * Return: 0 on success or negative errno value on failure. > + */ > +int accel_open(struct inode *inode, struct file *filp) > +{ > + struct drm_device *dev; > + struct drm_minor *minor; > + int retcode; > + > + minor = accel_minor_acquire(iminor(inode)); > + if (IS_ERR(minor)) > + return PTR_ERR(minor); > + > + dev = minor->dev; > + > + atomic_fetch_inc(&dev->open_count); Why fetch and could you even only increment once everything worked (to avoid having to decrement on the error unwind)? > + > + /* share address_space across all char-devs of a single device */ > + filp->f_mapping = dev->anon_inode->i_mapping; > + > + retcode = drm_open_helper(filp, minor); > + if (retcode) > + goto err_undo; > + > + return 0; > + > +err_undo: > + atomic_dec(&dev->open_count); > + accel_minor_release(minor); > + return retcode; > +} > +EXPORT_SYMBOL_GPL(accel_open); > + > static int accel_stub_open(struct inode *inode, struct file *filp) > { > - return -EOPNOTSUPP; > + const struct file_operations *new_fops; > + struct drm_minor *minor; > + int err; > + > + minor = accel_minor_acquire(iminor(inode)); > + if (IS_ERR(minor)) > + return PTR_ERR(minor); > + > + new_fops = fops_get(minor->dev->driver->fops); > + if (!new_fops) { > + err = -ENODEV; > + goto out; > + } > + > + replace_fops(filp, new_fops); > + if (filp->f_op->open) > + err = filp->f_op->open(inode, filp); > + else > + err = 0; > + > +out: > + accel_minor_release(minor); > + > + return err; > } > > static const struct file_operations accel_stub_fops = { > @@ -56,12 +293,15 @@ void accel_core_exit(void) > unregister_chrdev(ACCEL_MAJOR, "accel"); > debugfs_remove(accel_debugfs_root); > accel_sysfs_destroy(); > + idr_destroy(&accel_minors_idr); > } > > int __init accel_core_init(void) > { > int ret; > > + idr_init(&accel_minors_idr); > + > ret = accel_sysfs_init(); > if (ret < 0) { > DRM_ERROR("Cannot create ACCEL class: %d\n", ret); > diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c > index a8b4d918e9a3..64b4a3a87fbb 100644 > --- a/drivers/gpu/drm/drm_file.c > +++ b/drivers/gpu/drm/drm_file.c > @@ -326,7 +326,7 @@ static int drm_cpu_valid(void) > * Creates and initializes a drm_file structure for the file private data in \p > * filp and add it into the double linked list in \p dev. > */ > -static int drm_open_helper(struct file *filp, struct drm_minor *minor) > +int drm_open_helper(struct file *filp, struct drm_minor *minor) > { > struct drm_device *dev = minor->dev; > struct drm_file *priv; > diff --git a/include/drm/drm_accel.h b/include/drm/drm_accel.h > index 31b42d3d6a15..b0c20367faad 100644 > --- a/include/drm/drm_accel.h > +++ b/include/drm/drm_accel.h > @@ -8,12 +8,56 @@ > #ifndef DRM_ACCEL_H_ > #define DRM_ACCEL_H_ > > -#define ACCEL_MAJOR 261 > +#include > + > +#define ACCEL_MAJOR 261 > +#define ACCEL_MAX_MINORS 256 > + > +/** > + * DRM_ACCEL_FOPS - Default drm accelerators file operations > + * > + * This macro provides a shorthand for setting the accelerator file ops in the > + * &file_operations structure. If all you need are the default ops, use > + * DEFINE_DRM_ACCEL_FOPS instead. > + */ > +#define DRM_ACCEL_FOPS \ > + .open = accel_open,\ > + .release = drm_release,\ > + .unlocked_ioctl = drm_ioctl,\ > + .compat_ioctl = drm_compat_ioctl,\ > + .poll = drm_poll,\ > + .read = drm_read,\ > + .llseek = noop_llseek > + > +/** > + * DEFINE_DRM_ACCEL_FOPS() - macro to generate file operations for accelerators drivers > + * @name: name for the generated structure > + * > + * This macro autogenerates a suitable &struct file_operations for accelerators based > + * drivers, which can be assigned to &drm_driver.fops. Note that this structure > + * cannot be shared between drivers, because it contains a reference to the > + * current module using THIS_MODULE. > + * > + * Note that the declaration is already marked as static - if you need a > + * non-static version of this you're probably doing it wrong and will break the > + * THIS_MODULE reference by accident. > + */ > +#define DEFINE_DRM_ACCEL_FOPS(name) \ > + static const struct file_operations name = {\ > + .owner = THIS_MODULE,\ > + DRM_ACCEL_FOPS,\ > + } > > #if IS_ENABLED(CONFIG_DRM_ACCEL) > > void accel_core_exit(void); > int accel_core_init(void); > +void accel_minor_remove(int index); > +int accel_minor_alloc(void); > +void accel_minor_replace(struct drm_minor *minor, int index); > +void accel_set_device_instance_params(struct device *kdev, int index); > +int accel_open(struct inode *inode, struct file *filp); > +void accel_debugfs_init(struct drm_minor *minor, int minor_id); > > #else > > @@ -23,9 +67,31 @@ static inline void accel_core_exit(void) > > static inline int __init accel_core_init(void) > { > + /* Return 0 to allow drm_core_init to complete successfully */ > return 0; > } > > +static inline void accel_minor_remove(int index) > +{ > +} > + > +static inline int accel_minor_alloc(void) > +{ > + return -EOPNOTSUPP; > +} > + > +static inline void accel_minor_replace(struct drm_minor *minor, int index) > +{ > +} > + > +static inline void accel_set_device_instance_params(struct device *kdev, int index) > +{ > +} > + > +static inline void accel_debugfs_init(struct drm_minor *minor, int minor_id) > +{ > + > + > #endif /* IS_ENABLED(CONFIG_DRM_ACCEL) */ > > #endif /* DRM_ACCEL_H_ */ > diff --git a/include/drm/drm_device.h b/include/drm/drm_device.h > index 9923c7a6885e..933ce2048e20 100644 > --- a/include/drm/drm_device.h > +++ b/include/drm/drm_device.h > @@ -93,6 +93,9 @@ struct drm_device { > /** @render: Render node */ > struct drm_minor *render; > > + /** @accel: Compute Acceleration node */ > + struct drm_minor *accel; > + > /** > * @registered: > * > diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h > index f6159acb8856..706e68ca5116 100644 > --- a/include/drm/drm_drv.h > +++ b/include/drm/drm_drv.h > @@ -94,6 +94,14 @@ enum drm_driver_feature { > * synchronization of command submission. > */ > DRIVER_SYNCOBJ_TIMELINE = BIT(6), > + /** > + * @DRIVER_COMPUTE_ACCEL: > + * > + * Driver supports compute acceleration devices. This flag is mutually exclusive with > + * @DRIVER_RENDER and @DRIVER_MODESET. Devices that support both graphics and compute > + * acceleration should be handled by two drivers that are connected using auxiliry bus. > + */ > + DRIVER_COMPUTE_ACCEL = BIT(7), > > /* IMPORTANT: Below are all the legacy flags, add new ones above. */ > > diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h > index d780fd151789..0d1f853092ab 100644 > --- a/include/drm/drm_file.h > +++ b/include/drm/drm_file.h > @@ -51,11 +51,15 @@ struct file; > > /* Note that the order of this enum is ABI (it determines > * /dev/dri/renderD* numbers). > + * > + * Setting DRM_MINOR_ACCEL to 32 gives enough space for more drm minors to > + * be implemented before we hit any future > */ > enum drm_minor_type { > DRM_MINOR_PRIMARY, > DRM_MINOR_CONTROL, > DRM_MINOR_RENDER, > + DRM_MINOR_ACCEL = 32, Didn't patch 1/3 say it's a standalone character device major? Are there two ways to open the same thing? > }; > > /** > @@ -70,7 +74,7 @@ enum drm_minor_type { > struct drm_minor { > /* private: */ > int index; /* Minor device number */ > - int type; /* Control or render */ > + int type; /* Control or render or accel */ Could this be self documenting if type was enum drm_minor_type? > struct device *kdev; /* Linux device */ > struct drm_device *dev; > > @@ -397,7 +401,22 @@ static inline bool drm_is_render_client(const struct drm_file *file_priv) > return file_priv->minor->type == DRM_MINOR_RENDER; > } > > +/** > + * drm_is_accel_client - is this an open file of the compute acceleration node > + * @file_priv: DRM file > + * > + * Returns true if this is an open file of the compute acceleration node, i.e. > + * &drm_file.minor of @file_priv is a accel minor. > + * > + * See also the :ref:`section on accel nodes `. > + */ > +static inline bool drm_is_accel_client(const struct drm_file *file_priv) > +{ > + return file_priv->minor->type == DRM_MINOR_ACCEL; > +} > + > int drm_open(struct inode *inode, struct file *filp); > +int drm_open_helper(struct file *filp, struct drm_minor *minor); > ssize_t drm_read(struct file *filp, char __user *buffer, > size_t count, loff_t *offset); > int drm_release(struct inode *inode, struct file *filp); > -- > 2.25.1 > Regards, Tvrtko