[PATCH 11/34] drm: Convert crtc_idr to XArray

[Daniel Vetter]

On Fri, Feb 22, 2019 at 4:32 PM Matthew Wilcox <willy at 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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch