[Intel-gfx] [PATCH v2] drm/i915: Store a direct lookup from object handle to vma

[Daniel Vetter]

On Wed, Mar 22, 2017 at 04:22:38PM +0200, Joonas Lahtinen wrote:
> + Daniel for the rsvd2

I'd go with a shadow struct definition which matches the uapi one exactly,
except for having a proper name and type for this ... Or we do the
anynomous union thing again.
-Daniel

> 
> On ke, 2017-03-22 at 09:33 +0000, Chris Wilson wrote:
> > The advent of full-ppgtt lead to an extra indirection between the object
> > and its binding. That extra indirection has a noticeable impact on how
> > fast we can convert from the user handles to our internal vma for
> > execbuffer. In order to bypass the extra indirection, we use a
> > resizable hashtable to jump from the object to the per-ctx vma.
> > rhashtable was considered but we don't need the online resizing feature
> > and the extra complexity proved to undermine its usefulness. Instead, we
> > simply reallocate the hastable on demand in a background task and
> > serialize it before iterating.
> > 
> > In non-full-ppgtt modes, multiple files and multiple contexts can share
> > the same vma. This leads to having multiple possible handle->vma links,
> > so we only use the first to establish the fast path. The majority of
> > buffers are not shared and so we should still be able to realise
> > speedups with multiple clients.
> > 
> > v2: Prettier names, more magic.
> > 
> > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> 
> <SNIP>
>  
> > +static void resize_vma_ht(struct work_struct *work)
> > +{
> > +	struct i915_gem_context_vma_lut *lut =
> > +		container_of(work, typeof(*lut), resize);
> > +	unsigned int size, bits, new_bits, i;
> > +	struct hlist_head *new_ht;
> > +
> > +	bits = 1 + ilog2(4*lut->ht_count/3);
> > +	new_bits = min_t(unsigned int,
> > +			 max(bits, VMA_HT_BITS),
> > +			 sizeof(unsigned int)*8);
> 
> * BITS_PER_BYTE for extra clarity.
> 
> > +	if (new_bits == lut->ht_bits)
> > +		goto out;
> > +
> > +	new_ht = kzalloc(sizeof(*new_ht)<<new_bits, GFP_KERNEL | __GFP_NOWARN);
> > +	if (!new_ht)
> > +		new_ht = vzalloc(sizeof(*new_ht)<<new_bits);
> 
> No vcalloc :( Otherwise would've suggested
> 
> vzalloc(BIT(new_bits), sizeof(*new_ht), ...);
> 
> but
> 
> kzalloc(BIT(new_bits)*sizeof(*new_ht), ...)
> 
> might still be clearer.
> 
> > @@ -266,6 +331,16 @@ __create_hw_context(struct drm_i915_private *dev_priv,
> >  	list_add_tail(&ctx->link, &dev_priv->context_list);
> >  	ctx->i915 = dev_priv;
> >  
> > +	ctx->vma_lut.ht_bits = VMA_HT_BITS;
> > +	ctx->vma_lut.ht_size = BIT(VMA_HT_BITS);
> > +	ctx->vma_lut.ht = kcalloc(ctx->vma_lut.ht_size,
> > +				  sizeof(*ctx->vma_lut.ht),
> > +				  GFP_KERNEL);
> > +	if (!ctx->vma_lut.ht)
> > +		goto err_out;
> > +
> 
> Errors after this point will leak lut. Need err_lut label and call it
> from further error gotos.
> 
> > @@ -143,6 +143,31 @@ struct i915_gem_context {
> >  	/** ggtt_offset_bias: placement restriction for context objects */
> >  	u32 ggtt_offset_bias;
> >  
> > +	struct i915_gem_context_vma_lut {
> > +		/** ht_size: last request size to allocate the hashtable for. */
> > +		unsigned int ht_size;
> > +#define RESIZE_IN_PROGRESS BIT(0)
> 
> Easily conflicting name? Forward declare and hide in .c file? By making
> ht[0] at the end, and maybe pull out work. Or maybe just prefix :P
> 
> > +#define to_ptr(T, x) ((T *)(uintptr_t)(x))
> 
> i915_utils.h so we some day push to core. from_uintptr might make more
> sense, though.
> 
> The remainder is somewhat hard to review due to combined code motion
> and changes becoming mess, but didn't find anything else but I still
> dislike rsvd2 usage, and the magic bit it has.
> 
> We could at least #define proper_name rsvd2 in the .c file... Jani
> would be glad, I guess. I think if somebody touches a reserved field by
> name, they deserve to have their build broken.
> 
> Regards, Joonas
> -- 
> Joonas Lahtinen
> Open Source Technology Center
> Intel Corporation

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch