Re: [PATCH v2 2/9] drm/i915: Force the switch to the i915->kernel_context

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

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

> Quoting Mika Kuoppala (2017-11-03 14:03:25)
>> Chris Wilson <chris@chris-wilson.co.uk> writes:
>> 
>> > In the next few patches, we will have a hard requirement that we emit a
>> > context-switch to the perma-pinned i915->kernel_context (so that we can
>> > save the HW state using that context-switch). As the first context
>> > itself may be classed as a kernel context, we want to be explicit in our
>> > comparison. For an extra-layer of finesse, we can check the last
>> > unretired context on the engine; as well as the last retired context
>> > when idle.
>> >
>> > v2: verbose verbosity
>> > v3: Always force the switch, even when the engine is idle, and update
>> > the assert that this happens before suspend.
>> >
>> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> > Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> #v1
>> > ---
>> >  drivers/gpu/drm/i915/i915_gem.c        | 10 ++++++----
>> >  drivers/gpu/drm/i915/intel_engine_cs.c | 26 ++++++++++++++++++++++++--
>> >  2 files changed, 30 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> > index 9470ba0c1930..fe312ab3b664 100644
>> > --- a/drivers/gpu/drm/i915/i915_gem.c
>> > +++ b/drivers/gpu/drm/i915/i915_gem.c
>> > @@ -4687,14 +4687,16 @@ void __i915_gem_object_release_unless_active(struct drm_i915_gem_object *obj)
>> >               i915_gem_object_put(obj);
>> >  }
>> >  
>> > -static void assert_kernel_context_is_current(struct drm_i915_private *dev_priv)
>> > +static void assert_kernel_context_is_current(struct drm_i915_private *i915)
>> >  {
>> > +     struct i915_gem_context *kernel_context = i915->kernel_context;
>> >       struct intel_engine_cs *engine;
>> >       enum intel_engine_id id;
>> >  
>> > -     for_each_engine(engine, dev_priv, id)
>> > -             GEM_BUG_ON(engine->last_retired_context &&
>> > -                        !i915_gem_context_is_kernel(engine->last_retired_context));
>> > +     for_each_engine(engine, i915, id) {
>> > +             GEM_BUG_ON(__i915_gem_active_peek(&engine->timeline->last_request));
>> 
>> It could be that we have a active request for kernel context and we
>> still bail out with assert. Why?
>
> Because we waited for the system to be idle, so we expect the timeline
> to have been completely retired, which implies that
> timline->last_request is now NULL.

Ok, the callsite did reveal this. And I did reveal being lazy for not
looking.

assert_idle_on_kernel_context()?

Regardless,

Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

> -Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx