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