[Mesa-dev] [PATCH] i965: Skip register write checks if cmd_parser_version >= 2.

[Chris Wilson]

On Tue, Oct 25, 2016 at 05:57:14PM +0200, Daniel Vetter wrote:
> On Wed, Oct 19, 2016 at 02:26:15PM -0700, Kenneth Graunke wrote:
> > On Wednesday, October 19, 2016 8:35:11 PM PDT Chris Wilson wrote:
> > > On Wed, Oct 19, 2016 at 12:25:51PM -0700, Kenneth Graunke wrote:
> > > > If the kernel advertises a new enough command parser version, then we
> > > > can just assume that register writes will work and not bother executing
> > > > commands on the GPU to test it.
> > > 
> > > We do not guarantee that capabilities will not be removed, especially
> > > whitelisted registers, or that all platforms will implement the same set.
> > > -Chris
> > 
> > I only care about Haswell, Ivybridge, and Baytrail here, so "not all
> > platforms will implement the same set" is not a concern.
> > 
> > If you make the command parser start rejecting these, you /will/
> > break/cripple userspace, and as I understand it the kernel community
> > frowns upon that.
> > 
> > Mesa already does a CMD_PARSER_VERSION >= check for:
> > 
> > - Enabling L3 atomics when there's a DC partition (version >= 4)
> > - Enabling ARB_query_buffer_object on Haswell
> >   (need TIMESTAMP register, CS GPR registers, and MI_LRR: version >= 7)
> > 
> > - Enabling hardware conditional rendering support
> >   (need MI_PREDICATE registers, so version >= 2)
> > - Enabling compute shaders and ES3.1 compatibility on Haswell
> >   (need GPGPU dispatch compute indirect registers, so version >= 5)
> > 
> > A few things were implemented before the command parser landed, so they
> > rely on our ad-hoc check alone:
> > 
> > - ARB_transform_feedback{2,3,_instanced}
> > - ARB_draw_indirect
> > - AMD_performance_monitor / INTEL_performance_query
> > 
> > The only reason I implemented it this way is because the kernel did
> > not offer me an interface I could use to query this.  Now it does.
> > I can't see any reason why some basic cases need to guess-and-check
> > while others can use the query.  It should be consistent.
> > 
> > I could perhaps see OACONTROL being removed, since arguably the feature
> > it exposes currently provides broken counter data without Rob's work.
> > But the others...you're not going to remove.
> > 
> > If you start taking away features, the version getparam is entirely
> > meaningless.  We can't infer anything from it, and need to go back
> > to ad-hoc checking of every feature we ever use.  That's asinine.
> 
> Yeah, we won't/can't remove registers (on a given platform at least) from
> the allowed set. Ack on the patch.

The kernel interface is meant to be agnostic to the allowed set. If
there is a demonstrable requirement to remove a register from the
allowed set, e.g. leaks outside of the context image or subverts the
entire platform, we can and should remove it as we would with any other
security hole.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre