linux-kernel - Re: [RFC PATCH 3/3] drm: add dedicated minor for accelerator devices

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAFCwf12R1CWz8GdJ0sNsVL+_5b+G5Wqf5qwZ8ixtoXLOr2-obg@mail.gmail.com>
Date:   Mon, 24 Oct 2022 20:43:58 +0300
From:   Oded Gabbay <ogabbay@...nel.org>
To:     Jeffrey Hugo <quic_jhugo@...cinc.com>
Cc:     David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>,
        Arnd Bergmann <arnd@...db.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        Jason Gunthorpe <jgg@...dia.com>,
        John Hubbard <jhubbard@...dia.com>,
        Alex Deucher <alexander.deucher@....com>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        Yuji Ishikawa <yuji2.ishikawa@...hiba.co.jp>,
        Jiho Chu <jiho.chu@...sung.com>,
        Daniel Stone <daniel@...ishbar.org>,
        Tvrtko Ursulin <tvrtko.ursulin@...ux.intel.com>,
        Christoph Hellwig <hch@...radead.org>,
        Kevin Hilman <khilman@...libre.com>,
        Jagan Teki <jagan@...rulasolutions.com>,
        Jacek Lawrynowicz <jacek.lawrynowicz@...ux.intel.com>,
        Maciej Kwapulinski <maciej.kwapulinski@...ux.intel.com>
Subject: Re: [RFC PATCH 3/3] drm: add dedicated minor for accelerator devices

On Mon, Oct 24, 2022 at 6:21 PM Jeffrey Hugo <quic_jhugo@...cinc.com> wrote:
>
> 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@...nel.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.
Agreed. The only reason I haven't done it at this stage was because I
wanted to get an initial reaction to the code itself, see if the
direction is accepted.
I didn't want to write documentation and then completely re-write it.
So I will do it for the next patch-set, once I collect everyone's
feedback and I see there is a majority agreement.
>
> >
> > 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.
ok, I wasn't aware. I don't have any problem replacing the idr to xarray.

Thanks,
Oded