<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Tue, Nov 22, 2016 at 1:34 PM, Daniel Vetter <<a href="mailto:daniel@ffwll.ch" target="_blank">daniel@ffwll.ch</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Tue, Nov 08, 2016 at 12:51:48PM +0000, Robert Bragg wrote: > This v2 patch bumps the command parser version so it can be referenced in > corresponding i-g-t gem_exec_parse changes. > > --- >8 --- Scissors cut everything below, not everything above, hence next time around pls switch around your comment and the commit message, as-is not much left ;-) </blockquote><div> </div><div>Hmm, they cut away what's above and keep what's below in my experience - what command are you seeing the opposite with? </div><div>I just double checked this with git am --scissors </div><div>- Robert </div><div> </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Fixed up while applying. -Daniel <div><div class="h5"> > > Being able to program OACONTROL from a non-privileged batch buffer is > not sufficient to be able to configure the OA unit. This was originally > allowed to help enable Mesa to expose OA counters via the > INTEL_performance_query extension, but the current implementation based > on programming OACONTROL via a batch buffer isn't able to report useable > data without a more complete OA unit configuration. Mesa handles the > possibility that writes to OACONTROL may not be allowed and so only > advertises the extension after explicitly testing that a write to > OACONTROL succeeds. Based on this; removing OACONTROL from the whitelist > should be ok for userspace. > > Removing this simplifies adding a new kernel api for configuring the OA > unit without needing to consider the possibility that userspace might > trample on OACONTROL state which we'd like to start managing within > the kernel instead. In particular running any Mesa based GL application > currently results in clearing OACONTROL when initializing which would > disable the capturing of metrics. > > v2: > This bumps the command parser version from 8 to 9, as the change is > visible to userspace. > > Signed-off-by: Robert Bragg <<a href="mailto:robert@sixbynine.org">robert@sixbynine.org</a>> > Reviewed-by: Matthew Auld <<a href="mailto:matthew.auld@intel.com">matthew.auld@intel.com</a>> > Reviewed-by: Sourab Gupta <<a href="mailto:sourab.gupta@intel.com">sourab.gupta@intel.com</a>> > --- > drivers/gpu/drm/i915/i915_cmd_parser.c | 42 ++++------------------------------ > 1 file changed, 5 insertions(+), 37 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c > index c9d2ecd..f5762cd 100644 > --- a/drivers/gpu/drm/i915/i915_cmd_parser.c > +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c > @@ -450,7 +450,6 @@ static const struct drm_i915_reg_descriptor gen7_render_regs[] = { > REG64(PS_INVOCATION_COUNT), > REG64(PS_DEPTH_COUNT), > REG64_IDX(RING_TIMESTAMP, RENDER_RING_BASE), > - REG32(GEN7_OACONTROL), /* Only allowed for LRI and SRM. See below. */ > REG64(MI_PREDICATE_SRC0), > REG64(MI_PREDICATE_SRC1), > REG32(GEN7_3DPRIM_END_OFFSET), > @@ -1060,8 +1059,7 @@ bool intel_engine_needs_cmd_parser(struct intel_engine_cs *engine) > static bool check_cmd(const struct intel_engine_cs *engine, > const struct drm_i915_cmd_descriptor *desc, > const u32 *cmd, u32 length, > - const bool is_master, > - bool *oacontrol_set) > + const bool is_master) > { > if (desc->flags & CMD_DESC_SKIP) > return true; > @@ -1099,31 +1097,6 @@ static bool check_cmd(const struct intel_engine_cs *engine, > } > > /* > - * OACONTROL requires some special handling for > - * writes. We want to make sure that any batch which > - * enables OA also disables it before the end of the > - * batch. The goal is to prevent one process from > - * snooping on the perf data from another process. To do > - * that, we need to check the value that will be written > - * to the register. Hence, limit OACONTROL writes to > - * only MI_LOAD_REGISTER_IMM commands. > - */ > - if (reg_addr == i915_mmio_reg_offset(GEN7_OACONTROL)) { > - if (desc->cmd.value == MI_LOAD_REGISTER_MEM) { > - DRM_DEBUG_DRIVER("CMD: Rejected LRM to OACONTROL\n"); > - return false; > - } > - > - if (desc->cmd.value == MI_LOAD_REGISTER_REG) { > - DRM_DEBUG_DRIVER("CMD: Rejected LRR to OACONTROL\n"); > - return false; > - } > - > - if (desc->cmd.value == MI_LOAD_REGISTER_IMM(1)) > - *oacontrol_set = (cmd[offset + 1] != 0); > - } > - > - /* > * Check the value written to the register against the > * allowed mask/value pair given in the whitelist entry. > */ > @@ -1214,7 +1187,6 @@ int intel_engine_cmd_parser(struct intel_engine_cs *engine, > u32 *cmd, *batch_end; > struct drm_i915_cmd_descriptor default_desc = noop_desc; > const struct drm_i915_cmd_descriptor *desc = &default_desc; > - bool oacontrol_set = false; /* OACONTROL tracking. See check_cmd() */ > bool needs_clflush_after = false; > int ret = 0; > > @@ -1270,8 +1242,7 @@ int intel_engine_cmd_parser(struct intel_engine_cs *engine, > break; > } > > - if (!check_cmd(engine, desc, cmd, length, is_master, > - &oacontrol_set)) { > + if (!check_cmd(engine, desc, cmd, length, is_master)) { > ret = -EACCES; > break; > } > @@ -1279,11 +1250,6 @@ int intel_engine_cmd_parser(struct intel_engine_cs *engine, > cmd += length; > } > > - if (oacontrol_set) { > - DRM_DEBUG_DRIVER("CMD: batch set OACONTROL but did not clear it\n"); > - ret = -EINVAL; > - } > - > if (cmd >= batch_end) { > DRM_DEBUG_DRIVER("CMD: Got to the end of the buffer w/o a BBE cmd!\n"); > ret = -EINVAL; > @@ -1336,6 +1302,8 @@ int i915_cmd_parser_get_version(struct drm_i915_private *dev_priv) > * 8. Don't report cmd_check() failures as EINVAL errors to userspace; > * rely on the HW to NOOP disallowed commands as it would without > * the parser enabled. > + * 9. Don't whitelist or handle oacontrol specially, as ownership > + * for oacontrol state is moving to i915-perf. > */ > - return 8; > + return 9; > } > -- > 2.10.1 > </div></div>> _______________________________________________ > Intel-gfx mailing list > <a href="mailto:Intel-gfx@lists.freedesktop.org">Intel-gfx@lists.freedesktop.org</a> > <a href="https://lists.freedesktop.org/mailman/listinfo/intel-gfx" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/intel-gfx</a> -- Daniel Vetter Software Engineer, Intel Corporation <a href="http://blog.ffwll.ch" rel="noreferrer" target="_blank">http://blog.ffwll.ch</a> </blockquote></div> </div></div>