* [PATCH] drm/i915: Only fence tiled region of object. @ 2014-12-18 17:51 Bob Paauwe 2014-12-18 20:37 ` Daniel Vetter 0 siblings, 1 reply; 13+ messages in thread From: Bob Paauwe @ 2014-12-18 17:51 UTC (permalink / raw) To: intel-gfx; +Cc: danh 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> --- 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/i915: Only fence tiled region of object. 2014-12-18 17:51 [PATCH] drm/i915: Only fence tiled region of object Bob Paauwe @ 2014-12-18 20:37 ` Daniel Vetter 2014-12-18 21:04 ` Ville Syrjälä 2015-01-21 17:09 ` Jani Nikula 0 siblings, 2 replies; 13+ messages in thread From: Daniel Vetter @ 2014-12-18 20:37 UTC (permalink / raw) To: Bob Paauwe; +Cc: intel-gfx, danh 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. 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/i915: Only fence tiled region of object. 2014-12-18 20:37 ` Daniel Vetter @ 2014-12-18 21:04 ` Ville Syrjälä 2014-12-18 21:19 ` Daniel Vetter 2014-12-18 21:23 ` Imre Deak 2015-01-21 17:09 ` Jani Nikula 1 sibling, 2 replies; 13+ messages in thread From: Ville Syrjälä @ 2014-12-18 21:04 UTC (permalink / raw) To: Daniel Vetter; +Cc: danh, intel-gfx 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. > > 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/i915: Only fence tiled region of object. 2014-12-18 21:04 ` Ville Syrjälä @ 2014-12-18 21:19 ` Daniel Vetter 2014-12-18 22:14 ` Imre Deak 2014-12-18 21:23 ` Imre Deak 1 sibling, 1 reply; 13+ messages in thread From: Daniel Vetter @ 2014-12-18 21:19 UTC (permalink / raw) To: Ville Syrjälä; +Cc: danh, intel-gfx 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/i915: Only fence tiled region of object. 2014-12-18 21:19 ` Daniel Vetter @ 2014-12-18 22:14 ` Imre Deak 2014-12-19 8:26 ` Chris Wilson 0 siblings, 1 reply; 13+ messages in thread From: Imre Deak @ 2014-12-18 22:14 UTC (permalink / raw) To: Daniel Vetter; +Cc: danh, intel-gfx On Thu, 2014-12-18 at 22:19 +0100, Daniel Vetter wrote: > 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. It did that only for old GENs where the POT fence size constraint gives you no other choice to fix this issue. On new HW I also rounded down the fence size. --Imre > 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 > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/i915: Only fence tiled region of object. 2014-12-18 22:14 ` Imre Deak @ 2014-12-19 8:26 ` Chris Wilson 2014-12-19 9:05 ` Imre Deak 0 siblings, 1 reply; 13+ messages in thread From: Chris Wilson @ 2014-12-19 8:26 UTC (permalink / raw) To: Imre Deak; +Cc: danh, intel-gfx On Fri, Dec 19, 2014 at 12:14:00AM +0200, Imre Deak wrote: > On Thu, 2014-12-18 at 22:19 +0100, Daniel Vetter wrote: > > 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. > > It did that only for old GENs where the POT fence size constraint gives > you no other choice to fix this issue. On new HW I also rounded down the > fence size. If we were to be consistent, then we would pad in the GTT so that no other object fitted in the full fenced region. -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/i915: Only fence tiled region of object. 2014-12-19 8:26 ` Chris Wilson @ 2014-12-19 9:05 ` Imre Deak 2014-12-19 9:17 ` Chris Wilson 0 siblings, 1 reply; 13+ messages in thread From: Imre Deak @ 2014-12-19 9:05 UTC (permalink / raw) To: Chris Wilson; +Cc: danh, intel-gfx On Fri, 2014-12-19 at 08:26 +0000, Chris Wilson wrote: > On Fri, Dec 19, 2014 at 12:14:00AM +0200, Imre Deak wrote: > > On Thu, 2014-12-18 at 22:19 +0100, Daniel Vetter wrote: > > > 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. > > > > It did that only for old GENs where the POT fence size constraint gives > > you no other choice to fix this issue. On new HW I also rounded down the > > fence size. > > If we were to be consistent, then we would pad in the GTT so that no > other object fitted in the full fenced region. Yes, I did that. In v2 I changed this (based on your feedback) so the padding happens only on old GENs with the POT constraint, since on new GENs we can instead reduce the fence region size. The end of the buffer couldn't contain even a single whole pixel line, so would display incorrectly anyway. Btw, why are we not doing the fixup on old GENs too in this patch? --Imre _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/i915: Only fence tiled region of object. 2014-12-19 9:05 ` Imre Deak @ 2014-12-19 9:17 ` Chris Wilson 2014-12-19 10:15 ` Daniel Vetter 0 siblings, 1 reply; 13+ messages in thread From: Chris Wilson @ 2014-12-19 9:17 UTC (permalink / raw) To: Imre Deak; +Cc: danh, intel-gfx On Fri, Dec 19, 2014 at 11:05:36AM +0200, Imre Deak wrote: > On Fri, 2014-12-19 at 08:26 +0000, Chris Wilson wrote: > > On Fri, Dec 19, 2014 at 12:14:00AM +0200, Imre Deak wrote: > > > On Thu, 2014-12-18 at 22:19 +0100, Daniel Vetter wrote: > > > > 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. > > > > > > It did that only for old GENs where the POT fence size constraint gives > > > you no other choice to fix this issue. On new HW I also rounded down the > > > fence size. > > > > If we were to be consistent, then we would pad in the GTT so that no > > other object fitted in the full fenced region. > > Yes, I did that. In v2 I changed this (based on your feedback) so the > padding happens only on old GENs with the POT constraint, since on new > GENs we can instead reduce the fence region size. The end of the buffer > couldn't contain even a single whole pixel line, so would display > incorrectly anyway. Maybe they allocated one very large object for a mipmap, each level of varying pitches but uploading through a single fence with a single pitch, and so carefully wrote the trailing levels to account for the different tile row size (but it knew the individual pages would be correct). Technically reducing the fenced region size is an ABI change... :p -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/i915: Only fence tiled region of object. 2014-12-19 9:17 ` Chris Wilson @ 2014-12-19 10:15 ` Daniel Vetter 2014-12-19 12:31 ` Dan Hettena 2014-12-19 13:17 ` Chris Wilson 0 siblings, 2 replies; 13+ messages in thread From: Daniel Vetter @ 2014-12-19 10:15 UTC (permalink / raw) To: Chris Wilson, Imre Deak, Daniel Vetter, danh, intel-gfx On Fri, Dec 19, 2014 at 09:17:40AM +0000, Chris Wilson wrote: > On Fri, Dec 19, 2014 at 11:05:36AM +0200, Imre Deak wrote: > > On Fri, 2014-12-19 at 08:26 +0000, Chris Wilson wrote: > > > On Fri, Dec 19, 2014 at 12:14:00AM +0200, Imre Deak wrote: > > > > On Thu, 2014-12-18 at 22:19 +0100, Daniel Vetter wrote: > > > > > 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. > > > > > > > > It did that only for old GENs where the POT fence size constraint gives > > > > you no other choice to fix this issue. On new HW I also rounded down the > > > > fence size. > > > > > > If we were to be consistent, then we would pad in the GTT so that no > > > other object fitted in the full fenced region. > > > > Yes, I did that. In v2 I changed this (based on your feedback) so the > > padding happens only on old GENs with the POT constraint, since on new > > GENs we can instead reduce the fence region size. The end of the buffer > > couldn't contain even a single whole pixel line, so would display > > incorrectly anyway. > > Maybe they allocated one very large object for a mipmap, each level of > varying pitches but uploading through a single fence with a single > pitch, and so carefully wrote the trailing levels to account for the > different tile row size (but it knew the individual pages would be > correct). > > Technically reducing the fenced region size is an ABI change... :p Well the last incomplete tile-row couldn't ever really be used anyway since writes just go somewhere. So I don't think this is a user-observable ABI change. And it's simpler than enlarging the tiled gtt size on gen4+, which might result in some more gtt thrashing. So I only want to do that if we really need it. -Daniel -- 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/i915: Only fence tiled region of object. 2014-12-19 10:15 ` Daniel Vetter @ 2014-12-19 12:31 ` Dan Hettena 2014-12-19 13:17 ` Chris Wilson 1 sibling, 0 replies; 13+ messages in thread From: Dan Hettena @ 2014-12-19 12:31 UTC (permalink / raw) To: Daniel Vetter, Chris Wilson, Imre Deak, intel-gfx > > > > If we were to be consistent, then we would pad in the GTT so that no > > > > other object fitted in the full fenced region. > > > > > > Yes, I did that. In v2 I changed this (based on your feedback) so the > > > padding happens only on old GENs with the POT constraint, since on > new > > > GENs we can instead reduce the fence region size. The end of the buffer > > > couldn't contain even a single whole pixel line, so would display > > > incorrectly anyway. > > > > Maybe they allocated one very large object for a mipmap, each level of > > varying pitches but uploading through a single fence with a single > > pitch, and so carefully wrote the trailing levels to account for the > > different tile row size (but it knew the individual pages would be > > correct). > > > > Technically reducing the fenced region size is an ABI change... :p > > Well the last incomplete tile-row couldn't ever really be used anyway > since writes just go somewhere. So I don't think this is a user-observable > ABI change. And it's simpler than enlarging the tiled gtt size on gen4+, > which might result in some more gtt thrashing. So I only want to do that > if we really need it. That was my thinking. Also, I tracked this particular issue down by adding padding and adding assertions that the scratch page was still all zeroes. If it's truly the case that the scratch page is only clobbered on error, I think it would be best to keep it that way for future debugging. That's the reasoning behind shrinking the fenced region as opposed to just working around the problem with padding. Dan _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/i915: Only fence tiled region of object. 2014-12-19 10:15 ` Daniel Vetter 2014-12-19 12:31 ` Dan Hettena @ 2014-12-19 13:17 ` Chris Wilson 1 sibling, 0 replies; 13+ messages in thread From: Chris Wilson @ 2014-12-19 13:17 UTC (permalink / raw) To: Daniel Vetter; +Cc: intel-gfx On Fri, Dec 19, 2014 at 11:15:59AM +0100, Daniel Vetter wrote: > On Fri, Dec 19, 2014 at 09:17:40AM +0000, Chris Wilson wrote: > > On Fri, Dec 19, 2014 at 11:05:36AM +0200, Imre Deak wrote: > > > On Fri, 2014-12-19 at 08:26 +0000, Chris Wilson wrote: > > > > On Fri, Dec 19, 2014 at 12:14:00AM +0200, Imre Deak wrote: > > > > > On Thu, 2014-12-18 at 22:19 +0100, Daniel Vetter wrote: > > > > > > 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. > > > > > > > > > > It did that only for old GENs where the POT fence size constraint gives > > > > > you no other choice to fix this issue. On new HW I also rounded down the > > > > > fence size. > > > > > > > > If we were to be consistent, then we would pad in the GTT so that no > > > > other object fitted in the full fenced region. > > > > > > Yes, I did that. In v2 I changed this (based on your feedback) so the > > > padding happens only on old GENs with the POT constraint, since on new > > > GENs we can instead reduce the fence region size. The end of the buffer > > > couldn't contain even a single whole pixel line, so would display > > > incorrectly anyway. > > > > Maybe they allocated one very large object for a mipmap, each level of > > varying pitches but uploading through a single fence with a single > > pitch, and so carefully wrote the trailing levels to account for the > > different tile row size (but it knew the individual pages would be > > correct). > > > > Technically reducing the fenced region size is an ABI change... :p > > Well the last incomplete tile-row couldn't ever really be used anyway > since writes just go somewhere. They go to a fully deterministic page, so it could be used by a masochistic user. > So I don't think this is a user-observable > ABI change. And it's simpler than enlarging the tiled gtt size on gen4+, > which might result in some more gtt thrashing. So I only want to do that > if we really need it. I would have accepted that not even libva has ever done anything like this, and so the only known use has been in buggy code. -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/i915: Only fence tiled region of object. 2014-12-18 21:04 ` Ville Syrjälä 2014-12-18 21:19 ` Daniel Vetter @ 2014-12-18 21:23 ` Imre Deak 1 sibling, 0 replies; 13+ messages in thread From: Imre Deak @ 2014-12-18 21:23 UTC (permalink / raw) To: Ville Syrjälä; +Cc: danh, intel-gfx On Thu, 2014-12-18 at 23:04 +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. For reference: http://lists.freedesktop.org/archives/intel-gfx/2013-January/023385.html > > > > > 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 > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/i915: Only fence tiled region of object. 2014-12-18 20:37 ` Daniel Vetter 2014-12-18 21:04 ` Ville Syrjälä @ 2015-01-21 17:09 ` Jani Nikula 1 sibling, 0 replies; 13+ messages in thread From: Jani Nikula @ 2015-01-21 17:09 UTC (permalink / raw) To: Daniel Vetter, Bob Paauwe; +Cc: intel-gfx On Thu, 18 Dec 2014, Daniel Vetter <daniel@ffwll.ch> 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. > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > Cc: stable@vger.kernel.org Pushed to drm-intel-fixes, thanks for the patch and review. BR, Jani. > > 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 -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2015-01-21 17:20 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-12-18 17:51 [PATCH] drm/i915: Only fence tiled region of object Bob Paauwe 2014-12-18 20:37 ` Daniel Vetter 2014-12-18 21:04 ` Ville Syrjälä 2014-12-18 21:19 ` Daniel Vetter 2014-12-18 22:14 ` Imre Deak 2014-12-19 8:26 ` Chris Wilson 2014-12-19 9:05 ` Imre Deak 2014-12-19 9:17 ` Chris Wilson 2014-12-19 10:15 ` Daniel Vetter 2014-12-19 12:31 ` Dan Hettena 2014-12-19 13:17 ` Chris Wilson 2014-12-18 21:23 ` Imre Deak 2015-01-21 17:09 ` Jani Nikula
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.