Re: [Intel-gfx] [PATCH 4/5] drm/i915/gem: Convert vm idr to xarray
Quoting Ruhl, Michael J (2020-01-22 16:15:17)
>
>
> >-Original Message-
> >From: Chris Wilson
> >Sent: Wednesday, January 22, 2020 11:09 AM
> >To: Ruhl, Michael J ; intel-
> >g...@lists.freedesktop.org
> >Cc: Auld, Matthew
> >Subject: RE: [Intel-gfx] [PATCH 4/5] drm/i915/gem: Convert vm idr to xarray
> >
> >Quoting Ruhl, Michael J (2020-01-22 16:00:25)
> >> >-Original Message-
> >> >From: Intel-gfx On Behalf Of
> >Chris
> >> >Wilson
> >> >@@ -876,23 +868,13 @@ int i915_gem_vm_create_ioctl(struct drm_device
> >> >*dev, void *data,
> >> > goto err_put;
> >> > }
> >> >
> >> >- err = mutex_lock_interruptible(&file_priv->vm_idr_lock);
> >> >+ err = xa_alloc(&file_priv->vm_xa, &args->vm_id,
> >> >+ &ppgtt->vm, xa_limit_32b, GFP_KERNEL);
> >> > if (err)
> >> > goto err_put;
> >> >
> >> >- err = idr_alloc(&file_priv->vm_idr, &ppgtt->vm, 0, 0, GFP_KERNEL);
> >> >- if (err < 0)
> >> >- goto err_unlock;
> >> >-
> >> >- GEM_BUG_ON(err == 0); /* reserved for invalid/unassigned ppgtt */
> >>
> >> Moving this comment to the xa_init_flags() would help me understand
> >> why the index started at 1.
> >
> >Hey, I take 0 being reserved for granted, and had to think about why
> >the context_xa was not 1-biased!
> >
> >> >@@ -1021,35 +991,27 @@ static int get_ppgtt(struct drm_i915_file_private
> >> >*file_priv,
> >> >struct drm_i915_gem_context_param *args)
> >> > {
> >> > struct i915_address_space *vm;
> >> >- int ret;
> >> >+ int err = -ENODEV;
> >> >+ u32 id;
> >> >
> >> > if (!rcu_access_pointer(ctx->vm))
> >> > return -ENODEV;
> >> >
> >> > rcu_read_lock();
> >> > vm = context_get_vm_rcu(ctx);
> >> >+ if (vm)
> >> >+ err = xa_alloc(&file_priv->vm_xa, &id, vm,
> >> >+ xa_limit_32b, GFP_KERNEL);
> >> > rcu_read_unlock();
> >> >+ if (!err) {
> >> >+ i915_vm_open(vm);
> >>
> >> Why did you switch to success path in the if here?
> >
> >No good reason, just simple enough to fit inside one if {}.
> >
> >> Can you do:
> >>
> >> if (err)
> >> goto err_put;
> >>
> >> ?
> >>
> >> >- ret = mutex_lock_interruptible(&file_priv->vm_idr_lock);
> >> >- if (ret)
> >> >- goto err_put;
> >> >-
> >> >- ret = idr_alloc(&file_priv->vm_idr, vm, 0, 0, GFP_KERNEL);
> >> >- GEM_BUG_ON(!ret);
> >> >- if (ret < 0)
> >> >- goto err_unlock;
> >> >-
> >> >- i915_vm_open(vm);
> >> >-
> >> >- args->size = 0;
> >> >- args->value = ret;
> >> >+ args->size = 0;
> >> >+ args->value = id;
> >>
> >> Would passing args->value to the xa_alloc be a useful?
> >
> >General rule is not to alter user params except on success. While not
> >always required, the pattern does help to avoid common pitfalls where
> >userspace has to repeat an ioctl (e.g. SIGINT).
>
> Yup. Following that rule, xa_array() will only update the value on success.
>
> I was mostly commenting on it because you had done that in the previous
> xa_alloc call.
Went back and changed that so both paths look very similar, begging to
be refactored...
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/5] drm/i915/gem: Convert vm idr to xarray
>-Original Message-
>From: Chris Wilson
>Sent: Wednesday, January 22, 2020 11:09 AM
>To: Ruhl, Michael J ; intel-
>g...@lists.freedesktop.org
>Cc: Auld, Matthew
>Subject: RE: [Intel-gfx] [PATCH 4/5] drm/i915/gem: Convert vm idr to xarray
>
>Quoting Ruhl, Michael J (2020-01-22 16:00:25)
>> >-Original Message-
>> >From: Intel-gfx On Behalf Of
>Chris
>> >Wilson
>> >@@ -876,23 +868,13 @@ int i915_gem_vm_create_ioctl(struct drm_device
>> >*dev, void *data,
>> > goto err_put;
>> > }
>> >
>> >- err = mutex_lock_interruptible(&file_priv->vm_idr_lock);
>> >+ err = xa_alloc(&file_priv->vm_xa, &args->vm_id,
>> >+ &ppgtt->vm, xa_limit_32b, GFP_KERNEL);
>> > if (err)
>> > goto err_put;
>> >
>> >- err = idr_alloc(&file_priv->vm_idr, &ppgtt->vm, 0, 0, GFP_KERNEL);
>> >- if (err < 0)
>> >- goto err_unlock;
>> >-
>> >- GEM_BUG_ON(err == 0); /* reserved for invalid/unassigned ppgtt */
>>
>> Moving this comment to the xa_init_flags() would help me understand
>> why the index started at 1.
>
>Hey, I take 0 being reserved for granted, and had to think about why
>the context_xa was not 1-biased!
>
>> >@@ -1021,35 +991,27 @@ static int get_ppgtt(struct drm_i915_file_private
>> >*file_priv,
>> >struct drm_i915_gem_context_param *args)
>> > {
>> > struct i915_address_space *vm;
>> >- int ret;
>> >+ int err = -ENODEV;
>> >+ u32 id;
>> >
>> > if (!rcu_access_pointer(ctx->vm))
>> > return -ENODEV;
>> >
>> > rcu_read_lock();
>> > vm = context_get_vm_rcu(ctx);
>> >+ if (vm)
>> >+ err = xa_alloc(&file_priv->vm_xa, &id, vm,
>> >+ xa_limit_32b, GFP_KERNEL);
>> > rcu_read_unlock();
>> >+ if (!err) {
>> >+ i915_vm_open(vm);
>>
>> Why did you switch to success path in the if here?
>
>No good reason, just simple enough to fit inside one if {}.
>
>> Can you do:
>>
>> if (err)
>> goto err_put;
>>
>> ?
>>
>> >- ret = mutex_lock_interruptible(&file_priv->vm_idr_lock);
>> >- if (ret)
>> >- goto err_put;
>> >-
>> >- ret = idr_alloc(&file_priv->vm_idr, vm, 0, 0, GFP_KERNEL);
>> >- GEM_BUG_ON(!ret);
>> >- if (ret < 0)
>> >- goto err_unlock;
>> >-
>> >- i915_vm_open(vm);
>> >-
>> >- args->size = 0;
>> >- args->value = ret;
>> >+ args->size = 0;
>> >+ args->value = id;
>>
>> Would passing args->value to the xa_alloc be a useful?
>
>General rule is not to alter user params except on success. While not
>always required, the pattern does help to avoid common pitfalls where
>userspace has to repeat an ioctl (e.g. SIGINT).
Yup. Following that rule, xa_array() will only update the value on success.
I was mostly commenting on it because you had done that in the previous
xa_alloc call.
Thanks,
Mike
>-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/5] drm/i915/gem: Convert vm idr to xarray
Quoting Ruhl, Michael J (2020-01-22 16:00:25)
> >-Original Message-
> >From: Intel-gfx On Behalf Of Chris
> >Wilson
> >@@ -876,23 +868,13 @@ int i915_gem_vm_create_ioctl(struct drm_device
> >*dev, void *data,
> > goto err_put;
> > }
> >
> >- err = mutex_lock_interruptible(&file_priv->vm_idr_lock);
> >+ err = xa_alloc(&file_priv->vm_xa, &args->vm_id,
> >+ &ppgtt->vm, xa_limit_32b, GFP_KERNEL);
> > if (err)
> > goto err_put;
> >
> >- err = idr_alloc(&file_priv->vm_idr, &ppgtt->vm, 0, 0, GFP_KERNEL);
> >- if (err < 0)
> >- goto err_unlock;
> >-
> >- GEM_BUG_ON(err == 0); /* reserved for invalid/unassigned ppgtt */
>
> Moving this comment to the xa_init_flags() would help me understand
> why the index started at 1.
Hey, I take 0 being reserved for granted, and had to think about why
the context_xa was not 1-biased!
> >@@ -1021,35 +991,27 @@ static int get_ppgtt(struct drm_i915_file_private
> >*file_priv,
> >struct drm_i915_gem_context_param *args)
> > {
> > struct i915_address_space *vm;
> >- int ret;
> >+ int err = -ENODEV;
> >+ u32 id;
> >
> > if (!rcu_access_pointer(ctx->vm))
> > return -ENODEV;
> >
> > rcu_read_lock();
> > vm = context_get_vm_rcu(ctx);
> >+ if (vm)
> >+ err = xa_alloc(&file_priv->vm_xa, &id, vm,
> >+ xa_limit_32b, GFP_KERNEL);
> > rcu_read_unlock();
> >+ if (!err) {
> >+ i915_vm_open(vm);
>
> Why did you switch to success path in the if here?
No good reason, just simple enough to fit inside one if {}.
> Can you do:
>
> if (err)
> goto err_put;
>
> ?
>
> >- ret = mutex_lock_interruptible(&file_priv->vm_idr_lock);
> >- if (ret)
> >- goto err_put;
> >-
> >- ret = idr_alloc(&file_priv->vm_idr, vm, 0, 0, GFP_KERNEL);
> >- GEM_BUG_ON(!ret);
> >- if (ret < 0)
> >- goto err_unlock;
> >-
> >- i915_vm_open(vm);
> >-
> >- args->size = 0;
> >- args->value = ret;
> >+ args->size = 0;
> >+ args->value = id;
>
> Would passing args->value to the xa_alloc be a useful?
General rule is not to alter user params except on success. While not
always required, the pattern does help to avoid common pitfalls where
userspace has to repeat an ioctl (e.g. SIGINT).
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/5] drm/i915/gem: Convert vm idr to xarray
>-Original Message-
>From: Intel-gfx On Behalf Of Chris
>Wilson
>Sent: Monday, January 20, 2020 5:49 AM
>To: intel-gfx@lists.freedesktop.org
>Cc: Auld, Matthew
>Subject: [Intel-gfx] [PATCH 4/5] drm/i915/gem: Convert vm idr to xarray
>
>Replace the vm_idr + vm_idr_mutex to an XArray. The XArray data
>structure is now used to implement IDRs, and provides its won locking.
s/won/own
>We can simply remove the IDR wrapper and in the process also remove our
>extra mutex.
>
>Signed-off-by: Chris Wilson
>Cc: Tvrtko Ursulin
>---
> drivers/gpu/drm/i915/gem/i915_gem_context.c | 80 ++---
> drivers/gpu/drm/i915/i915_drv.h | 4 +-
> 2 files changed, 22 insertions(+), 62 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c
>b/drivers/gpu/drm/i915/gem/i915_gem_context.c
>index a2e57e62af30..d2e4e8cbf4d4 100644
>--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
>+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
>@@ -761,12 +761,6 @@ void i915_gem_driver_release__contexts(struct
>drm_i915_private *i915)
> flush_work(&i915->gem.contexts.free_work);
> }
>
>-static int vm_idr_cleanup(int id, void *p, void *data)
>-{
>- i915_vm_put(p);
>- return 0;
>-}
>-
> static int gem_context_register(struct i915_gem_context *ctx,
> struct drm_i915_file_private *fpriv,
> u32 *id)
>@@ -803,9 +797,7 @@ int i915_gem_context_open(struct drm_i915_private
>*i915,
> u32 id;
>
> xa_init_flags(&file_priv->context_xa, XA_FLAGS_ALLOC);
>-
>- mutex_init(&file_priv->vm_idr_lock);
>- idr_init_base(&file_priv->vm_idr, 1);
>+ xa_init_flags(&file_priv->vm_xa, XA_FLAGS_ALLOC1);
>
> ctx = i915_gem_create_context(i915, 0);
> if (IS_ERR(ctx)) {
>@@ -823,9 +815,8 @@ int i915_gem_context_open(struct drm_i915_private
>*i915,
> err_ctx:
> context_close(ctx);
> err:
>- idr_destroy(&file_priv->vm_idr);
>+ xa_destroy(&file_priv->vm_xa);
> xa_destroy(&file_priv->context_xa);
>- mutex_destroy(&file_priv->vm_idr_lock);
> return err;
> }
>
>@@ -833,6 +824,7 @@ void i915_gem_context_close(struct drm_file *file)
> {
> struct drm_i915_file_private *file_priv = file->driver_priv;
> struct drm_i915_private *i915 = file_priv->dev_priv;
>+ struct i915_address_space *vm;
> struct i915_gem_context *ctx;
> unsigned long idx;
>
>@@ -840,9 +832,9 @@ void i915_gem_context_close(struct drm_file *file)
> context_close(ctx);
> xa_destroy(&file_priv->context_xa);
>
>- idr_for_each(&file_priv->vm_idr, vm_idr_cleanup, NULL);
>- idr_destroy(&file_priv->vm_idr);
>- mutex_destroy(&file_priv->vm_idr_lock);
>+ xa_for_each(&file_priv->vm_xa, idx, vm)
>+ i915_vm_put(vm);
>+ xa_destroy(&file_priv->vm_xa);
>
> contexts_flush_free(&i915->gem.contexts);
> }
>@@ -876,23 +868,13 @@ int i915_gem_vm_create_ioctl(struct drm_device
>*dev, void *data,
> goto err_put;
> }
>
>- err = mutex_lock_interruptible(&file_priv->vm_idr_lock);
>+ err = xa_alloc(&file_priv->vm_xa, &args->vm_id,
>+ &ppgtt->vm, xa_limit_32b, GFP_KERNEL);
> if (err)
> goto err_put;
>
>- err = idr_alloc(&file_priv->vm_idr, &ppgtt->vm, 0, 0, GFP_KERNEL);
>- if (err < 0)
>- goto err_unlock;
>-
>- GEM_BUG_ON(err == 0); /* reserved for invalid/unassigned ppgtt */
Moving this comment to the xa_init_flags() would help me understand
why the index started at 1.
>- mutex_unlock(&file_priv->vm_idr_lock);
>-
>- args->vm_id = err;
> return 0;
>
>-err_unlock:
>- mutex_unlock(&file_priv->vm_idr_lock);
> err_put:
> i915_vm_put(&ppgtt->vm);
> return err;
>@@ -904,8 +886,6 @@ int i915_gem_vm_destroy_ioctl(struct drm_device
>*dev, void *data,
> struct drm_i915_file_private *file_priv = file->driver_priv;
> struct drm_i915_gem_vm_control *args = data;
> struct i915_address_space *vm;
>- int err;
>- u32 id;
>
> if (args->flags)
> return -EINVAL;
>@@ -913,17 +893,7 @@ int i915_gem_vm_destroy_ioctl(struct drm_device
>*dev, void *data,
> if (args->extensions)
> return -EINVAL;
>
>- id = args->vm_id;
>- if (!id)
>- return -ENOENT;
>-
>- err = mutex_lo
[Intel-gfx] [PATCH 4/5] drm/i915/gem: Convert vm idr to xarray
Replace the vm_idr + vm_idr_mutex to an XArray. The XArray data
structure is now used to implement IDRs, and provides its won locking.
We can simply remove the IDR wrapper and in the process also remove our
extra mutex.
Signed-off-by: Chris Wilson
Cc: Tvrtko Ursulin
---
drivers/gpu/drm/i915/gem/i915_gem_context.c | 80 ++---
drivers/gpu/drm/i915/i915_drv.h | 4 +-
2 files changed, 22 insertions(+), 62 deletions(-)
diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c
b/drivers/gpu/drm/i915/gem/i915_gem_context.c
index a2e57e62af30..d2e4e8cbf4d4 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c
@@ -761,12 +761,6 @@ void i915_gem_driver_release__contexts(struct
drm_i915_private *i915)
flush_work(&i915->gem.contexts.free_work);
}
-static int vm_idr_cleanup(int id, void *p, void *data)
-{
- i915_vm_put(p);
- return 0;
-}
-
static int gem_context_register(struct i915_gem_context *ctx,
struct drm_i915_file_private *fpriv,
u32 *id)
@@ -803,9 +797,7 @@ int i915_gem_context_open(struct drm_i915_private *i915,
u32 id;
xa_init_flags(&file_priv->context_xa, XA_FLAGS_ALLOC);
-
- mutex_init(&file_priv->vm_idr_lock);
- idr_init_base(&file_priv->vm_idr, 1);
+ xa_init_flags(&file_priv->vm_xa, XA_FLAGS_ALLOC1);
ctx = i915_gem_create_context(i915, 0);
if (IS_ERR(ctx)) {
@@ -823,9 +815,8 @@ int i915_gem_context_open(struct drm_i915_private *i915,
err_ctx:
context_close(ctx);
err:
- idr_destroy(&file_priv->vm_idr);
+ xa_destroy(&file_priv->vm_xa);
xa_destroy(&file_priv->context_xa);
- mutex_destroy(&file_priv->vm_idr_lock);
return err;
}
@@ -833,6 +824,7 @@ void i915_gem_context_close(struct drm_file *file)
{
struct drm_i915_file_private *file_priv = file->driver_priv;
struct drm_i915_private *i915 = file_priv->dev_priv;
+ struct i915_address_space *vm;
struct i915_gem_context *ctx;
unsigned long idx;
@@ -840,9 +832,9 @@ void i915_gem_context_close(struct drm_file *file)
context_close(ctx);
xa_destroy(&file_priv->context_xa);
- idr_for_each(&file_priv->vm_idr, vm_idr_cleanup, NULL);
- idr_destroy(&file_priv->vm_idr);
- mutex_destroy(&file_priv->vm_idr_lock);
+ xa_for_each(&file_priv->vm_xa, idx, vm)
+ i915_vm_put(vm);
+ xa_destroy(&file_priv->vm_xa);
contexts_flush_free(&i915->gem.contexts);
}
@@ -876,23 +868,13 @@ int i915_gem_vm_create_ioctl(struct drm_device *dev, void
*data,
goto err_put;
}
- err = mutex_lock_interruptible(&file_priv->vm_idr_lock);
+ err = xa_alloc(&file_priv->vm_xa, &args->vm_id,
+ &ppgtt->vm, xa_limit_32b, GFP_KERNEL);
if (err)
goto err_put;
- err = idr_alloc(&file_priv->vm_idr, &ppgtt->vm, 0, 0, GFP_KERNEL);
- if (err < 0)
- goto err_unlock;
-
- GEM_BUG_ON(err == 0); /* reserved for invalid/unassigned ppgtt */
-
- mutex_unlock(&file_priv->vm_idr_lock);
-
- args->vm_id = err;
return 0;
-err_unlock:
- mutex_unlock(&file_priv->vm_idr_lock);
err_put:
i915_vm_put(&ppgtt->vm);
return err;
@@ -904,8 +886,6 @@ int i915_gem_vm_destroy_ioctl(struct drm_device *dev, void
*data,
struct drm_i915_file_private *file_priv = file->driver_priv;
struct drm_i915_gem_vm_control *args = data;
struct i915_address_space *vm;
- int err;
- u32 id;
if (args->flags)
return -EINVAL;
@@ -913,17 +893,7 @@ int i915_gem_vm_destroy_ioctl(struct drm_device *dev, void
*data,
if (args->extensions)
return -EINVAL;
- id = args->vm_id;
- if (!id)
- return -ENOENT;
-
- err = mutex_lock_interruptible(&file_priv->vm_idr_lock);
- if (err)
- return err;
-
- vm = idr_remove(&file_priv->vm_idr, id);
-
- mutex_unlock(&file_priv->vm_idr_lock);
+ vm = xa_erase(&file_priv->vm_xa, args->vm_id);
if (!vm)
return -ENOENT;
@@ -1021,35 +991,27 @@ static int get_ppgtt(struct drm_i915_file_private
*file_priv,
struct drm_i915_gem_context_param *args)
{
struct i915_address_space *vm;
- int ret;
+ int err = -ENODEV;
+ u32 id;
if (!rcu_access_pointer(ctx->vm))
return -ENODEV;
rcu_read_lock();
vm = context_get_vm_rcu(ctx);
+ if (vm)
+ err = xa_alloc(&file_priv->vm_xa, &id, vm,
+ xa_limit_32b, GFP_KERNEL);
rcu_read_unlock();
+ if (!err) {
+ i915_vm_open(vm);
- ret = mutex_lock_interruptible(&file_priv->vm_idr_lock);
- if (ret)
-
