From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 1/2] drm/i915: Improve irq handling after gpu resets Date: Mon, 26 May 2014 12:48:05 +0200 Message-ID: <20140526104805.GF14357@phenom.ffwll.local> References: <1400774195-19414-5-git-send-email-daniel.vetter@ffwll.ch> <1400789902-31759-1-git-send-email-daniel.vetter@ffwll.ch> <20140526083633.GE27580@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail-wg0-f46.google.com (mail-wg0-f46.google.com [74.125.82.46]) by gabe.freedesktop.org (Postfix) with ESMTP id A7D4E6E299 for ; Mon, 26 May 2014 03:48:10 -0700 (PDT) Received: by mail-wg0-f46.google.com with SMTP id n12so7833118wgh.5 for ; Mon, 26 May 2014 03:48:09 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20140526083633.GE27580@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Cc: Daniel Vetter , Intel Graphics Development List-Id: intel-gfx@lists.freedesktop.org On Mon, May 26, 2014 at 11:36:33AM +0300, Ville Syrj=E4l=E4 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 danc= ing. > > 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=E4l=E4 > > Signed-off-by: Daniel Vetter > = > Both patches: > Reviewed-by: Ville Syrj=E4l=E4 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/i91= 5_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