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

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-01-17 16:44:54)
> 
> 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.

I'm just going to kill the mention of irqs enabled here, eventually. It
was interesting once because we messed up suspend a decade ago and had to
handle the case of waiting after we had already uninstalled the irq
handler.

This is nothing to do with irqs_enabled; the selftest variable was all
because the mock engine had no engine->irq_enable() callback, and now it
is not the only engine so the assumption that we must always have
engine->irq_enable() is invalid.
-Chris