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

[Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>]

On Mon, 2015-08-24 at 13:35 +0100, Chris Wilson wrote:
> On Mon, Aug 24, 2015 at 05:28:14PM +0530, ankitprasad.r.sharma@intel.com wrote:
> > +static int
> > +__i915_gem_object_get_pages(struct drm_i915_gem_object *obj)
> > +{
> > +	const struct drm_i915_gem_object_ops *ops = obj->ops;
> > +	int ret;
> > +
> > +	WARN_ON(obj->pages);
> > +
> > +	if (obj->madv != I915_MADV_WILLNEED) {
> > +		DRM_DEBUG("Attempting to obtain a purgeable object\n");
> > +		return -EFAULT;
> > +	}
> > +
> > +	BUG_ON(obj->pages_pin_count);
> 
> Put the parameter checking into i915_gem_object_get_pages(). The __i915
> version is only allowed from strict contexts and we can place the burden
> of being correct on the caller.
> 
> > +	ret = ops->get_pages(obj);
> > +	if (ret)
> > +		return ret;
> > +
> > +	obj->get_page.sg = obj->pages->sgl;
> > +	obj->get_page.last = 0;
> > +
> > +	return 0;
> > +}
> > +
> >  /* Ensure that the associated pages are gathered from the backing storage
> >   * and pinned into our object. i915_gem_object_get_pages() may be called
> >   * multiple times before they are released by a single call to
> > @@ -2339,28 +2377,17 @@ int
> >  i915_gem_object_get_pages(struct drm_i915_gem_object *obj)
> >  {
> >  	struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
> > -	const struct drm_i915_gem_object_ops *ops = obj->ops;
> >  	int ret;
> >  
> >  	if (obj->pages)
> >  		return 0;
> >  
> > -	if (obj->madv != I915_MADV_WILLNEED) {
> > -		DRM_DEBUG("Attempting to obtain a purgeable object\n");
> > -		return -EFAULT;
> > -	}
> > -
> > -	BUG_ON(obj->pages_pin_count);
> > -
> > -	ret = ops->get_pages(obj);
> > +	ret = __i915_gem_object_get_pages(obj);
> >  	if (ret)
> >  		return ret;
> >  
> >  	list_add_tail(&obj->global_list, &dev_priv->mm.unbound_list);
> 
> I am tempted to say this should be in a new
> 
> __i915_gem_object_get_pages__tail_locked()
> 
> so that we don't have to hunt down users if we ever need to modify the
> global lists.
Could not get you here. 
is it just to add list_add_tail in a separate function
__i915_gem_object_get_pages__tail_locked(), or a new variant of
__i915_gem_object_get_pages() that will also do the link list insertion

Thanks,
Ankit

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx