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

[Chris Wilson]

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;
}

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.

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