public inbox for intel-gfx@lists.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915/guc: symbolic names for user load/submission preferences
@ 2016-07-11 17:12 Dave Gordon
  2016-07-11 19:58 ` Chris Wilson
  2016-07-12  5:28 ` ✗ Ro.CI.BAT: failure for " Patchwork
  0 siblings, 2 replies; 5+ messages in thread
From: Dave Gordon @ 2016-07-11 17:12 UTC (permalink / raw)
  To: intel-gfx

The existing code that accesses the "enable_guc_loading" and
"enable_guc_submission" parameters uses explicit numerical
values for the various possibilities, including in some cases
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/i915_guc_submission.c |  2 +-
 drivers/gpu/drm/i915/intel_guc.h           | 15 +++++++++++++++
 drivers/gpu/drm/i915/intel_guc_loader.c    | 26 ++++++++++++++------------
 drivers/gpu/drm/i915/intel_lrc.c           |  6 +++---
 4 files changed, 33 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 2112e02..33c0e0ab 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..7ac835c 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -90,6 +90,21 @@ struct i915_guc_client {
 	uint64_t submissions[I915_NUM_ENGINES];
 };
 
+/* These represent user-requested preferences */
+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 605c696..2cd37db 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;
 
@@ -424,7 +424,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) {
@@ -493,7 +493,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;
@@ -519,9 +519,9 @@ 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 > 1) {
+	} else if (i915.enable_guc_submission >= GUC_SUBMISSION_MANDATORY) {
 		ret = -EIO;
 	} else {
 		ret = 0;
@@ -536,7 +536,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)
@@ -544,7 +544,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;
 }
@@ -686,10 +686,12 @@ void intel_guc_init(struct drm_device *dev)
 	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);
-	if (i915.enable_guc_submission < 0)
-		i915.enable_guc_submission = HAS_GUC_SCHED(dev);
+	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;
 
 	if (!HAS_GUC_UCODE(dev)) {
 		fw_path = NULL;
@@ -715,7 +717,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;
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 70c6990..2c530dc 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -719,7 +719,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
@@ -798,7 +798,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);
@@ -992,7 +992,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);
 
 	return 0;
-- 
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] 5+ messages in thread
* Re: [PATCH] drm/i915/guc: Protect against HAS_GUC_* returning true values other than one
@ 2016-07-05 12:32 Dave Gordon
  2016-07-13 13:01 ` [PATCH] drm/i915/guc: symbolic names for user load/submission preferences Dave Gordon
  0 siblings, 1 reply; 5+ messages in thread
From: Dave Gordon @ 2016-07-05 12:32 UTC (permalink / raw)
  To: Tvrtko Ursulin, Intel-gfx

On 05/07/16 12:56, Tvrtko Ursulin wrote:
>
> On 05/07/16 12:50, Dave Gordon wrote:
>> On 04/07/16 15:30, Tvrtko Ursulin wrote:
>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> At the moment HAS_GUC_UCODE == HAS_GUC == IS_GEN9 ==
>>> (INTEL_INFO(dev)->gen_mask & BIT(8)), which is true but not one. And
>>> module parameters are integers and not booleans so compiler will not
>>> normalize the value for us.
>>>
>>> Quick and easy fix for the GuC loading code and the whole area can
>>> be evaluated afterwards.
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Reported-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Dave Gordon <david.s.gordon@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/intel_guc_loader.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c
>>> b/drivers/gpu/drm/i915/intel_guc_loader.c
>>> index d925e2daeb24..72ea5b97e242 100644
>>> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
>>> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
>>> @@ -687,9 +687,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);
>>> +        i915.enable_guc_loading = !!HAS_GUC_UCODE(dev);
>>>       if (i915.enable_guc_submission < 0)
>>> -        i915.enable_guc_submission = HAS_GUC_SCHED(dev);
>>> +        i915.enable_guc_submission = !!HAS_GUC_SCHED(dev);
>>>
>>>       if (!HAS_GUC_UCODE(dev)) {
>>>           fw_path = NULL;
>>
>> Or we could just fix the IS_GENx() macros:
>
> You mean
>
> commit af1346a0f38fe5b762729a91ed10c7c7f59b76c9
> Author: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Date:   Mon Jul 4 15:50:23 2016 +0100
>
>      drm/i915: Explicitly convert some macros to boolean values
>
> :D

Yeah, I was reading email out-of-order. But I like mine better anyway 
(refactor into a single underlying macro, and more parentheses).

BTW I tried

#define	IS_GEN2(dev)	(IS_GEN(dev, 2, 2))

(because the IS_GEN() macro already has the !! booleanisation) but it 
increased the codesize by ~4K. Hence the separate _IS_GEN().

> Still, I think being explicit when assigning boolean type macros to
> integer is a good thing to do. Because I thought true is defined as
> non-zero in C. Unless I am behind the times.
>
> Regards,
> Tvrtko

The *result* of a comparison or other boolean operation is and always 
has been 0-or-1 in C (whereas in BCPL TRUE was -1). It's the *inputs* to 
boolean operations that are tested for zero/nonzero.

OTOH maybe I will change the enable_guc_{loading,submission) values to 
an enum or set of #defines, and then the assignment of the default 
values will use ?: to pick appropriate values.

.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-07-13 13:01 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-07-11 17:12 [PATCH] drm/i915/guc: symbolic names for user load/submission preferences Dave Gordon
2016-07-11 19:58 ` Chris Wilson
2016-07-12 11:49   ` Dave Gordon
2016-07-12  5:28 ` ✗ Ro.CI.BAT: failure for " Patchwork
  -- strict thread matches above, loose matches on Subject: below --
2016-07-05 12:32 [PATCH] drm/i915/guc: Protect against HAS_GUC_* returning true values other than one Dave Gordon
2016-07-13 13:01 ` [PATCH] drm/i915/guc: symbolic names for user load/submission preferences Dave Gordon

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox