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

From: Matthew Wilcox <willy@infradead.org>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: dri-devel@lists.freedesktop.org
Subject: Re: [PATCH 11/34] drm: Convert crtc_idr to XArray
Date: Fri, 22 Feb 2019 07:32:47 -0800	[thread overview]
Message-ID: <20190222153245.GY12668@bombadil.infradead.org> (raw)
In-Reply-To: <20190222094014.GY2665@phenom.ffwll.local>

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.

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel