>-----Original Message-----
>From: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
>Sent: Wednesday, January 22, 2020 11:09 AM
>To: Ruhl, Michael J <michael.j.ruhl@xxxxxxxxx>; intel-
>gfx@xxxxxxxxxxxxxxxxxxxxx
>Cc: Auld, Matthew <matthew.auld@xxxxxxxxx>
>Subject: RE: [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 <intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
