| Message ID | 1449665182-10054-3-git-send-email-ankitprasad.r.sharma@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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? > }; > > struct drm_i915_gem_pread { > Regards, Tvrtko
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
On 11/12/15 11:22, Ankitprasad Sharma wrote: > 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. Or perhaps it would be OK to just fail the execbuf? Just thinking to simplify things. Is it required (or expected) that users will need or want to create batch buffers from stolen? Regards, Tvrtko
On 11/12/15 12:19, Tvrtko Ursulin wrote: > > On 11/12/15 11:22, Ankitprasad Sharma wrote: >> 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> >>>> [snip!] >>>> + /** >>>> + * 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. > > Or perhaps it would be OK to just fail the execbuf? > > Just thinking to simplify things. Is it required (or expected) that > users will need or want to create batch buffers from stolen? > > Regards, > Tvrtko Let's NOT have batchbuffers in stolen. Or anywhere else exotic, just in regular shmfs-backed GEM objects (no phys, userptr, or dma_buf either). And I'd rather contexts and ringbuffers weren't placed there either, because the CPU needs to write those all the time. All special-purpose GEM objects should be usable ONLY as data buffers for the GPU, or for CPU access with pread/pwrite. The objects that the kernel needs to understand and manipulate (contexts, ringbuffers, and batches) should always be default (shmfs-backed) GEM objects, so that we don't have to propagate the understanding of all the exceptional cases into a multitude of different kernel functions. Oh, and I'd suggest that once we have more than two GEM object types, the pread/pwrite operations should be extracted and turned into vfuncs rather than adding complexity to the common ioctl/shmfs path. .Dave.
On Fri, Dec 11, 2015 at 12:49:37PM +0000, Dave Gordon wrote: > On 11/12/15 12:19, Tvrtko Ursulin wrote: > > > >On 11/12/15 11:22, Ankitprasad Sharma wrote: > >>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> > >>>> > [snip!] > >>>>+ /** > >>>>+ * 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. > > > >Or perhaps it would be OK to just fail the execbuf? > > > >Just thinking to simplify things. Is it required (or expected) that > >users will need or want to create batch buffers from stolen? > > > >Regards, > >Tvrtko > > Let's NOT have batchbuffers in stolen. Or anywhere else exotic, just in > regular shmfs-backed GEM objects (no phys, userptr, or dma_buf either). > And I'd rather contexts and ringbuffers weren't placed there either, because > the CPU needs to write those all the time. All special-purpose GEM objects > should be usable ONLY as data buffers for the GPU, or for CPU access with > pread/pwrite. The objects that the kernel needs to understand and manipulate > (contexts, ringbuffers, and batches) should always be default (shmfs-backed) > GEM objects, so that we don't have to propagate the understanding of all the > exceptional cases into a multitude of different kernel functions. Yeah, rejecting stolen batches makes sense I'd say. > Oh, and I'd suggest that once we have more than two GEM object types, the > pread/pwrite operations should be extracted and turned into vfuncs rather > than adding complexity to the common ioctl/shmfs path. While we discuss clenups around obj backing storage abstraction: Another thing worth considering is completing our extraction of the different types of obj into files: We already have dma-buf, stolen, userptr, and could extract shmem and phys_obj. Then pull them all together into a section about gem backing storage types in the docbook. Should at least allow the next person to see through this maze without first reading a few thousand git commits ;-) -Daniel
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) }; struct drm_i915_gem_pread {