diff mbox

[12/16] drm/915: fix relaxed tiling on gen2: tile height

Message ID 1305235044-9159-13-git-send-email-chris@chris-wilson.co.uk
State New, archived
Headers show

Commit Message

Chris Wilson May 12, 2011, 9:17 p.m. UTC
From: Daniel Vetter <daniel.vetter@ffwll.ch>

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>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

Comments

Keith Packard May 13, 2011, 1:13 a.m. UTC | #1
On Thu, 12 May 2011 22:17:20 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> From: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> 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.

Please explain (in the commit message) the impact on both new and old
user space. I remember (vaguely) that this patch will cause old user
space to have issues.

If so, we'll need a way to detect new user space and provide correct
behaviour there without impacting old user space.
Daniel Vetter May 15, 2011, 8:43 p.m. UTC | #2
On Thu, May 12, 2011 at 06:13:32PM -0700, Keith Packard wrote:
> On Thu, 12 May 2011 22:17:20 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > From: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > 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.
> 
> Please explain (in the commit message) the impact on both new and old
> user space. I remember (vaguely) that this patch will cause old user
> space to have issues.

No problem for old userspace. This only changes the number of rows from 32
to 16. This value is used in the kernel to align buffers correctly, i.e.
it will save perhaps a tiny bit of gtt. Userspace on the other hand
assumed only 8 rows, which lead to underallocating it the last tile row.

So this patch is more documentation of actual hw behaviour than anything
else - hopefully getting rid of gen2 tiling confusion once and for all.
-Daniel
Keith Packard May 15, 2011, 9:58 p.m. UTC | #3
On Sun, 15 May 2011 22:43:41 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:

> No problem for old userspace. This only changes the number of rows from 32
> to 16. This value is used in the kernel to align buffers correctly, i.e.
> it will save perhaps a tiny bit of gtt. Userspace on the other hand
> assumed only 8 rows, which lead to underallocating it the last tile
> row.

I have this vague memory of some problem in the past with tiling and old
user space for Gen2 hardware. I assume that old user space will just do
bad things, but that there's nothing the kernel can do to fix it, right?

The requirement here is that kernel changes not break user space, even
if the kernel was just masking a user-space bug. If that isn't true for
any old user space version, then this should be fine.

> So this patch is more documentation of actual hw behaviour than anything
> else - hopefully getting rid of gen2 tiling confusion once and for all.

Cool. Thanks for the clarification.
Daniel Vetter May 16, 2011, 5:48 p.m. UTC | #4
On Sun, May 15, 2011 at 02:58:26PM -0700, Keith Packard wrote:
> I have this vague memory of some problem in the past with tiling and old
> user space for Gen2 hardware. I assume that old user space will just do
> bad things, but that there's nothing the kernel can do to fix it, right?

Ok, I'll try to fill you in with a quick summary on this - after all,
we've not only had just one issue with tiling, so it's rather easy to get
lost ;-) Very short answer is "yes".

The tiling height bug (aligning to 8 rows instead of 16) in userspace has
been around forever (as far as I bothered to dig into history, at least).
But that never really showed up because we were rounding up to the next
fence size, which is a pot and at least .5mb (on gen2). So ceil(rows/8)
was never odd, which is the only case this bug manifests. This also wastes
tons of memory, so Chris Wilson developed the relaxed fencing scheme to
avoid allocating unneeded backing storage (and only reserving the full gtt
space if there's a fence attached to the object). Only with support for
relaxed fencing both in the kernel and userspace portion of the driver
could we actually hit the bug. Matters were made worse by our rather large
impedance mismatch between userspace and kernel releases: Distros have
been shipping broken userspace in their stable releases before the kernel
part has hit Linus-mainline (which is when people started to complain, at
around .38-rc6, iirc).

We've tried to detect such underallocated last tile rows in the kernel and
reject such tiling attempts. But libdrm reuses buffers as long as they're
big enough, so the last tile row might intentionally not be complete, but
also never used. We can't tell these two cases apart in the kernel, which
is why we've had to back out that change again and just absorb the
resulting flak.

Aside: I think we need to improve our efforts to make the new patches in
-next testable by the community to decrease that turn-around time mismatch
and catch such issues earlier. I'm tossing around ideas, but nothing
concrete yet.

Yours, Daniel
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index dd0cfac..afdbbd9 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1486,8 +1486,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;