On Wed, 2015-12-09 at 14:06 +0000, Tvrtko Ursulin wrote:
> Hi,
>
> On 09/12/15 12:46, ankitprasad.r.sharma@xxxxxxxxx wrote:
> > From: Ankitprasad Sharma <ankitprasad.r.sharma@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
