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

[Chris Wilson]

Quoting Lionel Landwerlin (2018-05-30 11:05:25)
> 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.

It probably shouldn't be, because the fence is related to this write.
(And it should be i915_request_await_request, but that'll require an
export.) The await + move-to-active (hmm, this is before fence export
refactoring, oh well, needs that patch as well) are paired. Ideally we
do all awaits first, checking for errors before touching the global
state as then we can discard the request; but here I think the coupling
is very, very special because the context objects do not follow the
normal rules. So something to be very wary of.

> > 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.

Yes, it has to be. I'm just stressing that we can't simply discard this
request part way through because it has manipulated global state.
-Chris