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

[Ben Widawsky <ben@bwidawsk.net>]

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?

> 
> 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

> 
> >  	trace_i915_gem_ring_dispatch(ring, seqno);
> >  
> >  	exec_start = batch_obj->gtt_offset + args->batch_start_offset;
> > @@ -1372,6 +1377,7 @@ i915_gem_execbuffer(struct drm_device *dev, void *data,
> >  	exec2.num_cliprects = args->num_cliprects;
> >  	exec2.cliprects_ptr = args->cliprects_ptr;
> >  	exec2.flags = I915_EXEC_RENDER;
> > +	exec2.context_info = 0;
> >  
> >  	ret = i915_gem_do_execbuffer(dev, data, file, &exec2, exec2_list);
> >  	if (!ret) {
> > diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h
> > index bead13e..03d159f 100644
> > --- a/include/drm/i915_drm.h
> > +++ b/include/drm/i915_drm.h
> > @@ -660,13 +660,15 @@ struct drm_i915_gem_execbuffer2 {
> >  #define I915_EXEC_CONSTANTS_ABSOLUTE 	(1<<6)
> >  #define I915_EXEC_CONSTANTS_REL_SURFACE (2<<6) /* gen4/5 only */
> >  	__u64 flags;
> > -	__u64 rsvd1;
> > +	__u64 context_info;
> >  	__u64 rsvd2;
> >  };
> >  
> >  /** Resets the SO write offset registers for transform feedback on gen7. */
> >  #define I915_EXEC_GEN7_SOL_RESET	(1<<8)
> >  
> > +#define I915_EXEC_CONTEXT_ID_MASK	(0xffffffff)
> > +
> >  struct drm_i915_gem_pin {
> >  	/** Handle of the buffer to be pinned. */
> >  	__u32 handle;
> > -- 
> > 1.7.9.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>