[PATCH 1/2] drm/i915: Disallow pin ioctl completely for kms drivers

[PATCH 1/2] drm/i915: Disallow pin ioctl completely for kms drivers

[Daniel Vetter]

The problem here is that SNA pins batchbuffers to etch out a bit more
performance. Iirc it started out as a w/a for i830M (which we've
implemented in the kernel since a long time already). The problem is
that the pin ioctl wasn't added in

commit d23db88c3ab233daed18709e3a24d6c95344117f
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Fri May 23 08:48:08 2014 +0200

    drm/i915: Prevent negative relocation deltas from wrapping

Fix this by simply disallowing pinning from userspace so that the
kernel is in full control of batch placement again. Especially since
distros are moving towards running X as non-root, so most users won't
even be able to see any benefits.

UMS support is dead now, but we need this minimal patch for
backporting. Follow-up patch will remove the pin ioctl code
completely.

References: https://bugs.freedesktop.org/show_bug.cgi?id=76554#c116
Cc: stable@vger.kernel.org
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 3c64eb6abf2d..fb5ce2b89aba 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4245,7 +4245,7 @@ i915_gem_pin_ioctl(struct drm_device *dev, void *data,
 	struct drm_i915_gem_object *obj;
 	int ret;
 
-	if (INTEL_INFO(dev)->gen >= 6)
+	if (drm_core_check_feature(dev, DRIVER_MODESET))
 		return -ENODEV;
 
 	ret = i915_mutex_lock_interruptible(dev);
@@ -4301,6 +4301,9 @@ i915_gem_unpin_ioctl(struct drm_device *dev, void *data,
 	struct drm_i915_gem_object *obj;
 	int ret;
 
+	if (drm_core_check_feature(dev, DRIVER_MODESET))
+		return -ENODEV;
+
 	ret = i915_mutex_lock_interruptible(dev);
 	if (ret)
 		return ret;
-- 
2.1.1

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

[PATCH 2/2] drm/i915: Remove user pinning code

[Daniel Vetter]

Now unused.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c   |   4 +-
 drivers/gpu/drm/i915/i915_dma.c       |  11 +++-
 drivers/gpu/drm/i915/i915_drv.h       |   8 ---
 drivers/gpu/drm/i915/i915_gem.c       | 106 +---------------------------------
 drivers/gpu/drm/i915/i915_gem_gtt.h   |   9 ++-
 drivers/gpu/drm/i915/i915_gpu_error.c |   2 -
 6 files changed, 17 insertions(+), 123 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index b4f23a8f7779..b50458360a2d 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -96,9 +96,7 @@ static int i915_capabilities(struct seq_file *m, void *data)
 
 static const char *get_pin_flag(struct drm_i915_gem_object *obj)
 {
-	if (obj->user_pin_count > 0)
-		return "P";
-	else if (i915_gem_obj_is_pinned(obj))
+	if (i915_gem_obj_is_pinned(obj))
 		return "p";
 	else
 		return " ";
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index ecee3bcc8772..887d88f4c688 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1004,6 +1004,13 @@ void i915_driver_postclose(struct drm_device *dev, struct drm_file *file)
 	kfree(file_priv);
 }
 
+static int
+i915_gem_reject_pin_ioctl(struct drm_device *dev, void *data,
+			  struct drm_file *file)
+{
+	return -ENODEV;
+}
+
 const struct drm_ioctl_desc i915_ioctls[] = {
 	DRM_IOCTL_DEF_DRV(I915_INIT, drm_noop, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
 	DRM_IOCTL_DEF_DRV(I915_FLUSH, drm_noop, DRM_AUTH),
@@ -1025,8 +1032,8 @@ const struct drm_ioctl_desc i915_ioctls[] = {
 	DRM_IOCTL_DEF_DRV(I915_GEM_INIT, drm_noop, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY|DRM_UNLOCKED),
 	DRM_IOCTL_DEF_DRV(I915_GEM_EXECBUFFER, i915_gem_execbuffer, DRM_AUTH|DRM_UNLOCKED),
 	DRM_IOCTL_DEF_DRV(I915_GEM_EXECBUFFER2, i915_gem_execbuffer2, DRM_AUTH|DRM_UNLOCKED|DRM_RENDER_ALLOW),
-	DRM_IOCTL_DEF_DRV(I915_GEM_PIN, i915_gem_pin_ioctl, DRM_AUTH|DRM_ROOT_ONLY|DRM_UNLOCKED),
-	DRM_IOCTL_DEF_DRV(I915_GEM_UNPIN, i915_gem_unpin_ioctl, DRM_AUTH|DRM_ROOT_ONLY|DRM_UNLOCKED),
+	DRM_IOCTL_DEF_DRV(I915_GEM_PIN, i915_gem_reject_pin_ioctl, DRM_AUTH|DRM_ROOT_ONLY|DRM_UNLOCKED),
+	DRM_IOCTL_DEF_DRV(I915_GEM_UNPIN, i915_gem_reject_pin_ioctl, DRM_AUTH|DRM_ROOT_ONLY|DRM_UNLOCKED),
 	DRM_IOCTL_DEF_DRV(I915_GEM_BUSY, i915_gem_busy_ioctl, DRM_AUTH|DRM_UNLOCKED|DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF_DRV(I915_GEM_SET_CACHING, i915_gem_set_caching_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF_DRV(I915_GEM_GET_CACHING, i915_gem_get_caching_ioctl, DRM_UNLOCKED|DRM_RENDER_ALLOW),
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e67ce6157c63..bfac5b1fdd5b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1958,10 +1958,6 @@ struct drm_i915_gem_object {
 	/** Record of address bit 17 of each page at last unbind. */
 	unsigned long *bit_17;
 
-	/** User space pin count and filp owning the pin */
-	unsigned long user_pin_count;
-	struct drm_file *pin_filp;
-
 	union {
 		/** for phy allocated objects */
 		struct drm_dma_handle *phys_handle;
@@ -2428,10 +2424,6 @@ int i915_gem_execbuffer(struct drm_device *dev, void *data,
 			struct drm_file *file_priv);
 int i915_gem_execbuffer2(struct drm_device *dev, void *data,
 			 struct drm_file *file_priv);
-int i915_gem_pin_ioctl(struct drm_device *dev, void *data,
-		       struct drm_file *file_priv);
-int i915_gem_unpin_ioctl(struct drm_device *dev, void *data,
-			 struct drm_file *file_priv);
 int i915_gem_busy_ioctl(struct drm_device *dev, void *data,
 			struct drm_file *file_priv);
 int i915_gem_get_caching_ioctl(struct drm_device *dev, void *data,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index fb5ce2b89aba..ef40b040f9f5 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3883,18 +3883,14 @@ static bool is_pin_display(struct drm_i915_gem_object *obj)
 	if (!vma)
 		return false;
 
-	/* There are 3 sources that pin objects:
+	/* There are 2 sources that pin objects:
 	 *   1. The display engine (scanouts, sprites, cursors);
 	 *   2. Reservations for execbuffer;
-	 *   3. The user.
 	 *
 	 * We can ignore reservations as we hold the struct_mutex and
-	 * are only called outside of the reservation path.  The user
-	 * can only increment pin_count once, and so if after
-	 * subtracting the potential reference by the user, any pin_count
-	 * remains, it must be due to another use by the display engine.
+	 * are only called outside of the reservation path.
 	 */
-	return vma->pin_count - !!obj->user_pin_count;
+	return vma->pin_count;
 }
 
 /*
@@ -4238,102 +4234,6 @@ i915_gem_object_unpin_fence(struct drm_i915_gem_object *obj)
 }
 
 int
-i915_gem_pin_ioctl(struct drm_device *dev, void *data,
-		   struct drm_file *file)
-{
-	struct drm_i915_gem_pin *args = data;
-	struct drm_i915_gem_object *obj;
-	int ret;
-
-	if (drm_core_check_feature(dev, DRIVER_MODESET))
-		return -ENODEV;
-
-	ret = i915_mutex_lock_interruptible(dev);
-	if (ret)
-		return ret;
-
-	obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle));
-	if (&obj->base == NULL) {
-		ret = -ENOENT;
-		goto unlock;
-	}
-
-	if (obj->madv != I915_MADV_WILLNEED) {
-		DRM_DEBUG("Attempting to pin a purgeable buffer\n");
-		ret = -EFAULT;
-		goto out;
-	}
-
-	if (obj->pin_filp != NULL && obj->pin_filp != file) {
-		DRM_DEBUG("Already pinned in i915_gem_pin_ioctl(): %d\n",
-			  args->handle);
-		ret = -EINVAL;
-		goto out;
-	}
-
-	if (obj->user_pin_count == ULONG_MAX) {
-		ret = -EBUSY;
-		goto out;
-	}
-
-	if (obj->user_pin_count == 0) {
-		ret = i915_gem_obj_ggtt_pin(obj, args->alignment, PIN_MAPPABLE);
-		if (ret)
-			goto out;
-	}
-
-	obj->user_pin_count++;
-	obj->pin_filp = file;
-
-	args->offset = i915_gem_obj_ggtt_offset(obj);
-out:
-	drm_gem_object_unreference(&obj->base);
-unlock:
-	mutex_unlock(&dev->struct_mutex);
-	return ret;
-}
-
-int
-i915_gem_unpin_ioctl(struct drm_device *dev, void *data,
-		     struct drm_file *file)
-{
-	struct drm_i915_gem_pin *args = data;
-	struct drm_i915_gem_object *obj;
-	int ret;
-
-	if (drm_core_check_feature(dev, DRIVER_MODESET))
-		return -ENODEV;
-
-	ret = i915_mutex_lock_interruptible(dev);
-	if (ret)
-		return ret;
-
-	obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle));
-	if (&obj->base == NULL) {
-		ret = -ENOENT;
-		goto unlock;
-	}
-
-	if (obj->pin_filp != file) {
-		DRM_DEBUG("Not pinned by caller in i915_gem_pin_ioctl(): %d\n",
-			  args->handle);
-		ret = -EINVAL;
-		goto out;
-	}
-	obj->user_pin_count--;
-	if (obj->user_pin_count == 0) {
-		obj->pin_filp = NULL;
-		i915_gem_object_ggtt_unpin(obj);
-	}
-
-out:
-	drm_gem_object_unreference(&obj->base);
-unlock:
-	mutex_unlock(&dev->struct_mutex);
-	return ret;
-}
-
-int
 i915_gem_busy_ioctl(struct drm_device *dev, void *data,
 		    struct drm_file *file)
 {
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index beaf4bcfdac8..92db6654f93b 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -146,11 +146,10 @@ struct i915_vma {
 
 	/**
 	 * How many users have pinned this object in GTT space. The following
-	 * users can each hold at most one reference: pwrite/pread, pin_ioctl
-	 * (via user_pin_count), execbuffer (objects are not allowed multiple
-	 * times for the same batchbuffer), and the framebuffer code. When
-	 * switching/pageflipping, the framebuffer code has at most two buffers
-	 * pinned per crtc.
+	 * users can each hold at most one reference: pwrite/pread, execbuffer
+	 * (objects are not allowed multiple times for the same batchbuffer),
+	 * and the framebuffer code. When switching/pageflipping, the
+	 * framebuffer code has at most two buffers pinned per crtc.
 	 *
 	 * In the worst case this is 1 + 1 + 1 + 2*2 = 7. That would fit into 3
 	 * bits with absolutely no headroom. So use 4 bits. */
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index cdaee6ce05f8..eea98d567431 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -679,8 +679,6 @@ static void capture_bo(struct drm_i915_error_buffer *err,
 	err->pinned = 0;
 	if (i915_gem_obj_is_pinned(obj))
 		err->pinned = 1;
-	if (obj->user_pin_count > 0)
-		err->pinned = -1;
 	err->tiling = obj->tiling_mode;
 	err->dirty = obj->dirty;
 	err->purgeable = obj->madv != I915_MADV_WILLNEED;
-- 
2.1.1

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

Re: [PATCH 1/2] drm/i915: Disallow pin ioctl completely for kms drivers

[Chris Wilson]

On Mon, Nov 24, 2014 at 11:30:24AM +0100, Daniel Vetter wrote:
> The problem here is that SNA pins batchbuffers to etch out a bit more
> performance. Iirc it started out as a w/a for i830M (which we've
> implemented in the kernel since a long time already).

Hmm, we only finally fixed it in the kernel a couple of months ago.

> The problem is
> that the pin ioctl wasn't added in
> 
> commit d23db88c3ab233daed18709e3a24d6c95344117f
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Fri May 23 08:48:08 2014 +0200
> 
>     drm/i915: Prevent negative relocation deltas from wrapping
> 
> Fix this by simply disallowing pinning from userspace so that the
> kernel is in full control of batch placement again. Especially since
> distros are moving towards running X as non-root, so most users won't
> even be able to see any benefits.

Pinning is a useful tool and it would also be useful to have again on
gen6+.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm/i915: Disallow pin ioctl completely for kms drivers

[Daniel Vetter]

On Mon, Nov 24, 2014 at 10:35:29AM +0000, Chris Wilson wrote:
> On Mon, Nov 24, 2014 at 11:30:24AM +0100, Daniel Vetter wrote:
> > The problem here is that SNA pins batchbuffers to etch out a bit more
> > performance. Iirc it started out as a w/a for i830M (which we've
> > implemented in the kernel since a long time already).
> 
> Hmm, we only finally fixed it in the kernel a couple of months ago.

The original wa hack has landed in 3.8:

commit b45305fce5bb1abec263fcff9d81ebecd6306ede
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Mon Dec 17 16:21:27 2012 +0100

    drm/i915: Implement workaround for broken CS tlb on i830/845

The follow-up fix from you was cc: stable:

commit c4d69da167fa967749aeb70bc0e94a457e5d00c1
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Mon Sep 8 14:25:41 2014 +0100

    drm/i915: Evict CS TLBs between batches

So as long as people backport all of this and don't try to run it on 3.8
we should be fine I think wrt backwards/forwards compat. I'll add this to
the commit message.

> > The problem is
> > that the pin ioctl wasn't added in
> > 
> > commit d23db88c3ab233daed18709e3a24d6c95344117f
> > Author: Chris Wilson <chris@chris-wilson.co.uk>
> > Date:   Fri May 23 08:48:08 2014 +0200
> > 
> >     drm/i915: Prevent negative relocation deltas from wrapping
> > 
> > Fix this by simply disallowing pinning from userspace so that the
> > kernel is in full control of batch placement again. Especially since
> > distros are moving towards running X as non-root, so most users won't
> > even be able to see any benefits.
> 
> Pinning is a useful tool and it would also be useful to have again on
> gen6+.

I think softpin or similar is doable with ppgtt. But with shared ggtt I'm
not really enthusiastic about providing this kind of rope to userspace.
And softpin is a different type of pinning, so we don't really lose
anything by ripping out the userspace hard pinning ioctls.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Disallow pin ioctl completely for kms drivers

[Chris Wilson]

On Mon, Nov 24, 2014 at 03:10:05PM +0100, Daniel Vetter wrote:
> On Mon, Nov 24, 2014 at 10:35:29AM +0000, Chris Wilson wrote:
> > On Mon, Nov 24, 2014 at 11:30:24AM +0100, Daniel Vetter wrote:
> > > The problem here is that SNA pins batchbuffers to etch out a bit more
> > > performance. Iirc it started out as a w/a for i830M (which we've
> > > implemented in the kernel since a long time already).
> > 
> > Hmm, we only finally fixed it in the kernel a couple of months ago.
> 
> The original wa hack has landed in 3.8:
> 
> commit b45305fce5bb1abec263fcff9d81ebecd6306ede
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Mon Dec 17 16:21:27 2012 +0100
> 
>     drm/i915: Implement workaround for broken CS tlb on i830/845
> 
> The follow-up fix from you was cc: stable:
> 
> commit c4d69da167fa967749aeb70bc0e94a457e5d00c1
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Mon Sep 8 14:25:41 2014 +0100
> 
>     drm/i915: Evict CS TLBs between batches
> 
> So as long as people backport all of this and don't try to run it on 3.8
> we should be fine I think wrt backwards/forwards compat. I'll add this to
> the commit message.

You should also mention that the kernel w/a is inferior to the userspace
w/a to tune of 10-20%.
 
> > > The problem is
> > > that the pin ioctl wasn't added in
> > > 
> > > commit d23db88c3ab233daed18709e3a24d6c95344117f
> > > Author: Chris Wilson <chris@chris-wilson.co.uk>
> > > Date:   Fri May 23 08:48:08 2014 +0200
> > > 
> > >     drm/i915: Prevent negative relocation deltas from wrapping
> > > 
> > > Fix this by simply disallowing pinning from userspace so that the
> > > kernel is in full control of batch placement again. Especially since
> > > distros are moving towards running X as non-root, so most users won't
> > > even be able to see any benefits.
> > 
> > Pinning is a useful tool and it would also be useful to have again on
> > gen6+.
> 
> I think softpin or similar is doable with ppgtt. But with shared ggtt I'm
> not really enthusiastic about providing this kind of rope to userspace.
> And softpin is a different type of pinning, so we don't really lose
> anything by ripping out the userspace hard pinning ioctls.

I am not talking about softpin, but being able to pin memory and a GGTT
binding itself is useful.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/2] drm/i915: Remove user pinning code

[shuang.he]

Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                 -2              367/367              365/367
ILK                 -3              375/375              372/375
SNB                                  450/450              450/450
IVB                 -2              503/503              501/503
BYT                                  289/289              289/289
HSW                 -3              567/567              564/567
BDW                 -1              417/417              416/417
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
PNV  igt_gem_pin      PASS(2, M23)      NSPT(1, M23)
PNV  igt_gen3_mixed_blits      PASS(5, M23)      CRASH(1, M23)
ILK  igt_gem_pin      PASS(2, M37)      NSPT(1, M37)
ILK  igt_gem_reset_stats_close-pending-fork-render      TIMEOUT(6, M37M26)PASS(1, M26)      TIMEOUT(1, M37)
ILK  igt_kms_flip_vblank-vs-hang      TIMEOUT(6, M37M26)PASS(1, M26)      TIMEOUT(1, M37)
IVB  igt_gem_bad_reloc_negative-reloc      NSPT(9, M34M21M4)PASS(1, M21)      NSPT(1, M34)
IVB  igt_gem_bad_reloc_negative-reloc-lut      NSPT(3, M21M34M4)PASS(10, M21M34M4)      NSPT(1, M34)
HSW  igt_gem_bad_reloc_negative-reloc-lut      NSPT(18, M40M20M19)PASS(1, M20)      NSPT(1, M19)
HSW  igt_kms_rotation_crc_primary-rotation      PASS(18, M20M40M19)      DMESG_WARN(1, M19)
HSW  igt_pm_rc6_residency_rc6-accuracy      PASS(19, M20M40M19)      FAIL(1, M19)
BDW  igt_gem_render_linear_blits      TIMEOUT(1, M30)PASS(1, M28)      TIMEOUT(1, M30)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/2] drm/i915: Disallow pin ioctl completely for kms drivers

[Jesse Barnes]

On 11/24/2014 06:13 AM, Chris Wilson wrote:
> On Mon, Nov 24, 2014 at 03:10:05PM +0100, Daniel Vetter wrote:
>> On Mon, Nov 24, 2014 at 10:35:29AM +0000, Chris Wilson wrote:
>>> Pinning is a useful tool and it would also be useful to have again on
>>> gen6+.
>>
>> I think softpin or similar is doable with ppgtt. But with shared ggtt I'm
>> not really enthusiastic about providing this kind of rope to userspace.
>> And softpin is a different type of pinning, so we don't really lose
>> anything by ripping out the userspace hard pinning ioctls.
> 
> I am not talking about softpin, but being able to pin memory and a GGTT
> binding itself is useful.

I see you merged this over Chris's objections and then shot down his
revert.  I'm not clear on why you're so opposed to the pin ioctl?  It's
a privileged op and definitely has its uses as Chris has repeatedly
pointed out.

Why so insistent on dropping this particular ioctl?  Do you see it
causing actual problems?  Or do you just like preventing userspace from
doing things you don't agree with?

(Sorry, catching up on ancient backlog from intel-gfx, so maybe there's
a thread I missed when re-looking at this.  If so, please point me at it.)

Thanks,
Jesse

Re: [Intel-gfx] [PATCH 1/2] drm/i915: Disallow pin ioctl completely for kms drivers

[Daniel Vetter]

On Mon, Feb 23, 2015 at 01:29:57PM -0800, Jesse Barnes wrote:
> On 11/24/2014 06:13 AM, Chris Wilson wrote:
> > On Mon, Nov 24, 2014 at 03:10:05PM +0100, Daniel Vetter wrote:
> >> On Mon, Nov 24, 2014 at 10:35:29AM +0000, Chris Wilson wrote:
> >>> Pinning is a useful tool and it would also be useful to have again on
> >>> gen6+.
> >>
> >> I think softpin or similar is doable with ppgtt. But with shared ggtt I'm
> >> not really enthusiastic about providing this kind of rope to userspace.
> >> And softpin is a different type of pinning, so we don't really lose
> >> anything by ripping out the userspace hard pinning ioctls.
> > 
> > I am not talking about softpin, but being able to pin memory and a GGTT
> > binding itself is useful.
> 
> I see you merged this over Chris's objections and then shot down his
> revert.  I'm not clear on why you're so opposed to the pin ioctl?  It's
> a privileged op and definitely has its uses as Chris has repeatedly
> pointed out.
> 
> Why so insistent on dropping this particular ioctl?  Do you see it
> causing actual problems?  Or do you just like preventing userspace from
> doing things you don't agree with?
> 
> (Sorry, catching up on ancient backlog from intel-gfx, so maybe there's
> a thread I missed when re-looking at this.  If so, please point me at it.)

People are way too happy to abuse it instead of using dma-buf. And at
least some of the uses sna has also cause a bunch of problems with being a
bit too clever around reloc handling (so we essentially _have_ to take the
toys away since giving it back would cause regressions).

If there's a real users then we can look at this again imo, but I think
most things are better solved with proper kernel interfaces since in the
end the kernel does mm for the gpu, and if userspace interferes we can't
do that.

So overall my answer is:
- re-enable will cause regressions
- I don't see a justified user
- we should never have allowed this with kms to begin with, it was an
  oversight.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [Intel-gfx] [PATCH 1/2] drm/i915: Disallow pin ioctl completely for kms drivers

[Jesse Barnes]

On 02/23/2015 03:40 PM, Daniel Vetter wrote:
> On Mon, Feb 23, 2015 at 01:29:57PM -0800, Jesse Barnes wrote:
>> On 11/24/2014 06:13 AM, Chris Wilson wrote:
>>> On Mon, Nov 24, 2014 at 03:10:05PM +0100, Daniel Vetter wrote:
>>>> On Mon, Nov 24, 2014 at 10:35:29AM +0000, Chris Wilson wrote:
>>>>> Pinning is a useful tool and it would also be useful to have again on
>>>>> gen6+.
>>>>
>>>> I think softpin or similar is doable with ppgtt. But with shared ggtt I'm
>>>> not really enthusiastic about providing this kind of rope to userspace.
>>>> And softpin is a different type of pinning, so we don't really lose
>>>> anything by ripping out the userspace hard pinning ioctls.
>>>
>>> I am not talking about softpin, but being able to pin memory and a GGTT
>>> binding itself is useful.
>>
>> I see you merged this over Chris's objections and then shot down his
>> revert.  I'm not clear on why you're so opposed to the pin ioctl?  It's
>> a privileged op and definitely has its uses as Chris has repeatedly
>> pointed out.
>>
>> Why so insistent on dropping this particular ioctl?  Do you see it
>> causing actual problems?  Or do you just like preventing userspace from
>> doing things you don't agree with?
>>
>> (Sorry, catching up on ancient backlog from intel-gfx, so maybe there's
>> a thread I missed when re-looking at this.  If so, please point me at it.)
> 
> People are way too happy to abuse it instead of using dma-buf. And at
> least some of the uses sna has also cause a bunch of problems with being a
> bit too clever around reloc handling (so we essentially _have_ to take the
> toys away since giving it back would cause regressions).

Some interfaces are more dangerous than others.  But that doesn't mean
they're necessarily bad.

> If there's a real users then we can look at this again imo, but I think
> most things are better solved with proper kernel interfaces since in the
> end the kernel does mm for the gpu, and if userspace interferes we can't
> do that.
> 
> So overall my answer is:
> - re-enable will cause regressions

Which regressions?  In SNA?  It sounded like Chris was the one
requesting this here.  And really, dropping pin altogether was a big
regression in the ABI to begin with and probably shouldn't have been
allowed (the one back in 2013; I think both Chris and Ben objected back
then too).

> - I don't see a justified user

What about SNA?  What about debug?  Yes there's an alternative in the
SNA case, but Chris mentioned it had a huge perf hit.  And fwiw the
Beignet team is using this too, so at the very least it needs to work on
aliasing PPGTT on gen7/7.5.

> - we should never have allowed this with kms to begin with, it was an
>   oversight.

Not sure about that; as Chris mentioned, mlock() has uses too.  It needs
to be limited, obviously, and can cause trouble if you're not careful.
But that's not a reason to disallow it or remove it altogether.

Anyway, the patches have no r-bs or acks, only nacks going back to gen6,
and you're still merging these.  That's what's not sitting well with me.

Jesse