[Intel-gfx] [PATCH v8 6/8] drm/i915: create context image vma in kernel context

[Lionel Landwerlin]

On 29/05/18 23:08, Chris Wilson wrote:
> Quoting Lionel Landwerlin (2018-05-29 22:35:08)
>> On 29/05/18 21:26, Michel Thierry wrote:
>>> Hi,
>>>
>>> On 5/29/2018 12:16 PM, Lionel Landwerlin wrote:
>>>> We want to be able to modify other context images from the kernel
>>>> context in a following commit. To be able to do this we need to map
>>>> the context image into the kernel context's ppgtt.
>>>>
>>>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/i915_gem_context.h |  1 +
>>>>    drivers/gpu/drm/i915/intel_lrc.c        | 19 ++++++++++++++-----
>>>>    2 files changed, 15 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_gem_context.h
>>>> b/drivers/gpu/drm/i915/i915_gem_context.h
>>>> index f40d85448a28..9c313c2edb09 100644
>>>> --- a/drivers/gpu/drm/i915/i915_gem_context.h
>>>> +++ b/drivers/gpu/drm/i915/i915_gem_context.h
>>>> @@ -155,6 +155,7 @@ struct i915_gem_context {
>>>>        struct intel_context {
>>>>            struct i915_gem_context *gem_context;
>>>>            struct i915_vma *state;
>>>> +        struct i915_vma *kernel_state; /**/
> It's debatable if we want to keep the pointer around.
>
> We have one for the context state vma and ring vma, because we need to
> keep a pointer for the object and so choose to keep the vma instead as
> we use that more often than the drm_i915_gem_object and can derive it
> from vma->obj.
>
> For this piece of code, which should be run that often I don't see a lot
> of advantage over using the rbtree search, and since the number of
> objects in that tree will not be large (2 at most), quick.
>
> /* Don't forget to declare the dependency tree! */
> prev = i915_gem_active_raw(&ce->ring->timeline->last_request,
> 			  &rq->i915->drm.struct_mutex);
> if (prev) {
> 	err = i915_sw_fence_await_sw_fence_gfp(&rq->submit,
> 					       &prev->submit,
> 					       I915_FENCE_GFP);
> 	if (err < 0)
> 		return err;
> }

Thanks, this is done by the caller.

>
> vma = i915_vma_instance(ce->state->obj, my_vm, NULL);
> if (IS_ERR(vma)) ...
>
> err = i915_vma_pin(vma, 0, PIN_USER);
> if (err) ...
>
> err = i915_vma_move_to_active(vma, rq, EXEC_OBJECT_WRITE);
> i915_vma_unpin(vma);
> if (err) ... /* not today, but tomorrow */
>
> Now you can
>
> cs = intel_ring_begin(rq, 4);
> ...
> *cs++ = gen8_canonical_addr(vma->node.state + offset);
> ...
> intel_ring_advance(rq, cs);
>
> On error, you will have to submit the request since it has interacted
> with the tracking.

Yep, done in the caller too.

> Don't bother pushing this into an engine vfunc, if you don't intend to 
> have multiple implementations. It's only a SDM, nothing that appears 
> the need to be specialised. Although you might argue the context 
> layout will require it later? As for the SDM, Joonas might complain 
> about the proliferation, but I'm not sure if we have a good repeating 
> pattern yet. -Chris
Hmm.. That's kind of useful to detect the ability to change powergating 
configs.

Thanks a lot,

-
Lionel