Re: [PATCH v3 1/4] drm/i915: Do not make assumptions on GGTT VMA sizes

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

On 05/06/2015 12:33 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)
> v3:
> - Do not bother with debug wording. (Tvrtko Ursulin)
> v4:
> - Clearer logic for calculating map_and_fenceable (Tvrtko Ursulin)
>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem.c     | 74 +++++++++++++++++++++++++------------
>   drivers/gpu/drm/i915/i915_gem_gtt.c | 23 ++++++++++++
>   drivers/gpu/drm/i915/i915_gem_gtt.h |  4 ++
>   3 files changed, 77 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index c378f04..473e947 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3498,7 +3498,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,
> @@ -3517,36 +3518,60 @@ i915_gem_object_bind_to_vm(struct drm_i915_gem_object *obj,
>   	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;
> +	}

I don't like this duplication, but apparently Chris wants it like this 
and plans to change it all shortly AFAIR. In the view of that:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko


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