Re: [PATCH] drm/i915: Fix obj->map_and_fenceable across tiling changes

[Daniel Vetter <daniel@ffwll.ch>]

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

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