[PATCH 0/5] various small patches for -next

[PATCH 0/5] various small patches for -next

[Daniel Vetter]

Hi all,

The first 3 are stuff that I've found useful while hunting down gem_stress
fallout.

The latter 2 wrap up the gen2 tiling on the kernel side.

Please review and consider merging for -next.

Thanks, Daniel

Daniel Vetter (5):
  drm/i915: downgrade non-lethal BUG_ONs
  drm/i915: not finding a fence is a non-recoverable condition
  drm/i915: check gpu_write_list in move_to_flushing
  drm/i915: fix relaxed tiling on gen2: y-tiling on i855gm
  drm/i915: fix relaxed tiling on gen2: tile height

 drivers/gpu/drm/i915/i915_gem.c            |   21 +++++++++++++++------
 drivers/gpu/drm/i915/i915_gem_evict.c      |    4 ++--
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |    4 ++--
 3 files changed, 19 insertions(+), 10 deletions(-)

-- 
1.7.4.1

[PATCH 1/5] drm/i915: downgrade non-lethal BUG_ONs

[Daniel Vetter]

If it's a simple gem accounting error that won't lead to immediate harm
(like a NULL-deref) or is a simple violation of a required invariant
that the caller should always check/ensure, downgrade the BUG_ON to
a WARN_ON and hope the system survives long enough to grab the dmesg.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem.c            |    6 +++---
 drivers/gpu/drm/i915/i915_gem_evict.c      |    4 ++--
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |    4 ++--
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index ae40272..1ef0b91 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1748,7 +1748,7 @@ i915_gem_object_move_to_flushing(struct drm_i915_gem_object *obj)
 	struct drm_device *dev = obj->base.dev;
 	drm_i915_private_t *dev_priv = dev->dev_private;
 
-	BUG_ON(!obj->active);
+	WARN_ON(!obj->active);
 	list_move_tail(&obj->mm_list, &dev_priv->mm.flushing_list);
 
 	i915_gem_object_move_off_active(obj);
@@ -1765,8 +1765,8 @@ i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj)
 	else
 		list_move_tail(&obj->mm_list, &dev_priv->mm.inactive_list);
 
-	BUG_ON(!list_empty(&obj->gpu_write_list));
-	BUG_ON(!obj->active);
+	WARN_ON(!list_empty(&obj->gpu_write_list));
+	WARN_ON(!obj->active);
 	obj->ring = NULL;
 
 	i915_gem_object_move_off_active(obj);
diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
index da05a26..db62fae 100644
--- a/drivers/gpu/drm/i915/i915_gem_evict.c
+++ b/drivers/gpu/drm/i915/i915_gem_evict.c
@@ -136,7 +136,7 @@ i915_gem_evict_something(struct drm_device *dev, int min_size,
 				       exec_list);
 
 		ret = drm_mm_scan_remove_block(obj->gtt_space);
-		BUG_ON(ret);
+		WARN_ON(ret);
 
 		list_del_init(&obj->exec_list);
 		drm_gem_object_unreference(&obj->base);
@@ -199,7 +199,7 @@ i915_gem_evict_everything(struct drm_device *dev, bool purgeable_only)
 	if (ret)
 		return ret;
 
-	BUG_ON(!list_empty(&dev_priv->mm.flushing_list));
+	WARN_ON(!list_empty(&dev_priv->mm.flushing_list));
 
 	return i915_gem_evict_inactive(dev, purgeable_only);
 }
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 316603e..8cac87c 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1093,7 +1093,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 								&objects, eb,
 								exec,
 								args->buffer_count);
-			BUG_ON(!mutex_is_locked(&dev->struct_mutex));
+			WARN_ON(!mutex_is_locked(&dev->struct_mutex));
 		}
 		if (ret)
 			goto err;
@@ -1122,7 +1122,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
 			if (ret)
 				goto err;
 
-			BUG_ON(ring->sync_seqno[i]);
+			WARN_ON(ring->sync_seqno[i]);
 		}
 	}
 
-- 
1.7.4.1

Re: [PATCH 1/5] drm/i915: downgrade non-lethal BUG_ONs

[Chris Wilson]

On Tue, 19 Apr 2011 22:46:00 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> If it's a simple gem accounting error that won't lead to immediate harm
> (like a NULL-deref) or is a simple violation of a required invariant
> that the caller should always check/ensure, downgrade the BUG_ON to
> a WARN_ON and hope the system survives long enough to grab the dmesg.

When converting to a WARN we should include the error checking and
propagate the failure back up rather than continuing on with inconsistent
state.

> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_gem.c            |    6 +++---
>  drivers/gpu/drm/i915/i915_gem_evict.c      |    4 ++--
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |    4 ++--
>  3 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index ae40272..1ef0b91 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1748,7 +1748,7 @@ i915_gem_object_move_to_flushing(struct drm_i915_gem_object *obj)
>  	struct drm_device *dev = obj->base.dev;
>  	drm_i915_private_t *dev_priv = dev->dev_private;
>  
> -	BUG_ON(!obj->active);
> +	WARN_ON(!obj->active);

This warning can be moved up into the caller and so share the warning
between move_to_flushing and move_to_inactive.

>  	list_move_tail(&obj->mm_list, &dev_priv->mm.flushing_list);
>  
>  	i915_gem_object_move_off_active(obj);
> @@ -1765,8 +1765,8 @@ i915_gem_object_move_to_inactive(struct drm_i915_gem_object *obj)
>  	else
>  		list_move_tail(&obj->mm_list, &dev_priv->mm.inactive_list);
>  
> -	BUG_ON(!list_empty(&obj->gpu_write_list));
> -	BUG_ON(!obj->active);
> +	WARN_ON(!list_empty(&obj->gpu_write_list));
> +	WARN_ON(!obj->active);
>  	obj->ring = NULL;
>  
>  	i915_gem_object_move_off_active(obj);
> diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
> index da05a26..db62fae 100644
> --- a/drivers/gpu/drm/i915/i915_gem_evict.c
> +++ b/drivers/gpu/drm/i915/i915_gem_evict.c
> @@ -136,7 +136,7 @@ i915_gem_evict_something(struct drm_device *dev, int min_size,
>  				       exec_list);
>  
>  		ret = drm_mm_scan_remove_block(obj->gtt_space);
> -		BUG_ON(ret);
> +		WARN_ON(ret);

drm_mm_scan_remove_block() should be returning a bool so that we (I!) don't
confuse it with an error code.

if (WARN_ON(ret)) return -EIO; /* or whatever is safe. */

If there is no safe way to handle the error, it is a BUG.

>  		list_del_init(&obj->exec_list);
>  		drm_gem_object_unreference(&obj->base);
> @@ -199,7 +199,7 @@ i915_gem_evict_everything(struct drm_device *dev, bool purgeable_only)
>  	if (ret)
>  		return ret;
>  
> -	BUG_ON(!list_empty(&dev_priv->mm.flushing_list));
> +	WARN_ON(!list_empty(&dev_priv->mm.flushing_list));

if (WARN_ON(!list_empty()) return -EIO;
 
>  	return i915_gem_evict_inactive(dev, purgeable_only);
>  }
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index 316603e..8cac87c 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -1093,7 +1093,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  								&objects, eb,
>  								exec,
>  								args->buffer_count);
> -			BUG_ON(!mutex_is_locked(&dev->struct_mutex));
> +			WARN_ON(!mutex_is_locked(&dev->struct_mutex));

I think this can be dropped after close inspection of the call path.

>  		}
>  		if (ret)
>  			goto err;
> @@ -1122,7 +1122,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
>  			if (ret)
>  				goto err;
>  
> -			BUG_ON(ring->sync_seqno[i]);
> +			WARN_ON(ring->sync_seqno[i]);

if (WARN_ON()) { ret = -EIO; goto err; }
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/5] drm/i915: downgrade non-lethal BUG_ONs

[Ben Widawsky]

On Wed, Apr 20, 2011 at 09:18:03AM +0100, Chris Wilson wrote:

> > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > index 316603e..8cac87c 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > @@ -1093,7 +1093,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> >  								&objects, eb,
> >  								exec,
> >  								args->buffer_count);
> > -			BUG_ON(!mutex_is_locked(&dev->struct_mutex));
> > +			WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> 
> I think this can be dropped after close inspection of the call path.
> 

Is that right? There are definitely cases where the mutex is released
and not reacquired. You would know better than I if those cases can
occur in a normal system. Assuming they can, Won't we just BUG_ON when
we try to release struct_mutex?

> -Chris
> 

Ben

Re: [PATCH 1/5] drm/i915: downgrade non-lethal BUG_ONs

[Chris Wilson]

On Wed, 20 Apr 2011 07:27:24 -0700, Ben Widawsky <ben@bwidawsk.net> wrote:
> On Wed, Apr 20, 2011 at 09:18:03AM +0100, Chris Wilson wrote:
> 
> > > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > > index 316603e..8cac87c 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > > @@ -1093,7 +1093,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> > >  								&objects, eb,
> > >  								exec,
> > >  								args->buffer_count);
> > > -			BUG_ON(!mutex_is_locked(&dev->struct_mutex));
> > > +			WARN_ON(!mutex_is_locked(&dev->struct_mutex));
> > 
> > I think this can be dropped after close inspection of the call path.
> > 
> 
> Is that right? There are definitely cases where the mutex is released
> and not reacquired. You would know better than I if those cases can
> occur in a normal system. Assuming they can, Won't we just BUG_ON when
> we try to release struct_mutex?

This particular BUG_ON() I added at Daniel's request to clarify the
reservation fallback logic. Code inspection should be sufficient to
verify that the BUG_ON() is not required, and by now we should be happy
that we didn't miss anything.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 2/5] drm/i915: not finding a fence is a non-recoverable condition

[Daniel Vetter]

This happens in two cases:
- userspace got its fence accounting wrong or
- the kernel got its fence accounting wrong.

In both cases there's absolutely no point in calling evict_everything,
that will not magically bring back the missing fence. So return a
different (hopefully somewhat sensible) error code.

This has the added benefit that out-of-gtt can be distinguish from
broken fence accounting by simply looking at the ioctl return code.

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

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 1ef0b91..5c900d3 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2735,7 +2735,7 @@ i915_gem_object_get_fence(struct drm_i915_gem_object *obj,
 
 	reg = i915_find_fence_reg(dev, pipelined);
 	if (reg == NULL)
-		return -ENOSPC;
+		return -EDEADLK;
 
 	if ((old = reg->obj)) {
 		drm_gem_object_reference(&old->base);
-- 
1.7.4.1

[PATCH 3/5] drm/i915: check gpu_write_list in move_to_flushing

[Daniel Vetter]

The corresponding WARN_ON with opposite sign already exists in
move_to_inactive, hence add it here for symmetry.

I've actually hit this while hunting down bugs in various
pipelined fencing patches.

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

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 5c900d3..f658f4f 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1749,6 +1749,7 @@ i915_gem_object_move_to_flushing(struct drm_i915_gem_object *obj)
 	drm_i915_private_t *dev_priv = dev->dev_private;
 
 	WARN_ON(!obj->active);
+	WARN_ON(list_empty(&obj->gpu_write_list));
 	list_move_tail(&obj->mm_list, &dev_priv->mm.flushing_list);
 
 	i915_gem_object_move_off_active(obj);
-- 
1.7.4.1

[PATCH 4/5] drm/i915: fix relaxed tiling on gen2: y-tiling on i855gm

[Daniel Vetter]

Experiments showed that y-tiled access from the cpu doesn't work on my gen2
machine.

Checking this in create_mmap_offset does not work due to libdrm bo reuse.

Chris Wilson also clarified (by checking internal docs) that only i855GM has
broken y-tiled fences for cpu access (guess what hw I own). Hence move the check
to deny y-tiled access to gem_fault and restrict it with IS_I85X. According to
docs, upload _should_ work to y-tiled textures with the blitter on all gen2
chips.

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

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index f658f4f..6471d51 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1255,6 +1255,13 @@ int i915_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
 		goto unlock;
 	}
 
+	/* i855gm has broken y-tiled fences for cpu access, blitter should work,
+	 * though. */
+	if (IS_I85X(dev) && obj->tiling_mode == I915_TILING_Y) {
+		ret = -EINVAL;
+		goto unlock;
+	}
+
 	/* Now bind it into the GTT if needed */
 	if (!obj->map_and_fenceable) {
 		ret = i915_gem_object_unbind(obj);
-- 
1.7.4.1

[PATCH 5/5] drm/i915: fix relaxed tiling on gen2: tile height

[Daniel Vetter]

A tile on gen2 has a size of 2kb, stride of 128 bytes and 16 rows.

Userspace was broken and assumed 8 rows. Chris Wilson noted that the
kernel unfortunately can't reliable check that because libdrm rounds
up the size to the next bucket.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 6471d51..262fa45 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1504,8 +1504,9 @@ i915_gem_get_unfenced_gtt_alignment(struct drm_i915_gem_object *obj)
 	 * edge of an even tile row (where tile rows are counted as if the bo is
 	 * placed in a fenced gtt region).
 	 */
-	if (IS_GEN2(dev) ||
-	    (obj->tiling_mode == I915_TILING_Y && HAS_128_BYTE_Y_TILING(dev)))
+	if (IS_GEN2(dev))
+		tile_height = 16;
+	else if (obj->tiling_mode == I915_TILING_Y && HAS_128_BYTE_Y_TILING(dev))
 		tile_height = 32;
 	else
 		tile_height = 8;
-- 
1.7.4.1

Re: [PATCH 0/5] various small patches for -next

[Chris Wilson]

On Tue, 19 Apr 2011 22:45:59 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> The first 3 are stuff that I've found useful while hunting down gem_stress
> fallout.
> 
> The latter 2 wrap up the gen2 tiling on the kernel side.
> 
> Please review and consider merging for -next.

I've picked up the last 4 patches. The first is good, but not good
enough. ;-)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre