Re: [PATCH v2 2/5] drm/i915: Do not make assumptions on GGTT VMA sizes

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux