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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
