On Fri, 04 Jan 2013 21:18:53 +0200, Imre Deak <imre.deak at intel.com> wrote:
> On Fri, 2013-01-04 at 17:47 +0000, Chris Wilson wrote:
> > Meep. A long time ago we got the calcuations wrong (slightly less for
> > gen2), but it was and still is a userspace bug with the potential of
> > handing the GPU.
> >
> > A multiple of tile_height tall and a mutiple of tile_width across should
> > always be an exact number of pages (and an exact multiple of tile-row
> > pages). And we should have been obeying that since the introduction of
> > set-tiling (ignoring the aforementioned bugs) - so I'd really like to
> > see any evidence of userspace getting that wrong.
>
> On Ubuntu 12.10, with xf86-video-intel 2.20.9 I get the attached log
> with the following patch applied:
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c
> b/drivers/gpu/drm/i915/i915_gem_tiling.c
> index 7b0a3b3..846e96f 100644
> --- a/drivers/gpu/drm/i915/i915_gem_tiling.c
> +++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
> @@ -236,9 +236,17 @@ i915_tiling_ok(struct drm_device *dev, int stride,
> int size, int tiling_mode)
> if (INTEL_INFO(dev)->gen >= 4) {
> if (stride & (tile_width - 1))
> return false;
> - return true;
> }
>
> + if (size % (stride / tile_width * PAGE_SIZE))
> + printk("unaligned tiling: comm %.*s size %u mode %c stride %d
> size-mod-tilerow-size %zd\n",
> + (int)sizeof(current->comm), current->comm,
> + size, tiling_mode == I915_TILING_X ? 'X' : 'Y',
> + stride, size % (stride / tile_width * PAGE_SIZE));
> +
> + if (INTEL_INFO(dev)->gen >= 4)
> + return true;
> +
> /* Pre-965 needs power of two tile widths */
> if (stride < tile_width)
> return false;
>
>
> dmesg | grep unaligned | cut -c 16- | sort | uniq -c
> 2 unaligned tiling: comm compiz size 10485760 mode X stride 6656 size-mod-tilerow-size 49152
> 2 unaligned tiling: comm compiz size 10485760 mode Y stride 6400 size-mod-tilerow-size 40960
> 1 unaligned tiling: comm compiz size 1572864 mode Y stride 2944 size-mod-tilerow-size 65536
Hmm, I also forgot to mention we then pluck a bo out of cache returning
a potentially larger than required size. However, looks like I need to
double check userspace to make sure we are overallocating tile rows.
So we have a choice of overallocating the GTT for fenced regions, or
rounding the fence region to a tile row which preserves the existing
functional userspace behaviour. The right answer is indeed to modify the
behaviour to overallocate physical space so the fence region doesn't
overlap another bo. Next up you need to review what happens after a
change of tiling and whether we rebind before the next fenced GTT
access.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
