On Tue, Oct 25, 2022 at 4:27 PM Michał Winiarski <michal.winiarski@xxxxxxxxx> wrote: > > On Mon, Oct 24, 2022 at 08:43:58PM +0300, Oded Gabbay wrote: > > On Mon, Oct 24, 2022 at 6:21 PM Jeffrey Hugo <quic_jhugo@xxxxxxxxxxx> 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@xxxxxxxxxx> > > > > --- > > > > 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. > > The conversion is sitting on the mailinglist for a while now > (unfortunately, without much interest). > Perhaps you could help with reviewing it? > https://lore.kernel.org/dri-devel/20220911211443.581481-2-michal.winiarski@xxxxxxxxx/ > > -Michał I'll do it. Oded > > > > > Thanks, > > Oded > >