[PATCH 0/3] drm: Use full allocated minor range for DRM

[PATCH 0/3] drm: Use full allocated minor range for DRM

[Michał Winiarski]

64 DRM device nodes is not enough for everyone.
Upgrade it to 512K (which definitely is more than enough).
Additionally - one minor tweak around minor IDR locking.

Michał Winiarski (3):
  drm: Don't reserve minors for control nodes
  drm: Expand max DRM device number to full MINORBITS
  drm: Use mutex for minors

 drivers/gpu/drm/drm_drv.c | 45 ++++++++++++++++++---------------------
 include/drm/drm_file.h    |  1 -
 2 files changed, 21 insertions(+), 25 deletions(-)

-- 
2.37.2

[PATCH 1/3] drm: Don't reserve minors for control nodes

[Michał Winiarski]

Control nodes are no longer with us.
While we still need to preserve render nodes numbering, there's no need
to reserve the range formerly used for control. Let's repurpose it to be
used by primary and remove control remains from the code entirely.

References: commit 0d49f303e8a7 ("drm: remove all control node code")
References: commit c9ac371d4b59 ("drm: Fix render node numbering regression from control node removal.")
Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
---
 drivers/gpu/drm/drm_drv.c | 4 ++--
 include/drm/drm_file.h    | 1 -
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 8214a0b1ab7f..d81783f43452 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -126,8 +126,8 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
 	spin_lock_irqsave(&drm_minor_lock, flags);
 	r = idr_alloc(&drm_minors_idr,
 		      NULL,
-		      64 * type,
-		      64 * (type + 1),
+		      128 * type,
+		      128 * (type + 1),
 		      GFP_NOWAIT);
 	spin_unlock_irqrestore(&drm_minor_lock, flags);
 	idr_preload_end();
diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
index d780fd151789..a3be533e99e0 100644
--- a/include/drm/drm_file.h
+++ b/include/drm/drm_file.h
@@ -54,7 +54,6 @@ struct file;
  */
 enum drm_minor_type {
 	DRM_MINOR_PRIMARY,
-	DRM_MINOR_CONTROL,
 	DRM_MINOR_RENDER,
 };
 
-- 
2.37.2

[PATCH 2/3] drm: Expand max DRM device number to full MINORBITS

[Michał Winiarski]

Even after getting rid of control nodes, we've only bumped max DRM
device number from 64 to 128.
That's not good enough for modern world where we have multi-GPU servers,
SR-IOV virtual functions and virtual devices used for testing.
Let's utilize full minor range for DRM devices.
To avoid regressing the existing userspace, we're still maintaining the
0-127 for primary, 128-255 for render. This numbering scheme is
continued for minors > 256 (256-383 for primary, 384-511 for render, and
so on).

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
---
 drivers/gpu/drm/drm_drv.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index d81783f43452..0dab1ef8a98d 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -113,7 +113,7 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
 {
 	struct drm_minor *minor;
 	unsigned long flags;
-	int r;
+	int r, start, end;
 
 	minor = drmm_kzalloc(dev, sizeof(*minor), GFP_KERNEL);
 	if (!minor)
@@ -122,15 +122,19 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
 	minor->type = type;
 	minor->dev = dev;
 
-	idr_preload(GFP_KERNEL);
-	spin_lock_irqsave(&drm_minor_lock, flags);
-	r = idr_alloc(&drm_minors_idr,
-		      NULL,
-		      128 * type,
-		      128 * (type + 1),
-		      GFP_NOWAIT);
-	spin_unlock_irqrestore(&drm_minor_lock, flags);
-	idr_preload_end();
+	start = 128 * type;
+	end = 128 * (type + 1);
+
+	do {
+		idr_preload(GFP_KERNEL);
+		spin_lock_irqsave(&drm_minor_lock, flags);
+		r = idr_alloc(&drm_minors_idr, NULL, start, end, GFP_NOWAIT);
+		spin_unlock_irqrestore(&drm_minor_lock, flags);
+		idr_preload_end();
+
+		start += 256;
+		end += 256;
+	} while ((r == -ENOSPC) && end <= (1 << MINORBITS));
 
 	if (r < 0)
 		return r;
-- 
2.37.2

[PATCH 3/3] drm: Use mutex for minors

[Michał Winiarski]

Operating on drm minor is not done in IRQ context, which means that we
could safely downgrade to regular non-irq spinlock.
But we can also go further and drop the idr_preload tricks by just using
a mutex.

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
---
 drivers/gpu/drm/drm_drv.c | 31 ++++++++++++-------------------
 1 file changed, 12 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 0dab1ef8a98d..b31497e28e6a 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -53,7 +53,7 @@ MODULE_AUTHOR("Gareth Hughes, Leif Delgass, José Fonseca, Jon Smirl");
 MODULE_DESCRIPTION("DRM shared core routines");
 MODULE_LICENSE("GPL and additional rights");
 
-static DEFINE_SPINLOCK(drm_minor_lock);
+static DEFINE_MUTEX(drm_minor_lock);
 static struct idr drm_minors_idr;
 
 /*
@@ -98,21 +98,19 @@ static struct drm_minor **drm_minor_get_slot(struct drm_device *dev,
 static void drm_minor_alloc_release(struct drm_device *dev, void *data)
 {
 	struct drm_minor *minor = data;
-	unsigned long flags;
 
 	WARN_ON(dev != minor->dev);
 
 	put_device(minor->kdev);
 
-	spin_lock_irqsave(&drm_minor_lock, flags);
+	mutex_lock(&drm_minor_lock);
 	idr_remove(&drm_minors_idr, minor->index);
-	spin_unlock_irqrestore(&drm_minor_lock, flags);
+	mutex_unlock(&drm_minor_lock);
 }
 
 static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
 {
 	struct drm_minor *minor;
-	unsigned long flags;
 	int r, start, end;
 
 	minor = drmm_kzalloc(dev, sizeof(*minor), GFP_KERNEL);
@@ -126,11 +124,9 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
 	end = 128 * (type + 1);
 
 	do {
-		idr_preload(GFP_KERNEL);
-		spin_lock_irqsave(&drm_minor_lock, flags);
-		r = idr_alloc(&drm_minors_idr, NULL, start, end, GFP_NOWAIT);
-		spin_unlock_irqrestore(&drm_minor_lock, flags);
-		idr_preload_end();
+		mutex_lock(&drm_minor_lock);
+		r = idr_alloc(&drm_minors_idr, NULL, start, end, GFP_KERNEL);
+		mutex_unlock(&drm_minor_lock);
 
 		start += 256;
 		end += 256;
@@ -156,7 +152,6 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
 static int drm_minor_register(struct drm_device *dev, unsigned int type)
 {
 	struct drm_minor *minor;
-	unsigned long flags;
 	int ret;
 
 	DRM_DEBUG("\n");
@@ -176,9 +171,9 @@ 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);
+	mutex_lock(&drm_minor_lock);
 	idr_replace(&drm_minors_idr, minor, minor->index);
-	spin_unlock_irqrestore(&drm_minor_lock, flags);
+	mutex_unlock(&drm_minor_lock);
 
 	DRM_DEBUG("new minor registered %d\n", minor->index);
 	return 0;
@@ -191,16 +186,15 @@ static int drm_minor_register(struct drm_device *dev, unsigned int type)
 static void drm_minor_unregister(struct drm_device *dev, unsigned int type)
 {
 	struct drm_minor *minor;
-	unsigned long flags;
 
 	minor = *drm_minor_get_slot(dev, type);
 	if (!minor || !device_is_registered(minor->kdev))
 		return;
 
 	/* replace @minor with NULL so lookups will fail from now on */
-	spin_lock_irqsave(&drm_minor_lock, flags);
+	mutex_lock(&drm_minor_lock);
 	idr_replace(&drm_minors_idr, NULL, minor->index);
-	spin_unlock_irqrestore(&drm_minor_lock, flags);
+	mutex_unlock(&drm_minor_lock);
 
 	device_del(minor->kdev);
 	dev_set_drvdata(minor->kdev, NULL); /* safety belt */
@@ -219,13 +213,12 @@ static void drm_minor_unregister(struct drm_device *dev, unsigned int type)
 struct drm_minor *drm_minor_acquire(unsigned int minor_id)
 {
 	struct drm_minor *minor;
-	unsigned long flags;
 
-	spin_lock_irqsave(&drm_minor_lock, flags);
+	mutex_lock(&drm_minor_lock);
 	minor = idr_find(&drm_minors_idr, minor_id);
 	if (minor)
 		drm_dev_get(minor->dev);
-	spin_unlock_irqrestore(&drm_minor_lock, flags);
+	mutex_unlock(&drm_minor_lock);
 
 	if (!minor) {
 		return ERR_PTR(-ENODEV);
-- 
2.37.2

Re: [PATCH 0/3] drm: Use full allocated minor range for DRM

[Simon Ser]

(It seems like the list was dropped in my reply, sorry about that.
Re-adding it now.)

On Thursday, August 18th, 2022 at 14:06, Michał Winiarski <michal.winiarski@intel.com> wrote:

> On Thu, Aug 18, 2022 at 07:39:13AM +0000, Simon Ser wrote:
> 
> > Hm, I'm a bit worried about the user-space implications of this… e.g. libdrm
> > can check for the major/minor to find out the type of a node. Dropping CONTROL
> > from the enum will break that.
> 
> Yeah, but that would only cause problems if there are more than 64 devices in
> the system, and the user-space in question is smart enough to support that.
> 
> IIUC libdrm only looks for 16 devices:
> https://gitlab.freedesktop.org/mesa/drm/-/blob/main/xf86drm.h#L47
> 
> I'm not very familiar with mesa codebase, but I think it has something similar:
> https://gitlab.freedesktop.org/mesa/mesa/-/blob/main/src/gallium/auxiliary/pipe-loader/pipe_loader_drm.c#L52
> 
> I expect other clients to also have something similar (loop over minors, 0-63
> for primary, 128-191 for render).
> 
> So this shouldn't really cause a regression, it's just that "old" userspace
> won't be able to use more devices (but it's also not able to use more devices
> without this series).

Unfortunately I think there are more assumptions all over the place, see e.g.
drmGetMinorType:
https://gitlab.freedesktop.org/mesa/drm/-/blob/main/xf86drm.c#L986

Also I'm not very found of dropping DRM_NODE_CONTROL from the kernel enum --
this results in DRM_NODE_RENDER=1 in the kernel but DRM_NODE_RENDER=2 in
user-space which sounds pretty error-prone.

> I could go with 0-63 primary, 64-127 empty, 128-191 render, 192-255 primary,
> 256-319 empty, (...)
> But it just seems like a waste to burn 1/3 of minors.

Could potentially work I guess.

> Perhaps it would also be possible to go with:
> 0-63 primary, 64-127 empty, 128-191 render, 192-512K continuous range
> where we distribute minors first-come first-serve, without any link to type (so
> usually we'd get continuous card192, renderD193, and so on)

We would need to re-design drmGetMinorType if we go down this path.

Re: [PATCH 0/3] drm: Use full allocated minor range for DRM

[Michał Winiarski]

On Fri, Aug 19, 2022 at 08:16:07AM +0000, Simon Ser wrote:
> (It seems like the list was dropped in my reply, sorry about that.
> Re-adding it now.)
> 
> On Thursday, August 18th, 2022 at 14:06, Michał Winiarski <michal.winiarski@intel.com> wrote:
> 
> > On Thu, Aug 18, 2022 at 07:39:13AM +0000, Simon Ser wrote:
> > 
> > > Hm, I'm a bit worried about the user-space implications of this… e.g. libdrm
> > > can check for the major/minor to find out the type of a node. Dropping CONTROL
> > > from the enum will break that.
> > 
> > Yeah, but that would only cause problems if there are more than 64 devices in
> > the system, and the user-space in question is smart enough to support that.
> > 
> > IIUC libdrm only looks for 16 devices:
> > https://gitlab.freedesktop.org/mesa/drm/-/blob/main/xf86drm.h#L47
> > 
> > I'm not very familiar with mesa codebase, but I think it has something similar:
> > https://gitlab.freedesktop.org/mesa/mesa/-/blob/main/src/gallium/auxiliary/pipe-loader/pipe_loader_drm.c#L52
> > 
> > I expect other clients to also have something similar (loop over minors, 0-63
> > for primary, 128-191 for render).
> > 
> > So this shouldn't really cause a regression, it's just that "old" userspace
> > won't be able to use more devices (but it's also not able to use more devices
> > without this series).
> 
> Unfortunately I think there are more assumptions all over the place, see e.g.
> drmGetMinorType:
> https://gitlab.freedesktop.org/mesa/drm/-/blob/main/xf86drm.c#L986
> 
> Also I'm not very found of dropping DRM_NODE_CONTROL from the kernel enum --
> this results in DRM_NODE_RENDER=1 in the kernel but DRM_NODE_RENDER=2 in
> user-space which sounds pretty error-prone.
> 
> > I could go with 0-63 primary, 64-127 empty, 128-191 render, 192-255 primary,
> > 256-319 empty, (...)
> > But it just seems like a waste to burn 1/3 of minors.
> 
> Could potentially work I guess.
> 
> > Perhaps it would also be possible to go with:
> > 0-63 primary, 64-127 empty, 128-191 render, 192-512K continuous range
> > where we distribute minors first-come first-serve, without any link to type (so
> > usually we'd get continuous card192, renderD193, and so on)
> 
> We would need to re-design drmGetMinorType if we go down this path.

It needs to be changed either way.
Even if we keep reserving 1/3 of minors, drmGetMinorType is still broken (will
return -1, for minors > 191).

Let's respin this with dropping patch 1 and reserving control minors.

-Michał