| Message ID | 1305235044-9159-13-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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.
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 --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;