[Intel-gfx] [PATCH v2] drm/i915: don't whitelist oacontrol in cmd parser

[Daniel Vetter]

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 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