| Message ID | 1442306007-10792-3-git-send-email-ankitprasad.r.sharma@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
On 09/15/2015 09:33 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) > > 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) > > 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_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 2193cc2..8319e07 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 8db905a..e6ef083 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -3159,7 +3159,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); This leave the 32-bit size argument to i915_pages_create_for_stolen(), but that is only a theoretical problem until larger than 4GB stolen setup appears. :) > 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 94533bf..6568a7f 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_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; > > @@ -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 a36cb95..99f2bce 100644 > --- a/drivers/gpu/drm/i915/i915_gem_stolen.c > +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c > @@ -457,7 +457,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; > @@ -467,7 +467,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 dbd16a2..f71f75c 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) > }; > > struct drm_i915_gem_pread { > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Regards, Tvrtko
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: @@ -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
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
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c index 2193cc2..8319e07 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 8db905a..e6ef083 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -3159,7 +3159,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 94533bf..6568a7f 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_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; @@ -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 a36cb95..99f2bce 100644 --- a/drivers/gpu/drm/i915/i915_gem_stolen.c +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c @@ -457,7 +457,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; @@ -467,7 +467,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 dbd16a2..f71f75c 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) }; struct drm_i915_gem_pread {