On Mon, Sep 12, 2022 at 12:17 AM Michał Winiarski <michal.winiar...@intel.com> 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.winiar...@intel.com> > Suggested-by: Matthew Wilcox <wi...@infradead.org> > --- > drivers/gpu/drm/drm_drv.c | 51 ++++++++++++++------------------------- > 1 file changed, 18 insertions(+), 33 deletions(-) > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > index 8214a0b1ab7f..61d24cdcd0f8 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,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); > - idr_remove(&drm_minors_idr, minor->index); > - spin_unlock_irqrestore(&drm_minor_lock, flags); > + xa_erase(&drm_minors_xa, minor->index); > } > > +#define DRM_MINOR_LIMIT(t) ({ typeof(t) _t = (t); XA_LIMIT(64 * _t, 64 * _t > + 63); }) > + > static int drm_minor_alloc(struct drm_device *dev, unsigned int type) > { > struct drm_minor *minor; > - unsigned long flags; > int r; > > minor = drmm_kzalloc(dev, sizeof(*minor), GFP_KERNEL); > @@ -122,21 +120,10 @@ 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, &minor->index, NULL, > DRM_MINOR_LIMIT(type), GFP_KERNEL); > if (r < 0) > return r; > > - minor->index = r; > - > r = drmm_add_action_or_reset(dev, drm_minor_alloc_release, minor); > if (r) > return r; > @@ -152,7 +139,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 +159,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_cmpxchg(&drm_minors_xa, minor->index, NULL, &minor, > GFP_KERNEL); I believe we should pass in "minor", without the &, as &minor will give you the address of the local pointer.
Oded > + 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 +177,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_store(&drm_minors_xa, minor->index, NULL, GFP_KERNEL); > > device_del(minor->kdev); > dev_set_drvdata(minor->kdev, NULL); /* safety belt */ > @@ -215,13 +202,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); > - minor = idr_find(&drm_minors_idr, minor_id); > + xa_lock(&drm_minors_xa); > + minor = xa_load(&drm_minors_xa, minor_id); > if (minor) > drm_dev_get(minor->dev); > - spin_unlock_irqrestore(&drm_minor_lock, flags); > + xa_unlock(&drm_minors_xa); > > 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); > + WARN_ON(!xa_empty(&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 >