| Message ID | 1416915776-20730-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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
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
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
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) {
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(+)