Message ID | 20161108125148.25007-1-robert@sixbynine.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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 ;-) 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@sixbynine.org> > Reviewed-by: Matthew Auld <matthew.auld@intel.com> > Reviewed-by: Sourab Gupta <sourab.gupta@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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Nov 22, 2016 at 1:34 PM, Daniel Vetter <daniel@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@sixbynine.org> > > Reviewed-by: Matthew Auld <matthew.auld@intel.com> > > Reviewed-by: Sourab Gupta <sourab.gupta@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@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch >
On Tue, Nov 22, 2016 at 02:09:08PM +0000, Robert Bragg wrote: > On Tue, Nov 22, 2016 at 1:34 PM, Daniel Vetter <daniel@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 Plain git am works the other way round, that's why I never noticed there's a special option. TIL ;-) -Daniel > > - 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@sixbynine.org> > > > Reviewed-by: Matthew Auld <matthew.auld@intel.com> > > > Reviewed-by: Sourab Gupta <sourab.gupta@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@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch > >
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; }