On Tue, Nov 22, 2016 at 1:34 PM, Daniel Vetter <daniel at ffwll.ch> wrote:
> 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 ;-) > Hmm, they cut away what's above and keep what's below in my experience - what command are you seeing the opposite with? I just double checked this with git am --scissors - Robert > > Fixed up while applying. > -Daniel > > > > > 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 <robert at sixbynine.org> > > Reviewed-by: Matthew Auld <matthew.auld at intel.com> > > Reviewed-by: Sourab Gupta <sourab.gupta at intel.com> > > --- > > 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 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx at lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch > -------------- next part -------------- An HTML attachment was scrubbed... URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20161122/c29c546c/attachment.html>