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

[Daniel Vetter]

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.

> > 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?

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