Re: [PATCH 2/4] drm/i915: Support for creating Stolen memory backed objects

[Daniel Vetter <daniel@ffwll.ch>]

On Sun, Sep 20, 2015 at 07:37:46PM +0530, Ankitprasad Sharma wrote:
> On Tue, 2015-09-15 at 10:49 +0100, Chris Wilson wrote:
> > On Tue, Sep 15, 2015 at 02:03:25PM +0530, ankitprasad.r.sharma@intel.com wrote:
> > >  i915_gem_create(struct drm_file *file,
> > >  		struct drm_device *dev,
> > >  		uint64_t size,
> > > +		uint32_t flags,
> > >  		uint32_t *handle_p)
> > >  {
> > >  	struct drm_i915_gem_object *obj;
> > > @@ -385,8 +386,31 @@ i915_gem_create(struct drm_file *file,
> > >  	if (size == 0)
> > >  		return -EINVAL;
> > >  
> > > +	if (flags & __I915_CREATE_UNKNOWN_FLAGS)
> > > +		return -EINVAL;
> > > +
> > >  	/* Allocate the new object */
> > > -	obj = i915_gem_alloc_object(dev, size);
> > > +	if (flags & I915_CREATE_PLACEMENT_STOLEN) {
> > > +		mutex_lock(&dev->struct_mutex);
> > > +		obj = i915_gem_object_create_stolen(dev, size);
> > > +		if (!obj) {
> > > +			mutex_unlock(&dev->struct_mutex);
> > > +			return -ENOMEM;
> > 
> > Note that you should change the i915_gem_object_create_stolen() to
> > report the precise error, as with the eviction support we may trigger
> > EINTR. Also ENOSPC will be preferrable for requesting a larger stolen
> > object than the available space (to help distinguish between true oom).
> > -Chris
> I would prefer to do this as a separate patch, as this might require a
> restructuring of the gem_stolen.c to some extent, something like this:

Yeah makes sense to have that split out, but I agree with Chris that we'll
need that error reporting. Follow-up patch on top of your series if fine
with me.
-Daniel

> 
> @@ -465,29 +467,29 @@ i915_gem_object_create_stolen(struct drm_device
> *dev, u64 size)
>  	int ret;
>  
>  	if (!drm_mm_initialized(&dev_priv->mm.stolen))
> -		return NULL;
> +		return ERR_PTR(-EINVAL);
>  
>  	DRM_DEBUG_KMS("creating stolen object: size=%llx\n", size);
>  	if (size == 0)
> -		return NULL;
> +		return ERR_PTR(-EINVAL);
>  
>  	stolen = kzalloc(sizeof(*stolen), GFP_KERNEL);
>  	if (!stolen)
> -		return NULL;
> +		return ERR_PTR(-ENOMEM);
>  
>  	ret = i915_gem_stolen_insert_node(dev_priv, stolen, size, 4096);
>  	if (ret) {
>  		kfree(stolen);
> -		return NULL;
> +		return ERR_PTR(-ENOSPC);
>  	}
>  
>  	obj = _i915_gem_object_create_stolen(dev, stolen);
> -	if (obj)
> +	if (!IS_ERR(obj))
>  		return obj;
>  
>  	i915_gem_stolen_remove_node(dev_priv, stolen);
>  	kfree(stolen);
> -	return NULL;
> +	return obj;
>  }
>  
> 
> Thanks,
> Ankit
> 
> 
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx