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

[Dave Gordon]

On 01/08/16 19:57, Dave Gordon wrote:
> 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.

Also, if you look ahead to the next patch, you'll see there's a big 
explanatory comment about the use of signed and ordered enums:

+/*
+ * These signed ranges represent user-requested preferences.
+ * Out-of-range values from the user will be clipped towards
+ * zero: any negative value is treated as -1, anything over 2
+ * is just 2. ANY user-supplied value also taints the kernel.
+ */
  enum {
         GUC_SUBMISSION_DEFAULT = -1,
         GUC_SUBMISSION_DISABLED = 0,
         GUC_SUBMISSION_PREFERRED,
         GUC_SUBMISSION_MANDATORY
  };
+enum {
+       FIRMWARE_LOAD_DEFAULT = -1,
+       FIRMWARE_LOAD_DISABLED = 0,
+       FIRMWARE_LOAD_PREFERRED,
+       FIRMWARE_LOAD_MANDATORY
+};
+
+/* These represent the actual firmware status  */
  enum intel_guc_fw_status {
         GUC_FIRMWARE_FAIL = -1,
         GUC_FIRMWARE_NONE = 0,

.Dave.