Message ID | 1415263235-8886-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Thu, Nov 06, 2014 at 08:40:35AM +0000, Chris Wilson wrote: > As obj->map_and_fenceable computation has changed to only be set when > the object is bound inside the global GTT (and is suitable aligned to a > fence region) we need to accommodate those changes when the tiling is > adjusted. The easiest solution is to unbind from the global GTT if we > are currently fenceable, but will not be after the tiling change. QA failed to supply the bisect for this regression, but most likely this has been introduced due to the change in handling obj->map_and_fenceable in commit e6a844687cf929ec053c7578d5ecc794a8a6c5cf Author: Chris Wilson <chris@chris-wilson.co.uk> Date: Mon Aug 11 12:00:12 2014 +0200 drm/i915: Force CPU relocations if not GTT mapped Note that the alignment check is a vestige from our (unsuccessful) attempts to reduce the alignment requirements of tiled but unfenced buffers on gen2/3. That leaves the actual bug of setting map_and_fenceable to true if we're not bound to ggtt, which violates the change introduced in the above patch. Unbinding in that case really looks like the simplest and safest option, we have to do it anyway. If Chris agrees, please add the above analysis to the commit message when merging to -fixes. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=85896 > Tested-by: huax.lu@intel.com Testcase: igt/gem_concurrent_blit > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> With all that this is Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> > --- > drivers/gpu/drm/i915/i915_gem_tiling.c | 19 +++---------------- > 1 file changed, 3 insertions(+), 16 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c > index 749ab485569e..cd7f4734c9f8 100644 > --- a/drivers/gpu/drm/i915/i915_gem_tiling.c > +++ b/drivers/gpu/drm/i915/i915_gem_tiling.c > @@ -375,22 +375,9 @@ i915_gem_set_tiling(struct drm_device *dev, void *data, > * has to also include the unfenced register the GPU uses > * whilst executing a fenced command for an untiled object. > */ > - > - obj->map_and_fenceable = > - !i915_gem_obj_ggtt_bound(obj) || > - (i915_gem_obj_ggtt_offset(obj) + > - obj->base.size <= dev_priv->gtt.mappable_end && > - i915_gem_object_fence_ok(obj, args->tiling_mode)); > - > - /* Rebind if we need a change of alignment */ > - if (!obj->map_and_fenceable) { > - u32 unfenced_align = > - i915_gem_get_gtt_alignment(dev, obj->base.size, > - args->tiling_mode, > - false); > - if (i915_gem_obj_ggtt_offset(obj) & (unfenced_align - 1)) > - ret = i915_gem_object_ggtt_unbind(obj); > - } > + if (obj->map_and_fenceable && > + !i915_gem_object_fence_ok(obj, args->tiling_mode)) > + ret = i915_gem_object_ggtt_unbind(obj); > > if (ret == 0) { > obj->fence_dirty = > -- > 2.1.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Nov 07, 2014 at 11:05:05AM +0100, Daniel Vetter wrote: > On Thu, Nov 06, 2014 at 08:40:35AM +0000, Chris Wilson wrote: > > As obj->map_and_fenceable computation has changed to only be set when > > the object is bound inside the global GTT (and is suitable aligned to a > > fence region) we need to accommodate those changes when the tiling is > > adjusted. The easiest solution is to unbind from the global GTT if we > > are currently fenceable, but will not be after the tiling change. > > QA failed to supply the bisect for this regression, but most likely this > has been introduced due to the change in handling obj->map_and_fenceable > in > > commit e6a844687cf929ec053c7578d5ecc794a8a6c5cf > Author: Chris Wilson <chris@chris-wilson.co.uk> > Date: Mon Aug 11 12:00:12 2014 +0200 > > drm/i915: Force CPU relocations if not GTT mapped I think it also took commit f8fcadba218fe6d23b2e353fea1cf0a4be4c9454 Author: Chris Wilson <chris@chris-wilson.co.uk> Date: Fri Oct 31 13:53:52 2014 +0000 drm/i915: Only mark as map-and-fenceable when bound into the GGTT to expose the bug in testing. > Note that the alignment check is a vestige from our (unsuccessful) > attempts to reduce the alignment requirements of tiled but unfenced > buffers on gen2/3. Also, that was when unbinding from the GTT meant UC writes and clflushing, so we went to great pains to avoid such. > That leaves the actual bug of setting map_and_fenceable to true if we're > not bound to ggtt, which violates the change introduced in the above > patch. Unbinding in that case really looks like the simplest and safest > option, we have to do it anyway. > > If Chris agrees, please add the above analysis to the commit message when > merging to -fixes. > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=85896 > > Tested-by: huax.lu@intel.com > > Testcase: igt/gem_concurrent_blit Testcase: igt/gem_concurrent_blit/gttX* It was also only triggered by recent additions to gem_concurrent_blit (which itself was trying to stress test our fence-vs-GPU serialisation for testing requests - so I can claim it was intentional!). However, it turns out to be easier to hit in practice than in testing. :| -Chris
On Fri, 07 Nov 2014, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Fri, Nov 07, 2014 at 11:05:05AM +0100, Daniel Vetter wrote: >> On Thu, Nov 06, 2014 at 08:40:35AM +0000, Chris Wilson wrote: >> > As obj->map_and_fenceable computation has changed to only be set when >> > the object is bound inside the global GTT (and is suitable aligned to a >> > fence region) we need to accommodate those changes when the tiling is >> > adjusted. The easiest solution is to unbind from the global GTT if we >> > are currently fenceable, but will not be after the tiling change. >> >> QA failed to supply the bisect for this regression, but most likely this >> has been introduced due to the change in handling obj->map_and_fenceable >> in >> >> commit e6a844687cf929ec053c7578d5ecc794a8a6c5cf >> Author: Chris Wilson <chris@chris-wilson.co.uk> >> Date: Mon Aug 11 12:00:12 2014 +0200 >> >> drm/i915: Force CPU relocations if not GTT mapped > > I think it also took > > commit f8fcadba218fe6d23b2e353fea1cf0a4be4c9454 > Author: Chris Wilson <chris@chris-wilson.co.uk> > Date: Fri Oct 31 13:53:52 2014 +0000 > > drm/i915: Only mark as map-and-fenceable when bound into the GGTT > > to expose the bug in testing. Valtteri confirmed this as the bisect result, and provided Tested-by on the fix. Pushed the fix to drm-intel-fixes with the commit message amended per input from Daniel and the bisect result from Valtteri; blame me for any mistakes in the commit message. Thanks for the patch and testing. BR, Jani. > >> Note that the alignment check is a vestige from our (unsuccessful) >> attempts to reduce the alignment requirements of tiled but unfenced >> buffers on gen2/3. > > Also, that was when unbinding from the GTT meant UC writes and clflushing, > so we went to great pains to avoid such. > >> That leaves the actual bug of setting map_and_fenceable to true if we're >> not bound to ggtt, which violates the change introduced in the above >> patch. Unbinding in that case really looks like the simplest and safest >> option, we have to do it anyway. >> >> If Chris agrees, please add the above analysis to the commit message when >> merging to -fixes. >> > >> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=85896 >> > Tested-by: huax.lu@intel.com >> >> Testcase: igt/gem_concurrent_blit > Testcase: igt/gem_concurrent_blit/gttX* > > It was also only triggered by recent additions to > gem_concurrent_blit (which itself was trying to stress test our > fence-vs-GPU serialisation for testing requests - so I can claim it was > intentional!). However, it turns out to be easier to hit in practice > than in testing. :| > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c index 749ab485569e..cd7f4734c9f8 100644 --- a/drivers/gpu/drm/i915/i915_gem_tiling.c +++ b/drivers/gpu/drm/i915/i915_gem_tiling.c @@ -375,22 +375,9 @@ i915_gem_set_tiling(struct drm_device *dev, void *data, * has to also include the unfenced register the GPU uses * whilst executing a fenced command for an untiled object. */ - - obj->map_and_fenceable = - !i915_gem_obj_ggtt_bound(obj) || - (i915_gem_obj_ggtt_offset(obj) + - obj->base.size <= dev_priv->gtt.mappable_end && - i915_gem_object_fence_ok(obj, args->tiling_mode)); - - /* Rebind if we need a change of alignment */ - if (!obj->map_and_fenceable) { - u32 unfenced_align = - i915_gem_get_gtt_alignment(dev, obj->base.size, - args->tiling_mode, - false); - if (i915_gem_obj_ggtt_offset(obj) & (unfenced_align - 1)) - ret = i915_gem_object_ggtt_unbind(obj); - } + if (obj->map_and_fenceable && + !i915_gem_object_fence_ok(obj, args->tiling_mode)) + ret = i915_gem_object_ggtt_unbind(obj); if (ret == 0) { obj->fence_dirty =