+ 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@xxxxxxxxxxxxxxxxxx>
<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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
