| Message ID | 20201016104444.1492028-15-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Remove obj->mm.lock! | expand |
On 10/16/20 12:43 PM, Maarten Lankhorst wrote: > We should not allow this any more, as it will break with the new userptr > implementation, it could still be made to work, but there's no point in > doing so. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Ifdefs don't appear consistent with the commit message. Wrong patch or separate patch? Also please add a discussion what impact this has on existing user-space.
Op 30-10-2020 om 10:26 schreef Thomas Hellström (Intel): > > On 10/16/20 12:43 PM, Maarten Lankhorst wrote: >> We should not allow this any more, as it will break with the new userptr >> implementation, it could still be made to work, but there's no point in >> doing so. >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Ifdefs don't appear consistent with the commit message. Wrong patch or separate patch? > > Also please add a discussion what impact this has on existing user-space. > > Impact should be low, xf86-video-intel with SNA was using unsynchronized, but will fall back to synchronized if not available.
Op 30-10-2020 om 10:26 schreef Thomas Hellström (Intel): > > On 10/16/20 12:43 PM, Maarten Lankhorst wrote: >> We should not allow this any more, as it will break with the new userptr >> implementation, it could still be made to work, but there's no point in >> doing so. >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Ifdefs don't appear consistent with the commit message. Wrong patch or separate patch? > > Also please add a discussion what impact this has on existing user-space. > > Regarding ifdefs, all the userptr codeis unused when mmu notifers are not available, so I made it conditional on that.
On 10/30/20 11:10 AM, Maarten Lankhorst wrote: > Op 30-10-2020 om 10:26 schreef Thomas Hellström (Intel): >> On 10/16/20 12:43 PM, Maarten Lankhorst wrote: >>> We should not allow this any more, as it will break with the new userptr >>> implementation, it could still be made to work, but there's no point in >>> doing so. >>> >>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> Ifdefs don't appear consistent with the commit message. Wrong patch or separate patch? >> >> Also please add a discussion what impact this has on existing user-space. >> >> > Impact should be low, xf86-video-intel with SNA was using unsynchronized, but will fall back to synchronized if not available. Sounds ok to me if that's the only impact. Also here the review comment was about adding to the commit message.
On 10/30/20 11:11 AM, Maarten Lankhorst wrote: > Op 30-10-2020 om 10:26 schreef Thomas Hellström (Intel): >> On 10/16/20 12:43 PM, Maarten Lankhorst wrote: >>> We should not allow this any more, as it will break with the new userptr >>> implementation, it could still be made to work, but there's no point in >>> doing so. >>> >>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> Ifdefs don't appear consistent with the commit message. Wrong patch or separate patch? >> >> Also please add a discussion what impact this has on existing user-space. >> >> > Regarding ifdefs, all the userptr codeis unused when mmu notifers are not available, so I made it conditional on that. Yes I have nothing against the cange itself. But this is a fixup change that has nothing to do with rejecting UNSYNCHRONIZED and thus it should be moved to a separate commit or perhaps squashed in the big userptr rewrite? /Thomas
Op 30-10-2020 om 15:18 schreef Thomas Hellström (Intel): > > On 10/30/20 11:11 AM, Maarten Lankhorst wrote: >> Op 30-10-2020 om 10:26 schreef Thomas Hellström (Intel): >>> On 10/16/20 12:43 PM, Maarten Lankhorst wrote: >>>> We should not allow this any more, as it will break with the new userptr >>>> implementation, it could still be made to work, but there's no point in >>>> doing so. >>>> >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>> Ifdefs don't appear consistent with the commit message. Wrong patch or separate patch? >>> >>> Also please add a discussion what impact this has on existing user-space. >>> >>> >> Regarding ifdefs, all the userptr codeis unused when mmu notifers are not available, so I made it conditional on that. > > Yes I have nothing against the cange itself. But this is a fixup change that has nothing to do with rejecting UNSYNCHRONIZED and thus it should be moved to a separate commit or perhaps squashed in the big userptr rewrite? > > /Thomas > > I'll do it in a separate commit, since it's a separate change. :)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c index 9a44d9a6b5ed..89d7e7980eae 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c @@ -1970,8 +1970,10 @@ static noinline int eb_relocate_parse_slow(struct i915_execbuffer *eb, err = 0; } +#ifdef CONFIG_MMU_NOTIFIER if (!err) flush_workqueue(eb->i915->mm.userptr_wq); +#endif err_relock: i915_gem_ww_ctx_init(&eb->ww, true); diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.h b/drivers/gpu/drm/i915/gem/i915_gem_object.h index 436ff0d4951f..a3774e80aedd 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.h @@ -531,7 +531,11 @@ void __i915_gem_object_invalidate_frontbuffer(struct drm_i915_gem_object *obj, static inline bool i915_gem_object_is_userptr(struct drm_i915_gem_object *obj) { +#ifdef CONFIG_MMU_NOTIFIER return obj->userptr.mm; +#else + return false; +#endif } static inline void diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h index e84b279bfee6..1f729e63867c 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h @@ -289,6 +289,7 @@ struct drm_i915_gem_object { unsigned long *bit_17; union { +#ifdef CONFIG_MMU_NOTIFIER struct i915_gem_userptr { uintptr_t ptr; @@ -296,6 +297,7 @@ struct drm_i915_gem_object { struct i915_mmu_object *mmu_object; struct work_struct *work; } userptr; +#endif unsigned long scratch; u64 encode; diff --git a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c index 3fd63fdd7466..a2b7f6db2f1a 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_userptr.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_userptr.c @@ -15,6 +15,8 @@ #include "i915_gem_object.h" #include "i915_scatterlist.h" +#if defined(CONFIG_MMU_NOTIFIER) + struct i915_mm_struct { struct mm_struct *mm; struct drm_i915_private *i915; @@ -24,7 +26,6 @@ struct i915_mm_struct { struct rcu_work work; }; -#if defined(CONFIG_MMU_NOTIFIER) #include <linux/interval_tree.h> struct i915_mmu_notifier { @@ -217,15 +218,11 @@ i915_mmu_notifier_find(struct i915_mm_struct *mm) } static int -i915_gem_userptr_init__mmu_notifier(struct drm_i915_gem_object *obj, - unsigned flags) +i915_gem_userptr_init__mmu_notifier(struct drm_i915_gem_object *obj) { struct i915_mmu_notifier *mn; struct i915_mmu_object *mo; - if (flags & I915_USERPTR_UNSYNCHRONIZED) - return capable(CAP_SYS_ADMIN) ? 0 : -EPERM; - if (GEM_WARN_ON(!obj->userptr.mm)) return -EINVAL; @@ -258,38 +255,6 @@ i915_mmu_notifier_free(struct i915_mmu_notifier *mn, kfree(mn); } -#else - -static void -__i915_gem_userptr_set_active(struct drm_i915_gem_object *obj, bool value) -{ -} - -static void -i915_gem_userptr_release__mmu_notifier(struct drm_i915_gem_object *obj) -{ -} - -static int -i915_gem_userptr_init__mmu_notifier(struct drm_i915_gem_object *obj, - unsigned flags) -{ - if ((flags & I915_USERPTR_UNSYNCHRONIZED) == 0) - return -ENODEV; - - if (!capable(CAP_SYS_ADMIN)) - return -EPERM; - - return 0; -} - -static void -i915_mmu_notifier_free(struct i915_mmu_notifier *mn, - struct mm_struct *mm) -{ -} - -#endif static struct i915_mm_struct * __i915_mm_struct_find(struct drm_i915_private *i915, struct mm_struct *real) @@ -731,6 +696,8 @@ static const struct drm_i915_gem_object_ops i915_gem_userptr_ops = { .release = i915_gem_userptr_release, }; +#endif + /* * Creates a new mm object that wraps some normal memory from the process * context - user memory. @@ -771,12 +738,12 @@ i915_gem_userptr_ioctl(struct drm_device *dev, void *data, struct drm_file *file) { - static struct lock_class_key lock_class; + static struct lock_class_key __maybe_unused lock_class; struct drm_i915_private *dev_priv = to_i915(dev); struct drm_i915_gem_userptr *args = data; - struct drm_i915_gem_object *obj; - int ret; - u32 handle; + struct drm_i915_gem_object __maybe_unused *obj; + int __maybe_unused ret; + u32 __maybe_unused handle; if (!HAS_LLC(dev_priv) && !HAS_SNOOP(dev_priv)) { /* We cannot support coherent userptr objects on hw without @@ -815,6 +782,9 @@ i915_gem_userptr_ioctl(struct drm_device *dev, if (!access_ok((char __user *)(unsigned long)args->user_ptr, args->user_size)) return -EFAULT; + if (args->flags & I915_USERPTR_UNSYNCHRONIZED) + return -ENODEV; + if (args->flags & I915_USERPTR_READ_ONLY) { /* * On almost all of the older hw, we cannot tell the GPU that @@ -824,6 +794,7 @@ i915_gem_userptr_ioctl(struct drm_device *dev, return -ENODEV; } +#ifdef CONFIG_MMU_NOTIFIER obj = i915_gem_object_alloc(); if (obj == NULL) return -ENOMEM; @@ -845,7 +816,7 @@ i915_gem_userptr_ioctl(struct drm_device *dev, */ ret = i915_gem_userptr_init__mm_struct(obj); if (ret == 0) - ret = i915_gem_userptr_init__mmu_notifier(obj, args->flags); + ret = i915_gem_userptr_init__mmu_notifier(obj); if (ret == 0) ret = drm_gem_handle_create(file, &obj->base, &handle); @@ -856,10 +827,14 @@ i915_gem_userptr_ioctl(struct drm_device *dev, args->handle = handle; return 0; +#else + return -ENODEV; +#endif } int i915_gem_init_userptr(struct drm_i915_private *dev_priv) { +#ifdef CONFIG_MMU_NOTIFIER spin_lock_init(&dev_priv->mm_lock); hash_init(dev_priv->mm_structs); @@ -869,11 +844,14 @@ int i915_gem_init_userptr(struct drm_i915_private *dev_priv) 0); if (!dev_priv->mm.userptr_wq) return -ENOMEM; +#endif return 0; } void i915_gem_cleanup_userptr(struct drm_i915_private *dev_priv) { +#ifdef CONFIG_MMU_NOTIFIER destroy_workqueue(dev_priv->mm.userptr_wq); +#endif } diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 7bd7b3e82c45..23db5a5f5fcb 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -589,12 +589,14 @@ struct i915_gem_mm { struct notifier_block vmap_notifier; struct shrinker shrinker; +#ifdef CONFIG_MMU_NOTIFIER /** * Workqueue to fault in userptr pages, flushed by the execbuf * when required but otherwise left to userspace to try again * on EAGAIN. */ struct workqueue_struct *userptr_wq; +#endif /* shrinker accounting, also useful for userland debugging */ u64 shrink_memory;
We should not allow this any more, as it will break with the new userptr implementation, it could still be made to work, but there's no point in doing so. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 2 + drivers/gpu/drm/i915/gem/i915_gem_object.h | 4 ++ .../gpu/drm/i915/gem/i915_gem_object_types.h | 2 + drivers/gpu/drm/i915/gem/i915_gem_userptr.c | 64 ++++++------------- drivers/gpu/drm/i915/i915_drv.h | 2 + 5 files changed, 31 insertions(+), 43 deletions(-)