Re: [PATCH 17/38] drm/i915: Create/destroy VM (ppGTT) for use with contexts

[Chris Wilson <chris@chris-wilson.co.uk>]

Quoting Tvrtko Ursulin (2019-03-06 11:27:37)
> 
> On 01/03/2019 14:03, Chris Wilson wrote:
> > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> > index 91926a407548..8c35b6019f0d 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > @@ -124,6 +124,8 @@ static void lut_close(struct i915_gem_context *ctx)
> >               struct i915_vma *vma = rcu_dereference_raw(*slot);
> >   
> >               radix_tree_iter_delete(&ctx->handles_vma, &iter, slot);
> > +
> > +             vma->open_count--;
> 
> Assuming none of the new code in this patch gets called, then this 
> change looks like standalone? What am I missing?

It's only required because of the new code. Previously it was always 1
for ppgtt.

> > +int i915_gem_vm_destroy_ioctl(struct drm_device *dev, void *data,
> > +                           struct drm_file *file)
> > +{
> > +     struct drm_i915_file_private *file_priv = file->driver_priv;
> > +     struct drm_i915_gem_vm_control *args = data;
> > +     struct i915_hw_ppgtt *ppgtt;
> > +     int err;
> > +     u32 id;
> > +
> 
> Do we want an -ENODEV check here as well?

I don't think so; you can only get here with a garbage id in that case
and ENOENT for feeding in an unknown id makes sense.
> 
> > +     if (args->flags)
> > +             return -EINVAL;
> > +
> > +     if (args->extensions)
> > +             return -EINVAL;
> > +
> > +     id = args->id;
> > +     if (!id)
> > +             return -ENOENT;
> > +
> > +     err = mutex_lock_interruptible(&file_priv->vm_lock);
> > +     if (err)
> > +             return err;
> > +
> > +     ppgtt = idr_remove(&file_priv->vm_idr, id);
> > +     if (ppgtt) {
> > +             GEM_BUG_ON(!ppgtt->user_handle);
> > +             ppgtt->user_handle = 0;
> > +     }
> > +
> > +     mutex_unlock(&file_priv->vm_lock);
> 
> Nit - move to just after idr_remove for symmetry with create?

We need to lock user_handle = 0, I think.

> > +     /*
> > +      * We need to flush any requests using the current ppgtt before
> > +      * we release it as the requests do not hold a reference themselves,
> > +      * only indirectly through the context.
> > +      */
> > +     err = context_barrier_task(ctx, -1, set_ppgtt_barrier, old);
> 
> s/-1/ALL_ENGINES/ ?

Grr, magic macros. It's not even an intel_engine_mask_t yet :-p

> > +             parent = i915_gem_create_context(i915, file->driver_priv);
> > +             if (IS_ERR(parent)) {
> > +                     err = PTR_ERR(parent);
> > +                     if (err == -ENODEV) /* no logical ctx support */
> 
> Check it earlier like igt_ctx_exec?

Possibly, I did have a reason but I can't recall what.

> > -     mock_file_free(i915, file);
> > -     return err;
> > +             mock_file_free(i915, file);
> > +             if (err)
> > +                     return err;
> > +     }
> > +
> > +     return 0;
> >   }
> 
> Is my eyeballing igt_ctx_exec and igt_ctx_shared_exec correct in 
> noticing the tests are basically the same apart from ppgtt sharing bit? 
> Could you consolidate and use test flags to control whether or not to share?

That takes effort! Memory says that the current tests shared the guts
and only the setup was custom.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx