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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 07/01/2015 10:25 AM, 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)
>
> 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 | 31 +++++++++++++++++++++++++++----
>   include/uapi/drm/i915_drm.h     | 15 +++++++++++++++
>   3 files changed, 45 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index c5349fa..6045749 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 = 1;

Shouldn't it be 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 a2a4a27..4acf331 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,29 @@ 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);

Probably need the interruptible variant so userspace can Ctrl-C if 
things get stuck in submission/waiting.

> +		obj = i915_gem_object_create_stolen(dev, size);
> +		if (!obj) {
> +			mutex_unlock(&dev->struct_mutex);
> +			return -ENOMEM;
> +		}
> +
> +		ret = i915_gem_exec_clear_object(obj, file->driver_priv);

I would put a comment here saying why it is important to clear stolen 
memory.

> +		if (ret) {
> +			i915_gem_object_free(obj);

This should probably be drm_gem_object_unreference.

> +			mutex_unlock(&dev->struct_mutex);
> +			return ret;
> +		}
> +
> +		mutex_unlock(&dev->struct_mutex);
> +	} else
> +		obj = i915_gem_alloc_object(dev, size);

Need curly braces on both branches.

>   	if (obj == NULL)
>   		return -ENOMEM;
>
> @@ -425,7 +447,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 +460,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..87992d1 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).

I'd just use English all the way. :)

> +	 *
> +	 * 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 or pread/pwrite */
>   };
>
>   struct drm_i915_gem_pread {
>

Regards,

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