[Intel-gfx] [PATCH 09/23] drm/i915: Use b->irq_enable() as predicate for mock engine
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Thu Jan 17 16:44:54 UTC 2019
On 17/01/2019 14:34, Chris Wilson wrote:
> Since commit d4ccceb05591 ("drm/i915/icl: Ringbuffer interrupt handling")
> we have required a mechanism to avoid touching the interrupt hardware
> for breadcrumbs, superseding our mock interface for selftests.
>
> References: d4ccceb05591 ("drm/i915/icl: Ringbuffer interrupt handling")
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> ---
> drivers/gpu/drm/i915/intel_breadcrumbs.c | 39 ++++++++------------
> drivers/gpu/drm/i915/intel_engine_cs.c | 11 ++----
> drivers/gpu/drm/i915/intel_ringbuffer.h | 1 -
> drivers/gpu/drm/i915/selftests/mock_engine.c | 1 -
> 4 files changed, 20 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> index 4ed7105d7ff5..7b517bf83507 100644
> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> @@ -158,6 +158,9 @@ static void intel_breadcrumbs_fake_irq(struct timer_list *t)
>
> static void irq_enable(struct intel_engine_cs *engine)
> {
> + if (!engine->irq_enable)
> + return;
> +
> /*
> * FIXME: Ideally we want this on the API boundary, but for the
> * sake of testing with mock breadcrumbs (no HW so unable to
Okay I think I misunderstood this patch in the last round. So you want
to avoid the GEM_BUG_ON below _and_ a dedicated boolean only for the
mock engine.
I only wonder on the remaining merit of this comment and actually a
GEM_BUG_ON, which will be hit and miss depending on the platform now.
Gut feeling says something is still not ideal here. Selftests variable
does actually feel better in this sense.
mock_engine seems only used from mock_gem_device, so could an
alternative be to set i915->runtime_pm.irqs_enabled there and keep the
GEM_BUG_ON in irq_enable above the !engine->irq_enable early return?
That would still provide the unconditional assert on the state of the
driver outside selftests.
Regards,
Tvrtko
> @@ -167,21 +170,20 @@ static void irq_enable(struct intel_engine_cs *engine)
> GEM_BUG_ON(!intel_irqs_enabled(engine->i915));
>
> /* Caller disables interrupts */
> - if (engine->irq_enable) {
> - spin_lock(&engine->i915->irq_lock);
> - engine->irq_enable(engine);
> - spin_unlock(&engine->i915->irq_lock);
> - }
> + spin_lock(&engine->i915->irq_lock);
> + engine->irq_enable(engine);
> + spin_unlock(&engine->i915->irq_lock);
> }
>
> static void irq_disable(struct intel_engine_cs *engine)
> {
> + if (!engine->irq_disable)
> + return;
> +
> /* Caller disables interrupts */
> - if (engine->irq_disable) {
> - spin_lock(&engine->i915->irq_lock);
> - engine->irq_disable(engine);
> - spin_unlock(&engine->i915->irq_lock);
> - }
> + spin_lock(&engine->i915->irq_lock);
> + engine->irq_disable(engine);
> + spin_unlock(&engine->i915->irq_lock);
> }
>
> void __intel_engine_disarm_breadcrumbs(struct intel_engine_cs *engine)
> @@ -293,25 +295,16 @@ static bool __intel_breadcrumbs_enable_irq(struct intel_breadcrumbs *b)
> if (b->irq_armed)
> return false;
>
> - /* The breadcrumb irq will be disarmed on the interrupt after the
> + /*
> + * The breadcrumb irq will be disarmed on the interrupt after the
> * waiters are signaled. This gives us a single interrupt window in
> * which we can add a new waiter and avoid the cost of re-enabling
> * the irq.
> */
> b->irq_armed = true;
>
> - if (I915_SELFTEST_ONLY(b->mock)) {
> - /* For our mock objects we want to avoid interaction
> - * with the real hardware (which is not set up). So
> - * we simply pretend we have enabled the powerwell
> - * and the irq, and leave it up to the mock
> - * implementation to call intel_engine_wakeup()
> - * itself when it wants to simulate a user interrupt,
> - */
> - return true;
> - }
> -
> - /* Since we are waiting on a request, the GPU should be busy
> + /*
> + * Since we are waiting on a request, the GPU should be busy
> * and should have its own rpm reference. This is tracked
> * by i915->gt.awake, we can forgo holding our own wakref
> * for the interrupt as before i915->gt.awake is released (when
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index e2f65c59d6e8..fc52737751e7 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -917,6 +917,9 @@ static bool ring_is_idle(struct intel_engine_cs *engine)
> intel_wakeref_t wakeref;
> bool idle = true;
>
> + if (I915_SELFTEST_ONLY(!engine->mmio_base))
> + return true;
> +
> /* If the whole device is asleep, the engine must be idle */
> wakeref = intel_runtime_pm_get_if_in_use(dev_priv);
> if (!wakeref)
> @@ -955,9 +958,6 @@ bool intel_engine_is_idle(struct intel_engine_cs *engine)
> if (!intel_engine_signaled(engine, intel_engine_last_submit(engine)))
> return false;
>
> - if (I915_SELFTEST_ONLY(engine->breadcrumbs.mock))
> - return true;
> -
> /* Waiting to drain ELSP? */
> if (READ_ONCE(engine->execlists.active)) {
> struct tasklet_struct *t = &engine->execlists.tasklet;
> @@ -983,10 +983,7 @@ bool intel_engine_is_idle(struct intel_engine_cs *engine)
> return false;
>
> /* Ring stopped? */
> - if (!ring_is_idle(engine))
> - return false;
> -
> - return true;
> + return ring_is_idle(engine);
> }
>
> bool intel_engines_are_idle(struct drm_i915_private *dev_priv)
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 1adf9845710c..17e05d11ee34 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -397,7 +397,6 @@ struct intel_engine_cs {
> unsigned int irq_count;
>
> bool irq_armed : 1;
> - I915_SELFTEST_DECLARE(bool mock : 1);
> } breadcrumbs;
>
> struct {
> diff --git a/drivers/gpu/drm/i915/selftests/mock_engine.c b/drivers/gpu/drm/i915/selftests/mock_engine.c
> index 50e1a0b1af7e..9fe5b2c8f8d4 100644
> --- a/drivers/gpu/drm/i915/selftests/mock_engine.c
> +++ b/drivers/gpu/drm/i915/selftests/mock_engine.c
> @@ -201,7 +201,6 @@ struct intel_engine_cs *mock_engine(struct drm_i915_private *i915,
> i915_timeline_set_subclass(&engine->base.timeline, TIMELINE_ENGINE);
>
> intel_engine_init_breadcrumbs(&engine->base);
> - engine->base.breadcrumbs.mock = true; /* prevent touching HW for irqs */
>
> /* fake hw queue */
> spin_lock_init(&engine->hw_lock);
>
More information about the Intel-gfx
mailing list