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

[PATCH v4 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).

To allow testing userspace support for >64 devices, add additional DRM
modparam (skip_legacy_minors) which causes DRM to skip allocating minors
in 0-192 range.
Additionally - convert minors to use XArray instead of IDR to simplify the
locking.

v1 -> v2:
Don't touch DRM_MINOR_CONTROL and its range (Simon Ser)

v2 -> v3:
Don't use legacy scheme for >=192 minor range (Dave Airlie)
Add modparam for testing (Dave Airlie)
Add lockdep annotation for IDR (Daniel Vetter)

v3 -> v4:
Convert from IDR to XArray (Matthew Wilcox)

Michał Winiarski (3):
  drm: Use XArray instead of IDR for minors
  drm: Expand max DRM device number to full MINORBITS
  drm: Introduce skip_legacy_minors modparam

 drivers/gpu/drm/drm_drv.c | 66 +++++++++++++++++++--------------------
 1 file changed, 33 insertions(+), 33 deletions(-)

-- 
2.37.3

[Intel-gfx] [PATCH v4 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).

To allow testing userspace support for >64 devices, add additional DRM
modparam (skip_legacy_minors) which causes DRM to skip allocating minors
in 0-192 range.
Additionally - convert minors to use XArray instead of IDR to simplify the
locking.

v1 -> v2:
Don't touch DRM_MINOR_CONTROL and its range (Simon Ser)

v2 -> v3:
Don't use legacy scheme for >=192 minor range (Dave Airlie)
Add modparam for testing (Dave Airlie)
Add lockdep annotation for IDR (Daniel Vetter)

v3 -> v4:
Convert from IDR to XArray (Matthew Wilcox)

Michał Winiarski (3):
  drm: Use XArray instead of IDR for minors
  drm: Expand max DRM device number to full MINORBITS
  drm: Introduce skip_legacy_minors modparam

 drivers/gpu/drm/drm_drv.c | 66 +++++++++++++++++++--------------------
 1 file changed, 33 insertions(+), 33 deletions(-)

-- 
2.37.3

[PATCH v4 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).

To allow testing userspace support for >64 devices, add additional DRM
modparam (skip_legacy_minors) which causes DRM to skip allocating minors
in 0-192 range.
Additionally - convert minors to use XArray instead of IDR to simplify the
locking.

v1 -> v2:
Don't touch DRM_MINOR_CONTROL and its range (Simon Ser)

v2 -> v3:
Don't use legacy scheme for >=192 minor range (Dave Airlie)
Add modparam for testing (Dave Airlie)
Add lockdep annotation for IDR (Daniel Vetter)

v3 -> v4:
Convert from IDR to XArray (Matthew Wilcox)

Michał Winiarski (3):
  drm: Use XArray instead of IDR for minors
  drm: Expand max DRM device number to full MINORBITS
  drm: Introduce skip_legacy_minors modparam

 drivers/gpu/drm/drm_drv.c | 66 +++++++++++++++++++--------------------
 1 file changed, 33 insertions(+), 33 deletions(-)

-- 
2.37.3

[PATCH v4 1/3] drm: Use XArray instead of IDR for minors

[Michał Winiarski]

IDR is deprecated, and since XArray manages its own state with internal
locking, it simplifies the locking on DRM side.
Additionally, don't use the IRQ-safe variant, since operating on drm
minor is not done in IRQ context.

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Suggested-by: Matthew Wilcox <willy@infradead.org>
---
 drivers/gpu/drm/drm_drv.c | 49 ++++++++++++++-------------------------
 1 file changed, 17 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 8214a0b1ab7f..41799e4d0432 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -34,6 +34,7 @@
 #include <linux/pseudo_fs.h>
 #include <linux/slab.h>
 #include <linux/srcu.h>
+#include <linux/xarray.h>
 
 #include <drm/drm_cache.h>
 #include <drm/drm_client.h>
@@ -53,8 +54,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 struct idr drm_minors_idr;
+static DEFINE_XARRAY_ALLOC(drm_minors_xa);
 
 /*
  * If the drm core fails to init for whatever reason,
@@ -98,21 +98,18 @@ 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);
-	idr_remove(&drm_minors_idr, minor->index);
-	spin_unlock_irqrestore(&drm_minor_lock, flags);
+	xa_release(&drm_minors_xa, minor->index);
 }
 
 static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
 {
 	struct drm_minor *minor;
-	unsigned long flags;
+	u32 id;
 	int r;
 
 	minor = drmm_kzalloc(dev, sizeof(*minor), GFP_KERNEL);
@@ -122,20 +119,12 @@ 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,
-		      64 * type,
-		      64 * (type + 1),
-		      GFP_NOWAIT);
-	spin_unlock_irqrestore(&drm_minor_lock, flags);
-	idr_preload_end();
-
+	r = xa_alloc(&drm_minors_xa, &id, NULL,
+		     XA_LIMIT(64 * type, 64 * (type + 1) - 1), GFP_KERNEL);
 	if (r < 0)
 		return r;
 
-	minor->index = r;
+	minor->index = id;
 
 	r = drmm_add_action_or_reset(dev, drm_minor_alloc_release, minor);
 	if (r)
@@ -152,7 +141,7 @@ 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;
+	void *entry;
 	int ret;
 
 	DRM_DEBUG("\n");
@@ -172,9 +161,12 @@ 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);
+	entry = xa_store(&drm_minors_xa, minor->index, &minor, GFP_KERNEL);
+	if (xa_is_err(entry)) {
+		ret = xa_err(entry);
+		goto err_debugfs;
+	}
+	WARN_ON(entry);
 
 	DRM_DEBUG("new minor registered %d\n", minor->index);
 	return 0;
@@ -187,16 +179,13 @@ 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);
-	idr_replace(&drm_minors_idr, NULL, minor->index);
-	spin_unlock_irqrestore(&drm_minor_lock, flags);
+	xa_erase(&drm_minors_xa, minor->index);
 
 	device_del(minor->kdev);
 	dev_set_drvdata(minor->kdev, NULL); /* safety belt */
@@ -215,13 +204,10 @@ 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);
-	minor = idr_find(&drm_minors_idr, minor_id);
+	minor = xa_load(&drm_minors_xa, minor_id);
 	if (minor)
 		drm_dev_get(minor->dev);
-	spin_unlock_irqrestore(&drm_minor_lock, flags);
 
 	if (!minor) {
 		return ERR_PTR(-ENODEV);
@@ -1037,7 +1023,7 @@ static void drm_core_exit(void)
 	unregister_chrdev(DRM_MAJOR, "drm");
 	debugfs_remove(drm_debugfs_root);
 	drm_sysfs_destroy();
-	idr_destroy(&drm_minors_idr);
+	xa_destroy(&drm_minors_xa);
 	drm_connector_ida_destroy();
 }
 
@@ -1046,7 +1032,6 @@ static int __init drm_core_init(void)
 	int ret;
 
 	drm_connector_ida_init();
-	idr_init(&drm_minors_idr);
 	drm_memcpy_init_early();
 
 	ret = drm_sysfs_init();
-- 
2.37.3

[Intel-gfx] [PATCH v4 1/3] drm: Use XArray instead of IDR for minors

[Michał Winiarski]

IDR is deprecated, and since XArray manages its own state with internal
locking, it simplifies the locking on DRM side.
Additionally, don't use the IRQ-safe variant, since operating on drm
minor is not done in IRQ context.

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Suggested-by: Matthew Wilcox <willy@infradead.org>
---
 drivers/gpu/drm/drm_drv.c | 49 ++++++++++++++-------------------------
 1 file changed, 17 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 8214a0b1ab7f..41799e4d0432 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -34,6 +34,7 @@
 #include <linux/pseudo_fs.h>
 #include <linux/slab.h>
 #include <linux/srcu.h>
+#include <linux/xarray.h>
 
 #include <drm/drm_cache.h>
 #include <drm/drm_client.h>
@@ -53,8 +54,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 struct idr drm_minors_idr;
+static DEFINE_XARRAY_ALLOC(drm_minors_xa);
 
 /*
  * If the drm core fails to init for whatever reason,
@@ -98,21 +98,18 @@ 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);
-	idr_remove(&drm_minors_idr, minor->index);
-	spin_unlock_irqrestore(&drm_minor_lock, flags);
+	xa_release(&drm_minors_xa, minor->index);
 }
 
 static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
 {
 	struct drm_minor *minor;
-	unsigned long flags;
+	u32 id;
 	int r;
 
 	minor = drmm_kzalloc(dev, sizeof(*minor), GFP_KERNEL);
@@ -122,20 +119,12 @@ 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,
-		      64 * type,
-		      64 * (type + 1),
-		      GFP_NOWAIT);
-	spin_unlock_irqrestore(&drm_minor_lock, flags);
-	idr_preload_end();
-
+	r = xa_alloc(&drm_minors_xa, &id, NULL,
+		     XA_LIMIT(64 * type, 64 * (type + 1) - 1), GFP_KERNEL);
 	if (r < 0)
 		return r;
 
-	minor->index = r;
+	minor->index = id;
 
 	r = drmm_add_action_or_reset(dev, drm_minor_alloc_release, minor);
 	if (r)
@@ -152,7 +141,7 @@ 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;
+	void *entry;
 	int ret;
 
 	DRM_DEBUG("\n");
@@ -172,9 +161,12 @@ 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);
+	entry = xa_store(&drm_minors_xa, minor->index, &minor, GFP_KERNEL);
+	if (xa_is_err(entry)) {
+		ret = xa_err(entry);
+		goto err_debugfs;
+	}
+	WARN_ON(entry);
 
 	DRM_DEBUG("new minor registered %d\n", minor->index);
 	return 0;
@@ -187,16 +179,13 @@ 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);
-	idr_replace(&drm_minors_idr, NULL, minor->index);
-	spin_unlock_irqrestore(&drm_minor_lock, flags);
+	xa_erase(&drm_minors_xa, minor->index);
 
 	device_del(minor->kdev);
 	dev_set_drvdata(minor->kdev, NULL); /* safety belt */
@@ -215,13 +204,10 @@ 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);
-	minor = idr_find(&drm_minors_idr, minor_id);
+	minor = xa_load(&drm_minors_xa, minor_id);
 	if (minor)
 		drm_dev_get(minor->dev);
-	spin_unlock_irqrestore(&drm_minor_lock, flags);
 
 	if (!minor) {
 		return ERR_PTR(-ENODEV);
@@ -1037,7 +1023,7 @@ static void drm_core_exit(void)
 	unregister_chrdev(DRM_MAJOR, "drm");
 	debugfs_remove(drm_debugfs_root);
 	drm_sysfs_destroy();
-	idr_destroy(&drm_minors_idr);
+	xa_destroy(&drm_minors_xa);
 	drm_connector_ida_destroy();
 }
 
@@ -1046,7 +1032,6 @@ static int __init drm_core_init(void)
 	int ret;
 
 	drm_connector_ida_init();
-	idr_init(&drm_minors_idr);
 	drm_memcpy_init_early();
 
 	ret = drm_sysfs_init();
-- 
2.37.3

[PATCH v4 1/3] drm: Use XArray instead of IDR for minors

[Michał Winiarski]

IDR is deprecated, and since XArray manages its own state with internal
locking, it simplifies the locking on DRM side.
Additionally, don't use the IRQ-safe variant, since operating on drm
minor is not done in IRQ context.

Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
Suggested-by: Matthew Wilcox <willy@infradead.org>
---
 drivers/gpu/drm/drm_drv.c | 49 ++++++++++++++-------------------------
 1 file changed, 17 insertions(+), 32 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 8214a0b1ab7f..41799e4d0432 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -34,6 +34,7 @@
 #include <linux/pseudo_fs.h>
 #include <linux/slab.h>
 #include <linux/srcu.h>
+#include <linux/xarray.h>
 
 #include <drm/drm_cache.h>
 #include <drm/drm_client.h>
@@ -53,8 +54,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 struct idr drm_minors_idr;
+static DEFINE_XARRAY_ALLOC(drm_minors_xa);
 
 /*
  * If the drm core fails to init for whatever reason,
@@ -98,21 +98,18 @@ 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);
-	idr_remove(&drm_minors_idr, minor->index);
-	spin_unlock_irqrestore(&drm_minor_lock, flags);
+	xa_release(&drm_minors_xa, minor->index);
 }
 
 static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
 {
 	struct drm_minor *minor;
-	unsigned long flags;
+	u32 id;
 	int r;
 
 	minor = drmm_kzalloc(dev, sizeof(*minor), GFP_KERNEL);
@@ -122,20 +119,12 @@ 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,
-		      64 * type,
-		      64 * (type + 1),
-		      GFP_NOWAIT);
-	spin_unlock_irqrestore(&drm_minor_lock, flags);
-	idr_preload_end();
-
+	r = xa_alloc(&drm_minors_xa, &id, NULL,
+		     XA_LIMIT(64 * type, 64 * (type + 1) - 1), GFP_KERNEL);
 	if (r < 0)
 		return r;
 
-	minor->index = r;
+	minor->index = id;
 
 	r = drmm_add_action_or_reset(dev, drm_minor_alloc_release, minor);
 	if (r)
@@ -152,7 +141,7 @@ 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;
+	void *entry;
 	int ret;
 
 	DRM_DEBUG("\n");
@@ -172,9 +161,12 @@ 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);
+	entry = xa_store(&drm_minors_xa, minor->index, &minor, GFP_KERNEL);
+	if (xa_is_err(entry)) {
+		ret = xa_err(entry);
+		goto err_debugfs;
+	}
+	WARN_ON(entry);
 
 	DRM_DEBUG("new minor registered %d\n", minor->index);
 	return 0;
@@ -187,16 +179,13 @@ 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);
-	idr_replace(&drm_minors_idr, NULL, minor->index);
-	spin_unlock_irqrestore(&drm_minor_lock, flags);
+	xa_erase(&drm_minors_xa, minor->index);
 
 	device_del(minor->kdev);
 	dev_set_drvdata(minor->kdev, NULL); /* safety belt */
@@ -215,13 +204,10 @@ 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);
-	minor = idr_find(&drm_minors_idr, minor_id);
+	minor = xa_load(&drm_minors_xa, minor_id);
 	if (minor)
 		drm_dev_get(minor->dev);
-	spin_unlock_irqrestore(&drm_minor_lock, flags);
 
 	if (!minor) {
 		return ERR_PTR(-ENODEV);
@@ -1037,7 +1023,7 @@ static void drm_core_exit(void)
 	unregister_chrdev(DRM_MAJOR, "drm");
 	debugfs_remove(drm_debugfs_root);
 	drm_sysfs_destroy();
-	idr_destroy(&drm_minors_idr);
+	xa_destroy(&drm_minors_xa);
 	drm_connector_ida_destroy();
 }
 
@@ -1046,7 +1032,6 @@ static int __init drm_core_init(void)
 	int ret;
 
 	drm_connector_ida_init();
-	idr_init(&drm_minors_idr);
 	drm_memcpy_init_early();
 
 	ret = drm_sysfs_init();
-- 
2.37.3

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

[Michał Winiarski]

Having a limit of 64 DRM devices is 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
numbering scheme where 0-63 is used for primary, 64-127 is reserved
(formerly for control) and 128-191 is used for render.
For minors >= 192, we're allocating minors dynamically on a first-come,
first-served basis.

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

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 41799e4d0432..2c6e0b8d3b7a 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -119,8 +119,17 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
 	minor->type = type;
 	minor->dev = dev;
 
+	/*
+	 * DRM used to support 64 devices, for backwards compatibility we need to maintain the
+	 * minor allocation scheme where minors 0-63 are primary nodes, 64-127 are control nodes,
+	 * and 128-191 are render nodes.
+	 * After reaching the limit, we're allocating minors dynamically - first-come, first-serve.
+	 */
 	r = xa_alloc(&drm_minors_xa, &id, NULL,
 		     XA_LIMIT(64 * type, 64 * (type + 1) - 1), GFP_KERNEL);
+	if (r == -EBUSY)
+		r = xa_alloc(&drm_minors_xa, &id, NULL,
+			     XA_LIMIT(192, (1 << MINORBITS) - 1), GFP_KERNEL);
 	if (r < 0)
 		return r;
 
-- 
2.37.3

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

[Michał Winiarski]

Having a limit of 64 DRM devices is 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
numbering scheme where 0-63 is used for primary, 64-127 is reserved
(formerly for control) and 128-191 is used for render.
For minors >= 192, we're allocating minors dynamically on a first-come,
first-served basis.

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

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 41799e4d0432..2c6e0b8d3b7a 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -119,8 +119,17 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
 	minor->type = type;
 	minor->dev = dev;
 
+	/*
+	 * DRM used to support 64 devices, for backwards compatibility we need to maintain the
+	 * minor allocation scheme where minors 0-63 are primary nodes, 64-127 are control nodes,
+	 * and 128-191 are render nodes.
+	 * After reaching the limit, we're allocating minors dynamically - first-come, first-serve.
+	 */
 	r = xa_alloc(&drm_minors_xa, &id, NULL,
 		     XA_LIMIT(64 * type, 64 * (type + 1) - 1), GFP_KERNEL);
+	if (r == -EBUSY)
+		r = xa_alloc(&drm_minors_xa, &id, NULL,
+			     XA_LIMIT(192, (1 << MINORBITS) - 1), GFP_KERNEL);
 	if (r < 0)
 		return r;
 
-- 
2.37.3

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

[Michał Winiarski]

Having a limit of 64 DRM devices is 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
numbering scheme where 0-63 is used for primary, 64-127 is reserved
(formerly for control) and 128-191 is used for render.
For minors >= 192, we're allocating minors dynamically on a first-come,
first-served basis.

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

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 41799e4d0432..2c6e0b8d3b7a 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -119,8 +119,17 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
 	minor->type = type;
 	minor->dev = dev;
 
+	/*
+	 * DRM used to support 64 devices, for backwards compatibility we need to maintain the
+	 * minor allocation scheme where minors 0-63 are primary nodes, 64-127 are control nodes,
+	 * and 128-191 are render nodes.
+	 * After reaching the limit, we're allocating minors dynamically - first-come, first-serve.
+	 */
 	r = xa_alloc(&drm_minors_xa, &id, NULL,
 		     XA_LIMIT(64 * type, 64 * (type + 1) - 1), GFP_KERNEL);
+	if (r == -EBUSY)
+		r = xa_alloc(&drm_minors_xa, &id, NULL,
+			     XA_LIMIT(192, (1 << MINORBITS) - 1), GFP_KERNEL);
 	if (r < 0)
 		return r;
 
-- 
2.37.3

[PATCH v4 3/3] drm: Introduce skip_legacy_minors modparam

[Michał Winiarski]

While there is support for >64 DRM devices on kernel side, existing
userspace may still have some hardcoded assumptions and it's possible
that it will require changes to be able to use more than 64 devices.
Add a modparam to simplify testing and development of >64 devices
support on userspace side by allocating minors from the >=192 range
(without the need of having >64 physical devices connected).

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

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 2c6e0b8d3b7a..11c691543fec 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -56,6 +56,11 @@ MODULE_LICENSE("GPL and additional rights");
 
 static DEFINE_XARRAY_ALLOC(drm_minors_xa);
 
+static bool skip_legacy_minors;
+module_param_unsafe(skip_legacy_minors, bool, 0400);
+MODULE_PARM_DESC(skip_legacy_minors,
+		 "Don't allocate minors in 0-192 range. This can be used for testing userspace support for >64 drm devices (default: false)");
+
 /*
  * If the drm core fails to init for whatever reason,
  * we should prevent any drivers from registering with it.
@@ -110,7 +115,7 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
 {
 	struct drm_minor *minor;
 	u32 id;
-	int r;
+	int r = -EBUSY;
 
 	minor = drmm_kzalloc(dev, sizeof(*minor), GFP_KERNEL);
 	if (!minor)
@@ -125,8 +130,9 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
 	 * and 128-191 are render nodes.
 	 * After reaching the limit, we're allocating minors dynamically - first-come, first-serve.
 	 */
-	r = xa_alloc(&drm_minors_xa, &id, NULL,
-		     XA_LIMIT(64 * type, 64 * (type + 1) - 1), GFP_KERNEL);
+	if (!skip_legacy_minors)
+		r = xa_alloc(&drm_minors_xa, &id, NULL,
+			     XA_LIMIT(64 * type, 64 * (type + 1) - 1), GFP_KERNEL);
 	if (r == -EBUSY)
 		r = xa_alloc(&drm_minors_xa, &id, NULL,
 			     XA_LIMIT(192, (1 << MINORBITS) - 1), GFP_KERNEL);
-- 
2.37.3

[Intel-gfx] [PATCH v4 3/3] drm: Introduce skip_legacy_minors modparam

[Michał Winiarski]

While there is support for >64 DRM devices on kernel side, existing
userspace may still have some hardcoded assumptions and it's possible
that it will require changes to be able to use more than 64 devices.
Add a modparam to simplify testing and development of >64 devices
support on userspace side by allocating minors from the >=192 range
(without the need of having >64 physical devices connected).

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

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 2c6e0b8d3b7a..11c691543fec 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -56,6 +56,11 @@ MODULE_LICENSE("GPL and additional rights");
 
 static DEFINE_XARRAY_ALLOC(drm_minors_xa);
 
+static bool skip_legacy_minors;
+module_param_unsafe(skip_legacy_minors, bool, 0400);
+MODULE_PARM_DESC(skip_legacy_minors,
+		 "Don't allocate minors in 0-192 range. This can be used for testing userspace support for >64 drm devices (default: false)");
+
 /*
  * If the drm core fails to init for whatever reason,
  * we should prevent any drivers from registering with it.
@@ -110,7 +115,7 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
 {
 	struct drm_minor *minor;
 	u32 id;
-	int r;
+	int r = -EBUSY;
 
 	minor = drmm_kzalloc(dev, sizeof(*minor), GFP_KERNEL);
 	if (!minor)
@@ -125,8 +130,9 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
 	 * and 128-191 are render nodes.
 	 * After reaching the limit, we're allocating minors dynamically - first-come, first-serve.
 	 */
-	r = xa_alloc(&drm_minors_xa, &id, NULL,
-		     XA_LIMIT(64 * type, 64 * (type + 1) - 1), GFP_KERNEL);
+	if (!skip_legacy_minors)
+		r = xa_alloc(&drm_minors_xa, &id, NULL,
+			     XA_LIMIT(64 * type, 64 * (type + 1) - 1), GFP_KERNEL);
 	if (r == -EBUSY)
 		r = xa_alloc(&drm_minors_xa, &id, NULL,
 			     XA_LIMIT(192, (1 << MINORBITS) - 1), GFP_KERNEL);
-- 
2.37.3

[PATCH v4 3/3] drm: Introduce skip_legacy_minors modparam

[Michał Winiarski]

While there is support for >64 DRM devices on kernel side, existing
userspace may still have some hardcoded assumptions and it's possible
that it will require changes to be able to use more than 64 devices.
Add a modparam to simplify testing and development of >64 devices
support on userspace side by allocating minors from the >=192 range
(without the need of having >64 physical devices connected).

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

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 2c6e0b8d3b7a..11c691543fec 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -56,6 +56,11 @@ MODULE_LICENSE("GPL and additional rights");
 
 static DEFINE_XARRAY_ALLOC(drm_minors_xa);
 
+static bool skip_legacy_minors;
+module_param_unsafe(skip_legacy_minors, bool, 0400);
+MODULE_PARM_DESC(skip_legacy_minors,
+		 "Don't allocate minors in 0-192 range. This can be used for testing userspace support for >64 drm devices (default: false)");
+
 /*
  * If the drm core fails to init for whatever reason,
  * we should prevent any drivers from registering with it.
@@ -110,7 +115,7 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
 {
 	struct drm_minor *minor;
 	u32 id;
-	int r;
+	int r = -EBUSY;
 
 	minor = drmm_kzalloc(dev, sizeof(*minor), GFP_KERNEL);
 	if (!minor)
@@ -125,8 +130,9 @@ static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
 	 * and 128-191 are render nodes.
 	 * After reaching the limit, we're allocating minors dynamically - first-come, first-serve.
 	 */
-	r = xa_alloc(&drm_minors_xa, &id, NULL,
-		     XA_LIMIT(64 * type, 64 * (type + 1) - 1), GFP_KERNEL);
+	if (!skip_legacy_minors)
+		r = xa_alloc(&drm_minors_xa, &id, NULL,
+			     XA_LIMIT(64 * type, 64 * (type + 1) - 1), GFP_KERNEL);
 	if (r == -EBUSY)
 		r = xa_alloc(&drm_minors_xa, &id, NULL,
 			     XA_LIMIT(192, (1 << MINORBITS) - 1), GFP_KERNEL);
-- 
2.37.3

[Intel-gfx] ✗ Fi.CI.BAT: failure for drm: Use full allocated minor range for DRM

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 16816 bytes --]

== Series Details ==

Series: drm: Use full allocated minor range for DRM
URL   : https://patchwork.freedesktop.org/series/108206/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_12081 -> Patchwork_108206v1
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_108206v1 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_108206v1, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/index.html

Participating hosts (41 -> 41)
------------------------------

  Additional (2): fi-rkl-11600 fi-snb-2520m 
  Missing    (2): fi-icl-u2 fi-bdw-samus 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_108206v1:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_module_load@load:
    - fi-ilk-650:         [PASS][1] -> [INCOMPLETE][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12081/fi-ilk-650/igt@i915_module_load@load.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/fi-ilk-650/igt@i915_module_load@load.html
    - fi-blb-e6850:       [PASS][3] -> [INCOMPLETE][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12081/fi-blb-e6850/igt@i915_module_load@load.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/fi-blb-e6850/igt@i915_module_load@load.html
    - fi-rkl-11600:       NOTRUN -> [INCOMPLETE][5]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/fi-rkl-11600/igt@i915_module_load@load.html
    - fi-skl-6600u:       [PASS][6] -> [INCOMPLETE][7]
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12081/fi-skl-6600u/igt@i915_module_load@load.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/fi-skl-6600u/igt@i915_module_load@load.html
    - fi-bdw-gvtdvm:      [PASS][8] -> [INCOMPLETE][9]
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12081/fi-bdw-gvtdvm/igt@i915_module_load@load.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/fi-bdw-gvtdvm/igt@i915_module_load@load.html
    - fi-pnv-d510:        [PASS][10] -> [INCOMPLETE][11]
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12081/fi-pnv-d510/igt@i915_module_load@load.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/fi-pnv-d510/igt@i915_module_load@load.html
    - fi-snb-2520m:       NOTRUN -> [INCOMPLETE][12]
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/fi-snb-2520m/igt@i915_module_load@load.html
    - fi-glk-j4005:       [PASS][13] -> [INCOMPLETE][14]
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12081/fi-glk-j4005/igt@i915_module_load@load.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/fi-glk-j4005/igt@i915_module_load@load.html
    - fi-rkl-guc:         [PASS][15] -> [INCOMPLETE][16]
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12081/fi-rkl-guc/igt@i915_module_load@load.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/fi-rkl-guc/igt@i915_module_load@load.html
    - fi-skl-6700k2:      [PASS][17] -> [INCOMPLETE][18]
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12081/fi-skl-6700k2/igt@i915_module_load@load.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/fi-skl-6700k2/igt@i915_module_load@load.html
    - fi-kbl-7567u:       [PASS][19] -> [INCOMPLETE][20]
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12081/fi-kbl-7567u/igt@i915_module_load@load.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/fi-kbl-7567u/igt@i915_module_load@load.html
    - fi-cfl-8700k:       [PASS][21] -> [INCOMPLETE][22]
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12081/fi-cfl-8700k/igt@i915_module_load@load.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/fi-cfl-8700k/igt@i915_module_load@load.html
    - fi-elk-e7500:       [PASS][23] -> [INCOMPLETE][24]
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12081/fi-elk-e7500/igt@i915_module_load@load.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/fi-elk-e7500/igt@i915_module_load@load.html
    - fi-bsw-nick:        [PASS][25] -> [INCOMPLETE][26]
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12081/fi-bsw-nick/igt@i915_module_load@load.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/fi-bsw-nick/igt@i915_module_load@load.html
    - fi-hsw-g3258:       [PASS][27] -> [INCOMPLETE][28]
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12081/fi-hsw-g3258/igt@i915_module_load@load.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/fi-hsw-g3258/igt@i915_module_load@load.html
    - fi-bsw-kefka:       [PASS][29] -> [INCOMPLETE][30]
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12081/fi-bsw-kefka/igt@i915_module_load@load.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/fi-bsw-kefka/igt@i915_module_load@load.html
    - fi-adl-ddr5:        [PASS][31] -> [INCOMPLETE][32]
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12081/fi-adl-ddr5/igt@i915_module_load@load.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/fi-adl-ddr5/igt@i915_module_load@load.html
    - fi-cfl-guc:         [PASS][33] -> [INCOMPLETE][34]
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12081/fi-cfl-guc/igt@i915_module_load@load.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/fi-cfl-guc/igt@i915_module_load@load.html
    - fi-glk-dsi:         [PASS][35] -> [INCOMPLETE][36]
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12081/fi-glk-dsi/igt@i915_module_load@load.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/fi-glk-dsi/igt@i915_module_load@load.html
    - fi-bdw-5557u:       [PASS][37] -> [INCOMPLETE][38]
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12081/fi-bdw-5557u/igt@i915_module_load@load.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/fi-bdw-5557u/igt@i915_module_load@load.html
    - fi-hsw-4770:        [PASS][39] -> [INCOMPLETE][40]
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12081/fi-hsw-4770/igt@i915_module_load@load.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/fi-hsw-4770/igt@i915_module_load@load.html
    - fi-cfl-8109u:       [PASS][41] -> [INCOMPLETE][42]
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12081/fi-cfl-8109u/igt@i915_module_load@load.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/fi-cfl-8109u/igt@i915_module_load@load.html
    - bat-adlp-4:         [PASS][43] -> [INCOMPLETE][44]
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12081/bat-adlp-4/igt@i915_module_load@load.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/bat-adlp-4/igt@i915_module_load@load.html
    - fi-ivb-3770:        [PASS][45] -> [INCOMPLETE][46]
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12081/fi-ivb-3770/igt@i915_module_load@load.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/fi-ivb-3770/igt@i915_module_load@load.html
    - fi-snb-2600:        [PASS][47] -> [INCOMPLETE][48]
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12081/fi-snb-2600/igt@i915_module_load@load.html
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/fi-snb-2600/igt@i915_module_load@load.html

  
#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * igt@i915_module_load@load:
    - {fi-jsl-1}:         [PASS][49] -> [INCOMPLETE][50]
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12081/fi-jsl-1/igt@i915_module_load@load.html
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/fi-jsl-1/igt@i915_module_load@load.html
    - {bat-rpls-1}:       [PASS][51] -> [INCOMPLETE][52]
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12081/bat-rpls-1/igt@i915_module_load@load.html
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/bat-rpls-1/igt@i915_module_load@load.html
    - {fi-ehl-2}:         [PASS][53] -> [INCOMPLETE][54]
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12081/fi-ehl-2/igt@i915_module_load@load.html
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/fi-ehl-2/igt@i915_module_load@load.html
    - {bat-dg2-11}:       [PASS][55] -> [INCOMPLETE][56]
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12081/bat-dg2-11/igt@i915_module_load@load.html
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/bat-dg2-11/igt@i915_module_load@load.html
    - {bat-adln-1}:       [PASS][57] -> [INCOMPLETE][58]
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12081/bat-adln-1/igt@i915_module_load@load.html
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/bat-adln-1/igt@i915_module_load@load.html
    - {bat-rplp-1}:       [PASS][59] -> [INCOMPLETE][60]
   [59]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12081/bat-rplp-1/igt@i915_module_load@load.html
   [60]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/bat-rplp-1/igt@i915_module_load@load.html
    - {bat-dg2-9}:        [PASS][61] -> [INCOMPLETE][62]
   [61]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12081/bat-dg2-9/igt@i915_module_load@load.html
   [62]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/bat-dg2-9/igt@i915_module_load@load.html
    - {bat-rpls-2}:       [PASS][63] -> [INCOMPLETE][64]
   [63]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12081/bat-rpls-2/igt@i915_module_load@load.html
   [64]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/bat-rpls-2/igt@i915_module_load@load.html
    - {bat-dg2-8}:        [PASS][65] -> [INCOMPLETE][66]
   [65]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12081/bat-dg2-8/igt@i915_module_load@load.html
   [66]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/bat-dg2-8/igt@i915_module_load@load.html

  
Known issues
------------

  Here are the changes found in Patchwork_108206v1 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@runner@aborted:
    - fi-skl-6700k2:      NOTRUN -> [FAIL][67] ([i915#4312] / [i915#6599])
   [67]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/fi-skl-6700k2/igt@runner@aborted.html
    - fi-cfl-8109u:       NOTRUN -> [FAIL][68] ([i915#4312] / [i915#6599])
   [68]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/fi-cfl-8109u/igt@runner@aborted.html
    - bat-adlp-4:         NOTRUN -> [FAIL][69] ([i915#4312] / [i915#6641])
   [69]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/bat-adlp-4/igt@runner@aborted.html
    - fi-ivb-3770:        NOTRUN -> [FAIL][70] ([i915#4312] / [i915#6219])
   [70]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/fi-ivb-3770/igt@runner@aborted.html
    - fi-elk-e7500:       NOTRUN -> [FAIL][71] ([i915#4312])
   [71]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/fi-elk-e7500/igt@runner@aborted.html
    - fi-snb-2600:        NOTRUN -> [FAIL][72] ([i915#4312])
   [72]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/fi-snb-2600/igt@runner@aborted.html
    - fi-ilk-650:         NOTRUN -> [FAIL][73] ([i915#4312] / [i915#4991])
   [73]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/fi-ilk-650/igt@runner@aborted.html
    - fi-skl-6600u:       NOTRUN -> [FAIL][74] ([i915#4312] / [i915#6599])
   [74]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/fi-skl-6600u/igt@runner@aborted.html
    - fi-glk-dsi:         NOTRUN -> [FAIL][75] ([i915#4312] / [i915#6599])
   [75]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/fi-glk-dsi/igt@runner@aborted.html
    - fi-pnv-d510:        NOTRUN -> [FAIL][76] ([i915#2403] / [i915#4312])
   [76]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/fi-pnv-d510/igt@runner@aborted.html
    - fi-bdw-5557u:       NOTRUN -> [FAIL][77] ([i915#4312])
   [77]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/fi-bdw-5557u/igt@runner@aborted.html
    - fi-snb-2520m:       NOTRUN -> [FAIL][78] ([i915#4312])
   [78]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/fi-snb-2520m/igt@runner@aborted.html
    - fi-glk-j4005:       NOTRUN -> [FAIL][79] ([i915#4312] / [i915#6599])
   [79]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/fi-glk-j4005/igt@runner@aborted.html
    - fi-rkl-guc:         NOTRUN -> [FAIL][80] ([i915#4312] / [i915#6599])
   [80]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/fi-rkl-guc/igt@runner@aborted.html
    - fi-kbl-7567u:       NOTRUN -> [FAIL][81] ([i915#4312] / [i915#6219])
   [81]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/fi-kbl-7567u/igt@runner@aborted.html
    - fi-cfl-8700k:       NOTRUN -> [FAIL][82] ([i915#4312] / [i915#6599])
   [82]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/fi-cfl-8700k/igt@runner@aborted.html
    - fi-bsw-nick:        NOTRUN -> [FAIL][83] ([i915#4312])
   [83]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/fi-bsw-nick/igt@runner@aborted.html
    - fi-rkl-11600:       NOTRUN -> [FAIL][84] ([i915#4312] / [i915#6599])
   [84]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/fi-rkl-11600/igt@runner@aborted.html
    - fi-hsw-g3258:       NOTRUN -> [FAIL][85] ([i915#4312] / [i915#6246])
   [85]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/fi-hsw-g3258/igt@runner@aborted.html
    - fi-bdw-gvtdvm:      NOTRUN -> [FAIL][86] ([i915#4312])
   [86]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/fi-bdw-gvtdvm/igt@runner@aborted.html
    - fi-bsw-kefka:       NOTRUN -> [FAIL][87] ([i915#4312])
   [87]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/fi-bsw-kefka/igt@runner@aborted.html
    - fi-adl-ddr5:        NOTRUN -> [FAIL][88] ([i915#4312] / [i915#6599])
   [88]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/fi-adl-ddr5/igt@runner@aborted.html
    - fi-cfl-guc:         NOTRUN -> [FAIL][89] ([i915#4312] / [i915#6599])
   [89]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/fi-cfl-guc/igt@runner@aborted.html

  
#### Warnings ####

  * igt@runner@aborted:
    - fi-hsw-4770:        [FAIL][90] ([fdo#109271] / [i915#4312] / [i915#5594] / [i915#6246]) -> [FAIL][91] ([i915#4312] / [i915#5594] / [i915#6246])
   [90]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12081/fi-hsw-4770/igt@runner@aborted.html
   [91]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/fi-hsw-4770/igt@runner@aborted.html
    - fi-blb-e6850:       [FAIL][92] ([fdo#109271] / [i915#2403] / [i915#4312]) -> [FAIL][93] ([i915#2403] / [i915#4312])
   [92]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12081/fi-blb-e6850/igt@runner@aborted.html
   [93]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/fi-blb-e6850/igt@runner@aborted.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [i915#2403]: https://gitlab.freedesktop.org/drm/intel/issues/2403
  [i915#4312]: https://gitlab.freedesktop.org/drm/intel/issues/4312
  [i915#4991]: https://gitlab.freedesktop.org/drm/intel/issues/4991
  [i915#5594]: https://gitlab.freedesktop.org/drm/intel/issues/5594
  [i915#6219]: https://gitlab.freedesktop.org/drm/intel/issues/6219
  [i915#6246]: https://gitlab.freedesktop.org/drm/intel/issues/6246
  [i915#6599]: https://gitlab.freedesktop.org/drm/intel/issues/6599
  [i915#6641]: https://gitlab.freedesktop.org/drm/intel/issues/6641


Build changes
-------------

  * Linux: CI_DRM_12081 -> Patchwork_108206v1

  CI-20190529: 20190529
  CI_DRM_12081: ef7295a11c7673ae29b70cec4ce36ee91855a740 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_6646: c1f420ae84b76079cd32ac014cfdf95b5f2922f7 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_108206v1: ef7295a11c7673ae29b70cec4ce36ee91855a740 @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

eb3923073e94 drm: Introduce skip_legacy_minors modparam
0ffff27c26aa drm: Expand max DRM device number to full MINORBITS
9d3acd2d8f7f drm: Use XArray instead of IDR for minors

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_108206v1/index.html

[-- Attachment #2: Type: text/html, Size: 21066 bytes --]

Re: [PATCH v4 1/3] drm: Use XArray instead of IDR for minors

[Matthew Wilcox]

On Tue, Sep 06, 2022 at 10:16:27PM +0200, Michał Winiarski wrote:
> IDR is deprecated, and since XArray manages its own state with internal
> locking, it simplifies the locking on DRM side.
> Additionally, don't use the IRQ-safe variant, since operating on drm
> minor is not done in IRQ context.
> 
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> Suggested-by: Matthew Wilcox <willy@infradead.org>

I have a few questions, but I like where you're going.

> @@ -98,21 +98,18 @@ 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);
> -	idr_remove(&drm_minors_idr, minor->index);
> -	spin_unlock_irqrestore(&drm_minor_lock, flags);
> +	xa_release(&drm_minors_xa, minor->index);

Has it definitely been unused at this point?  I would think that
xa_erase() (an unconditional store) would be the correct function to
call.

> @@ -122,20 +119,12 @@ 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,
> -		      64 * type,
> -		      64 * (type + 1),
> -		      GFP_NOWAIT);
> -	spin_unlock_irqrestore(&drm_minor_lock, flags);
> -	idr_preload_end();
> -
> +	r = xa_alloc(&drm_minors_xa, &id, NULL,
> +		     XA_LIMIT(64 * type, 64 * (type + 1) - 1), GFP_KERNEL);
>  	if (r < 0)
>  		return r;
>  
> -	minor->index = r;
> +	minor->index = id;

Wouldn't it be better to call:

	r = xa_alloc(&drm_minors_xa, &minor->index, NULL,
			XA_LIMIT(64 * type, 64 * (type + 1) - 1), GFP_KERNEL);

I might also prefer a little syntactic sugar like:

#define DRM_MINOR_LIMIT(type)	XA_LIMIT(64 * (type), 64 * (type) + 63)

but that's definitely a matter of taste.

> @@ -172,9 +161,12 @@ 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);
> +	entry = xa_store(&drm_minors_xa, minor->index, &minor, GFP_KERNEL);
> +	if (xa_is_err(entry)) {
> +		ret = xa_err(entry);
> +		goto err_debugfs;
> +	}
> +	WARN_ON(entry);

Might be better as an xa_cmpxchg()?

> @@ -187,16 +179,13 @@ 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);
> -	idr_replace(&drm_minors_idr, NULL, minor->index);
> -	spin_unlock_irqrestore(&drm_minor_lock, flags);
> +	xa_erase(&drm_minors_xa, minor->index);

This isn't an exact replacement, but I'm not sure whether that makes a
difference.  xa_erase() allows allocation of this ID again while
idr_replace() means that lookups return NULL, but the ID remains in
use.  The equivalent of idr_replace() is:
	xa_store(&drm_minors_xa, minor->index, NULL, GFP_KERNEL);

> @@ -215,13 +204,10 @@ 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);
> -	minor = idr_find(&drm_minors_idr, minor_id);
> +	minor = xa_load(&drm_minors_xa, minor_id);
>  	if (minor)
>  		drm_dev_get(minor->dev);
> -	spin_unlock_irqrestore(&drm_minor_lock, flags);

This is also not an exact equivalent as the dev_drm_get() is now outside
the lock.  Does that matter in this case?  I don't know the code well
enough to say.  If you want it to be equivalent, then:

	xa_lock(&drm_minors_xa);
	minor = xa_load(&drm_minors_xa, minor_id);
	if (minor)
		drm_dev_get(minor->dev);
	xa_unlock(&drm_minors_xa);

would be the code to use.

> @@ -1037,7 +1023,7 @@ static void drm_core_exit(void)
>  	unregister_chrdev(DRM_MAJOR, "drm");
>  	debugfs_remove(drm_debugfs_root);
>  	drm_sysfs_destroy();
> -	idr_destroy(&drm_minors_idr);
> +	xa_destroy(&drm_minors_xa);

I don't know if this is the right call.  xa_destroy() is the exact
replacement, but if the xarray should already be empty (and it should,
right?) then asserting the xa_empty() is true may be the better call
to make.


Phew, that was a lot of comments/questions.  I hope that was useful!

Re: [PATCH v4 1/3] drm: Use XArray instead of IDR for minors

[Matthew Wilcox]

On Tue, Sep 06, 2022 at 10:16:27PM +0200, Michał Winiarski wrote:
> IDR is deprecated, and since XArray manages its own state with internal
> locking, it simplifies the locking on DRM side.
> Additionally, don't use the IRQ-safe variant, since operating on drm
> minor is not done in IRQ context.
> 
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> Suggested-by: Matthew Wilcox <willy@infradead.org>

I have a few questions, but I like where you're going.

> @@ -98,21 +98,18 @@ 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);
> -	idr_remove(&drm_minors_idr, minor->index);
> -	spin_unlock_irqrestore(&drm_minor_lock, flags);
> +	xa_release(&drm_minors_xa, minor->index);

Has it definitely been unused at this point?  I would think that
xa_erase() (an unconditional store) would be the correct function to
call.

> @@ -122,20 +119,12 @@ 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,
> -		      64 * type,
> -		      64 * (type + 1),
> -		      GFP_NOWAIT);
> -	spin_unlock_irqrestore(&drm_minor_lock, flags);
> -	idr_preload_end();
> -
> +	r = xa_alloc(&drm_minors_xa, &id, NULL,
> +		     XA_LIMIT(64 * type, 64 * (type + 1) - 1), GFP_KERNEL);
>  	if (r < 0)
>  		return r;
>  
> -	minor->index = r;
> +	minor->index = id;

Wouldn't it be better to call:

	r = xa_alloc(&drm_minors_xa, &minor->index, NULL,
			XA_LIMIT(64 * type, 64 * (type + 1) - 1), GFP_KERNEL);

I might also prefer a little syntactic sugar like:

#define DRM_MINOR_LIMIT(type)	XA_LIMIT(64 * (type), 64 * (type) + 63)

but that's definitely a matter of taste.

> @@ -172,9 +161,12 @@ 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);
> +	entry = xa_store(&drm_minors_xa, minor->index, &minor, GFP_KERNEL);
> +	if (xa_is_err(entry)) {
> +		ret = xa_err(entry);
> +		goto err_debugfs;
> +	}
> +	WARN_ON(entry);

Might be better as an xa_cmpxchg()?

> @@ -187,16 +179,13 @@ 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);
> -	idr_replace(&drm_minors_idr, NULL, minor->index);
> -	spin_unlock_irqrestore(&drm_minor_lock, flags);
> +	xa_erase(&drm_minors_xa, minor->index);

This isn't an exact replacement, but I'm not sure whether that makes a
difference.  xa_erase() allows allocation of this ID again while
idr_replace() means that lookups return NULL, but the ID remains in
use.  The equivalent of idr_replace() is:
	xa_store(&drm_minors_xa, minor->index, NULL, GFP_KERNEL);

> @@ -215,13 +204,10 @@ 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);
> -	minor = idr_find(&drm_minors_idr, minor_id);
> +	minor = xa_load(&drm_minors_xa, minor_id);
>  	if (minor)
>  		drm_dev_get(minor->dev);
> -	spin_unlock_irqrestore(&drm_minor_lock, flags);

This is also not an exact equivalent as the dev_drm_get() is now outside
the lock.  Does that matter in this case?  I don't know the code well
enough to say.  If you want it to be equivalent, then:

	xa_lock(&drm_minors_xa);
	minor = xa_load(&drm_minors_xa, minor_id);
	if (minor)
		drm_dev_get(minor->dev);
	xa_unlock(&drm_minors_xa);

would be the code to use.

> @@ -1037,7 +1023,7 @@ static void drm_core_exit(void)
>  	unregister_chrdev(DRM_MAJOR, "drm");
>  	debugfs_remove(drm_debugfs_root);
>  	drm_sysfs_destroy();
> -	idr_destroy(&drm_minors_idr);
> +	xa_destroy(&drm_minors_xa);

I don't know if this is the right call.  xa_destroy() is the exact
replacement, but if the xarray should already be empty (and it should,
right?) then asserting the xa_empty() is true may be the better call
to make.


Phew, that was a lot of comments/questions.  I hope that was useful!

Re: [Intel-gfx] [PATCH v4 1/3] drm: Use XArray instead of IDR for minors

[Matthew Wilcox]

On Tue, Sep 06, 2022 at 10:16:27PM +0200, Michał Winiarski wrote:
> IDR is deprecated, and since XArray manages its own state with internal
> locking, it simplifies the locking on DRM side.
> Additionally, don't use the IRQ-safe variant, since operating on drm
> minor is not done in IRQ context.
> 
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> Suggested-by: Matthew Wilcox <willy@infradead.org>

I have a few questions, but I like where you're going.

> @@ -98,21 +98,18 @@ 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);
> -	idr_remove(&drm_minors_idr, minor->index);
> -	spin_unlock_irqrestore(&drm_minor_lock, flags);
> +	xa_release(&drm_minors_xa, minor->index);

Has it definitely been unused at this point?  I would think that
xa_erase() (an unconditional store) would be the correct function to
call.

> @@ -122,20 +119,12 @@ 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,
> -		      64 * type,
> -		      64 * (type + 1),
> -		      GFP_NOWAIT);
> -	spin_unlock_irqrestore(&drm_minor_lock, flags);
> -	idr_preload_end();
> -
> +	r = xa_alloc(&drm_minors_xa, &id, NULL,
> +		     XA_LIMIT(64 * type, 64 * (type + 1) - 1), GFP_KERNEL);
>  	if (r < 0)
>  		return r;
>  
> -	minor->index = r;
> +	minor->index = id;

Wouldn't it be better to call:

	r = xa_alloc(&drm_minors_xa, &minor->index, NULL,
			XA_LIMIT(64 * type, 64 * (type + 1) - 1), GFP_KERNEL);

I might also prefer a little syntactic sugar like:

#define DRM_MINOR_LIMIT(type)	XA_LIMIT(64 * (type), 64 * (type) + 63)

but that's definitely a matter of taste.

> @@ -172,9 +161,12 @@ 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);
> +	entry = xa_store(&drm_minors_xa, minor->index, &minor, GFP_KERNEL);
> +	if (xa_is_err(entry)) {
> +		ret = xa_err(entry);
> +		goto err_debugfs;
> +	}
> +	WARN_ON(entry);

Might be better as an xa_cmpxchg()?

> @@ -187,16 +179,13 @@ 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);
> -	idr_replace(&drm_minors_idr, NULL, minor->index);
> -	spin_unlock_irqrestore(&drm_minor_lock, flags);
> +	xa_erase(&drm_minors_xa, minor->index);

This isn't an exact replacement, but I'm not sure whether that makes a
difference.  xa_erase() allows allocation of this ID again while
idr_replace() means that lookups return NULL, but the ID remains in
use.  The equivalent of idr_replace() is:
	xa_store(&drm_minors_xa, minor->index, NULL, GFP_KERNEL);

> @@ -215,13 +204,10 @@ 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);
> -	minor = idr_find(&drm_minors_idr, minor_id);
> +	minor = xa_load(&drm_minors_xa, minor_id);
>  	if (minor)
>  		drm_dev_get(minor->dev);
> -	spin_unlock_irqrestore(&drm_minor_lock, flags);

This is also not an exact equivalent as the dev_drm_get() is now outside
the lock.  Does that matter in this case?  I don't know the code well
enough to say.  If you want it to be equivalent, then:

	xa_lock(&drm_minors_xa);
	minor = xa_load(&drm_minors_xa, minor_id);
	if (minor)
		drm_dev_get(minor->dev);
	xa_unlock(&drm_minors_xa);

would be the code to use.

> @@ -1037,7 +1023,7 @@ static void drm_core_exit(void)
>  	unregister_chrdev(DRM_MAJOR, "drm");
>  	debugfs_remove(drm_debugfs_root);
>  	drm_sysfs_destroy();
> -	idr_destroy(&drm_minors_idr);
> +	xa_destroy(&drm_minors_xa);

I don't know if this is the right call.  xa_destroy() is the exact
replacement, but if the xarray should already be empty (and it should,
right?) then asserting the xa_empty() is true may be the better call
to make.


Phew, that was a lot of comments/questions.  I hope that was useful!

Re: [PATCH v4 1/3] drm: Use XArray instead of IDR for minors

[Michał Winiarski]

On Tue, Sep 06, 2022 at 10:02:24PM +0100, Matthew Wilcox wrote:
> On Tue, Sep 06, 2022 at 10:16:27PM +0200, Michał Winiarski wrote:
> > IDR is deprecated, and since XArray manages its own state with internal
> > locking, it simplifies the locking on DRM side.
> > Additionally, don't use the IRQ-safe variant, since operating on drm
> > minor is not done in IRQ context.
> > 
> > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> > Suggested-by: Matthew Wilcox <willy@infradead.org>
> 
> I have a few questions, but I like where you're going.
> 
> > @@ -98,21 +98,18 @@ 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);
> > -	idr_remove(&drm_minors_idr, minor->index);
> > -	spin_unlock_irqrestore(&drm_minor_lock, flags);
> > +	xa_release(&drm_minors_xa, minor->index);
> 
> Has it definitely been unused at this point?  I would think that
> xa_erase() (an unconditional store) would be the correct function to
> call.

Yes, unless there's a programmers error somewhere - I'll replace it though.

> 
> > @@ -122,20 +119,12 @@ 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,
> > -		      64 * type,
> > -		      64 * (type + 1),
> > -		      GFP_NOWAIT);
> > -	spin_unlock_irqrestore(&drm_minor_lock, flags);
> > -	idr_preload_end();
> > -
> > +	r = xa_alloc(&drm_minors_xa, &id, NULL,
> > +		     XA_LIMIT(64 * type, 64 * (type + 1) - 1), GFP_KERNEL);
> >  	if (r < 0)
> >  		return r;
> >  
> > -	minor->index = r;
> > +	minor->index = id;
> 
> Wouldn't it be better to call:
> 
> 	r = xa_alloc(&drm_minors_xa, &minor->index, NULL,
> 			XA_LIMIT(64 * type, 64 * (type + 1) - 1), GFP_KERNEL);
> 
> I might also prefer a little syntactic sugar like:
> 
> #define DRM_MINOR_LIMIT(type)	XA_LIMIT(64 * (type), 64 * (type) + 63)
> 
> but that's definitely a matter of taste.

Sure.

> 
> > @@ -172,9 +161,12 @@ 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);
> > +	entry = xa_store(&drm_minors_xa, minor->index, &minor, GFP_KERNEL);
> > +	if (xa_is_err(entry)) {
> > +		ret = xa_err(entry);
> > +		goto err_debugfs;
> > +	}
> > +	WARN_ON(entry);
> 
> Might be better as an xa_cmpxchg()?

Ack.

> 
> > @@ -187,16 +179,13 @@ 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);
> > -	idr_replace(&drm_minors_idr, NULL, minor->index);
> > -	spin_unlock_irqrestore(&drm_minor_lock, flags);
> > +	xa_erase(&drm_minors_xa, minor->index);
> 
> This isn't an exact replacement, but I'm not sure whether that makes a
> difference.  xa_erase() allows allocation of this ID again while
> idr_replace() means that lookups return NULL, but the ID remains in
> use.  The equivalent of idr_replace() is:
> 	xa_store(&drm_minors_xa, minor->index, NULL, GFP_KERNEL);

It does makes a difference, I'll change it to the equivalent.

> 
> > @@ -215,13 +204,10 @@ 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);
> > -	minor = idr_find(&drm_minors_idr, minor_id);
> > +	minor = xa_load(&drm_minors_xa, minor_id);
> >  	if (minor)
> >  		drm_dev_get(minor->dev);
> > -	spin_unlock_irqrestore(&drm_minor_lock, flags);
> 
> This is also not an exact equivalent as the dev_drm_get() is now outside
> the lock.  Does that matter in this case?  I don't know the code well
> enough to say.  If you want it to be equivalent, then:
> 
> 	xa_lock(&drm_minors_xa);
> 	minor = xa_load(&drm_minors_xa, minor_id);
> 	if (minor)
> 		drm_dev_get(minor->dev);
> 	xa_unlock(&drm_minors_xa);
> 
> would be the code to use.

Again, it does matter, I'll change it.

> 
> > @@ -1037,7 +1023,7 @@ static void drm_core_exit(void)
> >  	unregister_chrdev(DRM_MAJOR, "drm");
> >  	debugfs_remove(drm_debugfs_root);
> >  	drm_sysfs_destroy();
> > -	idr_destroy(&drm_minors_idr);
> > +	xa_destroy(&drm_minors_xa);
> 
> I don't know if this is the right call.  xa_destroy() is the exact
> replacement, but if the xarray should already be empty (and it should,
> right?) then asserting the xa_empty() is true may be the better call
> to make.

Yeah - I'll replace it with WARN_ON.

> 
> 
> Phew, that was a lot of comments/questions.  I hope that was useful!

Very useful indeed, thanks!

-Michał

Re: [PATCH v4 1/3] drm: Use XArray instead of IDR for minors

[Michał Winiarski]

On Tue, Sep 06, 2022 at 10:02:24PM +0100, Matthew Wilcox wrote:
> On Tue, Sep 06, 2022 at 10:16:27PM +0200, Michał Winiarski wrote:
> > IDR is deprecated, and since XArray manages its own state with internal
> > locking, it simplifies the locking on DRM side.
> > Additionally, don't use the IRQ-safe variant, since operating on drm
> > minor is not done in IRQ context.
> > 
> > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> > Suggested-by: Matthew Wilcox <willy@infradead.org>
> 
> I have a few questions, but I like where you're going.
> 
> > @@ -98,21 +98,18 @@ 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);
> > -	idr_remove(&drm_minors_idr, minor->index);
> > -	spin_unlock_irqrestore(&drm_minor_lock, flags);
> > +	xa_release(&drm_minors_xa, minor->index);
> 
> Has it definitely been unused at this point?  I would think that
> xa_erase() (an unconditional store) would be the correct function to
> call.

Yes, unless there's a programmers error somewhere - I'll replace it though.

> 
> > @@ -122,20 +119,12 @@ 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,
> > -		      64 * type,
> > -		      64 * (type + 1),
> > -		      GFP_NOWAIT);
> > -	spin_unlock_irqrestore(&drm_minor_lock, flags);
> > -	idr_preload_end();
> > -
> > +	r = xa_alloc(&drm_minors_xa, &id, NULL,
> > +		     XA_LIMIT(64 * type, 64 * (type + 1) - 1), GFP_KERNEL);
> >  	if (r < 0)
> >  		return r;
> >  
> > -	minor->index = r;
> > +	minor->index = id;
> 
> Wouldn't it be better to call:
> 
> 	r = xa_alloc(&drm_minors_xa, &minor->index, NULL,
> 			XA_LIMIT(64 * type, 64 * (type + 1) - 1), GFP_KERNEL);
> 
> I might also prefer a little syntactic sugar like:
> 
> #define DRM_MINOR_LIMIT(type)	XA_LIMIT(64 * (type), 64 * (type) + 63)
> 
> but that's definitely a matter of taste.

Sure.

> 
> > @@ -172,9 +161,12 @@ 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);
> > +	entry = xa_store(&drm_minors_xa, minor->index, &minor, GFP_KERNEL);
> > +	if (xa_is_err(entry)) {
> > +		ret = xa_err(entry);
> > +		goto err_debugfs;
> > +	}
> > +	WARN_ON(entry);
> 
> Might be better as an xa_cmpxchg()?

Ack.

> 
> > @@ -187,16 +179,13 @@ 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);
> > -	idr_replace(&drm_minors_idr, NULL, minor->index);
> > -	spin_unlock_irqrestore(&drm_minor_lock, flags);
> > +	xa_erase(&drm_minors_xa, minor->index);
> 
> This isn't an exact replacement, but I'm not sure whether that makes a
> difference.  xa_erase() allows allocation of this ID again while
> idr_replace() means that lookups return NULL, but the ID remains in
> use.  The equivalent of idr_replace() is:
> 	xa_store(&drm_minors_xa, minor->index, NULL, GFP_KERNEL);

It does makes a difference, I'll change it to the equivalent.

> 
> > @@ -215,13 +204,10 @@ 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);
> > -	minor = idr_find(&drm_minors_idr, minor_id);
> > +	minor = xa_load(&drm_minors_xa, minor_id);
> >  	if (minor)
> >  		drm_dev_get(minor->dev);
> > -	spin_unlock_irqrestore(&drm_minor_lock, flags);
> 
> This is also not an exact equivalent as the dev_drm_get() is now outside
> the lock.  Does that matter in this case?  I don't know the code well
> enough to say.  If you want it to be equivalent, then:
> 
> 	xa_lock(&drm_minors_xa);
> 	minor = xa_load(&drm_minors_xa, minor_id);
> 	if (minor)
> 		drm_dev_get(minor->dev);
> 	xa_unlock(&drm_minors_xa);
> 
> would be the code to use.

Again, it does matter, I'll change it.

> 
> > @@ -1037,7 +1023,7 @@ static void drm_core_exit(void)
> >  	unregister_chrdev(DRM_MAJOR, "drm");
> >  	debugfs_remove(drm_debugfs_root);
> >  	drm_sysfs_destroy();
> > -	idr_destroy(&drm_minors_idr);
> > +	xa_destroy(&drm_minors_xa);
> 
> I don't know if this is the right call.  xa_destroy() is the exact
> replacement, but if the xarray should already be empty (and it should,
> right?) then asserting the xa_empty() is true may be the better call
> to make.

Yeah - I'll replace it with WARN_ON.

> 
> 
> Phew, that was a lot of comments/questions.  I hope that was useful!

Very useful indeed, thanks!

-Michał

Re: [Intel-gfx] [PATCH v4 1/3] drm: Use XArray instead of IDR for minors

[Michał Winiarski]

On Tue, Sep 06, 2022 at 10:02:24PM +0100, Matthew Wilcox wrote:
> On Tue, Sep 06, 2022 at 10:16:27PM +0200, Michał Winiarski wrote:
> > IDR is deprecated, and since XArray manages its own state with internal
> > locking, it simplifies the locking on DRM side.
> > Additionally, don't use the IRQ-safe variant, since operating on drm
> > minor is not done in IRQ context.
> > 
> > Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> > Suggested-by: Matthew Wilcox <willy@infradead.org>
> 
> I have a few questions, but I like where you're going.
> 
> > @@ -98,21 +98,18 @@ 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);
> > -	idr_remove(&drm_minors_idr, minor->index);
> > -	spin_unlock_irqrestore(&drm_minor_lock, flags);
> > +	xa_release(&drm_minors_xa, minor->index);
> 
> Has it definitely been unused at this point?  I would think that
> xa_erase() (an unconditional store) would be the correct function to
> call.

Yes, unless there's a programmers error somewhere - I'll replace it though.

> 
> > @@ -122,20 +119,12 @@ 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,
> > -		      64 * type,
> > -		      64 * (type + 1),
> > -		      GFP_NOWAIT);
> > -	spin_unlock_irqrestore(&drm_minor_lock, flags);
> > -	idr_preload_end();
> > -
> > +	r = xa_alloc(&drm_minors_xa, &id, NULL,
> > +		     XA_LIMIT(64 * type, 64 * (type + 1) - 1), GFP_KERNEL);
> >  	if (r < 0)
> >  		return r;
> >  
> > -	minor->index = r;
> > +	minor->index = id;
> 
> Wouldn't it be better to call:
> 
> 	r = xa_alloc(&drm_minors_xa, &minor->index, NULL,
> 			XA_LIMIT(64 * type, 64 * (type + 1) - 1), GFP_KERNEL);
> 
> I might also prefer a little syntactic sugar like:
> 
> #define DRM_MINOR_LIMIT(type)	XA_LIMIT(64 * (type), 64 * (type) + 63)
> 
> but that's definitely a matter of taste.

Sure.

> 
> > @@ -172,9 +161,12 @@ 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);
> > +	entry = xa_store(&drm_minors_xa, minor->index, &minor, GFP_KERNEL);
> > +	if (xa_is_err(entry)) {
> > +		ret = xa_err(entry);
> > +		goto err_debugfs;
> > +	}
> > +	WARN_ON(entry);
> 
> Might be better as an xa_cmpxchg()?

Ack.

> 
> > @@ -187,16 +179,13 @@ 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);
> > -	idr_replace(&drm_minors_idr, NULL, minor->index);
> > -	spin_unlock_irqrestore(&drm_minor_lock, flags);
> > +	xa_erase(&drm_minors_xa, minor->index);
> 
> This isn't an exact replacement, but I'm not sure whether that makes a
> difference.  xa_erase() allows allocation of this ID again while
> idr_replace() means that lookups return NULL, but the ID remains in
> use.  The equivalent of idr_replace() is:
> 	xa_store(&drm_minors_xa, minor->index, NULL, GFP_KERNEL);

It does makes a difference, I'll change it to the equivalent.

> 
> > @@ -215,13 +204,10 @@ 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);
> > -	minor = idr_find(&drm_minors_idr, minor_id);
> > +	minor = xa_load(&drm_minors_xa, minor_id);
> >  	if (minor)
> >  		drm_dev_get(minor->dev);
> > -	spin_unlock_irqrestore(&drm_minor_lock, flags);
> 
> This is also not an exact equivalent as the dev_drm_get() is now outside
> the lock.  Does that matter in this case?  I don't know the code well
> enough to say.  If you want it to be equivalent, then:
> 
> 	xa_lock(&drm_minors_xa);
> 	minor = xa_load(&drm_minors_xa, minor_id);
> 	if (minor)
> 		drm_dev_get(minor->dev);
> 	xa_unlock(&drm_minors_xa);
> 
> would be the code to use.

Again, it does matter, I'll change it.

> 
> > @@ -1037,7 +1023,7 @@ static void drm_core_exit(void)
> >  	unregister_chrdev(DRM_MAJOR, "drm");
> >  	debugfs_remove(drm_debugfs_root);
> >  	drm_sysfs_destroy();
> > -	idr_destroy(&drm_minors_idr);
> > +	xa_destroy(&drm_minors_xa);
> 
> I don't know if this is the right call.  xa_destroy() is the exact
> replacement, but if the xarray should already be empty (and it should,
> right?) then asserting the xa_empty() is true may be the better call
> to make.

Yeah - I'll replace it with WARN_ON.

> 
> 
> Phew, that was a lot of comments/questions.  I hope that was useful!

Very useful indeed, thanks!

-Michał