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

[Tvrtko Ursulin]

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 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.

Okay, my complaint was minimal anyway:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>

Regards,

Tvrtko