Re: [PATCH 09/23] drm/i915: Use b->irq_enable() as predicate for mock engine

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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@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);
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx