Re: [PATCH 1/2] drm/i915: Improve irq handling after gpu resets

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, May 26, 2014 at 11:36:33AM +0300, Ville Syrjälä wrote:
> On Thu, May 22, 2014 at 10:18:21PM +0200, Daniel Vetter wrote:
> > Currently we do a full re-init of all interrupts after a gpu hang.
> > Which is pretty bad since we don't restore the interrupts we've
> > enabled at runtime correctly. Even with that addressed it's rather
> > horribly race.
> > 
> > But on g4x and later we only reset the gt and not the entire gpu.
> > Which means we only need to reset the GT interrupt bits. Which has the
> > nice benefit that vblank waits, pipe CRC interrupts and everything
> > else display related just keeps on working.
> > 
> > The downside is that gt interrupt handling (i.e. ring->get/put_irq) is
> > still racy. But as long as the gpu hang reliably wakes all waters and
> > we have a short time where the refcount drops to 0 we'll recover. So
> > not that bad really.
> > 
> > v2: Ville noticed that GTIMR and PMIMR don't get cleared, only the
> > subordinate per-ring registers. So let's rip out all the interrupt dancing.
> > The FIXME comment is still required though since the ring irq handling
> > happens at the per-ring interrupt mask registers, too.
> > 
> > Testcase: igt/kms_flip/vblank-vs-hang
> > Testcase: igt/kms_pipe_crc_basic/hang-*
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Both patches:
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Thanks, will pull in the patches later (lunchtime firts).

> And to answer you earlier question, yes things seemd to work fine after
> a GPU reset if I didn't touch the interrupt registers. In fact I also
> tried killing most of the gem_hw_init() stuff (basically just left
> ring->init(), l3_remap, and context enable) and things still seemed to
> work just fine.

Hm, we have pretty spotty test coverage for this stuff against gpu resets.
Also in case of doubt I prefer we don't special-case hw init code without
a good reason. Duplicating such logic for driver init, resume, gpu reset
or runtime pm resume just leads to duplicated bugs ime.


> > ---
> >  drivers/gpu/drm/i915/i915_drv.c | 16 ++++++++--------
> >  1 file changed, 8 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index c83c83b74bf4..7ae906c811bb 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -811,17 +811,17 @@ int i915_reset(struct drm_device *dev)
> >  		}
> >  
> >  		/*
> > -		 * FIXME: This is horribly race against concurrent pageflip and
> > -		 * vblank wait ioctls since they can observe dev->irqs_disabled
> > -		 * being false when they shouldn't be able to.
> > +		 * FIXME: This races pretty badly against concurrent holders of
> > +		 * ring interrupts. This is possible since we've started to drop
> > +		 * dev->struct_mutex in select places when waiting for the gpu.
> >  		 */
> > -		drm_irq_uninstall(dev);
> > -		drm_irq_install(dev, dev->pdev->irq);
> >  
> > -		/* rps/rc6 re-init is necessary to restore state lost after the
> > -		 * reset and the re-install of drm irq. Skip for ironlake per
> > +		/*
> > +		 * rps/rc6 re-init is necessary to restore state lost after the
> > +		 * reset and the re-install of gt irqs. Skip for ironlake per
> >  		 * previous concerns that it doesn't respond well to some forms
> > -		 * of re-init after reset. */
> > +		 * of re-init after reset.
> > +		 */
> >  		if (INTEL_INFO(dev)->gen > 5)
> >  			intel_reset_gt_powersave(dev);
> 
> I'm suspecting that GPU reset doesn't affect the RPS/RC6 stuff either.
> But I suppose it shouldn't really hurt anything to do it, so it's just
> something to look into if we want to reduce the amount of stuff we do
> at reset.

Iirc we've had bugs in this area, and we have a testcase for at least some
of them: igt/pm_rps/reset. Not sure if there's more lurking.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch