Re: [PATCH 5/5] drm/i915: Use partial view in mmap fault handler

[Chris Wilson <chris@chris-wilson.co.uk>]

On Mon, Apr 27, 2015 at 03:12:01PM +0300, Joonas Lahtinen wrote:
> On ma, 2015-04-27 at 12:21 +0100, Chris Wilson wrote:
> > On Mon, Apr 27, 2015 at 02:01:59PM +0300, Joonas Lahtinen wrote:
> > > On pe, 2015-04-24 at 13:33 +0100, Chris Wilson wrote:
> > > > On Fri, Apr 24, 2015 at 03:10:20PM +0300, Joonas Lahtinen wrote:
> > > > > Use partial view for huge BOs (bigger than half the mappable aperture)
> > > > > in fault handler so that they can be accessed withough trying to make
> > > > > room for them by evicting other objects.
> > > > > 
> > > > > Signed-off-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/i915_gem.c |   67 ++++++++++++++++++++++++++-------------
> > > > >  1 file changed, 45 insertions(+), 22 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > > > > index c2c1819..eb30cee 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 half the aperture. */
> > > > > +	if (obj->base.size > dev_priv->gtt.mappable_end/2) {
> > > > > +		static const size_t 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(size_t,
> > > > > +			      chunk_size,
> > > > > +			      (vma->vm_end - vma->vm_start)/PAGE_SIZE -
> > > > > +			      view.params.partial.offset);
> > > > 
> > > > This isn't what I was imagining.
> > > > 
> > > > I was expecting to see error handling inside the fault path if we could
> > > > not pin the object. This way we could handle fragmentation or display
> > > > objects pinned outside the aperture.
> > > 
> > > After discussion with Daniel, the idea was dropped due to high amount of
> > > trashing which would occur if each object would be attempted to fit to
> > > the mappable aperture for each fault to that object.
> > 
> > The point is that we fail to install a partial view for pinned objects
> > outside of the aperture. Or did I miss how you handle them?
> > 
> 
> That is true.
> 
> By changing the comparison to be against full aperture size, then the
> patch will only bring new functionality to handle the cases when the
> object was actually impossible to map previously (and was early
> rejected).
> 
> I'd prefer to have this version in first (with change of removing the /2
> from aperture size comparison), to get some feedback from the XenGT team
> about the usability of it (speed-wise).

No. Daniel rejected a change just because we didn't have this series,
only to find this series doesn't even provide the support Daniel
wanted...

I don't see this as a useful stepping point. The current users of large
objects do not map through the GTT.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx