[Intel-gfx] [PATCH v2 5/5] drm/i915: Use partial view in mmap fault handler

[Joonas Lahtinen]

On to, 2015-04-30 at 15:54 +0100, Tvrtko Ursulin wrote:
> On 04/30/2015 12:21 PM, Joonas Lahtinen wrote:
> >
> > Use partial view for huge BOs (bigger than the mappable aperture)
> > in fault handler so that they can be accessed without trying to make
> > room for them by evicting other objects.
> >
> > v2:
> > - Only use partial views in the case where early rejection was
> >    previously done.
> > - Account variable type changes from previous reroll.
> >
> > 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 | 69 +++++++++++++++++++++++++++--------------
> >   1 file changed, 46 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index a020836..2f3fa0b 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -1635,6 +1635,7 @@ int i915_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
> >   	struct drm_i915_gem_object *obj = to_intel_bo(vma->vm_private_data);
> >   	struct drm_device *dev = obj->base.dev;
> >   	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct i915_ggtt_view view = i915_ggtt_view_normal;
> >   	pgoff_t page_offset;
> >   	unsigned long pfn;
> >   	int ret = 0;
> > @@ -1667,8 +1668,21 @@ int i915_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
> >   		goto unlock;
> >   	}
> >
> > -	/* Now bind it into the GTT if needed */
> > -	ret = i915_gem_obj_ggtt_pin(obj, 0, PIN_MAPPABLE);
> > +	/* Use a partial view if the object is bigger than the aperture. */
> > +	if (obj->base.size >= dev_priv->gtt.mappable_end) {
> > +		static const unsigned int chunk_size = 256; // 1 MiB
> > +		memset(&view, 0, sizeof(view));
> > +		view.type = I915_GGTT_VIEW_PARTIAL;
> > +		view.params.partial.offset = rounddown(page_offset, chunk_size);
> > +		view.params.partial.size =
> > +			min_t(unsigned int,
> > +			      chunk_size,
> > +			      (vma->vm_end - vma->vm_start)/PAGE_SIZE -
> > +			      view.params.partial.offset);
> > +	}
> > +
> > +	/* Now pin it into the GTT if needed */
> > +	ret = i915_gem_object_ggtt_pin(obj, &view, 0, PIN_MAPPABLE);
> >   	if (ret)
> >   		goto unlock;
> >
> > @@ -1681,30 +1695,44 @@ int i915_gem_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
> >   		goto unpin;
> >
> >   	/* Finally, remap it using the new GTT offset */
> > -	pfn = dev_priv->gtt.mappable_base + i915_gem_obj_ggtt_offset(obj);
> > +	pfn = dev_priv->gtt.mappable_base +
> > +		i915_gem_obj_ggtt_offset_view(obj, &view);
> >   	pfn >>= PAGE_SHIFT;
> >
> > -	if (!obj->fault_mappable) {
> > -		unsigned long size = min_t(unsigned long,
> > -					   vma->vm_end - vma->vm_start,
> > -					   obj->base.size);
> > -		int i;
> > +	if (unlikely(view.type == I915_GGTT_VIEW_PARTIAL)) {
> > +		unsigned long base = vma->vm_start +
> > +			(view.params.partial.offset << PAGE_SHIFT);
> > +		unsigned int i;
> >
> > -		for (i = 0; i < size >> PAGE_SHIFT; i++) {
> > -			ret = vm_insert_pfn(vma,
> > -					    (unsigned long)vma->vm_start + i * PAGE_SIZE,
> > -					    pfn + i);
> > +		for (i = 0; i < view.params.partial.size; i++) {
> > +			ret = vm_insert_pfn(vma, base + i * PAGE_SIZE, pfn + i);
> >   			if (ret)
> >   				break;
> >   		}
> > -
> >   		obj->fault_mappable = true;
> > -	} else
> > -		ret = vm_insert_pfn(vma,
> > -				    (unsigned long)vmf->virtual_address,
> > -				    pfn + page_offset);
> 
> If I read the diff correctly you don't have equivalent handling (as the 
> normal view) for when the case when the pre-fault fails somewhere in the 
> middle?
> 

True so, the flag fault_mappable is used for the normal view to track
whether all pages were inserted and it makes sense to just insert the
faulted one. I just didn't want to add another flag to track the same
for each vma.

Regards, Joonas

> That means if that happens you'll call vm_insert_pfn on the whole range 
> again - is that OK?
> 
> Regards,
> 
> Tvrtko