On Wed, Jul 01, 2015 at 04:06:49PM +0100, Tvrtko Ursulin wrote:
>
>
> On 07/01/2015 10:25 AM, 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)
> >
> >testcase: igt/gem_stolen
> >
> >Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@xxxxxxxxx>
> >---
> > 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?
But 1 is the 2nd number, discounting all those pesky negative versions :)
> > /* 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.
Paulo has been working on removing this struct_mutex requirement, in
which case internally we will take a stolen_mutex around the drm_mm as
required.
> >+ obj = i915_gem_object_create_stolen(dev, size);
> >+ if (!obj) {
> >+ mutex_unlock(&dev->struct_mutex);
> >+ return -ENOMEM;
> >+ }
> >+
And pushes the struct_mutex to here (one day, one glorious day that will
be a vm->mutex or something!).
And yes, you will want, nay must use,
ret = i915_mutex_interruptible(dev);
before thinking about using the GPU.
> >+ 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.
Userspace ABI (and kernel ABI in general) is that we do not hand back
uncleared buffers. Something to do with bank card details I guess.
So just:
/* always clear fresh buffers before handing to userspace */
An alternative is that I've been contemplating a private page pool to
reuse and not clear. It's a trade-off between having a large cache in
userspace, and a less flexible cache in the kernel with the supposed
advantage that the kernel cache could be more space efficient.
> >+ 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.
I am sure someone hacked CODING_STYLE. Or I should.
> >@@ -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. :)
Heh!, English is highly adaptible language and steals good words all
the time!
> >+ *
> >+ * 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 */
Note that we dropped the pread/pwrite restriction.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
