| Message ID | 1430392771.1189.7.camel@jlahtine-mobl1 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@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 | 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
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@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 | 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
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; + } 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) { + 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); + + 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
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@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 | 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(-)