[Intel-gfx] [PATCH v3 1/4] drm/i915/guc: symbolic names for GuC submission preferences

[Jani Nikula]

On Fri, 22 Jul 2016, Dave Gordon <david.s.gordon at intel.com> wrote:
> The existing code that accesses the "enable_guc_submission"
> parameter uses explicit numerical values for the various
> possibilities, including in one case relying on boolean 0/1
> mapping to specific values (which could be confusing for
> maintainers).
>
> So this patch just provides and uses names for the values
> representing the DEFAULT, DISABLED, PREFERRED, and MANDATORY
> submission options that the user can select (-1, 0, 1, 2
> respectively).
>
> This should produce identical code to the previous version!
>
> Signed-off-by: Dave Gordon <david.s.gordon at intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_guc_submission.c |  2 +-
>  drivers/gpu/drm/i915/intel_guc.h           |  6 ++++++
>  drivers/gpu/drm/i915/intel_guc_loader.c    | 15 ++++++++-------
>  drivers/gpu/drm/i915/intel_lrc.c           |  6 +++---
>  4 files changed, 18 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 01c1c16..e564c976 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -971,7 +971,7 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
>  	bitmap_clear(guc->doorbell_bitmap, 0, GUC_MAX_DOORBELLS);
>  	i915_guc_submission_disable(dev_priv);
>  
> -	if (!i915.enable_guc_submission)
> +	if (i915.enable_guc_submission == GUC_SUBMISSION_DISABLED)
>  		return 0; /* not enabled  */
>  
>  	if (guc->ctx_pool_obj)
> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
> index 3e3e743..52ecbba 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -90,6 +90,12 @@ struct i915_guc_client {
>  	uint64_t submissions[I915_NUM_ENGINES];
>  };
>  
> +enum {
> +	GUC_SUBMISSION_DEFAULT = -1,
> +	GUC_SUBMISSION_DISABLED = 0,
> +	GUC_SUBMISSION_PREFERRED,
> +	GUC_SUBMISSION_MANDATORY
> +};
>  enum intel_guc_fw_status {
>  	GUC_FIRMWARE_FAIL = -1,
>  	GUC_FIRMWARE_NONE = 0,
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index b883efd..d8bd4cb 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -189,7 +189,7 @@ static void set_guc_init_params(struct drm_i915_private *dev_priv)
>  	}
>  
>  	/* If GuC submission is enabled, set up additional parameters here */
> -	if (i915.enable_guc_submission) {
> +	if (i915.enable_guc_submission != GUC_SUBMISSION_DISABLED) {
>  		u32 pgs = i915_gem_obj_ggtt_offset(dev_priv->guc.ctx_pool_obj);
>  		u32 ctx_in_16 = GUC_MAX_GPU_CONTEXTS / 16;
>  
> @@ -495,7 +495,7 @@ int intel_guc_setup(struct drm_device *dev)
>  		intel_guc_fw_status_repr(guc_fw->guc_fw_fetch_status),
>  		intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
>  
> -	if (i915.enable_guc_submission) {
> +	if (i915.enable_guc_submission != GUC_SUBMISSION_DISABLED) {
>  		err = i915_guc_submission_enable(dev_priv);
>  		if (err)
>  			goto fail;
> @@ -523,7 +523,7 @@ int intel_guc_setup(struct drm_device *dev)
>  	 */
>  	if (i915.enable_guc_loading > 1) {
>  		ret = -EIO;
> -	} else if (i915.enable_guc_submission > 1) {
> +	} else if (i915.enable_guc_submission >= GUC_SUBMISSION_MANDATORY) {

I like the patches in general, but now these >= and <= seem rather out
of place. How about using == and != exclusively?

BR,
Jani.

>  		ret = -EIO;
>  	} else {
>  		ret = 0;
> @@ -538,7 +538,7 @@ int intel_guc_setup(struct drm_device *dev)
>  	else
>  		DRM_ERROR("GuC firmware load failed: %d\n", err);
>  
> -	if (i915.enable_guc_submission) {
> +	if (i915.enable_guc_submission != GUC_SUBMISSION_DISABLED) {
>  		if (fw_path == NULL)
>  			DRM_INFO("GuC submission without firmware not supported\n");
>  		if (ret == 0)
> @@ -546,7 +546,7 @@ int intel_guc_setup(struct drm_device *dev)
>  		else
>  			DRM_ERROR("GuC init failed: %d\n", ret);
>  	}
> -	i915.enable_guc_submission = 0;
> +	i915.enable_guc_submission = GUC_SUBMISSION_DISABLED;
>  
>  	return ret;
>  }
> @@ -690,8 +690,9 @@ void intel_guc_init(struct drm_device *dev)
>  	/* A negative value means "use platform default" */
>  	if (i915.enable_guc_loading < 0)
>  		i915.enable_guc_loading = HAS_GUC_UCODE(dev);
> -	if (i915.enable_guc_submission < 0)
> -		i915.enable_guc_submission = HAS_GUC_SCHED(dev);
> +	if (i915.enable_guc_submission <= GUC_SUBMISSION_DEFAULT)
> +		i915.enable_guc_submission = HAS_GUC_SCHED(dev) ?
> +			GUC_SUBMISSION_PREFERRED : GUC_SUBMISSION_DISABLED;
>  
>  	if (!HAS_GUC_UCODE(dev)) {
>  		fw_path = NULL;
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index daf1279..960e676 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -716,7 +716,7 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request
>  
>  	request->ringbuf = ce->ringbuf;
>  
> -	if (i915.enable_guc_submission) {
> +	if (i915.enable_guc_submission != GUC_SUBMISSION_DISABLED) {
>  		/*
>  		 * Check that the GuC has space for the request before
>  		 * going any further, as the i915_add_request() call
> @@ -795,7 +795,7 @@ int intel_logical_ring_alloc_request_extras(struct drm_i915_gem_request *request
>  	request->previous_context = engine->last_context;
>  	engine->last_context = request->ctx;
>  
> -	if (i915.enable_guc_submission)
> +	if (i915.enable_guc_submission != GUC_SUBMISSION_DISABLED)
>  		i915_guc_submit(request);
>  	else
>  		execlists_context_queue(request);
> @@ -988,7 +988,7 @@ static int intel_lr_context_pin(struct i915_gem_context *ctx,
>  	ce->state->dirty = true;
>  
>  	/* Invalidate GuC TLB. */
> -	if (i915.enable_guc_submission)
> +	if (i915.enable_guc_submission != GUC_SUBMISSION_DISABLED)
>  		I915_WRITE(GEN8_GTCR, GEN8_GTCR_INVALIDATE);
>  
>  	i915_gem_context_get(ctx);

-- 
Jani Nikula, Intel Open Source Technology Center