Re: [PATCH 14/18] drm/i915/context: switch contexts with execbuf2

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Mar 30, 2012 at 11:58:20AM -0700, Ben Widawsky wrote:
> On Thu, 29 Mar 2012 21:38:20 +0200
> Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > On Sun, Mar 18, 2012 at 01:39:54PM -0700, Ben Widawsky wrote:
> > > Use the rsvd1 field in execbuf2 to specify the context ID associated
> > > with the workload. This will allow the driver to do the proper context
> > > switch when/if needed.
> > > 
> > > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > > ---
> > >  drivers/gpu/drm/i915/i915_gem_execbuffer.c |    6 ++++++
> > >  include/drm/i915_drm.h                     |    4 +++-
> > >  2 files changed, 9 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > > index 81687af..c365e12 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > > @@ -1058,6 +1058,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> > >  	struct drm_i915_gem_object *batch_obj;
> > >  	struct drm_clip_rect *cliprects = NULL;
> > >  	struct intel_ring_buffer *ring;
> > > +	u32 ctx_id = args->context_info & I915_EXEC_CONTEXT_ID_MASK;
> > >  	u32 exec_start, exec_len;
> > >  	u32 seqno;
> > >  	u32 mask;
> > > @@ -1266,6 +1267,10 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data,
> > >  			goto err;
> > >  	}
> > >  
> > > +	ret = i915_switch_context(ring, file, ctx_id, seqno, 0);
> > > +	if (ret)
> > > +		goto err;
> > > +
> > 
> > Already complained a bit about these:
> > - Imo the seqno and hw_flags param can/should go away.
> 
> Responding to the other checks for this.
> 
> > - You need to add some sanity checks somewhere so that we correctly bail
> >   out of the do_execbuffer stuff without launching the batch. Obviously
> >   also a prime candidate for an i-g-t tests (because it'll nicely exercise
> >   a piece of code which usually is rather hard to tests).
> 
> I'm not sure what you're asking for, aside from - this is hairy code; be
> careful. What was your idea?

If userspace tries to run an execbuffer with
- an nonexisting context on render
- non-default context on !render_ring
it needs to get a -EINVAL and the kernel must not execute the batch. Your
code fails at that.

We also need a testcase for this case in i-g-t. Another testcase is trying
to destroy a non-existing context. I haven't rechecked whether your
current code handles that correctly.

> > Aside: I haven't yet looked at your testcases, so maybe I'll come up with
> > a crazy idea for another testcase ;-)
> 
> Testcases are pretty sparse in i-g-t. Most of it is done implicitly with
> mesa. If you have ideas, I'll be happy to write them

See above ;-) I'll look at your current set of testcase later and write
another mail with the things I might find missing.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48