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

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

On Wed, 2015-12-09 at 14:06 +0000, Tvrtko Ursulin wrote:
> Hi,
> 
> On 09/12/15 12:46, ankitprasad.r.sharma@intel.com wrote:
> > From: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
> >
> > Extend the drm_i915_gem_create structure to add support for
> > creating Stolen memory backed objects. Added a new flag through
> > which user can specify the preference to allocate the object from
> > stolen memory, which if set, an attempt will be made to allocate
> > the object from stolen memory subject to the availability of
> > free space in the stolen region.
> >
> > v2: Rebased to the latest drm-intel-nightly (Ankit)
> >
> > v3: Changed versioning of GEM_CREATE param, added new comments (Tvrtko)
> >
> > v4: Changed size from 32b to 64b to prevent userspace overflow (Tvrtko)
> > Corrected function arguments ordering (Chris)
> >
> > v5: Corrected function name (Chris)
> >
> > Testcase: igt/gem_stolen
> >
> > Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
> > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_dma.c        |  3 +++
> >   drivers/gpu/drm/i915/i915_drv.h        |  2 +-
> >   drivers/gpu/drm/i915/i915_gem.c        | 30 +++++++++++++++++++++++++++---
> >   drivers/gpu/drm/i915/i915_gem_stolen.c |  4 ++--
> >   include/uapi/drm/i915_drm.h            | 16 ++++++++++++++++
> >   5 files changed, 49 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> > index ffcb9c6..6927c7e 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -170,6 +170,9 @@ static int i915_getparam(struct drm_device *dev, void *data,
> >   	case I915_PARAM_HAS_RESOURCE_STREAMER:
> >   		value = HAS_RESOURCE_STREAMER(dev);
> >   		break;
> > +	case I915_PARAM_CREATE_VERSION:
> > +		value = 2;
> > +		break;
> >   	default:
> >   		DRM_DEBUG("Unknown parameter %d\n", param->param);
> >   		return -EINVAL;
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 8e554d3..d45274e 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -3213,7 +3213,7 @@ void i915_gem_stolen_remove_node(struct drm_i915_private *dev_priv,
> >   int i915_gem_init_stolen(struct drm_device *dev);
> >   void i915_gem_cleanup_stolen(struct drm_device *dev);
> >   struct drm_i915_gem_object *
> > -i915_gem_object_create_stolen(struct drm_device *dev, u32 size);
> > +i915_gem_object_create_stolen(struct drm_device *dev, u64 size);
> >   struct drm_i915_gem_object *
> >   i915_gem_object_create_stolen_for_preallocated(struct drm_device *dev,
> >   					       u32 stolen_offset,
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index d57e850..296e63f 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -375,6 +375,7 @@ static int
> >   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;
> > +		}
> > +
> > +		/* Always clear fresh buffers before handing to userspace */
> > +		ret = i915_gem_object_clear(obj);
> > +		if (ret) {
> > +			drm_gem_object_unreference(&obj->base);
> > +			mutex_unlock(&dev->struct_mutex);
> > +			return ret;
> > +		}
> > +
> > +		mutex_unlock(&dev->struct_mutex);
> > +	} else {
> > +		obj = i915_gem_alloc_object(dev, size);
> > +	}
> > +
> >   	if (obj == NULL)
> >   		return -ENOMEM;
> >
> > @@ -409,7 +433,7 @@ i915_gem_dumb_create(struct drm_file *file,
> >   	args->pitch = ALIGN(args->width * DIV_ROUND_UP(args->bpp, 8), 64);
> >   	args->size = args->pitch * args->height;
> >   	return i915_gem_create(file, dev,
> > -			       args->size, &args->handle);
> > +			       args->size, 0, &args->handle);
> >   }
> >
> >   /**
> > @@ -422,7 +446,7 @@ i915_gem_create_ioctl(struct drm_device *dev, void *data,
> >   	struct drm_i915_gem_create *args = data;
> >
> >   	return i915_gem_create(file, dev,
> > -			       args->size, &args->handle);
> > +			       args->size, args->flags, &args->handle);
> >   }
> >
> >   static inline int
> > diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> > index 598ed2f..b98a3bf 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> > @@ -583,7 +583,7 @@ cleanup:
> >   }
> >
> >   struct drm_i915_gem_object *
> > -i915_gem_object_create_stolen(struct drm_device *dev, u32 size)
> > +i915_gem_object_create_stolen(struct drm_device *dev, u64 size)
> >   {
> >   	struct drm_i915_private *dev_priv = dev->dev_private;
> >   	struct drm_i915_gem_object *obj;
> > @@ -593,7 +593,7 @@ i915_gem_object_create_stolen(struct drm_device *dev, u32 size)
> >   	if (!drm_mm_initialized(&dev_priv->mm.stolen))
> >   		return NULL;
> >
> > -	DRM_DEBUG_KMS("creating stolen object: size=%x\n", size);
> > +	DRM_DEBUG_KMS("creating stolen object: size=%llx\n", size);
> >   	if (size == 0)
> >   		return NULL;
> >
> > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> > index 67cebe6..8e7e3a4 100644
> > --- a/include/uapi/drm/i915_drm.h
> > +++ b/include/uapi/drm/i915_drm.h
> > @@ -356,6 +356,7 @@ typedef struct drm_i915_irq_wait {
> >   #define I915_PARAM_EU_TOTAL		 34
> >   #define I915_PARAM_HAS_GPU_RESET	 35
> >   #define I915_PARAM_HAS_RESOURCE_STREAMER 36
> > +#define I915_PARAM_CREATE_VERSION	 37
> >
> >   typedef struct drm_i915_getparam {
> >   	__s32 param;
> > @@ -455,6 +456,21 @@ struct drm_i915_gem_create {
> >   	 */
> >   	__u32 handle;
> >   	__u32 pad;
> > +	/**
> > +	 * Requested flags (currently used for placement
> > +	 * (which memory domain))
> > +	 *
> > +	 * You can request that the object be created from special memory
> > +	 * rather than regular system pages using this parameter. Such
> > +	 * irregular objects may have certain restrictions (such as CPU
> > +	 * access to a stolen object is verboten).
> > +	 *
> > +	 * This can be used in the future for other purposes too
> > +	 * e.g. specifying tiling/caching/madvise
> > +	 */
> > +	__u32 flags;
> > +#define I915_CREATE_PLACEMENT_STOLEN 	(1<<0) /* Cannot use CPU mmaps */
> > +#define __I915_CREATE_UNKNOWN_FLAGS	-(I915_CREATE_PLACEMENT_STOLEN << 1)
> 
> I've asked in another reply, now that userspace can create a stolen 
> object, what happens if it tries to use it for a batch buffer?
> 
> Can it end up in the relocate_entry_cpu with a batch buffer allocated 
> from stolen, which would then call i915_gem_object_get_page and crash?
Thanks for pointing it out.
Yes, this is definitely a possibility, if we allocate batchbuffers from
the stolen region. I have started working on that, to do
relocate_entry_stolen() if the object is allocated from stolen.
> 
> >   };
> >
> >   struct drm_i915_gem_pread {
> >
> 
> Regards,
> 
> Tvrtko
Thanks,
Ankit

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