Re: [CI] drm/i915/gem: Convert vm idr to xarray

[Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>]

On 22/01/2020 16:15, Chris Wilson wrote:
> Replace the vm_idr + vm_idr_mutex to an XArray. The XArray data
> structure is now used to implement IDRs, and provides its own locking.
> We can simply remove the IDR wrapper and in the process also remove our
> extra mutex.
>
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> Reviewed-by: Michael J. Ruhl <michael.j.ruhl@xxxxxxxxx>

Acked-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>

Regards,

Tvrtko

> ---
>   drivers/gpu/drm/i915/gem/i915_gem_context.c | 77 +++++++--------------
>   drivers/gpu/drm/i915/i915_drv.h             |  4 +-
>   2 files changed, 25 insertions(+), 56 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..5d4157e1ccf7 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)
> @@ -804,8 +798,8 @@ int i915_gem_context_open(struct drm_i915_private *i915,
>   
>   	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);
> +	/* 0 reserved for invalid/unassigned ppgtt */
> +	xa_init_flags(&file_priv->vm_xa, XA_FLAGS_ALLOC1);
>   
>   	ctx = i915_gem_create_context(i915, 0);
>   	if (IS_ERR(ctx)) {
> @@ -823,9 +817,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 +826,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 +834,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);
>   }
> @@ -854,6 +848,7 @@ int i915_gem_vm_create_ioctl(struct drm_device *dev, void *data,
>   	struct drm_i915_gem_vm_control *args = data;
>   	struct drm_i915_file_private *file_priv = file->driver_priv;
>   	struct i915_ppgtt *ppgtt;
> +	u32 id;
>   	int err;
>   
>   	if (!HAS_FULL_PPGTT(i915))
> @@ -876,23 +871,15 @@ 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, &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;
> +	GEM_BUG_ON(id == 0); /* reserved for invalid/unassigned ppgtt */
> +	args->vm_id = id;
>   	return 0;
>   
> -err_unlock:
> -	mutex_unlock(&file_priv->vm_idr_lock);
>   err_put:
>   	i915_vm_put(&ppgtt->vm);
>   	return err;
> @@ -904,8 +891,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 +898,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 +996,31 @@ 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;
> +	u32 id;
>   
>   	if (!rcu_access_pointer(ctx->vm))
>   		return -ENODEV;
>   
> +	err = -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();
> -
> -	ret = mutex_lock_interruptible(&file_priv->vm_idr_lock);
> -	if (ret)
> +	if (err)
>   		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);
>   
> +	GEM_BUG_ON(id == 0); /* reserved for invalid/unassigned ppgtt */
> +	args->value = id;
>   	args->size = 0;
> -	args->value = ret;
>   
> -	ret = 0;
> -err_unlock:
> -	mutex_unlock(&file_priv->vm_idr_lock);
>   err_put:
>   	i915_vm_put(vm);
> -	return ret;
> +	return err;
>   }
>   
>   static void set_ppgtt_barrier(void *data)
> @@ -1151,7 +1122,7 @@ static int set_ppgtt(struct drm_i915_file_private *file_priv,
>   		return -ENOENT;
>   
>   	rcu_read_lock();
> -	vm = idr_find(&file_priv->vm_idr, args->value);
> +	vm = xa_load(&file_priv->vm_xa, args->value);
>   	if (vm && !kref_get_unless_zero(&vm->ref))
>   		vm = NULL;
>   	rcu_read_unlock();
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 077af22b8340..50abf9113b2f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -203,9 +203,7 @@ struct drm_i915_file_private {
>   	} mm;
>   
>   	struct xarray context_xa;
> -
> -	struct idr vm_idr;
> -	struct mutex vm_idr_lock; /* guards vm_idr */
> +	struct xarray vm_xa;
>   
>   	unsigned int bsd_engine;
>   
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx