[Intel-gfx] [PATCH 2/5] drm/i915: Do not make assumptions on GGTT VMA sizes
Joonas Lahtinen
joonas.lahtinen at linux.intel.com
Tue Apr 28 00:23:14 PDT 2015
On ma, 2015-04-27 at 14:55 +0100, Tvrtko Ursulin wrote:
> 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 at chris-wilson.co.uk>
> > Signed-off-by: Joonas Lahtinen <joonas.lahtinen at 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 = ...
>
It was originally like that, this v2 moved it to its own branch because
Chris informed me that there is work going on which adds more
constraints, and that optimizing common code should only be done after
the dust has settled.
> >
> > 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?
True so.
>
> > 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.
This condition is there because the calculation applies to the whole
object flag 'map_and_fenceable'. And conditions when the whole object
are handled are when it's not in GGTT, or we're talking about normal
view in GGTT.
>
> > + 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?
>
There's no sense to call the this function with NULL pointer because it
is the main argument of the function, it'd OOPS in this case, so I
understood this would be the OK place to use it.
> 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
More information about the Intel-gfx
mailing list