| Message ID | 1462442809-1842-1-git-send-email-kenneth@whitecape.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, May 05, 2016 at 03:06:49AM -0700, Kenneth Graunke wrote: > Allowing register copies where the source and destination are both > whitelisted should be safe, and is useful. For example, Mesa uses > this to load the command streamer math registers with data from the > pipeline statistics counters. > > Signed-off-by: Kenneth Graunke <kenneth@whitecape.org> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_cmd_parser.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c > index 69a1ba8..14f3b44 100644 > --- a/drivers/gpu/drm/i915/i915_cmd_parser.c > +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c > @@ -215,7 +215,8 @@ static const struct drm_i915_cmd_descriptor hsw_render_cmds[] = { HWS RCS only. ? > CMD( MI_RS_CONTEXT, SMI, F, 1, S ), > CMD( MI_LOAD_SCAN_LINES_INCL, SMI, !F, 0x3F, M ), > CMD( MI_LOAD_SCAN_LINES_EXCL, SMI, !F, 0x3F, R ), > - CMD( MI_LOAD_REGISTER_REG, SMI, !F, 0xFF, R ), > + CMD( MI_LOAD_REGISTER_REG, SMI, !F, 0xFF, W, > + .reg = { .offset = 1, .mask = 0x007FFFFC, .step = 1 } ), Bits 22:2 are the address, 0:1 mbz. ? offset=1, length=3, step=1 => check both src/dst against whitelist ? > CMD( MI_RS_STORE_DATA_IMM, SMI, !F, 0xFF, S ), > CMD( MI_LOAD_URB_MEM, SMI, !F, 0xFF, S ), > CMD( MI_STORE_URB_MEM, SMI, !F, 0xFF, S ), > @@ -1113,6 +1114,12 @@ static bool check_cmd(const struct intel_engine_cs *engine, > return false; > } > > + if (desc->cmd.value == MI_LOAD_REGISTER_REG) { > + DRM_DEBUG_DRIVER("CMD: Rejected LRR to masked register 0x%08X\n", > + reg_addr); > + return false; > + } Since we can't inspect the value to see if meets the permitted mask. ? The only thing missing on the checklist would be an igt to load, copy, then store that register back to memory to check this works. -Chris
On Fri, May 06, 2016 at 08:13:22AM -0000, Patchwork wrote: > == Series Details == > > Series: drm/i915: Allow MI_LOAD_REGISTER_REG between whitelisted registers. (rev2) > URL : https://patchwork.freedesktop.org/series/6774/ > State : failure > > == Summary == > > Series 6774v2 drm/i915: Allow MI_LOAD_REGISTER_REG between whitelisted registers. > http://patchwork.freedesktop.org/api/1.0/series/6774/revisions/2/mbox/ > > Test gem_exec_suspend: > Subgroup basic-s3: > skip -> FAIL (bdw-nuci7-2) Impact is minimal, so I've trusted local results and the new test cases. -Chris
diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c index 69a1ba8..14f3b44 100644 --- a/drivers/gpu/drm/i915/i915_cmd_parser.c +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c @@ -215,7 +215,8 @@ static const struct drm_i915_cmd_descriptor hsw_render_cmds[] = { CMD( MI_RS_CONTEXT, SMI, F, 1, S ), CMD( MI_LOAD_SCAN_LINES_INCL, SMI, !F, 0x3F, M ), CMD( MI_LOAD_SCAN_LINES_EXCL, SMI, !F, 0x3F, R ), - CMD( MI_LOAD_REGISTER_REG, SMI, !F, 0xFF, R ), + CMD( MI_LOAD_REGISTER_REG, SMI, !F, 0xFF, W, + .reg = { .offset = 1, .mask = 0x007FFFFC, .step = 1 } ), CMD( MI_RS_STORE_DATA_IMM, SMI, !F, 0xFF, S ), CMD( MI_LOAD_URB_MEM, SMI, !F, 0xFF, S ), CMD( MI_STORE_URB_MEM, SMI, !F, 0xFF, S ), @@ -1113,6 +1114,12 @@ static bool check_cmd(const struct intel_engine_cs *engine, return false; } + if (desc->cmd.value == MI_LOAD_REGISTER_REG) { + DRM_DEBUG_DRIVER("CMD: Rejected LRR to masked register 0x%08X\n", + reg_addr); + return false; + } + if (desc->cmd.value == MI_LOAD_REGISTER_IMM(1) && (offset + 2 > length || (cmd[offset + 1] & reg->mask) != reg->value)) { @@ -1301,6 +1308,7 @@ int i915_cmd_parser_get_version(struct drm_i915_private *dev_priv) * 4. L3 atomic chicken bits of HSW_SCRATCH1 and HSW_ROW_CHICKEN3. * 5. GPGPU dispatch compute indirect registers. * 6. TIMESTAMP register and Haswell CS GPR registers + * 7. Allow MI_LOAD_REGISTER_REG between whitelisted registers. */ - return 6; + return 7; }
Allowing register copies where the source and destination are both whitelisted should be safe, and is useful. For example, Mesa uses this to load the command streamer math registers with data from the pipeline statistics counters. Signed-off-by: Kenneth Graunke <kenneth@whitecape.org> --- drivers/gpu/drm/i915/i915_cmd_parser.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)