On 17/01/2019 16:52, Chris Wilson wrote:
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.
Okay, my complaint was minimal anyway:
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
