On Thu, Oct 23, 2014 at 05:31:12AM -0700, Daniel Vetter wrote:
> On Wed, Oct 22, 2014 at 09:04:32AM -0700, Volkin, Bradley D wrote:
> > [snip]
> >
> > On Tue, Oct 21, 2014 at 08:50:33AM -0700, Daniel Vetter wrote:
> > > On Thu, Oct 16, 2014 at 12:24:42PM -0700, bradley.d.vol...@intel.com
> > > wrote:
> > > > diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > > > b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > > > index 1a0611b..1ed5702 100644
> > > > --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > > > +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> > > > @@ -1368,17 +1368,19 @@ i915_gem_do_execbuffer(struct drm_device *dev,
> > > > void *data,
> > > > batch_obj,
> > > > args->batch_start_offset,
> > > > file->is_master);
> > > > - if (ret)
> > > > - goto err;
> > > > -
> > > > - /*
> > > > - * XXX: Actually do this when enabling batch copy...
> > > > - *
> > > > - * Set the DISPATCH_SECURE bit to remove the NON_SECURE
> > > > bit
> > > > - * from MI_BATCH_BUFFER_START commands issued in the
> > > > - * dispatch_execbuffer implementations. We specifically
> > > > don't
> > > > - * want that set when the command parser is enabled.
> > > > - */
> > > > + if (ret) {
> > > > + if (ret != -EACCES)
> > > > + goto err;
> > > > + } else {
> > > > + /*
> > > > + * XXX: Actually do this when enabling batch
> > > > copy...
> > > > + *
> > > > + * Set the DISPATCH_SECURE bit to remove the
> > > > NON_SECURE bit
> > > > + * from MI_BATCH_BUFFER_START commands issued
> > > > in the
> > > > + * dispatch_execbuffer implementations. We
> > > > specifically don't
> > > > + * want that set when the command parser is
> > > > enabled.
> > > > + */
> > > > + }
> > >
> > > Tbh this hunk here confuses me ... Why do we need to change anything here?
> >
> > Yeah, it makes more sense with the batch copy code, it's just that this
> > patch has to go in before the patch where we set I915_DISPATCH_SECURE.
> > The final logic basically goes like this:
> >
> > ret = i915_parse_cmds()
> > if ret == 0
> > dispatch shadow_batch_obj, flags = I915_DISPATCH_SECURE
> > else if ret == -EACCES // i.e. i915_parse_cmds() found an MI_BB_S
> > dispatch batch_obj, flags = 0
> > else
> > return error
> >
> > The point is that there's a restriction that chained batches must have
> > the AddressSpace bit set to the same value as the parent batch (i.e.
> > GGTT when batch copy is present). But because of the way libva uses
> > chained batches we can't parse or copy the chained batch to safely put
> > it into GGTT. So we fall back to dispatching the userspace-supplied
> > batch from PPGTT. I should probably have mentioned this restriction in
> > the commit message.
>
> Yeah I've figured that this makes more sense with the actual batch copy.
> Hence the suggestion to just leave out this hunk for now - that shouldn't
> have a functional impact at this stage if I'm not again blind?
Oh, I see. Yeah, we can probably leave this part out of this patch and
just put it in with batch copy. I'll do a quick test and send an updated
patch if it looks good.
Brad
> >
> > As you suggest below, we could instead start to conditionally check
> > batches based on the flag from userspace. However, I thought we had
> > decided not to take that approach in general. Mesa already implements
> > all of the code that they need the command parser for, with a runtime
> > check as to whether hardware will nop their LRI, etc commands. So if
> > we run the parser on all batches, then once we switch to enabling mode
> > features magically work for them. Also, the I915_EXEC_SECURE flag is
> > currently root-only, so there's a bit of a semantic/API/whatever change
> > that we'd have to make there, or add a new flag I suppose. Maybe not a
> > big deal, but I think that the choice of running the parser on all
> > batches is the right direction.
>
> Hm yeah, that's a good reason to just go ahead. If people still hate the
> overhead too much we can just add more flags to execbuf ;-)
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
