Re: [PATCH] drm/i915: Only fence tiled region of object.

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Dec 18, 2014 at 11:04:11PM +0200, Ville Syrjälä wrote:
> On Thu, Dec 18, 2014 at 09:37:37PM +0100, Daniel Vetter wrote:
> > On Thu, Dec 18, 2014 at 09:51:26AM -0800, Bob Paauwe wrote:
> > > When creating a fence for a tiled object, only fence the area that
> > > makes up the actual tiles.  The object may be larger than the tiled
> > > area and if we allow those extra addresses to be fenced, they'll
> > > get converted to addresses beyond where the object is mapped. This
> > > opens up the possiblity of writes beyond the end of object.
> > > 
> > > To prevent this, we adjust the size of the fence to only encompass
> > > the area that makes up the actual tiles.  The extra space is considered
> > > un-tiled and now behaves as if it was a linear object.
> > > 
> > > Testcase: igt/gem_tiled_fence_overflow
> > > Reported-by: Dan Hettena <danh@ghs.com>
> > > Signed-off-by: Bob Paauwe <bob.j.paauwe@intel.com>
> > 
> > Presuming this indeed blows up (I didn't try your test) this is one for
> > Jani.
> 
> Hmm. Wasn't this problem discussed a few years ago already? My recollection
> is that Imre had patches but you said you don't care about the problem.

Imre had patches iirc to resize the allocation , which would have caused
major havoc with moving stuff around I think. Or maybe it reject
set_tiling if it didn't match (which breaks abi). I was always working
under the assumption that the fence never direct access out of it's
assigned range, just that it corrupts them. 

But that's obviously wrong, so we need to fix this.
-Daniel

> 
> > 
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: stable@vger.kernel.org
> > 
> > Thanks, Daniel
> > 
> > > ---
> > >  drivers/gpu/drm/i915/i915_gem.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > > index 67550ac..c9acbfa 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > @@ -3173,6 +3173,13 @@ static void i965_write_fence_reg(struct drm_device *dev, int reg,
> > >  		u32 size = i915_gem_obj_ggtt_size(obj);
> > >  		uint64_t val;
> > >  
> > > +		/* Adjust fence size to match tiled area */
> > > +		if (obj->tiling_mode != I915_TILING_NONE) {
> > > +			uint32_t row_size = obj->stride *
> > > +				(obj->tiling_mode == I915_TILING_Y ? 32 : 8);
> > > +			size = (size / row_size) * row_size;
> > > +		}
> > > +
> > >  		val = (uint64_t)((i915_gem_obj_ggtt_offset(obj) + size - 4096) &
> > >  				 0xfffff000) << 32;
> > >  		val |= i915_gem_obj_ggtt_offset(obj) & 0xfffff000;
> > > -- 
> > > 2.1.0
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx