[PATCH] Revert "drm/i915: Disallow pin ioctl completely for kms drivers"

[PATCH] Revert "drm/i915: Disallow pin ioctl completely for kms drivers"

[Chris Wilson]

This reverts commit c211a47c2c28562f8a3fff9e027be1a3ed9e154a.

This causes an unwarranteed API break for existing and active userspace.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c | 90 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 90 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 614bc2bc16fe..4a1ca7abd7f9 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4252,6 +4252,96 @@ 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;
+
+	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;
+
+	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)
 {
-- 
2.1.3

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

Re: [PATCH] Revert "drm/i915: Disallow pin ioctl completely for kms drivers"

[Daniel Vetter]

On Tue, Nov 25, 2014 at 11:42:56AM +0000, Chris Wilson wrote:
> This reverts commit c211a47c2c28562f8a3fff9e027be1a3ed9e154a.
> 
> This causes an unwarranteed API break for existing and active userspace.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Hm, SNA still seems to be able to cope with this and I really don't see
the point of keeping this interface going and patching it up. With GEM the
kernel should be in control of shared resources, letting userspace in to
the game just leads to tears. And we have them now. Keeping pinning around
just because we've forgotten to properly disable it was ok with me, but
fixing it up when it starts to fall apart really isn't.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_gem.c | 90 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 90 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 614bc2bc16fe..4a1ca7abd7f9 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4252,6 +4252,96 @@ 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;
> +
> +	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;
> +
> +	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)
>  {
> -- 
> 2.1.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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] Revert "drm/i915: Disallow pin ioctl completely for kms drivers"

[Chris Wilson]

On Tue, Nov 25, 2014 at 01:01:39PM +0100, Daniel Vetter wrote:
> On Tue, Nov 25, 2014 at 11:42:56AM +0000, Chris Wilson wrote:
> > This reverts commit c211a47c2c28562f8a3fff9e027be1a3ed9e154a.
> > 
> > This causes an unwarranteed API break for existing and active userspace.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Hm, SNA still seems to be able to cope with this and I really don't see
> the point of keeping this interface going and patching it up. With GEM the
> kernel should be in control of shared resources, letting userspace in to
> the game just leads to tears. And we have them now. Keeping pinning around
> just because we've forgotten to properly disable it was ok with me, but
> fixing it up when it starts to fall apart really isn't.

I strongly disagree. It is a powerful tool, equivalent to mlock(), and
similar to mlock() has its uses.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] Revert "drm/i915: Disallow pin ioctl completely for kms drivers"

[Daniel Vetter]

On Tue, Nov 25, 2014 at 12:06:33PM +0000, Chris Wilson wrote:
> On Tue, Nov 25, 2014 at 01:01:39PM +0100, Daniel Vetter wrote:
> > On Tue, Nov 25, 2014 at 11:42:56AM +0000, Chris Wilson wrote:
> > > This reverts commit c211a47c2c28562f8a3fff9e027be1a3ed9e154a.
> > > 
> > > This causes an unwarranteed API break for existing and active userspace.
> > > 
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > 
> > Hm, SNA still seems to be able to cope with this and I really don't see
> > the point of keeping this interface going and patching it up. With GEM the
> > kernel should be in control of shared resources, letting userspace in to
> > the game just leads to tears. And we have them now. Keeping pinning around
> > just because we've forgotten to properly disable it was ok with me, but
> > fixing it up when it starts to fall apart really isn't.
> 
> I strongly disagree. It is a powerful tool, equivalent to mlock(), and
> similar to mlock() has its uses.

There's multiple uses for mlock:
- One is preventing swapout for security reasons, and you can do that
  already (hackishly) with mlocking the cpu mmap.
- Another is preventing unbinding from address spaces/movement across numa
  domains, to avoid minor faults overheads. Softpin to avoid relocs sounds
  like the equivalant.

But none of the mlocks allow userspace to fix stuff into a limited shared
global resource like mappable ggtt for i915. A resource which is fully
managed by the kernel (except for pinning) and which can fragment badly.
mlock memory just brings oom a bit nearer (which can be handled with
cgroups and all that), but it doesn't hit fragmentation fun of a global
resource nearby. That is the part of pin I really don't like.
-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] Revert "drm/i915: Disallow pin ioctl completely for kms drivers"

[Chris Wilson]

On Tue, Nov 25, 2014 at 02:34:02PM +0100, Daniel Vetter wrote:
> On Tue, Nov 25, 2014 at 12:06:33PM +0000, Chris Wilson wrote:
> > On Tue, Nov 25, 2014 at 01:01:39PM +0100, Daniel Vetter wrote:
> > > On Tue, Nov 25, 2014 at 11:42:56AM +0000, Chris Wilson wrote:
> > > > This reverts commit c211a47c2c28562f8a3fff9e027be1a3ed9e154a.
> > > > 
> > > > This causes an unwarranteed API break for existing and active userspace.
> > > > 
> > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > 
> > > Hm, SNA still seems to be able to cope with this and I really don't see
> > > the point of keeping this interface going and patching it up. With GEM the
> > > kernel should be in control of shared resources, letting userspace in to
> > > the game just leads to tears. And we have them now. Keeping pinning around
> > > just because we've forgotten to properly disable it was ok with me, but
> > > fixing it up when it starts to fall apart really isn't.
> > 
> > I strongly disagree. It is a powerful tool, equivalent to mlock(), and
> > similar to mlock() has its uses.
> 
> There's multiple uses for mlock:
> - One is preventing swapout for security reasons, and you can do that
>   already (hackishly) with mlocking the cpu mmap.
> - Another is preventing unbinding from address spaces/movement across numa
>   domains, to avoid minor faults overheads. Softpin to avoid relocs sounds
>   like the equivalant.
> 
> But none of the mlocks allow userspace to fix stuff into a limited shared
> global resource like mappable ggtt for i915. 

Pardon? What exactly about physical memory isn't a limited shared global
resource? That's exactly why mlock() is privileged.

> A resource which is fully
> managed by the kernel (except for pinning) and which can fragment badly.
> mlock memory just brings oom a bit nearer (which can be handled with
> cgroups and all that), but it doesn't hit fragmentation fun of a global
> resource nearby. That is the part of pin I really don't like.

Indeed, that's exactly what I like about it. Locked memory that can't be
lost due to insane fragmentation.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] Revert "drm/i915: Disallow pin ioctl completely for kms drivers"

[Daniel Vetter]

On Tue, Nov 25, 2014 at 08:47:10PM +0000, Chris Wilson wrote:
> On Tue, Nov 25, 2014 at 02:34:02PM +0100, Daniel Vetter wrote:
> > On Tue, Nov 25, 2014 at 12:06:33PM +0000, Chris Wilson wrote:
> > > On Tue, Nov 25, 2014 at 01:01:39PM +0100, Daniel Vetter wrote:
> > > > On Tue, Nov 25, 2014 at 11:42:56AM +0000, Chris Wilson wrote:
> > > > > This reverts commit c211a47c2c28562f8a3fff9e027be1a3ed9e154a.
> > > > > 
> > > > > This causes an unwarranteed API break for existing and active userspace.
> > > > > 
> > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > 
> > > > Hm, SNA still seems to be able to cope with this and I really don't see
> > > > the point of keeping this interface going and patching it up. With GEM the
> > > > kernel should be in control of shared resources, letting userspace in to
> > > > the game just leads to tears. And we have them now. Keeping pinning around
> > > > just because we've forgotten to properly disable it was ok with me, but
> > > > fixing it up when it starts to fall apart really isn't.
> > > 
> > > I strongly disagree. It is a powerful tool, equivalent to mlock(), and
> > > similar to mlock() has its uses.
> > 
> > There's multiple uses for mlock:
> > - One is preventing swapout for security reasons, and you can do that
> >   already (hackishly) with mlocking the cpu mmap.
> > - Another is preventing unbinding from address spaces/movement across numa
> >   domains, to avoid minor faults overheads. Softpin to avoid relocs sounds
> >   like the equivalant.
> > 
> > But none of the mlocks allow userspace to fix stuff into a limited shared
> > global resource like mappable ggtt for i915. 
> 
> Pardon? What exactly about physical memory isn't a limited shared global
> resource? That's exactly why mlock() is privileged.

It doesn't fragment so badly. And we have cgroups to keep stuff in checks.

> > A resource which is fully
> > managed by the kernel (except for pinning) and which can fragment badly.
> > mlock memory just brings oom a bit nearer (which can be handled with
> > cgroups and all that), but it doesn't hit fragmentation fun of a global
> > resource nearby. That is the part of pin I really don't like.
> 
> Indeed, that's exactly what I like about it. Locked memory that can't be
> lost due to insane fragmentation.

Well SNA isn't the only thing in town, which is why the kernel should manage
this. Otherwise we could just do userspace managed memory without
eviction for everyone, that way you're guaranteed to never attempt
anything which could fail.
-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] Revert "drm/i915: Disallow pin ioctl completely for kms drivers"

[Jani Nikula]

On Tue, 25 Nov 2014, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> This reverts commit c211a47c2c28562f8a3fff9e027be1a3ed9e154a.
>
> This causes an unwarranteed API break for existing and active userspace.

Ville had an interesting observation regarding the commit being
reverted: https://bugs.freedesktop.org/show_bug.cgi?id=86679#c9

BR,
Jani.


>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 90 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 90 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 614bc2bc16fe..4a1ca7abd7f9 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4252,6 +4252,96 @@ 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;
> +
> +	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;
> +
> +	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)
>  {
> -- 
> 2.1.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] Revert "drm/i915: Disallow pin ioctl completely for kms drivers"

[Chris Wilson]

On Thu, Nov 27, 2014 at 03:56:38PM +0200, Jani Nikula wrote:
> On Tue, 25 Nov 2014, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > This reverts commit c211a47c2c28562f8a3fff9e027be1a3ed9e154a.
> >
> > This causes an unwarranteed API break for existing and active userspace.
> 
> Ville had an interesting observation regarding the commit being
> reverted: https://bugs.freedesktop.org/show_bug.cgi?id=86679#c9

Not really, it is just a hack to workaround the equally gross hack

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

which is to work around a hw bug on gen7+.

The fix would have either to been to also observe the bias inside
pin_ioctl.
-Chris
 

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx