On Fri, 22 Jul 2016, Dave Gordon <david.s.gor...@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.gor...@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursu...@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
