Re: [PATCH 06/11] drm/i915/gtt: Convert vm->scratch into an array

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Quoting Mika Kuoppala (2019-07-10 15:18:32)
>> Chris Wilson <chris@chris-wilson.co.uk> writes:
>> > -     if (i915_vm_is_4lvl(vm)) {
>> > -             if (unlikely(setup_page_dma(vm, &vm->scratch_pdp))) {
>> > -                     ret = -ENOMEM;
>> > -                     goto free_pd;
>> > -             }
>> > -             fill_page_dma(&vm->scratch_pdp,
>> > -                           gen8_pde_encode(vm->scratch_pdp.daddr,
>> > -                                           I915_CACHE_LLC));
>> > +             fill_px(&vm->scratch[i], vm->scratch[i - 1].encode);
>> > +             vm->scratch[i].encode =
>> > +                     gen8_pde_encode(px_dma(&vm->scratch[i]),
>> > +                                     I915_CACHE_LLC);
>> 
>> Ok. The new code makes perfect sense here.
>> 
>> And with it confusion arises: how did we manage
>> to get the old code work with pdp encoding pointing to itself?
>
> What the.... You're right. That must have caused some funky GPU hangs if
> people tried to access something far outside of their set.
>
> Hmm, you know that's exactly what live_contexts/vm_isolation tries. Well
> it tries to write into random invalid addresses and see if the writes
> affect scratch of another context. Did I choose randomly carefully
> enough? Hmm. offset &= -sizeof(u32); Is there a danger there that's only
> u32 and not u64. Maybe. But otherwise it looks like it should be picking
> a prng over the whole vm->total and so should be tripping over the
> recursion :|

Worth investigation surely.
But this patch looks good and  makes it sane,

Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx