| Message ID | 1469194342-8102-2-git-send-email-david.s.gordon@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 22 Jul 2016, Dave Gordon <david.s.gordon@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@intel.com> > Cc: Tvrtko Ursulin <tvrtko.ursulin@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);
On 01/08/16 14:54, Jani Nikula wrote: > On Fri, 22 Jul 2016, Dave Gordon <david.s.gordon@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@intel.com> >> Cc: Tvrtko Ursulin <tvrtko.ursulin@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); >
On Mon, 01 Aug 2016, Dave Gordon <david.s.gordon@intel.com> wrote: > On 01/08/16 14:54, Jani Nikula wrote: >> On Fri, 22 Jul 2016, Dave Gordon <david.s.gordon@intel.com> wrote: >>> - } 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. I'm not a huge fan of making assumptions about what the user possibly meant when giving incorrect input, "as a convenience". It teaches the user to be sloppy about it, and might lead to super annoying surprises when we actually start using those values for something else. > 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! Oh, completely agreed here, while I didn't spell this out in my first reply. This shouldn't block the patch. BR, Jani.
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@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@intel.com> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@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.
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) { 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);
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@intel.com> Cc: Tvrtko Ursulin <tvrtko.ursulin@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(-)