[Intel-gfx] [PATCH v2 2/5] drm/i915: Do not make assumptions on GGTT VMA sizes
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Thu Apr 30 05:03:42 PDT 2015
On 04/30/2015 12:19 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)
>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> Signed-off-by: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_gem.c | 103 +++++++++++++++++++++++-------------
> drivers/gpu/drm/i915/i915_gem_gtt.c | 23 ++++++++
> drivers/gpu/drm/i915/i915_gem_gtt.h | 4 ++
> 3 files changed, 92 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index e8f6f4c..9717c9d 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,
> @@ -3516,36 +3517,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 do not like this almost identical branches - so I defer to Chris to
okay that this is what he wanted.
> 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 object (view type=%u) alignment requested %u\n",
> + ggtt_view ? ggtt_view->type : 0,
> + 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 an object (view type=%u) larger than the aperture: size=%u > %s aperture=%lu\n",
> + ggtt_view ? ggtt_view->type : 0,
> + size,
> flags & PIN_MAPPABLE ? "mappable" : "total",
> end);
> return ERR_PTR(-E2BIG);
> @@ -4207,28 +4232,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) {
> + if ((bound ^ vma->bound) & GLOBAL_BIND) {
I still don't get this. !ggtt_view means GLOBAL_BIND cannot be set, what
am I missing? It wouldn't work if the condition was just the type check?
> + 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 fc562c6..640584f 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2847,3 +2847,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);
It is a marginal point, but I wonder is size zero could be considered as
a failure value and acted upon from the caller more gracefuly.
And in general I wonder if something like I915_BUG_ON which would
re-route fops to fail-all and grab struct_mutex, or something, would
maybe be an option.
Regards,
Tvrtko
More information about the Intel-gfx
mailing list