| Message ID | 20221022214622.18042-4-ogabbay@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | new subsystem for compute accelerator devices | expand |
On Sun, Oct 23, 2022 at 12:46:22AM +0300, 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 &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. > + * > + * RETURNS: > + * > + * 0 on success or negative errno value on failure. > + */ > +int accel_open(struct inode *inode, struct file *filp) > +{ > + struct drm_minor *minor; > + > + minor = drm_minor_acquire(iminor(inode), true); > + if (IS_ERR(minor)) > + return PTR_ERR(minor); > + > + return __drm_open(inode, filp, minor); > +} > +EXPORT_SYMBOL(accel_open); EXPORT_SYMBOL_GPL() please. And again, this should probably to into drivers/accel/ not here. thanks, greg k-h
On Sun, Oct 23, 2022 at 3:41 PM Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote: > > On Sun, Oct 23, 2022 at 12:46:22AM +0300, 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 &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. > > + * > > + * RETURNS: > > + * > > + * 0 on success or negative errno value on failure. > > + */ > > +int accel_open(struct inode *inode, struct file *filp) > > +{ > > + struct drm_minor *minor; > > + > > + minor = drm_minor_acquire(iminor(inode), true); > > + if (IS_ERR(minor)) > > + return PTR_ERR(minor); > > + > > + return __drm_open(inode, filp, minor); > > +} > > +EXPORT_SYMBOL(accel_open); > > EXPORT_SYMBOL_GPL() please. > > And again, this should probably to into drivers/accel/ not here. Got it, will do. Thanks, Oded > > thanks, > > greg k-h
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.
On Mon, Oct 24, 2022 at 6:21 PM Jeffrey Hugo <quic_jhugo@quicinc.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@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. 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
On Sun, 23 Oct 2022 00:46:22 +0300 Oded Gabbay <ogabbay@kernel.org> wrote: > 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; > + > /* > * If the drm core fails to init for whatever reason, > * we should prevent any drivers from registering with it. > @@ -94,6 +97,8 @@ static struct drm_minor **drm_minor_get_slot(struct drm_device *dev, > return &dev->primary; > case DRM_MINOR_RENDER: > return &dev->render; > + case DRM_MINOR_ACCEL: > + return &dev->accel; > default: > BUG(); > } > @@ -108,9 +113,15 @@ static void drm_minor_alloc_release(struct drm_device *dev, void *data) > > put_device(minor->kdev); > > - spin_lock_irqsave(&drm_minor_lock, flags); > - idr_remove(&drm_minors_idr, minor->index); > - spin_unlock_irqrestore(&drm_minor_lock, flags); > + if (minor->type == DRM_MINOR_ACCEL) { > + spin_lock_irqsave(&accel_minor_lock, flags); > + idr_remove(&accel_minors_idr, minor->index); > + spin_unlock_irqrestore(&accel_minor_lock, flags); > + } else { > + spin_lock_irqsave(&drm_minor_lock, flags); > + idr_remove(&drm_minors_idr, minor->index); > + spin_unlock_irqrestore(&drm_minor_lock, flags); > + } > } > > static int drm_minor_alloc(struct drm_device *dev, unsigned int type) > @@ -127,13 +138,23 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned int type) > minor->dev = dev; > > idr_preload(GFP_KERNEL); > - spin_lock_irqsave(&drm_minor_lock, flags); > - r = idr_alloc(&drm_minors_idr, > - NULL, > - 64 * type, > - 64 * (type + 1), > - GFP_NOWAIT); > - spin_unlock_irqrestore(&drm_minor_lock, flags); > + if (type == DRM_MINOR_ACCEL) { > + spin_lock_irqsave(&accel_minor_lock, flags); > + r = idr_alloc(&accel_minors_idr, > + NULL, > + 64 * (type - DRM_MINOR_ACCEL), > + 64 * (type - DRM_MINOR_ACCEL + 1), > + GFP_NOWAIT); > + spin_unlock_irqrestore(&accel_minor_lock, flags); > + } else { > + spin_lock_irqsave(&drm_minor_lock, flags); > + r = idr_alloc(&drm_minors_idr, > + NULL, > + 64 * type, > + 64 * (type + 1), > + GFP_NOWAIT); > + spin_unlock_irqrestore(&drm_minor_lock, flags); > + } Hi, There are many functions which checks drm type and decides its behaviors. It's good to re-use exiting codes, but accel devices use totally different major/minor, and so it needs to be moved to /drvier/accel/ (maybe later..). How about seperating functions for alloc/release minor (accel_minor_alloc..)? also, for others which have drm type related codes. > @@ -607,6 +652,14 @@ static int drm_dev_init(struct drm_device *dev, > /* no per-device feature limits by default */ > dev->driver_features = ~0u; > > + if (drm_core_check_feature(dev, DRIVER_COMPUTE_ACCEL) && > + (drm_core_check_feature(dev, DRIVER_RENDER) || > + drm_core_check_feature(dev, DRIVER_MODESET))) { > + > + DRM_ERROR("DRM driver can't be both a compute acceleration and graphics driver\n"); > + return -EINVAL; > + } > + It's fine for the device only for acceleration, but can't graphic devices have acceleration feature? Thanks, Jiho Chu
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@quicinc.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@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. > 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@intel.com/ -Michał > > Thanks, > Oded >
On Tue, Oct 25, 2022 at 9:43 AM Jiho Chu <jiho.chu@samsung.com> wrote: > > > On Sun, 23 Oct 2022 00:46:22 +0300 > Oded Gabbay <ogabbay@kernel.org> wrote: > > > 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; > > + > > /* > > * If the drm core fails to init for whatever reason, > > * we should prevent any drivers from registering with it. > > @@ -94,6 +97,8 @@ static struct drm_minor **drm_minor_get_slot(struct drm_device *dev, > > return &dev->primary; > > case DRM_MINOR_RENDER: > > return &dev->render; > > + case DRM_MINOR_ACCEL: > > + return &dev->accel; > > default: > > BUG(); > > } > > @@ -108,9 +113,15 @@ static void drm_minor_alloc_release(struct drm_device *dev, void *data) > > > > put_device(minor->kdev); > > > > - spin_lock_irqsave(&drm_minor_lock, flags); > > - idr_remove(&drm_minors_idr, minor->index); > > - spin_unlock_irqrestore(&drm_minor_lock, flags); > > + if (minor->type == DRM_MINOR_ACCEL) { > > + spin_lock_irqsave(&accel_minor_lock, flags); > > + idr_remove(&accel_minors_idr, minor->index); > > + spin_unlock_irqrestore(&accel_minor_lock, flags); > > + } else { > > + spin_lock_irqsave(&drm_minor_lock, flags); > > + idr_remove(&drm_minors_idr, minor->index); > > + spin_unlock_irqrestore(&drm_minor_lock, flags); > > + } > > } > > > > static int drm_minor_alloc(struct drm_device *dev, unsigned int type) > > @@ -127,13 +138,23 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned int type) > > minor->dev = dev; > > > > idr_preload(GFP_KERNEL); > > - spin_lock_irqsave(&drm_minor_lock, flags); > > - r = idr_alloc(&drm_minors_idr, > > - NULL, > > - 64 * type, > > - 64 * (type + 1), > > - GFP_NOWAIT); > > - spin_unlock_irqrestore(&drm_minor_lock, flags); > > + if (type == DRM_MINOR_ACCEL) { > > + spin_lock_irqsave(&accel_minor_lock, flags); > > + r = idr_alloc(&accel_minors_idr, > > + NULL, > > + 64 * (type - DRM_MINOR_ACCEL), > > + 64 * (type - DRM_MINOR_ACCEL + 1), > > + GFP_NOWAIT); > > + spin_unlock_irqrestore(&accel_minor_lock, flags); > > + } else { > > + spin_lock_irqsave(&drm_minor_lock, flags); > > + r = idr_alloc(&drm_minors_idr, > > + NULL, > > + 64 * type, > > + 64 * (type + 1), > > + GFP_NOWAIT); > > + spin_unlock_irqrestore(&drm_minor_lock, flags); > > + } > > Hi, > There are many functions which checks drm type and decides its behaviors. It's good to > re-use exiting codes, but accel devices use totally different major/minor, and so it needs to be moved to > /drvier/accel/ (maybe later..). How about seperating functions for alloc/release minor (accel_minor_alloc..)? > also, for others which have drm type related codes. My feeling was moving the minor code handling to a different file (in addition to moving the major code handling) will cause too much duplication. My main theme is that an accel minor is another minor in drm, even if a bit different. i.e. It uses the same drm_minor structure. The driver declares he wants to use this minor using a drm driver feature flag. imo, all of that indicates the code should be inside drm. > > > > > > @@ -607,6 +652,14 @@ static int drm_dev_init(struct drm_device *dev, > > /* no per-device feature limits by default */ > > dev->driver_features = ~0u; > > > > + if (drm_core_check_feature(dev, DRIVER_COMPUTE_ACCEL) && > > + (drm_core_check_feature(dev, DRIVER_RENDER) || > > + drm_core_check_feature(dev, DRIVER_MODESET))) { > > + > > + DRM_ERROR("DRM driver can't be both a compute acceleration and graphics driver\n"); > > + return -EINVAL; > > + } > > + > > It's fine for the device only for acceleration, but can't graphic devices have acceleration feature? Of course they can :) In that case, and if they want to expose an accel device char, they should write an accel driver and connect it to their main graphics driver via auxiliary bus. I could have added two flags - compute_accel, and compute_accel_only (similar to a patch that was sent to add render only flag), but imo it would make the code more convoluted. I prefer the clean separation and using standard auxiliary bus. Thanks, Oded > > > Thanks, > Jiho Chu
On Tue, Oct 25, 2022 at 4:27 PM Michał Winiarski <michal.winiarski@intel.com> 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@quicinc.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@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. > > 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@intel.com/ > > -Michał I'll do it. Oded > > > > > Thanks, > > Oded > >
On Wed, 26 Oct 2022 09:38:13 +0300 Oded Gabbay <ogabbay@kernel.org> wrote: > On Tue, Oct 25, 2022 at 9:43 AM Jiho Chu <jiho.chu@samsung.com> wrote: > > > > > > On Sun, 23 Oct 2022 00:46:22 +0300 > > Oded Gabbay <ogabbay@kernel.org> wrote: > > > > > 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; > > > + > > > /* > > > * If the drm core fails to init for whatever reason, > > > * we should prevent any drivers from registering with it. > > > @@ -94,6 +97,8 @@ static struct drm_minor **drm_minor_get_slot(struct drm_device *dev, > > > return &dev->primary; > > > case DRM_MINOR_RENDER: > > > return &dev->render; > > > + case DRM_MINOR_ACCEL: > > > + return &dev->accel; > > > default: > > > BUG(); > > > } > > > @@ -108,9 +113,15 @@ static void drm_minor_alloc_release(struct drm_device *dev, void *data) > > > > > > put_device(minor->kdev); > > > > > > - spin_lock_irqsave(&drm_minor_lock, flags); > > > - idr_remove(&drm_minors_idr, minor->index); > > > - spin_unlock_irqrestore(&drm_minor_lock, flags); > > > + if (minor->type == DRM_MINOR_ACCEL) { > > > + spin_lock_irqsave(&accel_minor_lock, flags); > > > + idr_remove(&accel_minors_idr, minor->index); > > > + spin_unlock_irqrestore(&accel_minor_lock, flags); > > > + } else { > > > + spin_lock_irqsave(&drm_minor_lock, flags); > > > + idr_remove(&drm_minors_idr, minor->index); > > > + spin_unlock_irqrestore(&drm_minor_lock, flags); > > > + } > > > } > > > > > > static int drm_minor_alloc(struct drm_device *dev, unsigned int type) > > > @@ -127,13 +138,23 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned int type) > > > minor->dev = dev; > > > > > > idr_preload(GFP_KERNEL); > > > - spin_lock_irqsave(&drm_minor_lock, flags); > > > - r = idr_alloc(&drm_minors_idr, > > > - NULL, > > > - 64 * type, > > > - 64 * (type + 1), > > > - GFP_NOWAIT); > > > - spin_unlock_irqrestore(&drm_minor_lock, flags); > > > + if (type == DRM_MINOR_ACCEL) { > > > + spin_lock_irqsave(&accel_minor_lock, flags); > > > + r = idr_alloc(&accel_minors_idr, > > > + NULL, > > > + 64 * (type - DRM_MINOR_ACCEL), > > > + 64 * (type - DRM_MINOR_ACCEL + 1), > > > + GFP_NOWAIT); > > > + spin_unlock_irqrestore(&accel_minor_lock, flags); > > > + } else { > > > + spin_lock_irqsave(&drm_minor_lock, flags); > > > + r = idr_alloc(&drm_minors_idr, > > > + NULL, > > > + 64 * type, > > > + 64 * (type + 1), > > > + GFP_NOWAIT); > > > + spin_unlock_irqrestore(&drm_minor_lock, flags); > > > + } > > > > Hi, > > There are many functions which checks drm type and decides its behaviors. It's good to > > re-use exiting codes, but accel devices use totally different major/minor, and so it needs to be moved to > > /drvier/accel/ (maybe later..). How about seperating functions for alloc/release minor (accel_minor_alloc..)? > > also, for others which have drm type related codes. > My feeling was moving the minor code handling to a different file (in > addition to moving the major code handling) will cause too much > duplication. > My main theme is that an accel minor is another minor in drm, even if > a bit different. i.e. It uses the same drm_minor structure. > The driver declares he wants to use this minor using a drm driver feature flag. > imo, all of that indicates the code should be inside drm. > > > > > > > > > > > @@ -607,6 +652,14 @@ static int drm_dev_init(struct drm_device *dev, > > > /* no per-device feature limits by default */ > > > dev->driver_features = ~0u; > > > > > > + if (drm_core_check_feature(dev, DRIVER_COMPUTE_ACCEL) && > > > + (drm_core_check_feature(dev, DRIVER_RENDER) || > > > + drm_core_check_feature(dev, DRIVER_MODESET))) { > > > + > > > + DRM_ERROR("DRM driver can't be both a compute acceleration and graphics driver\n"); > > > + return -EINVAL; > > > + } > > > + > > > > It's fine for the device only for acceleration, but can't graphic devices have acceleration feature? > Of course they can :) In that case, and if they want to expose an > accel device char, they should write an accel driver and connect it to > their main graphics driver via auxiliary bus. > > I could have added two flags - compute_accel, and compute_accel_only > (similar to a patch that was sent to add render only flag), but imo it > would make the code more convoluted. I prefer the clean separation and > using standard auxiliary bus. > > Thanks, > Oded > I understood. Seperation would be good as you mentioned in other mail. This subsystem would be better choice for acceleration only devices, who need some features of drm, but deoesnot want to include whole graphics related considerations. I'll prepare Samsung's NPU driver using this after your reference driver is presented (maybe habana'). Thanks. Jiho Chu
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; + /* * If the drm core fails to init for whatever reason, * we should prevent any drivers from registering with it. @@ -94,6 +97,8 @@ static struct drm_minor **drm_minor_get_slot(struct drm_device *dev, return &dev->primary; case DRM_MINOR_RENDER: return &dev->render; + case DRM_MINOR_ACCEL: + return &dev->accel; default: BUG(); } @@ -108,9 +113,15 @@ static void drm_minor_alloc_release(struct drm_device *dev, void *data) put_device(minor->kdev); - spin_lock_irqsave(&drm_minor_lock, flags); - idr_remove(&drm_minors_idr, minor->index); - spin_unlock_irqrestore(&drm_minor_lock, flags); + if (minor->type == DRM_MINOR_ACCEL) { + spin_lock_irqsave(&accel_minor_lock, flags); + idr_remove(&accel_minors_idr, minor->index); + spin_unlock_irqrestore(&accel_minor_lock, flags); + } else { + spin_lock_irqsave(&drm_minor_lock, flags); + idr_remove(&drm_minors_idr, minor->index); + spin_unlock_irqrestore(&drm_minor_lock, flags); + } } static int drm_minor_alloc(struct drm_device *dev, unsigned int type) @@ -127,13 +138,23 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned int type) minor->dev = dev; idr_preload(GFP_KERNEL); - spin_lock_irqsave(&drm_minor_lock, flags); - r = idr_alloc(&drm_minors_idr, - NULL, - 64 * type, - 64 * (type + 1), - GFP_NOWAIT); - spin_unlock_irqrestore(&drm_minor_lock, flags); + if (type == DRM_MINOR_ACCEL) { + spin_lock_irqsave(&accel_minor_lock, flags); + r = idr_alloc(&accel_minors_idr, + NULL, + 64 * (type - DRM_MINOR_ACCEL), + 64 * (type - DRM_MINOR_ACCEL + 1), + GFP_NOWAIT); + spin_unlock_irqrestore(&accel_minor_lock, flags); + } else { + spin_lock_irqsave(&drm_minor_lock, flags); + r = idr_alloc(&drm_minors_idr, + NULL, + 64 * type, + 64 * (type + 1), + GFP_NOWAIT); + spin_unlock_irqrestore(&drm_minor_lock, flags); + } idr_preload_end(); if (r < 0) @@ -167,7 +188,11 @@ static int drm_minor_register(struct drm_device *dev, unsigned int type) ret = drm_debugfs_init(minor, minor->index, drm_debugfs_root); if (ret) { - DRM_ERROR("DRM: Failed to initialize /sys/kernel/debug/dri.\n"); + if (minor->type == DRM_MINOR_ACCEL) + DRM_ERROR("DRM: Failed to initialize /sys/kernel/debug/accel.\n"); + else + DRM_ERROR("DRM: Failed to initialize /sys/kernel/debug/dri.\n"); + goto err_debugfs; } @@ -176,9 +201,15 @@ static int drm_minor_register(struct drm_device *dev, unsigned int type) goto err_debugfs; /* replace NULL with @minor so lookups will succeed from now on */ - spin_lock_irqsave(&drm_minor_lock, flags); - idr_replace(&drm_minors_idr, minor, minor->index); - spin_unlock_irqrestore(&drm_minor_lock, flags); + if (minor->type == DRM_MINOR_ACCEL) { + spin_lock_irqsave(&accel_minor_lock, flags); + idr_replace(&accel_minors_idr, minor, minor->index); + spin_unlock_irqrestore(&accel_minor_lock, flags); + } else { + spin_lock_irqsave(&drm_minor_lock, flags); + idr_replace(&drm_minors_idr, minor, minor->index); + spin_unlock_irqrestore(&drm_minor_lock, flags); + } DRM_DEBUG("new minor registered %d\n", minor->index); return 0; @@ -198,9 +229,15 @@ static void drm_minor_unregister(struct drm_device *dev, unsigned int type) return; /* replace @minor with NULL so lookups will fail from now on */ - spin_lock_irqsave(&drm_minor_lock, flags); - idr_replace(&drm_minors_idr, NULL, minor->index); - spin_unlock_irqrestore(&drm_minor_lock, flags); + if (minor->type == DRM_MINOR_ACCEL) { + spin_lock_irqsave(&accel_minor_lock, flags); + idr_replace(&accel_minors_idr, NULL, minor->index); + spin_unlock_irqrestore(&accel_minor_lock, flags); + } else { + spin_lock_irqsave(&drm_minor_lock, flags); + idr_replace(&drm_minors_idr, NULL, minor->index); + spin_unlock_irqrestore(&drm_minor_lock, flags); + } device_del(minor->kdev); dev_set_drvdata(minor->kdev, NULL); /* safety belt */ @@ -216,16 +253,24 @@ static void drm_minor_unregister(struct drm_device *dev, unsigned int type) * minor->dev pointer will stay valid! However, the device may get unplugged and * unregistered while you hold the minor. */ -struct drm_minor *drm_minor_acquire(unsigned int minor_id) +struct drm_minor *drm_minor_acquire(unsigned int minor_id, bool is_accel_minor) { struct drm_minor *minor; unsigned long flags; - spin_lock_irqsave(&drm_minor_lock, flags); - minor = idr_find(&drm_minors_idr, minor_id); - if (minor) - drm_dev_get(minor->dev); - spin_unlock_irqrestore(&drm_minor_lock, flags); + if (is_accel_minor) { + 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); + } else { + spin_lock_irqsave(&drm_minor_lock, flags); + minor = idr_find(&drm_minors_idr, minor_id); + if (minor) + drm_dev_get(minor->dev); + spin_unlock_irqrestore(&drm_minor_lock, flags); + } if (!minor) { return ERR_PTR(-ENODEV); @@ -607,6 +652,14 @@ static int drm_dev_init(struct drm_device *dev, /* no per-device feature limits by default */ dev->driver_features = ~0u; + if (drm_core_check_feature(dev, DRIVER_COMPUTE_ACCEL) && + (drm_core_check_feature(dev, DRIVER_RENDER) || + drm_core_check_feature(dev, DRIVER_MODESET))) { + + DRM_ERROR("DRM driver can't be both a compute acceleration and graphics driver\n"); + return -EINVAL; + } + drm_legacy_init_members(dev); INIT_LIST_HEAD(&dev->filelist); INIT_LIST_HEAD(&dev->filelist_internal); @@ -632,15 +685,21 @@ static int drm_dev_init(struct drm_device *dev, dev->anon_inode = inode; - if (drm_core_check_feature(dev, DRIVER_RENDER)) { - ret = drm_minor_alloc(dev, DRM_MINOR_RENDER); + if (drm_core_check_feature(dev, DRIVER_COMPUTE_ACCEL)) { + ret = drm_minor_alloc(dev, DRM_MINOR_ACCEL); if (ret) goto err; - } + } else { + if (drm_core_check_feature(dev, DRIVER_RENDER)) { + ret = drm_minor_alloc(dev, DRM_MINOR_RENDER); + if (ret) + goto err; + } - ret = drm_minor_alloc(dev, DRM_MINOR_PRIMARY); - if (ret) - goto err; + ret = drm_minor_alloc(dev, DRM_MINOR_PRIMARY); + if (ret) + goto err; + } ret = drm_legacy_create_map_hash(dev); if (ret) @@ -887,6 +946,10 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags) if (ret) goto err_minors; + ret = drm_minor_register(dev, DRM_MINOR_ACCEL); + if (ret) + goto err_minors; + ret = create_compat_control_link(dev); if (ret) goto err_minors; @@ -906,12 +969,13 @@ int drm_dev_register(struct drm_device *dev, unsigned long flags) driver->name, driver->major, driver->minor, driver->patchlevel, driver->date, dev->dev ? dev_name(dev->dev) : "virtual device", - dev->primary->index); + dev->primary ? dev->primary->index : dev->accel->index); goto out_unlock; err_minors: remove_compat_control_link(dev); + drm_minor_unregister(dev, DRM_MINOR_ACCEL); drm_minor_unregister(dev, DRM_MINOR_PRIMARY); drm_minor_unregister(dev, DRM_MINOR_RENDER); out_unlock: @@ -954,6 +1018,7 @@ void drm_dev_unregister(struct drm_device *dev) drm_legacy_rmmaps(dev); remove_compat_control_link(dev); + drm_minor_unregister(dev, DRM_MINOR_ACCEL); drm_minor_unregister(dev, DRM_MINOR_PRIMARY); drm_minor_unregister(dev, DRM_MINOR_RENDER); } @@ -999,18 +1064,11 @@ EXPORT_SYMBOL(drm_dev_set_unique); * registered minor. */ -static int drm_stub_open(struct inode *inode, struct file *filp) +static int stub_open(struct inode *inode, struct file *filp, struct drm_minor *minor) { const struct file_operations *new_fops; - struct drm_minor *minor; int err; - DRM_DEBUG("\n"); - - minor = drm_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; @@ -1029,18 +1087,51 @@ static int drm_stub_open(struct inode *inode, struct file *filp) return err; } +static int drm_stub_open(struct inode *inode, struct file *filp) +{ + struct drm_minor *minor; + + DRM_DEBUG("\n"); + + minor = drm_minor_acquire(iminor(inode), false); + if (IS_ERR(minor)) + return PTR_ERR(minor); + + return stub_open(inode, filp, minor); +} + +static int accel_stub_open(struct inode *inode, struct file *filp) +{ + struct drm_minor *minor; + + DRM_DEBUG("\n"); + + minor = drm_minor_acquire(iminor(inode), true); + if (IS_ERR(minor)) + return PTR_ERR(minor); + + return stub_open(inode, filp, minor); +} + static const struct file_operations drm_stub_fops = { .owner = THIS_MODULE, .open = drm_stub_open, .llseek = noop_llseek, }; +static const struct file_operations accel_stub_fops = { + .owner = THIS_MODULE, + .open = accel_stub_open, + .llseek = noop_llseek, +}; + static void accel_core_exit(void) { #ifdef CONFIG_ACCEL unregister_chrdev(ACCEL_MAJOR, "accel"); debugfs_remove(accel_debugfs_root); accel_sysfs_destroy(); + idr_destroy(&accel_minors_idr); #endif } @@ -1060,6 +1151,8 @@ static int __init accel_core_init(void) #ifdef CONFIG_ACCEL int ret; + idr_init(&accel_minors_idr); + ret = accel_sysfs_init(); if (ret < 0) { DRM_ERROR("Cannot create ACCEL class: %d\n", ret); @@ -1068,7 +1161,7 @@ static int __init accel_core_init(void) accel_debugfs_root = debugfs_create_dir("accel", NULL); - ret = register_chrdev(ACCEL_MAJOR, "accel", &drm_stub_fops); + ret = register_chrdev(ACCEL_MAJOR, "accel", &accel_stub_fops); if (ret < 0) goto error; diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c index a8b4d918e9a3..d9c60108ab7b 100644 --- a/drivers/gpu/drm/drm_file.c +++ b/drivers/gpu/drm/drm_file.c @@ -389,30 +389,12 @@ static int drm_open_helper(struct file *filp, struct drm_minor *minor) return 0; } -/** - * drm_open - open method for DRM 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 DRM device and instantiates all the per-file - * resources for it. It also calls the &drm_driver.open driver callback. - * - * RETURNS: - * - * 0 on success or negative errno value on failure. - */ -int drm_open(struct inode *inode, struct file *filp) +static int __drm_open(struct inode *inode, struct file *filp, struct drm_minor *minor) { struct drm_device *dev; - struct drm_minor *minor; int retcode; int need_setup = 0; - minor = drm_minor_acquire(iminor(inode)); - if (IS_ERR(minor)) - return PTR_ERR(minor); - dev = minor->dev; if (drm_dev_needs_global_mutex(dev)) mutex_lock(&drm_global_mutex); @@ -446,8 +428,57 @@ int drm_open(struct inode *inode, struct file *filp) drm_minor_release(minor); return retcode; } + +/** + * drm_open - open method for DRM 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 DRM device and instantiates all the per-file + * resources for it. It also calls the &drm_driver.open driver callback. + * + * RETURNS: + * + * 0 on success or negative errno value on failure. + */ +int drm_open(struct inode *inode, struct file *filp) +{ + struct drm_minor *minor; + + minor = drm_minor_acquire(iminor(inode), false); + if (IS_ERR(minor)) + return PTR_ERR(minor); + + return __drm_open(inode, filp, minor); +} EXPORT_SYMBOL(drm_open); +/** + * 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. + * + * RETURNS: + * + * 0 on success or negative errno value on failure. + */ +int accel_open(struct inode *inode, struct file *filp) +{ + struct drm_minor *minor; + + minor = drm_minor_acquire(iminor(inode), true); + if (IS_ERR(minor)) + return PTR_ERR(minor); + + return __drm_open(inode, filp, minor); +} +EXPORT_SYMBOL(accel_open); + void drm_lastclose(struct drm_device * dev) { DRM_DEBUG("\n"); diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h index cbeb9bd3c312..e12efc243527 100644 --- a/drivers/gpu/drm/drm_internal.h +++ b/drivers/gpu/drm/drm_internal.h @@ -78,7 +78,7 @@ void drm_prime_remove_buf_handle(struct drm_prime_file_private *prime_fpriv, uint32_t handle); /* drm_drv.c */ -struct drm_minor *drm_minor_acquire(unsigned int minor_id); +struct drm_minor *drm_minor_acquire(unsigned int minor_id, bool is_accel_minor); void drm_minor_release(struct drm_minor *minor); /* drm_managed.c */ diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c index 70b2a28f55c4..1335476ad373 100644 --- a/drivers/gpu/drm/drm_sysfs.c +++ b/drivers/gpu/drm/drm_sysfs.c @@ -51,6 +51,10 @@ static struct device_type drm_sysfs_device_minor = { .name = "drm_minor" }; +static struct device_type accel_sysfs_device_minor = { + .name = "accel_minor" +}; + static struct device_type drm_sysfs_device_connector = { .name = "drm_connector", }; @@ -523,19 +527,28 @@ struct device *drm_sysfs_minor_alloc(struct drm_minor *minor) struct device *kdev; int r; - if (minor->type == DRM_MINOR_RENDER) - minor_str = "renderD%d"; - else - minor_str = "card%d"; - kdev = kzalloc(sizeof(*kdev), GFP_KERNEL); if (!kdev) return ERR_PTR(-ENOMEM); device_initialize(kdev); - kdev->devt = MKDEV(DRM_MAJOR, minor->index); - kdev->class = drm_class; - kdev->type = &drm_sysfs_device_minor; + + if (minor->type == DRM_MINOR_ACCEL) { + minor_str = "accel%d"; + kdev->devt = MKDEV(ACCEL_MAJOR, minor->index); + kdev->class = accel_class; + kdev->type = &accel_sysfs_device_minor; + } else { + if (minor->type == DRM_MINOR_RENDER) + minor_str = "renderD%d"; + else + minor_str = "card%d"; + + kdev->devt = MKDEV(DRM_MAJOR, minor->index); + kdev->class = drm_class; + kdev->type = &drm_sysfs_device_minor; + } + kdev->parent = minor->dev->dev; kdev->release = drm_sysfs_release; dev_set_drvdata(kdev, minor); 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..4f64102ba8d9 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..e0895a90f394 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, }; /** @@ -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 */ 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 <drm_accel_node>`. + */ +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 accel_open(struct inode *inode, struct file *filp); 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);
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(-)