[Intel-gfx] [PATCH 07/17] drm/i915: Use the precomputed value for whether to enable command parsing

[John Harrison]

On 22/08/2016 09:03, Chris Wilson wrote:
> As i915.enable_cmd_parser is an unsafe option, make it read-only at
> runtime. Now that it is constant, we can use the value determined during
> initialisation as to whether we need the cmdparser at execbuffer time.
I'm confused. I can't see where i915.enable_cmd_parse is used now. It 
does not appear to be mentioned anywhere other than i915_params.c after 
this patch. It is not used to calculate engine->needs_cmd_parser. There 
also seems to be no longer a check for PPGTT either.

>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/i915_cmd_parser.c  | 21 ---------------------
>   drivers/gpu/drm/i915/i915_drv.h         |  1 -
>   drivers/gpu/drm/i915/i915_params.c      |  6 +++---
>   drivers/gpu/drm/i915/i915_params.h      |  2 +-
>   drivers/gpu/drm/i915/intel_ringbuffer.h | 15 +++++++++++++++
>   5 files changed, 19 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c b/drivers/gpu/drm/i915/i915_cmd_parser.c
> index 3c72b3b103e7..f749c8b52351 100644
> --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> @@ -1036,27 +1036,6 @@ unpin_src:
>   	return dst;
>   }
>   
> -/**
> - * intel_engine_needs_cmd_parser() - should a given engine use software
> - *                                   command parsing?
> - * @engine: the engine in question
> - *
> - * Only certain platforms require software batch buffer command parsing, and
> - * only when enabled via module parameter.
> - *
> - * Return: true if the engine requires software command parsing
> - */
> -bool intel_engine_needs_cmd_parser(struct intel_engine_cs *engine)
> -{
> -	if (!engine->needs_cmd_parser)
> -		return false;
> -
> -	if (!USES_PPGTT(engine->i915))
> -		return false;
> -
> -	return (i915.enable_cmd_parser == 1);
> -}
> -
>   static bool check_cmd(const struct intel_engine_cs *engine,
>   		      const struct drm_i915_cmd_descriptor *desc,
>   		      const u32 *cmd, u32 length,
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 5613f2886ad3..049ac53b05da 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3603,7 +3603,6 @@ const char *i915_cache_level_str(struct drm_i915_private *i915, int type);
>   int i915_cmd_parser_get_version(struct drm_i915_private *dev_priv);
>   void intel_engine_init_cmd_parser(struct intel_engine_cs *engine);
>   void intel_engine_cleanup_cmd_parser(struct intel_engine_cs *engine);
> -bool intel_engine_needs_cmd_parser(struct intel_engine_cs *engine);
>   int intel_engine_cmd_parser(struct intel_engine_cs *engine,
>   			    struct drm_i915_gem_object *batch_obj,
>   			    struct drm_i915_gem_object *shadow_batch_obj,
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index e72a41223535..5879a7eae6cc 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -50,7 +50,7 @@ struct i915_params i915 __read_mostly = {
>   	.error_capture = true,
>   	.invert_brightness = 0,
>   	.disable_display = 0,
> -	.enable_cmd_parser = 1,
> +	.enable_cmd_parser = true,
>   	.use_mmio_flip = 0,
>   	.mmio_debug = 0,
>   	.verbose_state_checks = 1,
> @@ -187,9 +187,9 @@ MODULE_PARM_DESC(invert_brightness,
>   module_param_named(disable_display, i915.disable_display, bool, 0400);
>   MODULE_PARM_DESC(disable_display, "Disable display (default: false)");
>   
> -module_param_named_unsafe(enable_cmd_parser, i915.enable_cmd_parser, int, 0600);
> +module_param_named_unsafe(enable_cmd_parser, i915.enable_cmd_parser, bool, 0400);
>   MODULE_PARM_DESC(enable_cmd_parser,
> -		 "Enable command parsing (1=enabled [default], 0=disabled)");
> +		 "Enable command parsing (true=enabled [default], false=disabled)");
>   
>   module_param_named_unsafe(use_mmio_flip, i915.use_mmio_flip, int, 0600);
>   MODULE_PARM_DESC(use_mmio_flip,
> diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
> index 94efc899c1ef..ad0b1d280c2b 100644
> --- a/drivers/gpu/drm/i915/i915_params.h
> +++ b/drivers/gpu/drm/i915/i915_params.h
> @@ -44,7 +44,6 @@ struct i915_params {
>   	int disable_power_well;
>   	int enable_ips;
>   	int invert_brightness;
> -	int enable_cmd_parser;
>   	int enable_guc_loading;
>   	int enable_guc_submission;
>   	int guc_log_level;
> @@ -53,6 +52,7 @@ struct i915_params {
>   	int edp_vswing;
>   	unsigned int inject_load_failure;
>   	/* leave bools at the end to not create holes */
> +	bool enable_cmd_parser;
>   	bool enable_hangcheck;
>   	bool fastboot;
>   	bool prefault_disable;
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 84aea549de5d..c8cbe61deece 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -368,6 +368,21 @@ intel_engine_flag(const struct intel_engine_cs *engine)
>   	return 1 << engine->id;
>   }
>   
> +/**
> + * intel_engine_needs_cmd_parser() - should a given engine use software
> + *                                   command parsing?
> + * @engine: the engine in question
> + *
> + * Only certain platforms require software batch buffer command parsing, and
> + * only when enabled via module parameter.
> + *
> + * Return: true if the engine requires software command parsing
> + */
> +static inline bool intel_engine_needs_cmd_parser(struct intel_engine_cs *engine)
> +{
> +	return engine->needs_cmd_parser;
> +}
> +
>   static inline u32
>   intel_engine_sync_index(struct intel_engine_cs *engine,
>   			struct intel_engine_cs *other)