Re: [PATCH] drm/i915: Add OACONTROL to the command parser register whitelist.

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Fri, 16 May 2014 20:20:50 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Fri, May 16, 2014 at 12:05:45PM -0700, Jesse Barnes wrote:
> > On Thu, 27 Mar 2014 16:22:44 -0700
> > Kenneth Graunke <kenneth@whitecape.org> wrote:
> > 
> > > On 03/27/2014 03:44 PM, Daniel Vetter wrote:
> > > > On Thu, Mar 27, 2014 at 10:34 PM, Kenneth Graunke <kenneth@whitecape.org> wrote:
> > > >> Why are we parsing batches with I915_EXEC_SECURE at all?  Secure batches
> > > >> are only issued from trusted code which is guaranteed to be running as
> > > >> root.  I don't see any benefit to scanning those batches, and there's
> > > >> definitely overhead.
> > > >>
> > > >> I mean, sure, it may be reasonable in the short term as a way to test
> > > >> the command parser, but I certainly hope we don't *ship* that.
> > > > 
> > > > Everyone runs X as root, but I kinda want X to also be able to run as
> > > > non-root. The cmd parser has a special list of drm master register
> > > > lists which should allow this, but if we just bypass the cmd parser
> > > > for all normal X installs we'll have 0 test coverage on this. Which
> > > > means broken like hell.
> > > > 
> > > > Hence I actually intend to ship this, yes. Chris doesn't like it either really.
> > > > -Daniel
> > > 
> > > Seriously?  Hurt performance on every user's system just so you can test
> > > things?  That a classic case of the tail wagging the dog.
> > > 
> > > Why not make a i915.enable_cmd_parser=2 value which enables it all the
> > > time and use that when running igt?  Clearly being able to test this is
> > > valuable, but enabling it universally is *not* OK.
> > 
> > Daniel, I'm not sure what you mean by 0 coverage.  Surely DRI clients
> > count for something?  And X shouldn't be submitting all its batches
> > with the secure bit set, right?  If so, we ought to fix that and only
> > use it for ones where it's necessary (e.g. wait events or similar).  I
> > agree with Ken and Chris here.
> > 
> > Chris?
> 
> We haven't even fixed the major regression from enabling FBC. What's
> another massive slowdown?

I thought you had that fixed in the X driver by avoiding front buffer
rendering altogether.  If that's the case we just need an ioctl to opt
out of front buffer tracking, right?  Presumably that flag would follow
the current DRM_MASTER process...

> Yes, X only sets the secure bit when it pokes the display registers, and
> those registers should be privileged even with a cmd parser in place
> (which they are).
> 
> Daniel's argument presumes that we haven't been patching out the
> cmd parser all this time anyway.

Yeah I know we have some perf issues as it is; it would be nice if the
overhead were so minimal that it didn't matter.  But just on principle,
scanning secure buffers seems wrong, and I'm trying to understand why
Daniel would want it.

-- 
Jesse Barnes, Intel Open Source Technology Center