From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 14/18] drm/i915/context: switch contexts with execbuf2 Date: Fri, 30 Mar 2012 21:20:40 +0200 Message-ID: <20120330192040.GD23526@phenom.ffwll.local> References: <1332103198-25852-1-git-send-email-ben@bwidawsk.net> <1332103198-25852-15-git-send-email-ben@bwidawsk.net> <20120329193820.GH27737@phenom.ffwll.local> <20120330115820.6eafe8e3@bwidawsk.net> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-wi0-f177.google.com (mail-wi0-f177.google.com [209.85.212.177]) by gabe.freedesktop.org (Postfix) with ESMTP id 3404F9E7A6 for ; Fri, 30 Mar 2012 12:19:57 -0700 (PDT) Received: by wibhj13 with SMTP id hj13so694128wib.12 for ; Fri, 30 Mar 2012 12:19:56 -0700 (PDT) In-Reply-To: <20120330115820.6eafe8e3@bwidawsk.net> Content-Disposition: inline List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Ben Widawsky Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Fri, Mar 30, 2012 at 11:58:20AM -0700, Ben Widawsky wrote: > On Thu, 29 Mar 2012 21:38:20 +0200 > Daniel Vetter 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 > > > --- > > > 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