On to, 2015-04-30 at 13:03 +0100, Tvrtko Ursulin wrote:
> 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@xxxxxxxxxxxxxxxxxx>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> > Signed-off-by: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> > ---
> > 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.
>
Confirmed in IRC that the above is according to what Chris meant.
> > 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?
>
Clarified this.
> > + 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.
>
I wouldn't like to overcomplicate the error handling for for imaginary
or promille use cases, so I think it is a good rule of thumb that bug on
if the kernel would OOPS anyway (which holds here).
Regards, Joonas
> Regards,
>
> Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
