* [PATCH v3 0/4] drm/i915/guc: use symbolic names for module parameter values
@ 2016-07-22 13:32 Dave Gordon
2016-07-22 13:32 ` [PATCH v3 1/4] drm/i915/guc: symbolic names for GuC submission preferences Dave Gordon
` (4 more replies)
0 siblings, 5 replies; 10+ messages in thread
From: Dave Gordon @ 2016-07-22 13:32 UTC (permalink / raw)
To: intel-gfx
There are various literal constants used in the GuC module-parameter
processing code; this sequence of patches replaces them with symbolic
names for greater clarity.
And then it re-enables GuC submission by default :)
v3:
Original patch broken into two (1/4 + 2/4)
Name for GuC log level NONE added (3/4
Re-enable GuC loading & submission (4/4)
Added cover letter :)
Dave Gordon (4):
drm/i915/guc: symbolic names for GuC submission preferences
drm/i915/guc: symbolic names for GuC firmare loading preferences
drm/i915/guc: symbolic name for GuC log-level none
drm/i915/guc: use symbolic names in setting defaults for module
parameters
drivers/gpu/drm/i915/i915_guc_submission.c | 4 ++--
drivers/gpu/drm/i915/i915_params.c | 6 +++---
drivers/gpu/drm/i915/intel_guc.h | 20 ++++++++++++++++++++
drivers/gpu/drm/i915/intel_guc_fwif.h | 1 +
drivers/gpu/drm/i915/intel_guc_loader.c | 30 ++++++++++++++++--------------
drivers/gpu/drm/i915/intel_lrc.c | 6 +++---
6 files changed, 45 insertions(+), 22 deletions(-)
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3 1/4] drm/i915/guc: symbolic names for GuC submission preferences
2016-07-22 13:32 [PATCH v3 0/4] drm/i915/guc: use symbolic names for module parameter values Dave Gordon
@ 2016-07-22 13:32 ` Dave Gordon
2016-08-01 13:54 ` Jani Nikula
2016-07-22 13:32 ` [PATCH v3 2/4] drm/i915/guc: symbolic names for GuC firmare loading preferences Dave Gordon
` (3 subsequent siblings)
4 siblings, 1 reply; 10+ messages in thread
From: Dave Gordon @ 2016-07-22 13:32 UTC (permalink / raw)
To: intel-gfx
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) {
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);
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v3 2/4] drm/i915/guc: symbolic names for GuC firmare loading preferences
2016-07-22 13:32 [PATCH v3 0/4] drm/i915/guc: use symbolic names for module parameter values Dave Gordon
2016-07-22 13:32 ` [PATCH v3 1/4] drm/i915/guc: symbolic names for GuC submission preferences Dave Gordon
@ 2016-07-22 13:32 ` Dave Gordon
2016-07-22 13:32 ` [PATCH v3 3/4] drm/i915/guc: symbolic name for GuC log-level none Dave Gordon
` (2 subsequent siblings)
4 siblings, 0 replies; 10+ messages in thread
From: Dave Gordon @ 2016-07-22 13:32 UTC (permalink / raw)
To: intel-gfx
The existing code that accesses the "enable_guc_loading"
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
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/intel_guc.h | 14 ++++++++++++++
drivers/gpu/drm/i915/intel_guc_loader.c | 13 +++++++------
2 files changed, 21 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index 52ecbba..4963522 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -90,12 +90,26 @@ struct i915_guc_client {
uint64_t submissions[I915_NUM_ENGINES];
};
+/*
+ * 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,
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index d8bd4cb..c2005b5 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -426,7 +426,7 @@ int intel_guc_setup(struct drm_device *dev)
intel_guc_fw_status_repr(guc_fw->guc_fw_load_status));
/* Loading forbidden, or no firmware to load? */
- if (!i915.enable_guc_loading) {
+ if (i915.enable_guc_loading == FIRMWARE_LOAD_DISABLED) {
err = 0;
goto fail;
} else if (fw_path == NULL) {
@@ -521,7 +521,7 @@ int intel_guc_setup(struct drm_device *dev)
* nonfatal error (i.e. it doesn't prevent driver load, but
* marks the GPU as wedged until reset).
*/
- if (i915.enable_guc_loading > 1) {
+ if (i915.enable_guc_loading >= FIRMWARE_LOAD_MANDATORY) {
ret = -EIO;
} else if (i915.enable_guc_submission >= GUC_SUBMISSION_MANDATORY) {
ret = -EIO;
@@ -687,9 +687,10 @@ void intel_guc_init(struct drm_device *dev)
struct intel_guc_fw *guc_fw = &dev_priv->guc.guc_fw;
const char *fw_path;
- /* A negative value means "use platform default" */
- if (i915.enable_guc_loading < 0)
- i915.enable_guc_loading = HAS_GUC_UCODE(dev);
+ /* Any negative value means "use platform default" */
+ if (i915.enable_guc_loading <= FIRMWARE_LOAD_DEFAULT)
+ i915.enable_guc_loading = HAS_GUC_UCODE(dev) ?
+ FIRMWARE_LOAD_PREFERRED : FIRMWARE_LOAD_DISABLED;
if (i915.enable_guc_submission <= GUC_SUBMISSION_DEFAULT)
i915.enable_guc_submission = HAS_GUC_SCHED(dev) ?
GUC_SUBMISSION_PREFERRED : GUC_SUBMISSION_DISABLED;
@@ -718,7 +719,7 @@ void intel_guc_init(struct drm_device *dev)
guc_fw->guc_fw_load_status = GUC_FIRMWARE_NONE;
/* Early (and silent) return if GuC loading is disabled */
- if (!i915.enable_guc_loading)
+ if (i915.enable_guc_loading == FIRMWARE_LOAD_DISABLED)
return;
if (fw_path == NULL)
return;
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v3 3/4] drm/i915/guc: symbolic name for GuC log-level none
2016-07-22 13:32 [PATCH v3 0/4] drm/i915/guc: use symbolic names for module parameter values Dave Gordon
2016-07-22 13:32 ` [PATCH v3 1/4] drm/i915/guc: symbolic names for GuC submission preferences Dave Gordon
2016-07-22 13:32 ` [PATCH v3 2/4] drm/i915/guc: symbolic names for GuC firmare loading preferences Dave Gordon
@ 2016-07-22 13:32 ` Dave Gordon
2016-07-22 13:32 ` [PATCH v3 4/4] drm/i915/guc: use symbolic names in setting defaults for module parameters Dave Gordon
2016-07-22 14:02 ` ✗ Ro.CI.BAT: warning for drm/i915/guc: use symbolic names for module parameter values Patchwork
4 siblings, 0 replies; 10+ messages in thread
From: Dave Gordon @ 2016-07-22 13:32 UTC (permalink / raw)
To: intel-gfx
The existing code that accesses the "guc_log_level" parameter
uses an explicit numerical value for the "no logging" case,
whereas there are symbolic names for the other levels.
So this patch just provides and uses a name for the default log
level (NONE), with the same numeric value that is already used.
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_fwif.h | 1 +
drivers/gpu/drm/i915/intel_guc_loader.c | 2 +-
3 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index e564c976..112e755 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -849,7 +849,7 @@ static void guc_create_log(struct intel_guc *guc)
obj = gem_allocate_guc_obj(dev_priv, size);
if (!obj) {
/* logging will be off */
- i915.guc_log_level = -1;
+ i915.guc_log_level = GUC_LOG_VERBOSITY_NONE;
return;
}
diff --git a/drivers/gpu/drm/i915/intel_guc_fwif.h b/drivers/gpu/drm/i915/intel_guc_fwif.h
index 944786d..bd89f4e 100644
--- a/drivers/gpu/drm/i915/intel_guc_fwif.h
+++ b/drivers/gpu/drm/i915/intel_guc_fwif.h
@@ -132,6 +132,7 @@
#define GUC_LOG_VERBOSITY_HIGH (2 << GUC_LOG_VERBOSITY_SHIFT)
#define GUC_LOG_VERBOSITY_ULTRA (3 << GUC_LOG_VERBOSITY_SHIFT)
/* Verbosity range-check limits, without the shift */
+#define GUC_LOG_VERBOSITY_NONE (-1)
#define GUC_LOG_VERBOSITY_MIN 0
#define GUC_LOG_VERBOSITY_MAX 3
#define GUC_LOG_VERBOSITY_MASK 0x0000000f
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index c2005b5..b9c9f15 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -175,7 +175,7 @@ static void set_guc_init_params(struct drm_i915_private *dev_priv)
params[GUC_CTL_FEATURE] |= GUC_CTL_DISABLE_SCHEDULER |
GUC_CTL_VCS2_ENABLED;
- if (i915.guc_log_level >= 0) {
+ if (i915.guc_log_level > GUC_LOG_VERBOSITY_NONE) {
params[GUC_CTL_LOG_PARAMS] = guc->log_flags;
params[GUC_CTL_DEBUG] =
i915.guc_log_level << GUC_LOG_VERBOSITY_SHIFT;
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v3 4/4] drm/i915/guc: use symbolic names in setting defaults for module parameters
2016-07-22 13:32 [PATCH v3 0/4] drm/i915/guc: use symbolic names for module parameter values Dave Gordon
` (2 preceding siblings ...)
2016-07-22 13:32 ` [PATCH v3 3/4] drm/i915/guc: symbolic name for GuC log-level none Dave Gordon
@ 2016-07-22 13:32 ` Dave Gordon
2016-07-22 14:02 ` ✗ Ro.CI.BAT: warning for drm/i915/guc: use symbolic names for module parameter values Patchwork
4 siblings, 0 replies; 10+ messages in thread
From: Dave Gordon @ 2016-07-22 13:32 UTC (permalink / raw)
To: intel-gfx
Of course, this also re-enables GuC loading and submission
by default on suitable platforms, since it's Intel's Plan
of Record that GuC submission shall be used where available.
Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
drivers/gpu/drm/i915/i915_params.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index b6e404c..16ad975 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -54,9 +54,9 @@ struct i915_params i915 __read_mostly = {
.verbose_state_checks = 1,
.nuclear_pageflip = 0,
.edp_vswing = 0,
- .enable_guc_loading = 0,
- .enable_guc_submission = 0,
- .guc_log_level = -1,
+ .enable_guc_loading = FIRMWARE_LOAD_DEFAULT,
+ .enable_guc_submission = GUC_SUBMISSION_DEFAULT,
+ .guc_log_level = GUC_LOG_VERBOSITY_NONE,
.enable_dp_mst = true,
.inject_load_failure = 0,
.enable_dpcd_backlight = false,
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 10+ messages in thread
* ✗ Ro.CI.BAT: warning for drm/i915/guc: use symbolic names for module parameter values
2016-07-22 13:32 [PATCH v3 0/4] drm/i915/guc: use symbolic names for module parameter values Dave Gordon
` (3 preceding siblings ...)
2016-07-22 13:32 ` [PATCH v3 4/4] drm/i915/guc: use symbolic names in setting defaults for module parameters Dave Gordon
@ 2016-07-22 14:02 ` Patchwork
4 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2016-07-22 14:02 UTC (permalink / raw)
To: Dave Gordon; +Cc: intel-gfx
== Series Details ==
Series: drm/i915/guc: use symbolic names for module parameter values
URL : https://patchwork.freedesktop.org/series/10188/
State : warning
== Summary ==
Series 10188v1 drm/i915/guc: use symbolic names for module parameter values
http://patchwork.freedesktop.org/api/1.0/series/10188/revisions/1/mbox
Test drv_module_reload_basic:
pass -> DMESG-WARN (ro-skl3-i5-6260u)
fi-hsw-i7-4770k total:244 pass:216 dwarn:0 dfail:0 fail:8 skip:20
fi-skl-i5-6260u total:244 pass:224 dwarn:0 dfail:0 fail:8 skip:12
fi-skl-i7-6700k total:244 pass:210 dwarn:0 dfail:0 fail:8 skip:26
fi-snb-i7-2600 total:244 pass:196 dwarn:0 dfail:0 fail:8 skip:40
ro-bdw-i5-5250u total:244 pass:219 dwarn:4 dfail:0 fail:8 skip:13
ro-bdw-i7-5557U total:244 pass:220 dwarn:3 dfail:0 fail:8 skip:13
ro-bdw-i7-5600u total:244 pass:204 dwarn:0 dfail:0 fail:8 skip:32
ro-bsw-n3050 total:218 pass:173 dwarn:0 dfail:0 fail:2 skip:42
ro-byt-n2820 total:244 pass:197 dwarn:0 dfail:0 fail:9 skip:38
ro-hsw-i3-4010u total:244 pass:212 dwarn:0 dfail:0 fail:8 skip:24
ro-hsw-i7-4770r total:244 pass:212 dwarn:0 dfail:0 fail:8 skip:24
ro-ilk-i7-620lm total:244 pass:172 dwarn:0 dfail:0 fail:9 skip:63
ro-ilk1-i5-650 total:239 pass:172 dwarn:0 dfail:0 fail:9 skip:58
ro-ivb-i7-3770 total:244 pass:203 dwarn:0 dfail:0 fail:8 skip:33
ro-skl3-i5-6260u total:244 pass:223 dwarn:1 dfail:0 fail:8 skip:12
ro-snb-i7-2620M total:244 pass:193 dwarn:0 dfail:0 fail:9 skip:42
fi-kbl-qkkr failed to connect after reboot
Results at /archive/results/CI_IGT_test/RO_Patchwork_1578/
7df4039 drm-intel-nightly: 2016y-07m-22d-09h-32m-51s UTC integration manifest
505bcb2 drm/i915/guc: use symbolic names in setting defaults for module parameters
9decbc0 drm/i915/guc: symbolic name for GuC log-level none
6f280db drm/i915/guc: symbolic names for GuC firmare loading preferences
f5fa596 drm/i915/guc: symbolic names for GuC submission preferences
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 1/4] drm/i915/guc: symbolic names for GuC submission preferences
2016-07-22 13:32 ` [PATCH v3 1/4] drm/i915/guc: symbolic names for GuC submission preferences Dave Gordon
@ 2016-08-01 13:54 ` Jani Nikula
2016-08-01 18:57 ` Dave Gordon
0 siblings, 1 reply; 10+ messages in thread
From: Jani Nikula @ 2016-08-01 13:54 UTC (permalink / raw)
To: Dave Gordon, intel-gfx
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);
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 1/4] drm/i915/guc: symbolic names for GuC submission preferences
2016-08-01 13:54 ` Jani Nikula
@ 2016-08-01 18:57 ` Dave Gordon
2016-08-02 7:23 ` Jani Nikula
2016-08-02 9:47 ` Dave Gordon
0 siblings, 2 replies; 10+ messages in thread
From: Dave Gordon @ 2016-08-01 18:57 UTC (permalink / raw)
To: Jani Nikula, intel-gfx
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);
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 1/4] drm/i915/guc: symbolic names for GuC submission preferences
2016-08-01 18:57 ` Dave Gordon
@ 2016-08-02 7:23 ` Jani Nikula
2016-08-02 9:47 ` Dave Gordon
1 sibling, 0 replies; 10+ messages in thread
From: Jani Nikula @ 2016-08-02 7:23 UTC (permalink / raw)
To: Dave Gordon, intel-gfx
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.
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 1/4] drm/i915/guc: symbolic names for GuC submission preferences
2016-08-01 18:57 ` Dave Gordon
2016-08-02 7:23 ` Jani Nikula
@ 2016-08-02 9:47 ` Dave Gordon
1 sibling, 0 replies; 10+ messages in thread
From: Dave Gordon @ 2016-08-02 9:47 UTC (permalink / raw)
To: Jani Nikula, intel-gfx
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.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-08-02 9:47 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-22 13:32 [PATCH v3 0/4] drm/i915/guc: use symbolic names for module parameter values Dave Gordon
2016-07-22 13:32 ` [PATCH v3 1/4] drm/i915/guc: symbolic names for GuC submission preferences Dave Gordon
2016-08-01 13:54 ` Jani Nikula
2016-08-01 18:57 ` Dave Gordon
2016-08-02 7:23 ` Jani Nikula
2016-08-02 9:47 ` Dave Gordon
2016-07-22 13:32 ` [PATCH v3 2/4] drm/i915/guc: symbolic names for GuC firmare loading preferences Dave Gordon
2016-07-22 13:32 ` [PATCH v3 3/4] drm/i915/guc: symbolic name for GuC log-level none Dave Gordon
2016-07-22 13:32 ` [PATCH v3 4/4] drm/i915/guc: use symbolic names in setting defaults for module parameters Dave Gordon
2016-07-22 14:02 ` ✗ Ro.CI.BAT: warning for drm/i915/guc: use symbolic names for module parameter values Patchwork
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.