[Intel-gfx] [RFC 20/22] drm/i915: Fix MI_STORE_DWORD_IMM parser defintion
From: Brad Volkin bradley.d.vol...@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.
Signed-off-by: Brad Volkin bradley.d.vol...@intel.com
---
drivers/gpu/drm/i915/i915_cmd_parser.c | 35 +++---
1 file changed, 28 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c
b/drivers/gpu/drm/i915/i915_cmd_parser.c
index adc7d94..8481ef0 100644
--- a/drivers/gpu/drm/i915/i915_cmd_parser.c
+++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
@@ -61,13 +61,6 @@ static const struct drm_i915_cmd_descriptor common_cmds[] = {
CMD( MI_REPORT_HEAD, SMI,F, 1, S ),
CMD( MI_SUSPEND_FLUSH, SMI,F, 1, S ),
CMD( MI_SEMAPHORE_MBOX,SMI, !F, 0xFF, R ),
- CMD( MI_STORE_DWORD_IMM, SMI, !F, 0x3FF, B,
- .bits = {{
- .offset = 0,
- .mask = MI_GLOBAL_GTT,
- .expected = 0
- }},
- .bits_count = 1 ),
CMD( MI_STORE_DWORD_INDEX, SMI, !F, 0xFF, R ),
CMD( MI_LOAD_REGISTER_IMM(1), SMI, !F, 0xFF, W,
.reg = { .offset = 1, .mask = 0x007C } ),
@@ -97,6 +90,13 @@ static const struct drm_i915_cmd_descriptor render_cmds[] = {
CMD( MI_DISPLAY_FLIP, SMI, !F, 0xFF, R ),
CMD( MI_PREDICATE, SMI,F, 1, S ),
CMD( MI_TOPOLOGY_FILTER, SMI,F, 1, S ),
+ CMD( MI_STORE_DWORD_IMM, SMI, !F, 0x3F, B,
+ .bits = {{
+ .offset = 0,
+ .mask = MI_GLOBAL_GTT,
+ .expected = 0
+ }},
+ .bits_count = 1 ),
CMD( MI_CLFLUSH, SMI, !F, 0x3FF, B,
.bits = {{
.offset = 0,
@@ -165,6 +165,13 @@ static const struct drm_i915_cmd_descriptor
hsw_render_cmds[] = {
static const struct drm_i915_cmd_descriptor video_cmds[] = {
CMD( MI_ARB_ON_OFF,SMI,F, 1, R ),
+ CMD( MI_STORE_DWORD_IMM, SMI, !F, 0xFF, B,
+ .bits = {{
+ .offset = 0,
+ .mask = MI_GLOBAL_GTT,
+ .expected = 0
+ }},
+ .bits_count = 1 ),
CMD( MI_FLUSH_DW, SMI, !F, 0x3F, B,
.bits = {{
.offset = 0,
@@ -202,6 +209,13 @@ static const struct drm_i915_cmd_descriptor video_cmds[] =
{
static const struct drm_i915_cmd_descriptor vecs_cmds[] = {
CMD( MI_ARB_ON_OFF,SMI,F, 1, R ),
+ CMD( MI_STORE_DWORD_IMM, SMI, !F, 0xFF, B,
+ .bits = {{
+ .offset = 0,
+ .mask = MI_GLOBAL_GTT,
+ .expected = 0
+ }},
+ .bits_count = 1 ),
CMD( MI_FLUSH_DW, SMI, !F, 0x3F, B,
.bits = {{
.offset = 0,
@@ -234,6 +248,13 @@ static const struct drm_i915_cmd_descriptor vecs_cmds[] = {
static const struct drm_i915_cmd_descriptor blt_cmds[] = {
CMD( MI_DISPLAY_FLIP, SMI, !F, 0xFF, R ),
+ CMD( MI_STORE_DWORD_IMM, SMI, !F, 0x1FF, B,
+ .bits = {{
+ .offset = 0,
+ .mask = MI_GLOBAL_GTT,
+ .expected = 0
+ }},
+ .bits_count = 1 ),
CMD( MI_FLUSH_DW, SMI, !F, 0x3F, B,
.bits = {{
.offset = 0,
--
1.8.4.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 20/22] drm/i915: Fix MI_STORE_DWORD_IMM parser defintion
On Tue, Nov 26, 2013 at 08:51:37AM -0800, bradley.d.vol...@intel.com wrote: From: Brad Volkin bradley.d.vol...@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. 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.) 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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 20/22] drm/i915: Fix MI_STORE_DWORD_IMM parser defintion
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.vol...@intel.com wrote: From: Brad Volkin bradley.d.vol...@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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
