[Intel-gfx] [PATCH 02/13] drm/i915: Implement command buffer parsing logic

[Volkin, Bradley D]

On Tue, Feb 04, 2014 at 11:33:31AM -0800, Daniel Vetter wrote:
> On Tue, Feb 04, 2014 at 10:45:45AM -0800, Volkin, Bradley D wrote:
> > The current table structure is that we have tables per-ring and per-gen (plus the table
> > for common MI commands) and all tables are treated as blacklist/greylist. The proposed
> > flow here would indicate that we need tables per-ring, per-client, per-gen and that some
> > would be treated as a whitelist and some as a blacklist/greylist.
> > 
> > I think the benefit to these changes amounts to preventing clients from issuing invalid
> > MI commands, but clients can do that today, so it's not a regression right? It could also
> > make it easier to safely cover new platforms if MI commands were added, though the parser
> > is strictly limited to gen7 today and would need additional work to enable it for new gens.
> 
> The benefit is in enabling future platforms - with an explicit MI
> whitelist we'd forced to re-audit MI_ commands fully and there's no chance
> to miss something. Also the MI_ commands are the tricky ones, so if one
> slips through we have a problem.
> 
> Relying on the command streamer hw to catch invalid opcodes is something
> we need to, so no benefit for that really.
> 
> > The set of MI commands for current gens is small compared to the other command ranges.
> > On the one hand, that makes it easier to create a whitelist of the commands. On the other
> > hand, it also makes it easy to just audit the commands in the spec and validate that the
> > blacklist/greylist covers the potential issues.
> 
> Since we only care about the set of allowed MI commands the list probably
> even shrinks a bit.
> 
> > So, if we maintain the conservative parser enabling and re-audit the lists when enabling
> > new gens, and if we can live with clients issuing totally invalid commands (and I get the
> > feeling that we have to), then I think the current solution gets us the benefits we want
> > with less complexity.
> > 
> > Let me know what you think. I'll continue working through the other feedback either way.
> 
> I didn't really consider that the MI whitelist would have a bigger impact
> on the code really. So if you expect this change to cause a bit a delay in
> getting the next round ready then we can postpone this a bit - for me the
> important part is to get the parser in soonish so that we can start to
> catch regressions (if there are any we haven't considered yet). But
> longer-term I think switching to a whiteliste for MI commands is the right
> approach.

Ok, let me get the next round out and then I'll look at this in more detail.

> 
> > > Iirc we need a special-cases list for blitter commands anyway due to their
> > > irregular lenght, so maybe we could do a full whitelist for that one, too.
> > 
> > All rings have commands with irregular length encodings. I believe they also all have
> > commands with non-fixed lengths (e.g. XY_TEXT_IMMEDIATE_BLT on blt, MEDIA_OBJECT on render).
> 
> Yeah, variable length commands are everywhere, but I've thought commands
> which don't use the usual lenght-2 encoding (i.e. those which are just 1
> dword) are restricted to MI commands and the blitter. Am I mistaken on
> this?

>From what I see, there's the 1-dword MI commands on all rings. MFX_WAIT uses
length-1 on VCS. The blitter looks like length-2 everywhere.

Also, the MI commands and a few places on RCS and BCS use more/fewer bits for
the length field than other commands for that client/subclient.
-Brad

> 
> Cheers, Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch