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

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

+ Daniel for the rsvd2

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@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx