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@xxxxxxxxxxxxxxx> > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxx> Both patches: Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> 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. > --- > 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. -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
