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

Tvrtko Ursulin
2015-05-06T12:23:10Z

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