diff mbox

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

Message ID 1437573109-19211-3-git-send-email-ankitprasad.r.sharma@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

ankitprasad.r.sharma@intel.com July 22, 2015, 1:51 p.m. UTC 
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)

Testcase: igt/gem_stolen

Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c |  3 +++
 drivers/gpu/drm/i915/i915_gem.c | 33 +++++++++++++++++++++++++++++----
 include/uapi/drm/i915_drm.h     | 15 +++++++++++++++
 3 files changed, 47 insertions(+), 4 deletions(-)

Comments

Tvrtko Ursulin July 22, 2015, 3:14 p.m. UTC | #1 
Hi,

On 07/22/2015 02:51 PM, 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)
>
> Testcase: igt/gem_stolen
>
> Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_dma.c |  3 +++
>   drivers/gpu/drm/i915/i915_gem.c | 33 +++++++++++++++++++++++++++++----
>   include/uapi/drm/i915_drm.h     | 15 +++++++++++++++
>   3 files changed, 47 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index c5349fa..bfb07ab 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -167,6 +167,9 @@ static int i915_getparam(struct drm_device *dev, void *data,
>   		value = i915.enable_hangcheck &&
>   			intel_has_gpu_reset(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_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index fc434ae..9e7e182 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -391,7 +391,8 @@ static int
>   i915_gem_create(struct drm_file *file,
>   		struct drm_device *dev,
>   		uint64_t size,
> -		uint32_t *handle_p)
> +		uint32_t *handle_p,
> +		uint32_t flags)
>   {
>   	struct drm_i915_gem_object *obj;
>   	int ret;
> @@ -401,8 +402,31 @@ i915_gem_create(struct drm_file *file,
>   	if (size == 0)
>   		return -EINVAL;
>
> +	if (flags & ~(I915_CREATE_PLACEMENT_STOLEN))
> +		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);

Is the compiler not complaining that "uint64_t size" is being passed 
into "u32 size" here?

Either since there are no checks, can't userspace overflow u32 with a 
right value and get success and much smaller object than intended?

Perhaps should test with an IGT.

Regards,

Tvrtko
Chris Wilson July 22, 2015, 3:27 p.m. UTC | #2 
On Wed, Jul 22, 2015 at 04:14:37PM +0100, Tvrtko Ursulin wrote:
> 
> Hi,
> 
> On 07/22/2015 02:51 PM, 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)
> >
> >Testcase: igt/gem_stolen
> >
> >Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
> >---
> >  drivers/gpu/drm/i915/i915_dma.c |  3 +++
> >  drivers/gpu/drm/i915/i915_gem.c | 33 +++++++++++++++++++++++++++++----
> >  include/uapi/drm/i915_drm.h     | 15 +++++++++++++++
> >  3 files changed, 47 insertions(+), 4 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> >index c5349fa..bfb07ab 100644
> >--- a/drivers/gpu/drm/i915/i915_dma.c
> >+++ b/drivers/gpu/drm/i915/i915_dma.c
> >@@ -167,6 +167,9 @@ static int i915_getparam(struct drm_device *dev, void *data,
> >  		value = i915.enable_hangcheck &&
> >  			intel_has_gpu_reset(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_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >index fc434ae..9e7e182 100644
> >--- a/drivers/gpu/drm/i915/i915_gem.c
> >+++ b/drivers/gpu/drm/i915/i915_gem.c
> >@@ -391,7 +391,8 @@ static int
> >  i915_gem_create(struct drm_file *file,
> >  		struct drm_device *dev,
> >  		uint64_t size,
> >-		uint32_t *handle_p)
> >+		uint32_t *handle_p,
> >+		uint32_t flags)

out parameters go at the end, please.
-Chris
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index c5349fa..bfb07ab 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -167,6 +167,9 @@  static int i915_getparam(struct drm_device *dev, void *data,
 		value = i915.enable_hangcheck &&
 			intel_has_gpu_reset(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_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index fc434ae..9e7e182 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -391,7 +391,8 @@  static int
 i915_gem_create(struct drm_file *file,
 		struct drm_device *dev,
 		uint64_t size,
-		uint32_t *handle_p)
+		uint32_t *handle_p,
+		uint32_t flags)
 {
 	struct drm_i915_gem_object *obj;
 	int ret;
@@ -401,8 +402,31 @@  i915_gem_create(struct drm_file *file,
 	if (size == 0)
 		return -EINVAL;
 
+	if (flags & ~(I915_CREATE_PLACEMENT_STOLEN))
+		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_clear_object(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;
 
@@ -425,7 +449,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, &args->handle, 0);
 }
 
 /**
@@ -438,7 +462,8 @@  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->handle,
+			       args->flags);
 }
 
 static inline int
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index f88cc1c..f041016 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -355,6 +355,7 @@  typedef struct drm_i915_irq_wait {
 #define I915_PARAM_SUBSLICE_TOTAL	 33
 #define I915_PARAM_EU_TOTAL		 34
 #define I915_PARAM_HAS_GPU_RESET	 35
+#define I915_PARAM_CREATE_VERSION	 36
 
 typedef struct drm_i915_getparam {
 	int param;
@@ -450,6 +451,20 @@  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 */
 };
 
 struct drm_i915_gem_pread {