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@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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
