[Intel-gfx] [PATCH v3 1/4] drm/i915/guc: symbolic names for GuC submission preferences
Dave Gordon
david.s.gordon at intel.com
Mon Aug 1 18:57:54 UTC 2016
On 01/08/16 14:54, Jani Nikula wrote:
> 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.
That would leave us with undefined behaviour for values outside the
recognised range. This way it clips out-of-range values to the nearest
extremum. Of course we could make it fail completely for invalid values,
but that's just really annoying for the developer or admin who's
mistyped -1 as -2 or forgotten what the maximum supported value is in
this release. Alternatively we could convert all out-of-range values to
"system default" i.e. ignored, which might still be annoying but not
quite as much.
Any other suggestions for how to handle out-of-range values?
But if we were changing the policy shouldn't that be a separate patch?
This patch is supposed to change only the way the code is written, with
no effect to existing behaviour!
.Dave.
>> 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);
>
More information about the Intel-gfx
mailing list