| Message ID | 20190221184226.2149-22-willy@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Convert DRM to XArray | expand |
On Thu, Feb 21, 2019 at 10:41:41AM -0800, Matthew Wilcox wrote: > - Rename it to 'objects', as requested in todo.rst Yay, thanks! > - Also convert leases IDR to XArray as the two are occasionally used by > the same code (see drm_mode_get_lease_ioctl()) > - Refactor drm_mode_create_lease_ioctl() to create the new drm_master > early to avoid creating an XArray on the stack and reparenting it > afterwards. All lgtm, also the idr_replace replacement. One thing I wonder: For the lesse object xa, we really only store 0/1 in there, and I don't think that'll change. There was once the idea that we'd look up objects for a lease directly, bypassing the main object idr. But that doesn't work due to unregister/hotunplug rules, or at least it would be pain not worth having. Might be worth it to a lookup structure optimized for that. Or does XA already autocompress that for us? The object id are likely fairly compressed, good chance all the ones you need for a lease will fit into the first 64 id. > Signed-off-by: Matthew Wilcox <willy@infradead.org> > --- > Documentation/gpu/todo.rst | 3 - > drivers/gpu/drm/drm_auth.c | 4 +- > drivers/gpu/drm/drm_lease.c | 136 ++++++++++++++---------------- > drivers/gpu/drm/drm_mode_config.c | 3 +- > drivers/gpu/drm/drm_mode_object.c | 47 +++++------ > include/drm/drm_auth.h | 2 +- > include/drm/drm_mode_config.h | 6 +- > 7 files changed, 90 insertions(+), 111 deletions(-) > > diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst > index 14191b64446d..41da7b06195c 100644 > --- a/Documentation/gpu/todo.rst > +++ b/Documentation/gpu/todo.rst > @@ -354,9 +354,6 @@ KMS cleanups > > Some of these date from the very introduction of KMS in 2008 ... > > -- drm_mode_config.crtc_idr is misnamed, since it contains all KMS object. Should > - be renamed to drm_mode_config.object_idr. > - > - drm_display_mode doesn't need to be derived from drm_mode_object. That's > leftovers from older (never merged into upstream) KMS designs where modes > where set using their ID, including support to add/remove modes. > diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c > index 1813507f9b9c..c6967f0b095d 100644 > --- a/drivers/gpu/drm/drm_auth.c > +++ b/drivers/gpu/drm/drm_auth.c > @@ -110,7 +110,7 @@ struct drm_master *drm_master_create(struct drm_device *dev) > /* initialize the tree of output resource lessees */ > master->lessor = NULL; > master->lessee_id = 0; > - idr_init(&master->leases); > + xa_init(&master->leases); XA_FALGS_ALLOC1, for safety to make sure we never store 0 (considered invalid id throughout at least modern drm)? -Daniel > xa_init_flags(&master->lessees, XA_FLAGS_ALLOC1); > > return master; > @@ -345,7 +345,7 @@ static void drm_master_destroy(struct kref *kref) > > drm_legacy_master_rmmaps(dev, master); > > - idr_destroy(&master->leases); > + xa_destroy(&master->leases); > > kfree(master->unique); > kfree(master); > diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c > index 47830f9ec616..1e88f406c738 100644 > --- a/drivers/gpu/drm/drm_lease.c > +++ b/drivers/gpu/drm/drm_lease.c > @@ -20,8 +20,6 @@ > #include <drm/drm_auth.h> > #include <drm/drm_crtc_helper.h> > > -static uint64_t drm_lease_idr_object; > - > /** > * drm_lease_owner - return ancestor owner drm_master > * @master: drm_master somewhere within tree of lessees and lessors > @@ -69,7 +67,7 @@ static int _drm_lease_held_master(struct drm_master *master, int id) > { > lockdep_assert_held(&master->dev->mode_config.idr_mutex); > if (master->lessor) > - return idr_find(&master->leases, id) != NULL; > + return xa_load(&master->leases, id) != NULL; > return true; > } > > @@ -183,7 +181,7 @@ uint32_t drm_lease_filter_crtcs(struct drm_file *file_priv, uint32_t crtcs_in) > /* > * drm_lease_create - create a new drm_master with leased objects (idr_mutex not held) > * @lessor: lease holder (or owner) of objects > - * @leases: objects to lease to the new drm_master > + * @lessee: leaser of objects > * > * Uses drm_master_create to allocate a new drm_master, then checks to > * make sure all of the desired objects can be leased, atomically > @@ -195,35 +193,30 @@ uint32_t drm_lease_filter_crtcs(struct drm_file *file_priv, uint32_t crtcs_in) > * ERR_PTR(-EEXIST) same object specified more than once in the provided list > * ERR_PTR(-ENOMEM) allocation failed > */ > -static struct drm_master *drm_lease_create(struct drm_master *lessor, struct idr *leases) > +static struct drm_master *drm_lease_create(struct drm_master *lessor, > + struct drm_master *lessee) > { > struct drm_device *dev = lessor->dev; > int error; > - struct drm_master *lessee; > - int object; > void *entry; > + unsigned long index; > > DRM_DEBUG_LEASE("lessor %d\n", lessor->lessee_id); > > - lessee = drm_master_create(lessor->dev); > - if (!lessee) { > - DRM_DEBUG_LEASE("drm_master_create failed\n"); > - return ERR_PTR(-ENOMEM); > - } > - > mutex_lock(&dev->mode_config.idr_mutex); > > - idr_for_each_entry(leases, entry, object) { > + xa_for_each(&lessee->leases, index, entry) { > error = 0; > - if (!idr_find(&dev->mode_config.crtc_idr, object)) > + if (!xa_load(&dev->mode_config.objects, index)) > error = -ENOENT; > - else if (!_drm_lease_held_master(lessor, object)) > + else if (!_drm_lease_held_master(lessor, index)) > error = -EACCES; > - else if (_drm_has_leased(lessor, object)) > + else if (_drm_has_leased(lessor, index)) > error = -EBUSY; > > if (error != 0) { > - DRM_DEBUG_LEASE("object %d failed %d\n", object, error); > + DRM_DEBUG_LEASE("object %d failed %d\n", (u32)index, > + error); > goto out_lessee; > } > } > @@ -236,8 +229,6 @@ static struct drm_master *drm_lease_create(struct drm_master *lessor, struct idr > > lessee->lessor = drm_master_get(lessor); > > - /* Move the leases over */ > - lessee->leases = *leases; > DRM_DEBUG_LEASE("new lessee %d %p, lessor %d %p\n", lessee->lessee_id, lessee, lessor->lessee_id, lessor); > > mutex_unlock(&dev->mode_config.idr_mutex); > @@ -246,8 +237,6 @@ static struct drm_master *drm_lease_create(struct drm_master *lessor, struct idr > out_lessee: > mutex_unlock(&dev->mode_config.idr_mutex); > > - drm_master_put(&lessee); > - > return ERR_PTR(error); > } > > @@ -292,8 +281,6 @@ void drm_lease_destroy(struct drm_master *master) > */ > static void _drm_lease_revoke(struct drm_master *top) > { > - int object; > - void *entry; > struct drm_master *master = top; > > lockdep_assert_held(&top->dev->mode_config.idr_mutex); > @@ -309,8 +296,7 @@ static void _drm_lease_revoke(struct drm_master *top) > DRM_DEBUG_LEASE("revoke leases for %p %d\n", master, master->lessee_id); > > /* Evacuate the lease */ > - idr_for_each_entry(&master->leases, entry, object) > - idr_remove(&master->leases, object); > + xa_destroy(&master->leases); > > /* Depth-first tree walk */ > tmp = xa_find(&master->lessees, &index, ULONG_MAX, XA_PRESENT); > @@ -378,11 +364,9 @@ static int validate_lease(struct drm_device *dev, > return 0; > } > > -static int fill_object_idr(struct drm_device *dev, > - struct drm_file *lessor_priv, > - struct idr *leases, > - int object_count, > - u32 *object_ids) > +static int fill_object_array(struct drm_device *dev, > + struct drm_file *lessor_priv, struct xarray *leases, > + int object_count, u32 *object_ids) > { > struct drm_mode_object **objects; > u32 o; > @@ -431,14 +415,14 @@ static int fill_object_idr(struct drm_device *dev, > DRM_DEBUG_LEASE("Adding object %d to lease\n", object_id); > > /* > - * We're using an IDR to hold the set of leased > + * We're using an array to hold the set of leased > * objects, but we don't need to point at the object's > - * data structure from the lease as the main crtc_idr > + * data structure from the lease as the main object array > * will be used to actually find that. Instead, all we > * really want is a 'leased/not-leased' result, for > * which any non-NULL pointer will work fine. > */ > - ret = idr_alloc(leases, &drm_lease_idr_object , object_id, object_id + 1, GFP_KERNEL); > + ret = xa_insert(leases, object_id, xa_mk_value(0), GFP_KERNEL); > if (ret < 0) { > DRM_DEBUG_LEASE("Object %d cannot be inserted into leases (%d)\n", > object_id, ret); > @@ -446,19 +430,21 @@ static int fill_object_idr(struct drm_device *dev, > } > if (obj->type == DRM_MODE_OBJECT_CRTC && !universal_planes) { > struct drm_crtc *crtc = obj_to_crtc(obj); > - ret = idr_alloc(leases, &drm_lease_idr_object, crtc->primary->base.id, crtc->primary->base.id + 1, GFP_KERNEL); > + ret = xa_insert(leases, crtc->primary->base.id, > + xa_mk_value(0), GFP_KERNEL); > if (ret < 0) { > DRM_DEBUG_LEASE("Object primary plane %d cannot be inserted into leases (%d)\n", > object_id, ret); > goto out_free_objects; > } > - if (crtc->cursor) { > - ret = idr_alloc(leases, &drm_lease_idr_object, crtc->cursor->base.id, crtc->cursor->base.id + 1, GFP_KERNEL); > - if (ret < 0) { > - DRM_DEBUG_LEASE("Object cursor plane %d cannot be inserted into leases (%d)\n", > - object_id, ret); > - goto out_free_objects; > - } > + if (!crtc->cursor) > + continue; > + ret = xa_insert(leases, crtc->cursor->base.id, > + xa_mk_value(0), GFP_KERNEL); > + if (ret < 0) { > + DRM_DEBUG_LEASE("Object cursor plane %d cannot be inserted into leases (%d)\n", > + object_id, ret); > + goto out_free_objects; > } > } > } > @@ -490,9 +476,8 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, > struct drm_mode_create_lease *cl = data; > size_t object_count; > int ret = 0; > - struct idr leases; > struct drm_master *lessor = lessor_priv->master; > - struct drm_master *lessee = NULL; > + struct drm_master *lessee; > struct file *lessee_file = NULL; > struct file *lessor_file = lessor_priv->filp; > struct drm_file *lessee_priv; > @@ -520,37 +505,42 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, > return -EINVAL; > } > > - object_count = cl->object_count; > + lessee = drm_master_create(lessor->dev); > + if (!lessee) { > + DRM_DEBUG_LEASE("drm_master_create failed\n"); > + return -ENOMEM; > + } > > - object_ids = memdup_user(u64_to_user_ptr(cl->object_ids), object_count * sizeof(__u32)); > - if (IS_ERR(object_ids)) > - return PTR_ERR(object_ids); > + object_count = cl->object_count; > > - idr_init(&leases); > + object_ids = memdup_user(u64_to_user_ptr(cl->object_ids), > + array_size(object_count, sizeof(__u32))); > + if (IS_ERR(object_ids)) { > + ret = PTR_ERR(object_ids); > + goto out_lessee; > + } > > - /* fill and validate the object idr */ > - ret = fill_object_idr(dev, lessor_priv, &leases, > + /* fill and validate the object array */ > + ret = fill_object_array(dev, lessor_priv, &lessee->leases, > object_count, object_ids); > kfree(object_ids); > if (ret) { > DRM_DEBUG_LEASE("lease object lookup failed: %i\n", ret); > - idr_destroy(&leases); > - return ret; > + goto out_lessee; > } > > /* Allocate a file descriptor for the lease */ > - fd = get_unused_fd_flags(cl->flags & (O_CLOEXEC | O_NONBLOCK)); > - if (fd < 0) { > - idr_destroy(&leases); > - return fd; > - } > + ret = get_unused_fd_flags(cl->flags & (O_CLOEXEC | O_NONBLOCK)); > + if (ret < 0) > + goto out_lessee; > + fd = ret; > > DRM_DEBUG_LEASE("Creating lease\n"); > - lessee = drm_lease_create(lessor, &leases); > + lessee = drm_lease_create(lessor, lessee); > > if (IS_ERR(lessee)) { > ret = PTR_ERR(lessee); > - goto out_leases; > + goto out_fd; > } > > /* Clone the lessor file to create a new file for us */ > @@ -558,7 +548,7 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, > lessee_file = file_clone_open(lessor_file); > if (IS_ERR(lessee_file)) { > ret = PTR_ERR(lessee_file); > - goto out_lessee; > + goto out_fd; > } > > lessee_priv = lessee_file->private_data; > @@ -579,13 +569,11 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, > DRM_DEBUG_LEASE("drm_mode_create_lease_ioctl succeeded\n"); > return 0; > > +out_fd: > + put_unused_fd(fd); > out_lessee: > drm_master_put(&lessee); > > -out_leases: > - put_unused_fd(fd); > - idr_destroy(&leases); > - > DRM_DEBUG_LEASE("drm_mode_create_lease_ioctl failed: %d\n", ret); > return ret; > } > @@ -627,7 +615,7 @@ int drm_mode_list_lessees_ioctl(struct drm_device *dev, > count = 0; > xa_for_each(&lessor->lessees, index, lessee) { > /* Only list un-revoked leases */ > - if (!idr_is_empty(&lessee->leases)) { > + if (!xa_empty(&lessee->leases)) { > if (count_lessees > count) { > DRM_DEBUG_LEASE("Add lessee %d\n", lessee->lessee_id); > ret = put_user(lessee->lessee_id, lessee_ids + count); > @@ -663,10 +651,10 @@ int drm_mode_get_lease_ioctl(struct drm_device *dev, > __u32 __user *object_ids = (__u32 __user *) (uintptr_t) (arg->objects_ptr); > __u32 count_objects = arg->count_objects; > struct drm_master *lessee = lessee_priv->master; > - struct idr *object_idr; > + struct xarray *objects; > int count; > void *entry; > - int object; > + unsigned long index; > int ret = 0; > > if (arg->pad) > @@ -682,16 +670,16 @@ int drm_mode_get_lease_ioctl(struct drm_device *dev, > > if (lessee->lessor == NULL) > /* owner can use all objects */ > - object_idr = &lessee->dev->mode_config.crtc_idr; > + objects = &lessee->dev->mode_config.objects; > else > - /* lessee can only use allowed object */ > - object_idr = &lessee->leases; > + /* lessee can only use allowed objects */ > + objects = &lessee->leases; > > count = 0; > - idr_for_each_entry(object_idr, entry, object) { > + xa_for_each(objects, index, entry) { > if (count_objects > count) { > - DRM_DEBUG_LEASE("adding object %d\n", object); > - ret = put_user(object, object_ids + count); > + DRM_DEBUG_LEASE("adding object %d\n", (u32)index); > + ret = put_user((u32)index, object_ids + count); > if (ret) > break; > } > diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c > index 609b30d7dcb1..10c616e0f591 100644 > --- a/drivers/gpu/drm/drm_mode_config.c > +++ b/drivers/gpu/drm/drm_mode_config.c > @@ -393,7 +393,7 @@ void drm_mode_config_init(struct drm_device *dev) > INIT_LIST_HEAD(&dev->mode_config.property_list); > INIT_LIST_HEAD(&dev->mode_config.property_blob_list); > INIT_LIST_HEAD(&dev->mode_config.plane_list); > - idr_init(&dev->mode_config.crtc_idr); > + xa_init_flags(&dev->mode_config.objects, XA_FLAGS_ALLOC1); > xa_init_flags(&dev->mode_config.tiles, XA_FLAGS_ALLOC1); > ida_init(&dev->mode_config.connector_ida); > spin_lock_init(&dev->mode_config.connector_list_lock); > @@ -495,7 +495,6 @@ void drm_mode_config_cleanup(struct drm_device *dev) > } > > ida_destroy(&dev->mode_config.connector_ida); > - idr_destroy(&dev->mode_config.crtc_idr); > drm_modeset_lock_fini(&dev->mode_config.connection_mutex); > } > EXPORT_SYMBOL(drm_mode_config_cleanup); > diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c > index 004191d01772..686fba472abf 100644 > --- a/drivers/gpu/drm/drm_mode_object.c > +++ b/drivers/gpu/drm/drm_mode_object.c > @@ -37,24 +37,23 @@ int __drm_mode_object_add(struct drm_device *dev, struct drm_mode_object *obj, > { > int ret; > > - mutex_lock(&dev->mode_config.idr_mutex); > - ret = idr_alloc(&dev->mode_config.crtc_idr, register_obj ? obj : NULL, > - 1, 0, GFP_KERNEL); > - if (ret >= 0) { > - /* > - * Set up the object linking under the protection of the idr > - * lock so that other users can't see inconsistent state. > - */ > - obj->id = ret; > - obj->type = obj_type; > - if (obj_free_cb) { > - obj->free_cb = obj_free_cb; > - kref_init(&obj->refcount); > - } > + /* > + * Initialise the object before putting the object in the array > + * so that other users can't see inconsistent state. The ID is > + * initialised before the object is inserted into the array with > + * a write barrier so even RCU-protected walkers can't see an > + * uninitialised ID. > + */ > + obj->type = obj_type; > + if (obj_free_cb) { > + obj->free_cb = obj_free_cb; > + kref_init(&obj->refcount); > } > - mutex_unlock(&dev->mode_config.idr_mutex); > > - return ret < 0 ? ret : 0; > + ret = xa_alloc(&dev->mode_config.objects, &obj->id, > + register_obj ? obj : NULL, xa_limit_31b, GFP_KERNEL); > + > + return ret; > } > > /** > @@ -78,9 +77,7 @@ int drm_mode_object_add(struct drm_device *dev, > void drm_mode_object_register(struct drm_device *dev, > struct drm_mode_object *obj) > { > - mutex_lock(&dev->mode_config.idr_mutex); > - idr_replace(&dev->mode_config.crtc_idr, obj, obj->id); > - mutex_unlock(&dev->mode_config.idr_mutex); > + xa_store(&dev->mode_config.objects, obj->id, obj, 0); > } > > /** > @@ -97,12 +94,10 @@ void drm_mode_object_register(struct drm_device *dev, > void drm_mode_object_unregister(struct drm_device *dev, > struct drm_mode_object *object) > { > - mutex_lock(&dev->mode_config.idr_mutex); > if (object->id) { > - idr_remove(&dev->mode_config.crtc_idr, object->id); > + xa_erase(&dev->mode_config.objects, object->id); > object->id = 0; > } > - mutex_unlock(&dev->mode_config.idr_mutex); > } > > /** > @@ -128,10 +123,10 @@ struct drm_mode_object *__drm_mode_object_find(struct drm_device *dev, > struct drm_file *file_priv, > uint32_t id, uint32_t type) > { > - struct drm_mode_object *obj = NULL; > + struct drm_mode_object *obj; > > - mutex_lock(&dev->mode_config.idr_mutex); > - obj = idr_find(&dev->mode_config.crtc_idr, id); > + xa_lock(&dev->mode_config.objects); > + obj = xa_load(&dev->mode_config.objects, id); > if (obj && type != DRM_MODE_OBJECT_ANY && obj->type != type) > obj = NULL; > if (obj && obj->id != id) > @@ -145,7 +140,7 @@ struct drm_mode_object *__drm_mode_object_find(struct drm_device *dev, > if (!kref_get_unless_zero(&obj->refcount)) > obj = NULL; > } > - mutex_unlock(&dev->mode_config.idr_mutex); > + xa_unlock(&dev->mode_config.objects); > > return obj; > } > diff --git a/include/drm/drm_auth.h b/include/drm/drm_auth.h > index fbb58264538b..88c8bbf14916 100644 > --- a/include/drm/drm_auth.h > +++ b/include/drm/drm_auth.h > @@ -90,7 +90,7 @@ struct drm_master { > > struct drm_master *lessor; > int lessee_id; > - struct idr leases; > + struct xarray leases; > struct xarray lessees; > }; > > diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h > index fea334d99201..64bdf66d878c 100644 > --- a/include/drm/drm_mode_config.h > +++ b/include/drm/drm_mode_config.h > @@ -397,12 +397,12 @@ struct drm_mode_config { > struct mutex idr_mutex; > > /** > - * @crtc_idr: > + * @objects: > * > - * Main KMS ID tracking object. Use this idr for all IDs, fb, crtc, > + * Main KMS ID tracking object. Use this array for all IDs, fb, crtc, > * connector, modes - just makes life easier to have only one. > */ > - struct idr crtc_idr; > + struct xarray objects; > > /** > * @tiles: > -- > 2.20.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Fri, Feb 22, 2019 at 10:40:14AM +0100, Daniel Vetter wrote: > On Thu, Feb 21, 2019 at 10:41:41AM -0800, Matthew Wilcox wrote: > > - Rename it to 'objects', as requested in todo.rst > > Yay, thanks! > > > - Also convert leases IDR to XArray as the two are occasionally used by > > the same code (see drm_mode_get_lease_ioctl()) > > - Refactor drm_mode_create_lease_ioctl() to create the new drm_master > > early to avoid creating an XArray on the stack and reparenting it > > afterwards. > > All lgtm, also the idr_replace replacement. > > One thing I wonder: For the lesse object xa, we really only store 0/1 in > there, and I don't think that'll change. There was once the idea that we'd > look up objects for a lease directly, bypassing the main object idr. But > that doesn't work due to unregister/hotunplug rules, or at least it would > be pain not worth having. Might be worth it to a lookup structure > optimized for that. Or does XA already autocompress that for us? The > object id are likely fairly compressed, good chance all the ones you need > for a lease will fit into the first 64 id. The XArray doesn't compress the contents of the user pointers. It might be worth augmenting the IDA to have all the functionality you need (there's no ida_test() today, for example). I didn't want to take that on as part of this project, and it's certainly no worse than the IDR. But it's on my radar along with a couple of other places in the kernel that could benefit from the IDA if only it had a couple of extra features (eg the fd table would like to an ida_for_each()). It'd involve changing drm_mode_get_lease_ioctl() and maybe a couple of other places where we use config.objects and leases interchangably, so I wasn't entirely convinced it'd be worth it. > > +++ b/drivers/gpu/drm/drm_auth.c > > @@ -110,7 +110,7 @@ struct drm_master *drm_master_create(struct drm_device *dev) > > /* initialize the tree of output resource lessees */ > > master->lessor = NULL; > > master->lessee_id = 0; > > - idr_init(&master->leases); > > + xa_init(&master->leases); > > XA_FALGS_ALLOC1, for safety to make sure we never store 0 (considered > invalid id throughout at least modern drm)? This xarray turns out not to be an allocating XArray, it's just one we store pointers in at a predetermined ID. Marking it as allocating wouldn't be terribly harmful, just slightly inefficient.
On Fri, Feb 22, 2019 at 4:32 PM Matthew Wilcox <willy@infradead.org> wrote: > > On Fri, Feb 22, 2019 at 10:40:14AM +0100, Daniel Vetter wrote: > > On Thu, Feb 21, 2019 at 10:41:41AM -0800, Matthew Wilcox wrote: > > > - Rename it to 'objects', as requested in todo.rst > > > > Yay, thanks! > > > > > - Also convert leases IDR to XArray as the two are occasionally used by > > > the same code (see drm_mode_get_lease_ioctl()) > > > - Refactor drm_mode_create_lease_ioctl() to create the new drm_master > > > early to avoid creating an XArray on the stack and reparenting it > > > afterwards. > > > > All lgtm, also the idr_replace replacement. > > > > One thing I wonder: For the lesse object xa, we really only store 0/1 in > > there, and I don't think that'll change. There was once the idea that we'd > > look up objects for a lease directly, bypassing the main object idr. But > > that doesn't work due to unregister/hotunplug rules, or at least it would > > be pain not worth having. Might be worth it to a lookup structure > > optimized for that. Or does XA already autocompress that for us? The > > object id are likely fairly compressed, good chance all the ones you need > > for a lease will fit into the first 64 id. > > The XArray doesn't compress the contents of the user pointers. It might be > worth augmenting the IDA to have all the functionality you need (there's > no ida_test() today, for example). I didn't want to take that on as part > of this project, and it's certainly no worse than the IDR. But it's on > my radar along with a couple of other places in the kernel that could benefit > from the IDA if only it had a couple of extra features (eg the fd table > would like to an ida_for_each()). > > It'd involve changing drm_mode_get_lease_ioctl() and maybe a couple of > other places where we use config.objects and leases interchangably, so > I wasn't entirely convinced it'd be worth it. Makes all sense. > > > +++ b/drivers/gpu/drm/drm_auth.c > > > @@ -110,7 +110,7 @@ struct drm_master *drm_master_create(struct drm_device *dev) > > > /* initialize the tree of output resource lessees */ > > > master->lessor = NULL; > > > master->lessee_id = 0; > > > - idr_init(&master->leases); > > > + xa_init(&master->leases); > > > > XA_FALGS_ALLOC1, for safety to make sure we never store 0 (considered > > invalid id throughout at least modern drm)? > > This xarray turns out not to be an allocating XArray, it's just one > we store pointers in at a predetermined ID. Marking it as allocating > wouldn't be terribly harmful, just slightly inefficient. tbf I didn't read what exactly the flag means in detail, just wondered whether it could help in making sure we never store something at id 0 (which would be a bug). But neither did the current code do such checks, so fine either way. -Daniel
diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst index 14191b64446d..41da7b06195c 100644 --- a/Documentation/gpu/todo.rst +++ b/Documentation/gpu/todo.rst @@ -354,9 +354,6 @@ KMS cleanups Some of these date from the very introduction of KMS in 2008 ... -- drm_mode_config.crtc_idr is misnamed, since it contains all KMS object. Should - be renamed to drm_mode_config.object_idr. - - drm_display_mode doesn't need to be derived from drm_mode_object. That's leftovers from older (never merged into upstream) KMS designs where modes where set using their ID, including support to add/remove modes. diff --git a/drivers/gpu/drm/drm_auth.c b/drivers/gpu/drm/drm_auth.c index 1813507f9b9c..c6967f0b095d 100644 --- a/drivers/gpu/drm/drm_auth.c +++ b/drivers/gpu/drm/drm_auth.c @@ -110,7 +110,7 @@ struct drm_master *drm_master_create(struct drm_device *dev) /* initialize the tree of output resource lessees */ master->lessor = NULL; master->lessee_id = 0; - idr_init(&master->leases); + xa_init(&master->leases); xa_init_flags(&master->lessees, XA_FLAGS_ALLOC1); return master; @@ -345,7 +345,7 @@ static void drm_master_destroy(struct kref *kref) drm_legacy_master_rmmaps(dev, master); - idr_destroy(&master->leases); + xa_destroy(&master->leases); kfree(master->unique); kfree(master); diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c index 47830f9ec616..1e88f406c738 100644 --- a/drivers/gpu/drm/drm_lease.c +++ b/drivers/gpu/drm/drm_lease.c @@ -20,8 +20,6 @@ #include <drm/drm_auth.h> #include <drm/drm_crtc_helper.h> -static uint64_t drm_lease_idr_object; - /** * drm_lease_owner - return ancestor owner drm_master * @master: drm_master somewhere within tree of lessees and lessors @@ -69,7 +67,7 @@ static int _drm_lease_held_master(struct drm_master *master, int id) { lockdep_assert_held(&master->dev->mode_config.idr_mutex); if (master->lessor) - return idr_find(&master->leases, id) != NULL; + return xa_load(&master->leases, id) != NULL; return true; } @@ -183,7 +181,7 @@ uint32_t drm_lease_filter_crtcs(struct drm_file *file_priv, uint32_t crtcs_in) /* * drm_lease_create - create a new drm_master with leased objects (idr_mutex not held) * @lessor: lease holder (or owner) of objects - * @leases: objects to lease to the new drm_master + * @lessee: leaser of objects * * Uses drm_master_create to allocate a new drm_master, then checks to * make sure all of the desired objects can be leased, atomically @@ -195,35 +193,30 @@ uint32_t drm_lease_filter_crtcs(struct drm_file *file_priv, uint32_t crtcs_in) * ERR_PTR(-EEXIST) same object specified more than once in the provided list * ERR_PTR(-ENOMEM) allocation failed */ -static struct drm_master *drm_lease_create(struct drm_master *lessor, struct idr *leases) +static struct drm_master *drm_lease_create(struct drm_master *lessor, + struct drm_master *lessee) { struct drm_device *dev = lessor->dev; int error; - struct drm_master *lessee; - int object; void *entry; + unsigned long index; DRM_DEBUG_LEASE("lessor %d\n", lessor->lessee_id); - lessee = drm_master_create(lessor->dev); - if (!lessee) { - DRM_DEBUG_LEASE("drm_master_create failed\n"); - return ERR_PTR(-ENOMEM); - } - mutex_lock(&dev->mode_config.idr_mutex); - idr_for_each_entry(leases, entry, object) { + xa_for_each(&lessee->leases, index, entry) { error = 0; - if (!idr_find(&dev->mode_config.crtc_idr, object)) + if (!xa_load(&dev->mode_config.objects, index)) error = -ENOENT; - else if (!_drm_lease_held_master(lessor, object)) + else if (!_drm_lease_held_master(lessor, index)) error = -EACCES; - else if (_drm_has_leased(lessor, object)) + else if (_drm_has_leased(lessor, index)) error = -EBUSY; if (error != 0) { - DRM_DEBUG_LEASE("object %d failed %d\n", object, error); + DRM_DEBUG_LEASE("object %d failed %d\n", (u32)index, + error); goto out_lessee; } } @@ -236,8 +229,6 @@ static struct drm_master *drm_lease_create(struct drm_master *lessor, struct idr lessee->lessor = drm_master_get(lessor); - /* Move the leases over */ - lessee->leases = *leases; DRM_DEBUG_LEASE("new lessee %d %p, lessor %d %p\n", lessee->lessee_id, lessee, lessor->lessee_id, lessor); mutex_unlock(&dev->mode_config.idr_mutex); @@ -246,8 +237,6 @@ static struct drm_master *drm_lease_create(struct drm_master *lessor, struct idr out_lessee: mutex_unlock(&dev->mode_config.idr_mutex); - drm_master_put(&lessee); - return ERR_PTR(error); } @@ -292,8 +281,6 @@ void drm_lease_destroy(struct drm_master *master) */ static void _drm_lease_revoke(struct drm_master *top) { - int object; - void *entry; struct drm_master *master = top; lockdep_assert_held(&top->dev->mode_config.idr_mutex); @@ -309,8 +296,7 @@ static void _drm_lease_revoke(struct drm_master *top) DRM_DEBUG_LEASE("revoke leases for %p %d\n", master, master->lessee_id); /* Evacuate the lease */ - idr_for_each_entry(&master->leases, entry, object) - idr_remove(&master->leases, object); + xa_destroy(&master->leases); /* Depth-first tree walk */ tmp = xa_find(&master->lessees, &index, ULONG_MAX, XA_PRESENT); @@ -378,11 +364,9 @@ static int validate_lease(struct drm_device *dev, return 0; } -static int fill_object_idr(struct drm_device *dev, - struct drm_file *lessor_priv, - struct idr *leases, - int object_count, - u32 *object_ids) +static int fill_object_array(struct drm_device *dev, + struct drm_file *lessor_priv, struct xarray *leases, + int object_count, u32 *object_ids) { struct drm_mode_object **objects; u32 o; @@ -431,14 +415,14 @@ static int fill_object_idr(struct drm_device *dev, DRM_DEBUG_LEASE("Adding object %d to lease\n", object_id); /* - * We're using an IDR to hold the set of leased + * We're using an array to hold the set of leased * objects, but we don't need to point at the object's - * data structure from the lease as the main crtc_idr + * data structure from the lease as the main object array * will be used to actually find that. Instead, all we * really want is a 'leased/not-leased' result, for * which any non-NULL pointer will work fine. */ - ret = idr_alloc(leases, &drm_lease_idr_object , object_id, object_id + 1, GFP_KERNEL); + ret = xa_insert(leases, object_id, xa_mk_value(0), GFP_KERNEL); if (ret < 0) { DRM_DEBUG_LEASE("Object %d cannot be inserted into leases (%d)\n", object_id, ret); @@ -446,19 +430,21 @@ static int fill_object_idr(struct drm_device *dev, } if (obj->type == DRM_MODE_OBJECT_CRTC && !universal_planes) { struct drm_crtc *crtc = obj_to_crtc(obj); - ret = idr_alloc(leases, &drm_lease_idr_object, crtc->primary->base.id, crtc->primary->base.id + 1, GFP_KERNEL); + ret = xa_insert(leases, crtc->primary->base.id, + xa_mk_value(0), GFP_KERNEL); if (ret < 0) { DRM_DEBUG_LEASE("Object primary plane %d cannot be inserted into leases (%d)\n", object_id, ret); goto out_free_objects; } - if (crtc->cursor) { - ret = idr_alloc(leases, &drm_lease_idr_object, crtc->cursor->base.id, crtc->cursor->base.id + 1, GFP_KERNEL); - if (ret < 0) { - DRM_DEBUG_LEASE("Object cursor plane %d cannot be inserted into leases (%d)\n", - object_id, ret); - goto out_free_objects; - } + if (!crtc->cursor) + continue; + ret = xa_insert(leases, crtc->cursor->base.id, + xa_mk_value(0), GFP_KERNEL); + if (ret < 0) { + DRM_DEBUG_LEASE("Object cursor plane %d cannot be inserted into leases (%d)\n", + object_id, ret); + goto out_free_objects; } } } @@ -490,9 +476,8 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, struct drm_mode_create_lease *cl = data; size_t object_count; int ret = 0; - struct idr leases; struct drm_master *lessor = lessor_priv->master; - struct drm_master *lessee = NULL; + struct drm_master *lessee; struct file *lessee_file = NULL; struct file *lessor_file = lessor_priv->filp; struct drm_file *lessee_priv; @@ -520,37 +505,42 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, return -EINVAL; } - object_count = cl->object_count; + lessee = drm_master_create(lessor->dev); + if (!lessee) { + DRM_DEBUG_LEASE("drm_master_create failed\n"); + return -ENOMEM; + } - object_ids = memdup_user(u64_to_user_ptr(cl->object_ids), object_count * sizeof(__u32)); - if (IS_ERR(object_ids)) - return PTR_ERR(object_ids); + object_count = cl->object_count; - idr_init(&leases); + object_ids = memdup_user(u64_to_user_ptr(cl->object_ids), + array_size(object_count, sizeof(__u32))); + if (IS_ERR(object_ids)) { + ret = PTR_ERR(object_ids); + goto out_lessee; + } - /* fill and validate the object idr */ - ret = fill_object_idr(dev, lessor_priv, &leases, + /* fill and validate the object array */ + ret = fill_object_array(dev, lessor_priv, &lessee->leases, object_count, object_ids); kfree(object_ids); if (ret) { DRM_DEBUG_LEASE("lease object lookup failed: %i\n", ret); - idr_destroy(&leases); - return ret; + goto out_lessee; } /* Allocate a file descriptor for the lease */ - fd = get_unused_fd_flags(cl->flags & (O_CLOEXEC | O_NONBLOCK)); - if (fd < 0) { - idr_destroy(&leases); - return fd; - } + ret = get_unused_fd_flags(cl->flags & (O_CLOEXEC | O_NONBLOCK)); + if (ret < 0) + goto out_lessee; + fd = ret; DRM_DEBUG_LEASE("Creating lease\n"); - lessee = drm_lease_create(lessor, &leases); + lessee = drm_lease_create(lessor, lessee); if (IS_ERR(lessee)) { ret = PTR_ERR(lessee); - goto out_leases; + goto out_fd; } /* Clone the lessor file to create a new file for us */ @@ -558,7 +548,7 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, lessee_file = file_clone_open(lessor_file); if (IS_ERR(lessee_file)) { ret = PTR_ERR(lessee_file); - goto out_lessee; + goto out_fd; } lessee_priv = lessee_file->private_data; @@ -579,13 +569,11 @@ int drm_mode_create_lease_ioctl(struct drm_device *dev, DRM_DEBUG_LEASE("drm_mode_create_lease_ioctl succeeded\n"); return 0; +out_fd: + put_unused_fd(fd); out_lessee: drm_master_put(&lessee); -out_leases: - put_unused_fd(fd); - idr_destroy(&leases); - DRM_DEBUG_LEASE("drm_mode_create_lease_ioctl failed: %d\n", ret); return ret; } @@ -627,7 +615,7 @@ int drm_mode_list_lessees_ioctl(struct drm_device *dev, count = 0; xa_for_each(&lessor->lessees, index, lessee) { /* Only list un-revoked leases */ - if (!idr_is_empty(&lessee->leases)) { + if (!xa_empty(&lessee->leases)) { if (count_lessees > count) { DRM_DEBUG_LEASE("Add lessee %d\n", lessee->lessee_id); ret = put_user(lessee->lessee_id, lessee_ids + count); @@ -663,10 +651,10 @@ int drm_mode_get_lease_ioctl(struct drm_device *dev, __u32 __user *object_ids = (__u32 __user *) (uintptr_t) (arg->objects_ptr); __u32 count_objects = arg->count_objects; struct drm_master *lessee = lessee_priv->master; - struct idr *object_idr; + struct xarray *objects; int count; void *entry; - int object; + unsigned long index; int ret = 0; if (arg->pad) @@ -682,16 +670,16 @@ int drm_mode_get_lease_ioctl(struct drm_device *dev, if (lessee->lessor == NULL) /* owner can use all objects */ - object_idr = &lessee->dev->mode_config.crtc_idr; + objects = &lessee->dev->mode_config.objects; else - /* lessee can only use allowed object */ - object_idr = &lessee->leases; + /* lessee can only use allowed objects */ + objects = &lessee->leases; count = 0; - idr_for_each_entry(object_idr, entry, object) { + xa_for_each(objects, index, entry) { if (count_objects > count) { - DRM_DEBUG_LEASE("adding object %d\n", object); - ret = put_user(object, object_ids + count); + DRM_DEBUG_LEASE("adding object %d\n", (u32)index); + ret = put_user((u32)index, object_ids + count); if (ret) break; } diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index 609b30d7dcb1..10c616e0f591 100644 --- a/drivers/gpu/drm/drm_mode_config.c +++ b/drivers/gpu/drm/drm_mode_config.c @@ -393,7 +393,7 @@ void drm_mode_config_init(struct drm_device *dev) INIT_LIST_HEAD(&dev->mode_config.property_list); INIT_LIST_HEAD(&dev->mode_config.property_blob_list); INIT_LIST_HEAD(&dev->mode_config.plane_list); - idr_init(&dev->mode_config.crtc_idr); + xa_init_flags(&dev->mode_config.objects, XA_FLAGS_ALLOC1); xa_init_flags(&dev->mode_config.tiles, XA_FLAGS_ALLOC1); ida_init(&dev->mode_config.connector_ida); spin_lock_init(&dev->mode_config.connector_list_lock); @@ -495,7 +495,6 @@ void drm_mode_config_cleanup(struct drm_device *dev) } ida_destroy(&dev->mode_config.connector_ida); - idr_destroy(&dev->mode_config.crtc_idr); drm_modeset_lock_fini(&dev->mode_config.connection_mutex); } EXPORT_SYMBOL(drm_mode_config_cleanup); diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c index 004191d01772..686fba472abf 100644 --- a/drivers/gpu/drm/drm_mode_object.c +++ b/drivers/gpu/drm/drm_mode_object.c @@ -37,24 +37,23 @@ int __drm_mode_object_add(struct drm_device *dev, struct drm_mode_object *obj, { int ret; - mutex_lock(&dev->mode_config.idr_mutex); - ret = idr_alloc(&dev->mode_config.crtc_idr, register_obj ? obj : NULL, - 1, 0, GFP_KERNEL); - if (ret >= 0) { - /* - * Set up the object linking under the protection of the idr - * lock so that other users can't see inconsistent state. - */ - obj->id = ret; - obj->type = obj_type; - if (obj_free_cb) { - obj->free_cb = obj_free_cb; - kref_init(&obj->refcount); - } + /* + * Initialise the object before putting the object in the array + * so that other users can't see inconsistent state. The ID is + * initialised before the object is inserted into the array with + * a write barrier so even RCU-protected walkers can't see an + * uninitialised ID. + */ + obj->type = obj_type; + if (obj_free_cb) { + obj->free_cb = obj_free_cb; + kref_init(&obj->refcount); } - mutex_unlock(&dev->mode_config.idr_mutex); - return ret < 0 ? ret : 0; + ret = xa_alloc(&dev->mode_config.objects, &obj->id, + register_obj ? obj : NULL, xa_limit_31b, GFP_KERNEL); + + return ret; } /** @@ -78,9 +77,7 @@ int drm_mode_object_add(struct drm_device *dev, void drm_mode_object_register(struct drm_device *dev, struct drm_mode_object *obj) { - mutex_lock(&dev->mode_config.idr_mutex); - idr_replace(&dev->mode_config.crtc_idr, obj, obj->id); - mutex_unlock(&dev->mode_config.idr_mutex); + xa_store(&dev->mode_config.objects, obj->id, obj, 0); } /** @@ -97,12 +94,10 @@ void drm_mode_object_register(struct drm_device *dev, void drm_mode_object_unregister(struct drm_device *dev, struct drm_mode_object *object) { - mutex_lock(&dev->mode_config.idr_mutex); if (object->id) { - idr_remove(&dev->mode_config.crtc_idr, object->id); + xa_erase(&dev->mode_config.objects, object->id); object->id = 0; } - mutex_unlock(&dev->mode_config.idr_mutex); } /** @@ -128,10 +123,10 @@ struct drm_mode_object *__drm_mode_object_find(struct drm_device *dev, struct drm_file *file_priv, uint32_t id, uint32_t type) { - struct drm_mode_object *obj = NULL; + struct drm_mode_object *obj; - mutex_lock(&dev->mode_config.idr_mutex); - obj = idr_find(&dev->mode_config.crtc_idr, id); + xa_lock(&dev->mode_config.objects); + obj = xa_load(&dev->mode_config.objects, id); if (obj && type != DRM_MODE_OBJECT_ANY && obj->type != type) obj = NULL; if (obj && obj->id != id) @@ -145,7 +140,7 @@ struct drm_mode_object *__drm_mode_object_find(struct drm_device *dev, if (!kref_get_unless_zero(&obj->refcount)) obj = NULL; } - mutex_unlock(&dev->mode_config.idr_mutex); + xa_unlock(&dev->mode_config.objects); return obj; } diff --git a/include/drm/drm_auth.h b/include/drm/drm_auth.h index fbb58264538b..88c8bbf14916 100644 --- a/include/drm/drm_auth.h +++ b/include/drm/drm_auth.h @@ -90,7 +90,7 @@ struct drm_master { struct drm_master *lessor; int lessee_id; - struct idr leases; + struct xarray leases; struct xarray lessees; }; diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h index fea334d99201..64bdf66d878c 100644 --- a/include/drm/drm_mode_config.h +++ b/include/drm/drm_mode_config.h @@ -397,12 +397,12 @@ struct drm_mode_config { struct mutex idr_mutex; /** - * @crtc_idr: + * @objects: * - * Main KMS ID tracking object. Use this idr for all IDs, fb, crtc, + * Main KMS ID tracking object. Use this array for all IDs, fb, crtc, * connector, modes - just makes life easier to have only one. */ - struct idr crtc_idr; + struct xarray objects; /** * @tiles:
- Rename it to 'objects', as requested in todo.rst - Also convert leases IDR to XArray as the two are occasionally used by the same code (see drm_mode_get_lease_ioctl()) - Refactor drm_mode_create_lease_ioctl() to create the new drm_master early to avoid creating an XArray on the stack and reparenting it afterwards. Signed-off-by: Matthew Wilcox <willy@infradead.org> --- Documentation/gpu/todo.rst | 3 - drivers/gpu/drm/drm_auth.c | 4 +- drivers/gpu/drm/drm_lease.c | 136 ++++++++++++++---------------- drivers/gpu/drm/drm_mode_config.c | 3 +- drivers/gpu/drm/drm_mode_object.c | 47 +++++------ include/drm/drm_auth.h | 2 +- include/drm/drm_mode_config.h | 6 +- 7 files changed, 90 insertions(+), 111 deletions(-)