Re: [PATCH 2/5] drm/i915: Do not make assumptions on GGTT VMA sizes

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

Hi,

On 04/24/2015 01:09 PM, Joonas Lahtinen wrote:
>
> GGTT VMA sizes might be smaller than the whole object size due to
> different GGTT views.
>
> v2:
> - Separate GGTT view constraint calculations from normal view
>    constraint calculations (Chris Wilson)
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem.c     |  106 ++++++++++++++++++++++-------------
>   drivers/gpu/drm/i915/i915_gem_gtt.c |   23 ++++++++
>   drivers/gpu/drm/i915/i915_gem_gtt.h |    4 ++
>   3 files changed, 95 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index e8f6f4c..111ac8a 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3497,7 +3497,8 @@ static bool i915_gem_valid_gtt_space(struct i915_vma *vma,
>   }
>
>   /**
> - * Finds free space in the GTT aperture and binds the object there.
> + * Finds free space in the GTT aperture and binds the object or a view of it
> + * there.
>    */
>   static struct i915_vma *
>   i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
> @@ -3513,39 +3514,66 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
>   		flags & PIN_OFFSET_BIAS ? flags & PIN_OFFSET_MASK : 0;
>   	unsigned long end =
>   		flags & PIN_MAPPABLE ? dev_priv->gtt.mappable_end : vm->total;
> +	const bool is_object = !ggtt_view ||
> +			       ggtt_view->type == I915_GGTT_VIEW_NORMAL;
>   	struct i915_vma *vma;
>   	int ret;
>
> -	if(WARN_ON(i915_is_ggtt(vm) != !!ggtt_view))
> -		return ERR_PTR(-EINVAL);
> +	if (i915_is_ggtt(vm)) {
> +		u32 view_size;
> +
> +		if (WARN_ON(!ggtt_view))
> +			return ERR_PTR(-EINVAL);
>
> -	fence_size = i915_gem_get_gtt_size(dev,
> -					   obj->base.size,
> -					   obj->tiling_mode);
> -	fence_alignment = i915_gem_get_gtt_alignment(dev,
> -						     obj->base.size,
> -						     obj->tiling_mode, true);
> -	unfenced_alignment =
> -		i915_gem_get_gtt_alignment(dev,
> -					   obj->base.size,
> -					   obj->tiling_mode, false);
> +		view_size = i915_ggtt_view_size(obj, ggtt_view);
> +
> +		fence_size = i915_gem_get_gtt_size(dev,
> +						   view_size,
> +						   obj->tiling_mode);
> +		fence_alignment = i915_gem_get_gtt_alignment(dev,
> +							     view_size,
> +							     obj->tiling_mode,
> +							     true);
> +		unfenced_alignment = i915_gem_get_gtt_alignment(dev,
> +								view_size,
> +								obj->tiling_mode,
> +								false);
> +		size = flags & PIN_MAPPABLE ? fence_size : view_size;
> +	} else {
> +		fence_size = i915_gem_get_gtt_size(dev,
> +						   obj->base.size,
> +						   obj->tiling_mode);
> +		fence_alignment = i915_gem_get_gtt_alignment(dev,
> +							     obj->base.size,
> +							     obj->tiling_mode,
> +							     true);
> +		unfenced_alignment =
> +			i915_gem_get_gtt_alignment(dev,
> +						   obj->base.size,
> +						   obj->tiling_mode,
> +						   false);
> +		size = flags & PIN_MAPPABLE ? fence_size : obj->base.size;
> +	}

Looks like you could avoid duplicating the if/else below if you did 
something like:

view_size = ggtt_view ? i915_ggtt_view_size(...) : obj->base.size;

And then have only one instance of:

fenced_size = ... with view_size for size, etc..
unfenced_size..
*alignment = ...
size = ...

>
>   	if (alignment == 0)
>   		alignment = flags & PIN_MAPPABLE ? fence_alignment :
>   						unfenced_alignment;
>   	if (flags & PIN_MAPPABLE && alignment & (fence_alignment - 1)) {
> -		DRM_DEBUG("Invalid object alignment requested %u\n", alignment);
> +		DRM_DEBUG("Invalid %s alignment requested %u\n",
> +			  is_object ? "object" : "GGTT view",
> +			  alignment);
>   		return ERR_PTR(-EINVAL);
>   	}
>
> -	size = flags & PIN_MAPPABLE ? fence_size : obj->base.size;
> -
> -	/* If the object is bigger than the entire aperture, reject it early
> -	 * before evicting everything in a vain attempt to find space.
> +	/* If binding the object/GGTT view requires more space than the entire
> +	 * aperture has, reject it early before evicting everything in a vain
> +	 * attempt to find space.
>   	 */
> -	if (obj->base.size > end) {
> -		DRM_DEBUG("Attempting to bind an object larger than the aperture: object=%zd > %s aperture=%lu\n",
> -			  obj->base.size,
> +	if (size > end) {
> +		DRM_DEBUG("Attempting to bind %s (view type=%u) larger than the aperture: size=%u > %s aperture=%lu\n",
> +			  is_object ? "an object" : "a GGTT view ",
> +			  is_object ? 0 : ggtt_view->type,
> +			  size,

I am not sure if this is_object thing here and above is worth it. Maybe 
just add ggtt_view->type? Should carry the same amount of info for 
debugging?

>   			  flags & PIN_MAPPABLE ? "mappable" : "total",
>   			  end);
>   		return ERR_PTR(-E2BIG);
> @@ -4207,28 +4235,30 @@ i915_gem_object_do_pin(struct drm_i915_gem_object *obj,
>   			return ret;
>   	}
>
> -	if ((bound ^ vma->bound) & GLOBAL_BIND) {
> -		bool mappable, fenceable;
> -		u32 fence_size, fence_alignment;
> +	if (!ggtt_view || ggtt_view->type == I915_GGTT_VIEW_NORMAL) {

Why this condition, and why "!ggtt_view" - that means PPGTT, no? GGTT 
and ggtt_view == NULL is not allowed by the WARN_ON earlier in this 
function.

> +		if ((bound ^ vma->bound) & GLOBAL_BIND) {
> +			bool mappable, fenceable;
> +			u32 fence_size, fence_alignment;
>
> -		fence_size = i915_gem_get_gtt_size(obj->base.dev,
> -						   obj->base.size,
> -						   obj->tiling_mode);
> -		fence_alignment = i915_gem_get_gtt_alignment(obj->base.dev,
> -							     obj->base.size,
> -							     obj->tiling_mode,
> -							     true);
> +			fence_size = i915_gem_get_gtt_size(obj->base.dev,
> +							   obj->base.size,
> +							   obj->tiling_mode);
> +			fence_alignment = i915_gem_get_gtt_alignment(obj->base.dev,
> +								     obj->base.size,
> +								     obj->tiling_mode,
> +								     true);
>
> -		fenceable = (vma->node.size == fence_size &&
> -			     (vma->node.start & (fence_alignment - 1)) == 0);
> +			fenceable = (vma->node.size == fence_size &&
> +				     (vma->node.start & (fence_alignment - 1)) == 0);
>
> -		mappable = (vma->node.start + fence_size <=
> -			    dev_priv->gtt.mappable_end);
> +			mappable = (vma->node.start + fence_size <=
> +				    dev_priv->gtt.mappable_end);
>
> -		obj->map_and_fenceable = mappable && fenceable;
> -	}
> +			obj->map_and_fenceable = mappable && fenceable;
> +		}
>
> -	WARN_ON(flags & PIN_MAPPABLE && !obj->map_and_fenceable);
> +		WARN_ON(flags & PIN_MAPPABLE && !obj->map_and_fenceable);
> +	}
>
>   	vma->pin_count++;
>   	return 0;
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index ee9ff8e..5babbd3 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2842,3 +2842,26 @@ int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
>
>   	return 0;
>   }
> +
> +/**
> + * i915_ggtt_view_size - Get the size of a GGTT view.
> + * @obj: Object the view is of.
> + * @view: The view in question.
> + *
> + * @return The size of the GGTT view in bytes.
> + */
> +size_t
> +i915_ggtt_view_size(struct drm_i915_gem_object *obj,
> +		    const struct i915_ggtt_view *view)
> +{
> +	BUG_ON(!view);

WARN_ON and return object size maybe?

Once I mentioned, if we really wanted to stop the driver completely in 
some cases, should add something like I915_BUG_ON() and some flags at 
strategic places. Maybe it would be possible.

> +
> +	if (view->type == I915_GGTT_VIEW_NORMAL ||
> +	    view->type == I915_GGTT_VIEW_ROTATED) {
> +		return obj->base.size;
> +	} else {
> +		WARN_ONCE(1, "GGTT view %u not implemented!\n", view->type);
> +		return obj->base.size;
> +	}
> +}
> +
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index 4e6cac5..34b7cca 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -498,4 +498,8 @@ i915_ggtt_view_equal(const struct i915_ggtt_view *a,
>   	return a->type == b->type;
>   }
>
> +size_t
> +i915_ggtt_view_size(struct drm_i915_gem_object *obj,
> +		    const struct i915_ggtt_view *view);
> +
>   #endif
>

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx