From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 02/13] drm/i915: Implement command buffer parsing logic Date: Tue, 4 Feb 2014 20:33:31 +0100 Message-ID: <20140204193331.GZ17001@phenom.ffwll.local> References: <1391032514-19136-3-git-send-email-bradley.d.volkin@intel.com> <20140129222836.GN28110@nuc-i3427.alporthouse.com> <20140130085328.GG17001@phenom.ffwll.local> <20140130090528.GI17001@phenom.ffwll.local> <20140130091206.GK17001@phenom.ffwll.local> <20140130110715.GO17001@phenom.ffwll.local> <20140130180527.GB26895@vpg-ubuntu-bdvolkin> <20140203230019.GA18649@bdvolkin-ubuntu-desktop> <20140204102036.GA17001@phenom.ffwll.local> <20140204184545.GA20309@bdvolkin-ubuntu-desktop> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ee0-f42.google.com (mail-ee0-f42.google.com [74.125.83.42]) by gabe.freedesktop.org (Postfix) with ESMTP id 461A8FA60E for ; Tue, 4 Feb 2014 11:33:35 -0800 (PST) Received: by mail-ee0-f42.google.com with SMTP id b15so2707844eek.1 for ; Tue, 04 Feb 2014 11:33:34 -0800 (PST) Content-Disposition: inline In-Reply-To: <20140204184545.GA20309@bdvolkin-ubuntu-desktop> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: "Volkin, Bradley D" Cc: "intel-gfx@lists.freedesktop.org" List-Id: intel-gfx@lists.freedesktop.org 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