Re: [PATCH 2/2] drm/i915: Don't BUG_ON in i915_gem_obj_offset

From: Ben Widawsky <ben@bwidawsk.net>
To: Dave Airlie <airlied@gmail.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 2/2] drm/i915: Don't BUG_ON in i915_gem_obj_offset
Date: Tue, 24 Jun 2014 21:00:19 -0700	[thread overview]
Message-ID: <20140625040018.GA1072@bwidawsk.net> (raw)
In-Reply-To: <CAPM=9txwenqWwXvNzh-6FP7M9Rj0WiRKH1u_fdx39Kr6UVfMCw@mail.gmail.com>

On Wed, Jun 25, 2014 at 12:00:35PM +1000, Dave Airlie wrote:
> On 25 June 2014 11:30, Ben Widawsky <ben@bwidawsk.net> wrote:
> > On Wed, Jun 18, 2014 at 12:04:46AM +0200, Daniel Vetter wrote:
> >> On Tue, Jun 17, 2014 at 10:34:38PM +0200, Daniel Vetter wrote:
> >> > A WARN_ON is perfectly fine.
> >> >
> >> > The BUG in here seems to be the cause behind hard-hangs when I cat the
> >> > i915_gem_pageflip debugfs file (which calls this from an irq
> >> > spinlock). But only while running a full igt run after a while. I
> >> > still need to root cause the underlying issue.
> >> >
> >> > I'll also start reject patches which add new BUG_ON but don't come
> >> > with a really good justification for it. The general rule really
> >> > should be to just WARN and hope the driver survives for long enough.
> >> >
> >> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >>
> >> Both patches merged, this one improved per Chris' suggestions on irc.
> >> -Daniel
> >>
> >
> > Hey, here's an idea. How about we root cause bugs instead of making
> > blanket statements about the validity of real assertions? If the callers
> > of ggtt_offset are calling it on unbound objects, it's a violation of
> > the design. And in the other cases, it's a real bug.
> >
> > I'd NAK this patch if it wasn't already merged, and my NAK meant
> > something.
> >
> 
> Its kinda hard to debug an assert if it takes the whole box down, and you
> never see the assert printed anywhere. Any why should the whole
> kernel die because the GPU driver stuffed up.
> 
> Maybe you confused this with userspace.
> 
> Dave.

I shouldn't have said this. I was really pissed off that our PPGTT code
(which was relatively stable with some missing corner cases on merge) is
bit-rotting. It's essentially unusable on BDW, and has cost me more than
a week straight. Many of the original checks I had in place are now gone
for what usually appears to be laziness.

I have a very long-going fundamental disagreement with Daniel about
invariants (and apparently you as well). In either event, the snide
sarcasm was inappropriate of me.

So let's please nip this thread in the bud here.

Thanks.

-- 
Ben Widawsky, Intel Open Source Technology Center