All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH fixes] drm/i915: Fix unfenced alignment on pre-G33 hardware
@ 2011-06-06 14:18 Chris Wilson
  2011-06-06 16:03 ` Keith Packard
  0 siblings, 1 reply; 5+ messages in thread
From: Chris Wilson @ 2011-06-06 14:18 UTC (permalink / raw)
  To: intel-gfx; +Cc: stable

Align unfenced buffers on older hardware to the power-of-two object size.
The docs suggest that it should be possible to align only to a power-of-two
tile height, but using the already computed fence size is easier. We
also have to make sure that we unbind misaligned buffers upon tiling
changes.

Reported-and-tested-by: Sitosfe Wheeler <sitsofe@yahoo.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=36326
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: stable@kernel.org
---
 drivers/gpu/drm/i915/i915_dma.c        |    2 +-
 drivers/gpu/drm/i915/i915_drv.h        |    3 ++-
 drivers/gpu/drm/i915/i915_gem.c        |   30 ++++++++++++++----------------
 drivers/gpu/drm/i915/i915_gem_tiling.c |    3 ++-
 4 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 403e69f..1a5bd0f 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -771,7 +771,7 @@ static int i915_getparam(struct drm_device *dev, void *data,
 		value = HAS_BLT(dev);
 		break;
 	case I915_PARAM_HAS_RELAXED_FENCING:
-		value = 1;
+		value = 2;
 		break;
 	case I915_PARAM_HAS_COHERENT_RINGS:
 		value = 1;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b492779..a030e38 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1188,7 +1188,8 @@ void i915_gem_free_all_phys_object(struct drm_device *dev);
 void i915_gem_release(struct drm_device *dev, struct drm_file *file);
 
 uint32_t
-i915_gem_get_unfenced_gtt_alignment(struct drm_i915_gem_object *obj);
+i915_gem_get_unfenced_gtt_alignment(struct drm_i915_gem_object *obj,
+				    int tiling_mode);
 
 /* i915_gem_gtt.c */
 void i915_gem_restore_gtt_mappings(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 7ce3f35..896aac7 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1430,35 +1430,32 @@ i915_gem_get_gtt_alignment(struct drm_i915_gem_object *obj)
  * i915_gem_get_unfenced_gtt_alignment - return required GTT alignment for an
  *					 unfenced object
  * @obj: object to check
+ * @tiling_mode: tiling mode of the object
  *
  * Return the required GTT alignment for an object, only taking into account
  * unfenced tiled surface requirements.
  */
 uint32_t
-i915_gem_get_unfenced_gtt_alignment(struct drm_i915_gem_object *obj)
+i915_gem_get_unfenced_gtt_alignment(struct drm_i915_gem_object *obj,
+				    int tiling_mode)
 {
 	struct drm_device *dev = obj->base.dev;
-	int tile_height;
+
+	if (tiling_mode == I915_TILING_NONE)
+		return 4096;
 
 	/*
 	 * Minimum alignment is 4k (GTT page size) for sane hw.
 	 */
-	if (INTEL_INFO(dev)->gen >= 4 || IS_G33(dev) ||
-	    obj->tiling_mode == I915_TILING_NONE)
+	dev = obj->base.dev;
+	if (INTEL_INFO(dev)->gen >= 4 || IS_G33(dev))
 		return 4096;
 
-	/*
-	 * Older chips need unfenced tiled buffers to be aligned to the left
-	 * edge of an even tile row (where tile rows are counted as if the bo is
-	 * placed in a fenced gtt region).
+	/* Previous hardware however needs to be aligned to a power-of-two
+	 * tile height. The simplest method for determining this is to reuse
+	 * the power-of-tile object size.
 	 */
-	if (IS_GEN2(dev) ||
-	    (obj->tiling_mode == I915_TILING_Y && HAS_128_BYTE_Y_TILING(dev)))
-		tile_height = 32;
-	else
-		tile_height = 8;
-
-	return tile_height * obj->stride * 2;
+	return i915_gem_get_gtt_size(obj);
 }
 
 int
@@ -2752,7 +2749,8 @@ i915_gem_object_bind_to_gtt(struct drm_i915_gem_object *obj,
 
 	fence_size = i915_gem_get_gtt_size(obj);
 	fence_alignment = i915_gem_get_gtt_alignment(obj);
-	unfenced_alignment = i915_gem_get_unfenced_gtt_alignment(obj);
+	unfenced_alignment =
+		i915_gem_get_unfenced_gtt_alignment(obj, obj->tiling_mode);
 
 	if (alignment == 0)
 		alignment = map_and_fenceable ? fence_alignment :
diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c
index 281ad3d..c775f03 100644
--- a/drivers/gpu/drm/i915/i915_gem_tiling.c
+++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
@@ -348,7 +348,8 @@ i915_gem_set_tiling(struct drm_device *dev, void *data,
 		/* Rebind if we need a change of alignment */
 		if (!obj->map_and_fenceable) {
 			u32 unfenced_alignment =
-				i915_gem_get_unfenced_gtt_alignment(obj);
+				i915_gem_get_unfenced_gtt_alignment(obj,
+								    args->tiling_mode);
 			if (obj->gtt_offset & (unfenced_alignment - 1))
 				ret = i915_gem_object_unbind(obj);
 		}
-- 
1.7.5.3

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH fixes] drm/i915: Fix unfenced alignment on pre-G33 hardware
  2011-06-06 14:18 [PATCH fixes] drm/i915: Fix unfenced alignment on pre-G33 hardware Chris Wilson
@ 2011-06-06 16:03 ` Keith Packard
  2011-06-06 17:50   ` [Intel-gfx] " Chris Wilson
  0 siblings, 1 reply; 5+ messages in thread
From: Keith Packard @ 2011-06-06 16:03 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx; +Cc: stable


[-- Attachment #1.1: Type: text/plain, Size: 901 bytes --]

On Mon,  6 Jun 2011 15:18:44 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

>  	case I915_PARAM_HAS_RELAXED_FENCING:
> -		value = 1;
> +		value = 2;

This looks like a change in ABI to me. I think this means you want a new
ioctl so that applications using the existing HAS_RELAXED_FENCING ioctl
and expecting a boolean will continue to work.

>  uint32_t
> -i915_gem_get_unfenced_gtt_alignment(struct drm_i915_gem_object *obj);
> +i915_gem_get_unfenced_gtt_alignment(struct drm_i915_gem_object *obj,
> +				    int tiling_mode);
...
> -				i915_gem_get_unfenced_gtt_alignment(obj);
> +				i915_gem_get_unfenced_gtt_alignment(obj,
> +								    args->tiling_mode);

This combination looks like a simple bug fix -- not using the new tiling
mode when computing the required alignment. Can you separate out this
From the power-of-two change?

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Intel-gfx] [PATCH fixes] drm/i915: Fix unfenced alignment on pre-G33 hardware
  2011-06-06 16:03 ` Keith Packard
@ 2011-06-06 17:50   ` Chris Wilson
  2011-06-06 18:09     ` Keith Packard
  0 siblings, 1 reply; 5+ messages in thread
From: Chris Wilson @ 2011-06-06 17:50 UTC (permalink / raw)
  To: Keith Packard, intel-gfx; +Cc: stable

On Mon, 06 Jun 2011 09:03:30 -0700, Keith Packard <keithp@keithp.com> wrote:
> On Mon,  6 Jun 2011 15:18:44 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> >  	case I915_PARAM_HAS_RELAXED_FENCING:
> > -		value = 1;
> > +		value = 2;
> 
> This looks like a change in ABI to me. I think this means you want a new
> ioctl so that applications using the existing HAS_RELAXED_FENCING ioctl
> and expecting a boolean will continue to work.

Hah. Anyway it is actually irrelevant as it turns out, the kernel is broken
with any per-surface tiling on gen2/gen3.

> >  uint32_t
> > -i915_gem_get_unfenced_gtt_alignment(struct drm_i915_gem_object *obj);
> > +i915_gem_get_unfenced_gtt_alignment(struct drm_i915_gem_object *obj,
> > +				    int tiling_mode);
> ...
> > -				i915_gem_get_unfenced_gtt_alignment(obj);
> > +				i915_gem_get_unfenced_gtt_alignment(obj,
> > +								    args->tiling_mode);
> 
> This combination looks like a simple bug fix -- not using the new tiling
> mode when computing the required alignment. Can you separate out this
> From the power-of-two change?

Yes, I can. But the simple bug fix doesn't fix anything without the other
chunk. Do you still want it split?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

_______________________________________________
stable mailing list
stable@linux.kernel.org
http://linux.kernel.org/mailman/listinfo/stable

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH fixes] drm/i915: Fix unfenced alignment on pre-G33 hardware
  2011-06-06 17:50   ` [Intel-gfx] " Chris Wilson
@ 2011-06-06 18:09     ` Keith Packard
  2011-06-06 20:56       ` Chris Wilson
  0 siblings, 1 reply; 5+ messages in thread
From: Keith Packard @ 2011-06-06 18:09 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx; +Cc: stable


[-- Attachment #1.1: Type: text/plain, Size: 600 bytes --]

On Mon, 06 Jun 2011 18:50:16 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> Hah. Anyway it is actually irrelevant as it turns out, the kernel is broken
> with any per-surface tiling on gen2/gen3.

Right, seems like we need to signal user space that tiling works now,
which should involve a new ioctl of some form?

> Yes, I can. But the simple bug fix doesn't fix anything without the other
> chunk. Do you still want it split?

Isn't it a general bug? We're asking for an alignment value using the
wrong tiling mode when changing tiling modes.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH fixes] drm/i915: Fix unfenced alignment on pre-G33 hardware
  2011-06-06 18:09     ` Keith Packard
@ 2011-06-06 20:56       ` Chris Wilson
  0 siblings, 0 replies; 5+ messages in thread
From: Chris Wilson @ 2011-06-06 20:56 UTC (permalink / raw)
  To: Keith Packard, intel-gfx; +Cc: stable

On Mon, 06 Jun 2011 11:09:30 -0700, Keith Packard <keithp@keithp.com> wrote:
> On Mon, 06 Jun 2011 18:50:16 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > Hah. Anyway it is actually irrelevant as it turns out, the kernel is broken
> > with any per-surface tiling on gen2/gen3.
> 
> Right, seems like we need to signal user space that tiling works now,
> which should involve a new ioctl of some form?

So PARAM_HAS_PER_SURFACE_TILING and re-enable fencing for render
operations on gen2/3.

> > Yes, I can. But the simple bug fix doesn't fix anything without the other
> > chunk. Do you still want it split?
> 
> Isn't it a general bug? We're asking for an alignment value using the
> wrong tiling mode when changing tiling modes.

Indeed. I just can't test the patch independently since the hangs look
identical.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2011-06-06 20:56 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-06-06 14:18 [PATCH fixes] drm/i915: Fix unfenced alignment on pre-G33 hardware Chris Wilson
2011-06-06 16:03 ` Keith Packard
2011-06-06 17:50   ` [Intel-gfx] " Chris Wilson
2011-06-06 18:09     ` Keith Packard
2011-06-06 20:56       ` Chris Wilson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.