Quoting Ruhl, Michael J (2020-01-22 16:15:17)
>
>
> >-----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.
Went back and changed that so both paths look very similar, begging to
be refactored...
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
