lkml.org 
[lkml]   [2022]   [Oct]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [RFC PATCH 3/3] drm: add dedicated minor for accelerator devices
From
On 10/22/2022 3:46 PM, 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/. In most places, this is hidden inside the drm
> core functions except when calling drm_minor_acquire(), where I had to
> add an extra parameter to specify the idr to use (because the
> accelerators minors index and the drm primary minor index both begin
> at 0).
>
> Signed-off-by: Oded Gabbay <ogabbay@kernel.org>
> ---
> drivers/gpu/drm/drm_drv.c | 171 +++++++++++++++++++++++++--------
> drivers/gpu/drm/drm_file.c | 69 +++++++++----
> drivers/gpu/drm/drm_internal.h | 2 +-
> drivers/gpu/drm/drm_sysfs.c | 29 ++++--
> include/drm/drm_device.h | 3 +
> include/drm/drm_drv.h | 8 ++
> include/drm/drm_file.h | 21 +++-
> 7 files changed, 235 insertions(+), 68 deletions(-)

Can we please add something to Documentation? I know this leverages DRM
a lot, but I believe that a new subsystem should not be introduced
without documentation. A lot of the info in the commit message is very
good, but should not be buried in the git log.

Besides, imagine this has been in mainline for N years, and someone
completely new to the kernel wants to write an accel driver. They
should be able to get started with something from Documentation that
at-least gives that person some insight into what to grep the code for.

>
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index b58ffb1433d6..c13701a8d4be 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -56,6 +56,9 @@ MODULE_LICENSE("GPL and additional rights");
> static DEFINE_SPINLOCK(drm_minor_lock);
> static struct idr drm_minors_idr;
>
> +static DEFINE_SPINLOCK(accel_minor_lock);
> +static struct idr accel_minors_idr;

IDR is deprecated. XArray is the preferred mechanism.
Yes, there already is IDR here, but I believe we should not be adding
new uses. Maybe at some point, the current IDR will be converted. Also
with XArray, I think you don't need the spinlock since XArray has
internal locking already.

\
 
 \ /
  Last update: 2022-10-24 18:38    [W:0.344 / U:0.352 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site