[Intel-gfx] [PATCH 5/5] drm/i915: fix gtt space allocated for tiled objects

[Imre Deak]

On Fri, 2013-01-04 at 20:32 +0000, Chris Wilson wrote:
> 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.

Ok, I checked now the result is somewhat similar with 2.20.17.

> 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.

Do you mean rounding the size down to the closest tile row address (and
program that to the fence reg)? I had such a version, but I thought it
got too complicated due to the old HW's power-of-two alignment
requirement. Here we would still need to over allocate if the
power-of-two size happens to be non tile-row size aligned.. But I guess
it's still doable if we want to save some gtt space.

> 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.

Ah right, atm only an incorrect alignment will cause an unbind, but we
would also need to check the new gtt size if we chose to over allocate.

--Imre