[Intel-gfx] [PATCH 1/3] drm/i915: Support for pre-populating the object with system pages

[Chris Wilson]

On Thu, Aug 27, 2015 at 12:04:37PM +0530, Ankitprasad Sharma wrote:
> On Tue, 2015-08-25 at 11:51 +0100, Siluvery, Arun wrote:
> > On 24/08/2015 12:58, ankitprasad.r.sharma at intel.com wrote:
> > > From: Ankitprasad Sharma <ankitprasad.r.sharma at intel.com>
> > >
> > > This patch provides support for the User to populate the object
> > > with system pages at its creation time. Since this can be safely
> > > performed without holding the 'struct_mutex', it would help to reduce
> > > the time 'struct_mutex' is kept locked especially during the exec-buffer
> > > path, where it is generally held for the longest time.
> > >
> > > Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma at intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/i915_dma.c |  2 +-
> > >   drivers/gpu/drm/i915/i915_gem.c | 51 +++++++++++++++++++++++++++++++----------
> > >   include/uapi/drm/i915_drm.h     | 11 ++++-----
> > >   3 files changed, 45 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > > index 8319e07..955aa16 100644
> > > --- a/drivers/gpu/drm/i915/i915_dma.c
> > > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > > @@ -171,7 +171,7 @@ static int i915_getparam(struct drm_device *dev, void *data,
> > >   		value = HAS_RESOURCE_STREAMER(dev);
> > >   		break;
> > >   	case I915_PARAM_CREATE_VERSION:
> > > -		value = 2;
> > > +		value = 3;
> > >   		break;
> > >   	default:
> > >   		DRM_DEBUG("Unknown parameter %d\n", param->param);
> > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > > index c44bd05..3904feb 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > > @@ -46,6 +46,7 @@ static void
> > >   i915_gem_object_retire__write(struct drm_i915_gem_object *obj);
> > >   static void
> > >   i915_gem_object_retire__read(struct drm_i915_gem_object *obj, int ring);
> > > +static int __i915_gem_object_get_pages(struct drm_i915_gem_object *obj);
> > >
> > >   static bool cpu_cache_is_coherent(struct drm_device *dev,
> > >   				  enum i915_cache_level level)
> > > @@ -414,6 +415,18 @@ i915_gem_create(struct drm_file *file,
> > >   	if (obj == NULL)
> > >   		return -ENOMEM;
> > >
> > > +	if (flags & I915_CREATE_POPULATE) {
> > > +		struct drm_i915_private *dev_priv = dev->dev_private;
> > > +
> > > +		ret = __i915_gem_object_get_pages(obj);
> > > +		if (ret)
> > > +			return ret;
> > > +
> > > +		mutex_lock(&dev->struct_mutex);
> > > +		list_add_tail(&obj->global_list, &dev_priv->mm.unbound_list);
> > > +		mutex_unlock(&dev->struct_mutex);
> > > +	}
> > > +
> > >   	ret = drm_gem_handle_create(file, &obj->base, &handle);
> > 
> > If I915_CREATE_POPULATE is set, don't we have to release the pages when 
> > this call fails?
> I guess the i915_gem_object_get_pages_* takes care of releasing the
> pages when returning an error.

Oh, it should just be calling drm_gem_object_unreference_unlocked(). No
need to worry about obj->pages as they will be reaped along with the
object.

> One more thing,
> What can be preferred here when an error is returned by
> __i915_gem_object_get_pages? 
> Shall we return error to the userspace after unreferencing the object or
> to continue without pre-populating pages (and returning object handle)?

Report the error. Userspace can then decide if it wants to allocate an
object without prepulated pages, try searching/reaping its own caches
harder for an available object, or do something different entirely to
avoid excess memory usage.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre