[Intel-gfx] [PATCH 1/2] drm/i915: Stop overwriting RING_IMR in rcs resume

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-04-18 16:08:58)
> 
> On 18/04/2019 15:59, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-04-18 15:34:09)
> >>
> >> On 18/04/2019 14:27, Chris Wilson wrote:
> >>> We store the engine->imr mask and set up the RING_IMR register on
> >>> restarting the engine. We do not then want to overwrite it with
> >>> an incomplete mask later as we may then lose interrupts!
> >>>
> >>> Reported-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >>> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >>> ---
> >>>    drivers/gpu/drm/i915/intel_ringbuffer.c | 3 ---
> >>>    1 file changed, 3 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> >>> index 029fd8ec1857..00bd9eeb053d 100644
> >>> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> >>> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> >>> @@ -873,9 +873,6 @@ static int init_render_ring(struct intel_engine_cs *engine)
> >>>        if (IS_GEN_RANGE(dev_priv, 6, 7))
> >>>                I915_WRITE(INSTPM, _MASKED_BIT_ENABLE(INSTPM_FORCE_ORDERING));
> >>>    
> >>> -     if (INTEL_GEN(dev_priv) >= 6)
> >>> -             ENGINE_WRITE(engine, RING_IMR, ~engine->irq_keep_mask);
> >>> -
> >>>        return 0;
> >>>    }
> >>>    
> >>>
> >>
> >> Is it perhaps a way to enable L3_DPF on IVB and HSW when breadcrumbs are
> >> off (no one listening)? Mind you, the same should be done in this case
> >> under execlists for ctx switch irq. I just can't find the code there
> >> which unmasks it unless a single engine->irq_enable/irq_disable cycle is
> >> guaranteed on load indirectly. Is there a request_wait or wait_for_idle
> >> hidden somewhere during init? Or I missed the ball completely?
> > 
> > See intel_engine_reset_breadcrumbs(), we always re-emit the breadcrumb
> > setup (IMR) for either the disabled or enabled state. That should make
> > sure engine->irq_keep_mask is applied.
> 
> Ah.. thanks! Although it is a bit conceptually wrong that breadcrumbs 
> have to do the basic/unrelated engine setup. As such the legacy 
> submission indeed looks more correct, but still wrong that engine reset 
> can turn off user interrupts. Blah.. another pedestrian task for a rainy 
> day.

I kind of agree, but it's quite tangled up -- how dare they put extra
state in my breadcrumb register :)

Take the view that breadcrumbs is a badly named RING_IMR wrapper. Maybe
you can find an angle there.
-Chris