[Intel-gfx] [RFC 20/22] drm/i915: Fix MI_STORE_DWORD_IMM parser defintion

[Volkin, Bradley D]

On Tue, Nov 26, 2013 at 10:08:48AM -0800, Chris Wilson wrote:
> On Tue, Nov 26, 2013 at 08:51:37AM -0800, bradley.d.volkin at intel.com wrote:
> > From: Brad Volkin <bradley.d.volkin at intel.com>
> > 
> > The length mask is different for each ring and the size can vary,
> > so we should duplicate the definition with the correct encoding
> > for each ring.
> 
> Jumping in here since this highlights the most confusing aspect of this
> series - the meta patching. Please implement the command parsing
> infrastructure upfront and in a very small number of patches (most
> preferably one) that avoids having to add fixes late in the series.

Sure. As this is my first contribution, I'm still working on how to best
split up a series, so I'm happy to clean up that aspect. It sounds like
the series should look more like:
- All parser infrastructure and implementation (basically squash current 1-9,
  plus the bsearch changes, plus REJECT changes)
- N patches to set commands for register whitelist and bitmask checking
- Enable the parser

Correct?

> 
> I think using
> s/S/ALLOW/
> s/R/REJECT/
> s/B/BLACKLIST/
> s/W/WHITELIST/
> makes the action much more clear, and would rather that every unsafe
> command starts off as REJECT. (With the whitelist/blacklisting being
> added as separate patches with justification as they are now.)

I had split out the REJECTs to make the justification easier to find
in the commit message, but I can reject them from the start too.

For 'B', the term 'blacklist' to me implies a set of things that are
unconditionally not allowed, whereas the 'B' commands are conditionally
allowed based on the bitmask checks. Are you just asking for a readability
change in expanding the action names used in the table, or are you looking
for any semantic changes to what the parser checks? Or am I overthinking
this comment?

Brad

> 
> Since we do disable the security, I would also reject all
> unknown/unmatched commands and make ALLOW explicit.
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre