[PATCH 1/2] drm/i915: Grab dev->struct_mutex in i915_gem_pageflip_info

[PATCH 1/2] drm/i915: Grab dev->struct_mutex in i915_gem_pageflip_info

[Daniel Vetter]

We could walk of a bad list otherwise when someone concurrently
unbinds stuff for fun.

I've suspected this as the root-cause behind seemingly inconsistent
state, but alas it's not.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 0e0e6b6bffd1..5c029bd53a26 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -513,6 +513,11 @@ static int i915_gem_pageflip_info(struct seq_file *m, void *data)
 	struct drm_device *dev = node->minor->dev;
 	unsigned long flags;
 	struct intel_crtc *crtc;
+	int ret;
+
+	ret = mutex_lock_interruptible(&dev->struct_mutex);
+	if (ret)
+		return ret;
 
 	for_each_intel_crtc(dev, crtc) {
 		const char pipe = pipe_name(crtc->pipe);
@@ -554,6 +559,8 @@ static int i915_gem_pageflip_info(struct seq_file *m, void *data)
 		spin_unlock_irqrestore(&dev->event_lock, flags);
 	}
 
+	mutex_unlock(&dev->struct_mutex);
+
 	return 0;
 }
 
-- 
1.8.1.4

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

[Daniel Vetter]

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>
---
 drivers/gpu/drm/i915/i915_gem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 35b5027ba5a4..841e9cd38dc1 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -5101,7 +5101,7 @@ unsigned long i915_gem_obj_offset(struct drm_i915_gem_object *o,
 	    vm == &dev_priv->mm.aliasing_ppgtt->base)
 		vm = &dev_priv->gtt.base;
 
-	BUG_ON(list_empty(&o->vma_list));
+	WARN_ON(list_empty(&o->vma_list));
 	list_for_each_entry(vma, &o->vma_list, vma_link) {
 		if (vma->vm == vm)
 			return vma->node.start;
-- 
1.8.1.4

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

[Daniel Vetter]

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

> ---
>  drivers/gpu/drm/i915/i915_gem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 35b5027ba5a4..841e9cd38dc1 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -5101,7 +5101,7 @@ unsigned long i915_gem_obj_offset(struct drm_i915_gem_object *o,
>  	    vm == &dev_priv->mm.aliasing_ppgtt->base)
>  		vm = &dev_priv->gtt.base;
>  
> -	BUG_ON(list_empty(&o->vma_list));
> +	WARN_ON(list_empty(&o->vma_list));
>  	list_for_each_entry(vma, &o->vma_list, vma_link) {
>  		if (vma->vm == vm)
>  			return vma->node.start;
> -- 
> 1.8.1.4
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Ben Widawsky]

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.

-- 
Ben Widawsky, Intel Open Source Technology Center

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

[Dave Airlie]

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.

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

[Ben Widawsky]

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