| Message ID | 1371037046-3732-20-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jun 12, 2013 at 01:37:21PM +0200, Daniel Vetter wrote: > The code to handle it is broken - there's simply no code to clear CS > parser errors on gen5+. And behold, for all the other rings we also > don't enable it! > > Leave the handling code itself in place just to be consistent with the > existing mess though. And in case someone feels like fixing it all up. > > This has been errornously enabled in > > commit 12638c57f31952127c734c26315e1348fa1334c2 > Author: Ben Widawsky <ben@bwidawsk.net> > Date: Tue May 28 19:22:31 2013 -0700 > > drm/i915: Enable vebox interrupts > > Cc: Damien Lespiau <damien.lespiau@intel.com> > Cc: Ben Widawsky <ben@bwidawsk.net> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > Why not fix the problem instead of just disabling the interrupt? Then the "existing mess" is justified. It really shouldn't be terribly difficult to fix it. [snip]
On Wed, Jun 12, 2013 at 10:13:41AM -0700, Ben Widawsky wrote: > On Wed, Jun 12, 2013 at 01:37:21PM +0200, Daniel Vetter wrote: > > The code to handle it is broken - there's simply no code to clear CS > > parser errors on gen5+. And behold, for all the other rings we also > > don't enable it! > > > > Leave the handling code itself in place just to be consistent with the > > existing mess though. And in case someone feels like fixing it all up. > > > > This has been errornously enabled in > > > > commit 12638c57f31952127c734c26315e1348fa1334c2 > > Author: Ben Widawsky <ben@bwidawsk.net> > > Date: Tue May 28 19:22:31 2013 -0700 > > > > drm/i915: Enable vebox interrupts > > > > Cc: Damien Lespiau <damien.lespiau@intel.com> > > Cc: Ben Widawsky <ben@bwidawsk.net> > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > Why not fix the problem instead of just disabling the interrupt? Then > the "existing mess" is justified. It really shouldn't be terribly > difficult to fix it. Haven't seen it proven useful, and I don't want to blow through time just for fun on it. Hangcheck seems to be able to deal with it just fine. -Daniel
On Wed, Jun 12, 2013 at 07:18:38PM +0200, Daniel Vetter wrote: > On Wed, Jun 12, 2013 at 10:13:41AM -0700, Ben Widawsky wrote: > > On Wed, Jun 12, 2013 at 01:37:21PM +0200, Daniel Vetter wrote: > > > The code to handle it is broken - there's simply no code to clear CS > > > parser errors on gen5+. And behold, for all the other rings we also > > > don't enable it! > > > > > > Leave the handling code itself in place just to be consistent with the > > > existing mess though. And in case someone feels like fixing it all up. > > > > > > This has been errornously enabled in > > > > > > commit 12638c57f31952127c734c26315e1348fa1334c2 > > > Author: Ben Widawsky <ben@bwidawsk.net> > > > Date: Tue May 28 19:22:31 2013 -0700 > > > > > > drm/i915: Enable vebox interrupts > > > > > > Cc: Damien Lespiau <damien.lespiau@intel.com> > > > Cc: Ben Widawsky <ben@bwidawsk.net> > > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > > > > Why not fix the problem instead of just disabling the interrupt? Then > > the "existing mess" is justified. It really shouldn't be terribly > > difficult to fix it. > > Haven't seen it proven useful, and I don't want to blow through time just > for fun on it. Hangcheck seems to be able to deal with it just fine. > -Daniel I don't get it. Isn't it just clearing a bit in the handler, vs. clearing the flag here? > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Jun 12, 2013 at 8:19 PM, Ben Widawsky <ben@bwidawsk.net> wrote: > On Wed, Jun 12, 2013 at 07:18:38PM +0200, Daniel Vetter wrote: >> On Wed, Jun 12, 2013 at 10:13:41AM -0700, Ben Widawsky wrote: >> > On Wed, Jun 12, 2013 at 01:37:21PM +0200, Daniel Vetter wrote: >> > > The code to handle it is broken - there's simply no code to clear CS >> > > parser errors on gen5+. And behold, for all the other rings we also >> > > don't enable it! >> > > >> > > Leave the handling code itself in place just to be consistent with the >> > > existing mess though. And in case someone feels like fixing it all up. >> > > >> > > This has been errornously enabled in >> > > >> > > commit 12638c57f31952127c734c26315e1348fa1334c2 >> > > Author: Ben Widawsky <ben@bwidawsk.net> >> > > Date: Tue May 28 19:22:31 2013 -0700 >> > > >> > > drm/i915: Enable vebox interrupts >> > > >> > > Cc: Damien Lespiau <damien.lespiau@intel.com> >> > > Cc: Ben Widawsky <ben@bwidawsk.net> >> > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> >> > > >> > Why not fix the problem instead of just disabling the interrupt? Then >> > the "existing mess" is justified. It really shouldn't be terribly >> > difficult to fix it. >> >> Haven't seen it proven useful, and I don't want to blow through time just >> for fun on it. Hangcheck seems to be able to deal with it just fine. >> -Daniel > I don't get it. Isn't it just clearing a bit in the handler, vs. > clearing the flag here? On a quick Bspec read we'd need to set up per-ring error interrupt registers, put them to sensible value in them, wire up the interrupt handling for each ring since ilk+ and then also test it. I really don't see the point in doing that. The only upshot of all that work is that if there's an instruction error we'd get an interrupt instead of waiting for the hangcheck to kick in. If you insist I can follow-up with a patch to just rip out the bare-bones handler we have now. Everything else really feels like wasted effort to me. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Wed, Jun 12, 2013 at 08:32:44PM +0200, Daniel Vetter wrote: > On Wed, Jun 12, 2013 at 8:19 PM, Ben Widawsky <ben@bwidawsk.net> wrote: > > On Wed, Jun 12, 2013 at 07:18:38PM +0200, Daniel Vetter wrote: > >> On Wed, Jun 12, 2013 at 10:13:41AM -0700, Ben Widawsky wrote: > >> > On Wed, Jun 12, 2013 at 01:37:21PM +0200, Daniel Vetter wrote: > >> > > The code to handle it is broken - there's simply no code to clear CS > >> > > parser errors on gen5+. And behold, for all the other rings we also > >> > > don't enable it! > >> > > > >> > > Leave the handling code itself in place just to be consistent with the > >> > > existing mess though. And in case someone feels like fixing it all up. > >> > > > >> > > This has been errornously enabled in > >> > > > >> > > commit 12638c57f31952127c734c26315e1348fa1334c2 > >> > > Author: Ben Widawsky <ben@bwidawsk.net> > >> > > Date: Tue May 28 19:22:31 2013 -0700 > >> > > > >> > > drm/i915: Enable vebox interrupts > >> > > > >> > > Cc: Damien Lespiau <damien.lespiau@intel.com> > >> > > Cc: Ben Widawsky <ben@bwidawsk.net> > >> > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > >> > > > >> > Why not fix the problem instead of just disabling the interrupt? Then > >> > the "existing mess" is justified. It really shouldn't be terribly > >> > difficult to fix it. > >> > >> Haven't seen it proven useful, and I don't want to blow through time just > >> for fun on it. Hangcheck seems to be able to deal with it just fine. > >> -Daniel > > I don't get it. Isn't it just clearing a bit in the handler, vs. > > clearing the flag here? > > On a quick Bspec read we'd need to set up per-ring error interrupt > registers, put them to sensible value in them, wire up the interrupt > handling for each ring since ilk+ and then also test it. I really > don't see the point in doing that. The only upshot of all that work is > that if there's an instruction error we'd get an interrupt instead of > waiting for the hangcheck to kick in. > > If you insist I can follow-up with a patch to just rip out the > bare-bones handler we have now. Everything else really feels like > wasted effort to me. > -Daniel No. I don't insist. I was just thinking these kind of error interrupts should be always on, like L3 parity. But that is indeed more than just clearing a bit (though not THAT much).
On Wed, Jun 12, 2013 at 08:32:44PM +0200, Daniel Vetter wrote: > On Wed, Jun 12, 2013 at 8:19 PM, Ben Widawsky <ben@bwidawsk.net> wrote: > > On Wed, Jun 12, 2013 at 07:18:38PM +0200, Daniel Vetter wrote: > >> On Wed, Jun 12, 2013 at 10:13:41AM -0700, Ben Widawsky wrote: > >> > On Wed, Jun 12, 2013 at 01:37:21PM +0200, Daniel Vetter wrote: > >> > > The code to handle it is broken - there's simply no code to clear CS > >> > > parser errors on gen5+. And behold, for all the other rings we also > >> > > don't enable it! > >> > > > >> > > Leave the handling code itself in place just to be consistent with the > >> > > existing mess though. And in case someone feels like fixing it all up. > >> > > > >> > > This has been errornously enabled in > >> > > > >> > > commit 12638c57f31952127c734c26315e1348fa1334c2 > >> > > Author: Ben Widawsky <ben@bwidawsk.net> > >> > > Date: Tue May 28 19:22:31 2013 -0700 > >> > > > >> > > drm/i915: Enable vebox interrupts > >> > > > >> > > Cc: Damien Lespiau <damien.lespiau@intel.com> > >> > > Cc: Ben Widawsky <ben@bwidawsk.net> > >> > > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > >> > > > >> > Why not fix the problem instead of just disabling the interrupt? Then > >> > the "existing mess" is justified. It really shouldn't be terribly > >> > difficult to fix it. > >> > >> Haven't seen it proven useful, and I don't want to blow through time just > >> for fun on it. Hangcheck seems to be able to deal with it just fine. > >> -Daniel > > I don't get it. Isn't it just clearing a bit in the handler, vs. > > clearing the flag here? > > On a quick Bspec read we'd need to set up per-ring error interrupt > registers, put them to sensible value in them, wire up the interrupt > handling for each ring since ilk+ and then also test it. I really > don't see the point in doing that. The only upshot of all that work is > that if there's an instruction error we'd get an interrupt instead of > waiting for the hangcheck to kick in. Hmm. I still think you're overthinking this. I don't think it requires so much work to get the parser interrupts, but I don't insist. Reviewed-by: Ben Widawsky <ben@bwidawsk.net> > > If you insist I can follow-up with a patch to just rip out the > bare-bones handler we have now. Everything else really feels like > wasted effort to me. > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index cd7135d..293ee68 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -2715,8 +2715,7 @@ static int ivybridge_irq_postinstall(struct drm_device *dev) I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR)); if (HAS_VEBOX(dev)) - pm_irqs |= PM_VEBOX_USER_INTERRUPT | - PM_VEBOX_CS_ERROR_INTERRUPT; + pm_irqs |= PM_VEBOX_USER_INTERRUPT; /* Our enable/disable rps functions may touch these registers so * make sure to set a known state for only the non-RPS bits. diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index b75e9d0..a86d5d6 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -1998,8 +1998,7 @@ int intel_init_vebox_ring_buffer(struct drm_device *dev) ring->add_request = gen6_add_request; ring->get_seqno = gen6_ring_get_seqno; ring->set_seqno = ring_set_seqno; - ring->irq_enable_mask = PM_VEBOX_USER_INTERRUPT | - PM_VEBOX_CS_ERROR_INTERRUPT; + ring->irq_enable_mask = PM_VEBOX_USER_INTERRUPT; ring->irq_get = hsw_vebox_get_irq; ring->irq_put = hsw_vebox_put_irq; ring->dispatch_execbuffer = gen6_ring_dispatch_execbuffer;
The code to handle it is broken - there's simply no code to clear CS parser errors on gen5+. And behold, for all the other rings we also don't enable it! Leave the handling code itself in place just to be consistent with the existing mess though. And in case someone feels like fixing it all up. This has been errornously enabled in commit 12638c57f31952127c734c26315e1348fa1334c2 Author: Ben Widawsky <ben@bwidawsk.net> Date: Tue May 28 19:22:31 2013 -0700 drm/i915: Enable vebox interrupts Cc: Damien Lespiau <damien.lespiau@intel.com> Cc: Ben Widawsky <ben@bwidawsk.net> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/i915_irq.c | 3 +-- drivers/gpu/drm/i915/intel_ringbuffer.c | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-)