[PATCH] drm/i915/guc: Protect against HAS_GUC_* returning true values other than one

[PATCH] drm/i915/guc: Protect against HAS_GUC_* returning true values other than one

[Tvrtko Ursulin]

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;
-- 
1.9.1

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

Re: [PATCH] drm/i915/guc: Protect against HAS_GUC_* returning true values other than one

[Chris Wilson]

On Mon, Jul 04, 2016 at 03:30:33PM +0100, 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>
Tested-by: Chris Wilson <chris@chris-wilson.co.uk>

I still have 3 message (not all error though) telling me it failed to
load the firmware...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Ro.CI.BAT: warning for drm/i915/guc: Protect against HAS_GUC_* returning true values other than one

[Patchwork]

== Series Details ==

Series: drm/i915/guc: Protect against HAS_GUC_* returning true values other than one
URL   : https://patchwork.freedesktop.org/series/9473/
State : warning

== Summary ==

Series 9473v1 drm/i915/guc: Protect against HAS_GUC_* returning true values other than one
http://patchwork.freedesktop.org/api/1.0/series/9473/revisions/1/mbox

Test drv_hangman:
        Subgroup error-state-basic:
                fail       -> PASS       (ro-skl3-i5-6260u)
Test drv_module_reload_basic:
                pass       -> DMESG-WARN (ro-bdw-i7-5600u)
                dmesg-fail -> DMESG-WARN (ro-skl3-i5-6260u)
Test gem_busy:
        Subgroup basic-blt:
                skip       -> PASS       (ro-skl3-i5-6260u)
        Subgroup basic-bsd:
                skip       -> PASS       (ro-skl3-i5-6260u)
        Subgroup basic-bsd1:
                skip       -> PASS       (ro-skl3-i5-6260u)
        Subgroup basic-bsd2:
                skip       -> PASS       (ro-skl3-i5-6260u)
        Subgroup basic-parallel-blt:
                skip       -> PASS       (ro-skl3-i5-6260u)
        Subgroup basic-parallel-bsd:
                skip       -> PASS       (ro-skl3-i5-6260u)
        Subgroup basic-parallel-bsd1:
                skip       -> PASS       (ro-skl3-i5-6260u)
        Subgroup basic-parallel-bsd2:
                skip       -> PASS       (ro-skl3-i5-6260u)
        Subgroup basic-parallel-render:
                skip       -> PASS       (ro-skl3-i5-6260u)
        Subgroup basic-parallel-vebox:
                skip       -> PASS       (ro-skl3-i5-6260u)
        Subgroup basic-render:
                skip       -> PASS       (ro-skl3-i5-6260u)
        Subgroup basic-vebox:
                skip       -> PASS       (ro-skl3-i5-6260u)
Test gem_cpu_reloc:
        Subgroup basic:
                fail       -> PASS       (ro-skl3-i5-6260u)
Test gem_cs_tlb:
        Subgroup basic-default:
                fail       -> PASS       (ro-skl3-i5-6260u)
Test gem_ctx_create:
        Subgroup basic-files:
                fail       -> PASS       (ro-skl3-i5-6260u)
Test gem_ctx_exec:
        Subgroup basic:
                fail       -> PASS       (ro-skl3-i5-6260u)
Test gem_ctx_switch:
        Subgroup basic-default:
                skip       -> PASS       (ro-skl3-i5-6260u)
Test gem_exec_basic:
        Subgroup basic-blt:
                fail       -> PASS       (ro-skl3-i5-6260u)
        Subgroup basic-bsd:
                fail       -> PASS       (ro-skl3-i5-6260u)
        Subgroup basic-bsd1:
                fail       -> PASS       (ro-skl3-i5-6260u)
        Subgroup basic-bsd2:
                fail       -> PASS       (ro-skl3-i5-6260u)
        Subgroup basic-default:
                fail       -> PASS       (ro-skl3-i5-6260u)
        Subgroup basic-render:
                fail       -> PASS       (ro-skl3-i5-6260u)
        Subgroup basic-vebox:
                fail       -> PASS       (ro-skl3-i5-6260u)
        Subgroup gtt-blt:
                fail       -> PASS       (ro-skl3-i5-6260u)
        Subgroup gtt-bsd:
                fail       -> PASS       (ro-skl3-i5-6260u)
        Subgroup gtt-bsd1:
                fail       -> PASS       (ro-skl3-i5-6260u)
        Subgroup gtt-bsd2:
                fail       -> PASS       (ro-skl3-i5-6260u)
        Subgroup gtt-default:
                fail       -> PASS       (ro-skl3-i5-6260u)
        Subgroup gtt-render:
                fail       -> PASS       (ro-skl3-i5-6260u)
        Subgroup gtt-vebox:
                fail       -> PASS       (ro-skl3-i5-6260u)
        Subgroup readonly-blt:
                fail       -> PASS       (ro-skl3-i5-6260u)
        Subgroup readonly-bsd:
                fail       -> PASS       (ro-skl3-i5-6260u)
        Subgroup readonly-bsd1:
                fail       -> PASS       (ro-skl3-i5-6260u)
        Subgroup readonly-bsd2:
                fail       -> PASS       (ro-skl3-i5-6260u)
        Subgroup readonly-default:
                fail       -> PASS       (ro-skl3-i5-6260u)
        Subgroup readonly-render:
                fail       -> PASS       (ro-skl3-i5-6260u)
        Subgroup readonly-vebox:
                fail       -> PASS       (ro-skl3-i5-6260u)
Test gem_exec_create:
        Subgroup basic:
                fail       -> PASS       (ro-skl3-i5-6260u)
Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-uc:
                fail       -> DMESG-FAIL (ro-skl3-i5-6260u)
        Subgroup basic-batch-kernel-default-wb:
                fail       -> PASS       (ro-skl3-i5-6260u)
WARNING: Long output truncated
fi-skl-i7-6700k failed to connect after reboot
ro-ivb-i7-3770 failed to connect after reboot

Results at /archive/results/CI_IGT_test/RO_Patchwork_1403/

54c86e3 drm-intel-nightly: 2016y-07m-04d-11h-55m-58s UTC integration manifest
8a49606 drm/i915/guc: Protect against HAS_GUC_* returning true values other than one

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

Re: [PATCH] drm/i915/guc: Protect against HAS_GUC_* returning true values other than one

[Antoine, Peter]

This is not quite right.
If no guc loading then there should be no guc_submission can't have submission without loading.

But, I guess that should be handled later.

Peter.

-----Original Message-----
From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of Tvrtko Ursulin
Sent: Monday, July 4, 2016 3:31 PM
To: Intel-gfx@lists.freedesktop.org
Subject: [Intel-gfx] [PATCH] drm/i915/guc: Protect against HAS_GUC_* returning true values other than one

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;
--
1.9.1

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

Re: [PATCH] drm/i915/guc: Protect against HAS_GUC_* returning true values other than one

[Dave Gordon]

[-- Attachment #1: Type: text/plain, Size: 1454 bytes --]

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:

.Dave.


[-- Attachment #2: 0001-drm-i915-IS_GENx-must-return-bool.patch --]
[-- Type: text/x-patch, Size: 2503 bytes --]

>From 4c82153bd0a520d1d85757ccfc2241776c7634af Mon Sep 17 00:00:00 2001
From: Dave Gordon <david.s.gordon@intel.com>
Date: Tue, 5 Jul 2016 12:11:12 +0100
Subject: [PATCH] drm/i915: IS_GENx() must return bool
Organization: Intel Corporation (UK) Ltd. - Co. Reg. #1134945 - Pipers Way, Swindon SN3 1RJ

Since "ae5702d2 drm/i915: Make IS_GENx macros work on a mask" which
optimised the IS_GENx() macros to perform a simple bitmask operation
rather than an arithmetic comparison, the values of these macros have
been powers-of-2 integers rather than true booleans.

This confuses some code that expects them to be specifically 0 or 1
rather than just 0 or nonzero.

So here we convert all the individual GENx() macros to use a single
underlying common macro, to which we add "!!" to convert the result to
an actual bool. The compiler knows when this actually makes a difference
and doesn't insert any instructions if it only needs a zero/nonzero
test, so this patch increases the binary size by only ~40 bytes total,
for the cases where we actually want the values 0 or 1.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f0b1f43..431d862 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2763,14 +2763,15 @@ struct drm_i915_cmd_table {
  * have their own (e.g. HAS_PCH_SPLIT for ILK+ display, IS_foo for particular
  * chips, etc.).
  */
-#define IS_GEN2(dev)	(INTEL_INFO(dev)->gen_mask & BIT(1))
-#define IS_GEN3(dev)	(INTEL_INFO(dev)->gen_mask & BIT(2))
-#define IS_GEN4(dev)	(INTEL_INFO(dev)->gen_mask & BIT(3))
-#define IS_GEN5(dev)	(INTEL_INFO(dev)->gen_mask & BIT(4))
-#define IS_GEN6(dev)	(INTEL_INFO(dev)->gen_mask & BIT(5))
-#define IS_GEN7(dev)	(INTEL_INFO(dev)->gen_mask & BIT(6))
-#define IS_GEN8(dev)	(INTEL_INFO(dev)->gen_mask & BIT(7))
-#define IS_GEN9(dev)	(INTEL_INFO(dev)->gen_mask & BIT(8))
+#define	_IS_GEN(x, dev)	(!!(INTEL_INFO(dev)->gen_mask & BIT((x)-1)))
+#define IS_GEN2(dev)	_IS_GEN(2, dev)
+#define IS_GEN3(dev)	_IS_GEN(3, dev)
+#define IS_GEN4(dev)	_IS_GEN(4, dev)
+#define IS_GEN5(dev)	_IS_GEN(5, dev)
+#define IS_GEN6(dev)	_IS_GEN(6, dev)
+#define IS_GEN7(dev)	_IS_GEN(7, dev)
+#define IS_GEN8(dev)	_IS_GEN(8, dev)
+#define IS_GEN9(dev)	_IS_GEN(9, dev)
 
 #define ENGINE_MASK(id)	BIT(id)
 #define RENDER_RING	ENGINE_MASK(RCS)
-- 
1.9.1


[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH] drm/i915/guc: Protect against HAS_GUC_* returning true values other than one

[Tvrtko Ursulin]

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

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/guc: Protect against HAS_GUC_* returning true values other than one

[Dave Gordon]

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

[PATCH] drm/i915/guc: symbolic names for user load/submission preferences

[Dave Gordon]

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

✗ Ro.CI.BAT: failure for drm/i915/guc: Protect against HAS_GUC_* returning true values other than one (rev3)

[Patchwork]

== Series Details ==

Series: drm/i915/guc: Protect against HAS_GUC_* returning true values other than one (rev3)
URL   : https://patchwork.freedesktop.org/series/9473/
State : failure

== Summary ==

Applying: drm/i915/guc: Protect against HAS_GUC_* returning true values other than one
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/i915/i915_drv.h
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/i915/i915_drv.h
CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/i915_drv.h
error: Failed to merge in the changes.
Patch failed at 0001 drm/i915/guc: Protect against HAS_GUC_* returning true values other than one
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

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

✗ Ro.CI.BAT: warning for drm/i915/guc: Protect against HAS_GUC_* returning true values other than one (rev4)

[Patchwork]

== Series Details ==

Series: drm/i915/guc: Protect against HAS_GUC_* returning true values other than one (rev4)
URL   : https://patchwork.freedesktop.org/series/9473/
State : warning

== Summary ==

Series 9473v4 drm/i915/guc: Protect against HAS_GUC_* returning true values other than one
http://patchwork.freedesktop.org/api/1.0/series/9473/revisions/4/mbox

Test gem_exec_suspend:
        Subgroup basic-s3:
                pass       -> DMESG-WARN (ro-bdw-i7-5557U)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-a:
                dmesg-warn -> PASS       (ro-skl3-i5-6260u)
                skip       -> DMESG-WARN (ro-bdw-i7-5557U)

fi-kbl-qkkr      total:237  pass:174  dwarn:27  dfail:2   fail:7   skip:27 
fi-skl-i5-6260u  total:237  pass:189  dwarn:27  dfail:2   fail:7   skip:12 
fi-skl-i7-6700k  total:237  pass:200  dwarn:2   dfail:0   fail:9   skip:26 
ro-bdw-i5-5250u  total:237  pass:210  dwarn:2   dfail:0   fail:9   skip:16 
ro-bdw-i7-5557U  total:237  pass:210  dwarn:3   dfail:0   fail:9   skip:15 
ro-bsw-n3050     total:217  pass:170  dwarn:0   dfail:0   fail:4   skip:42 
ro-skl3-i5-6260u total:237  pass:213  dwarn:3   dfail:0   fail:9   skip:12 
fi-snb-i7-2600 failed to connect after reboot
ro-bdw-i7-5600u failed to connect after reboot
ro-byt-n2820 failed to connect after reboot
ro-hsw-i3-4010u failed to connect after reboot
ro-hsw-i7-4770r failed to connect after reboot
ro-ilk1-i5-650 failed to connect after reboot
ro-ilk-i7-620lm failed to connect after reboot
ro-ivb-i7-3770 failed to connect after reboot
ro-snb-i7-2620M failed to connect after reboot

Results at /archive/results/CI_IGT_test/RO_Patchwork_1483/

e8b1f0b drm-intel-nightly: 2016y-07m-13d-10h-57m-19s UTC integration manifest
40c374e drm/i915/guc: symbolic names for user load/submission preferences

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

Re: ✗ Ro.CI.BAT: warning for drm/i915/guc: Protect against HAS_GUC_* returning true values other than one (rev4)

[Dave Gordon]

On 13/07/16 14:38, Patchwork wrote:
> == Series Details ==
>
> Series: drm/i915/guc: Protect against HAS_GUC_* returning true values other than one (rev4)
> URL   : https://patchwork.freedesktop.org/series/9473/
> State : warning
>
> == Summary ==
>
> Series 9473v4 drm/i915/guc: Protect against HAS_GUC_* returning true values other than one
> http://patchwork.freedesktop.org/api/1.0/series/9473/revisions/4/mbox
>
> Test gem_exec_suspend:
>          Subgroup basic-s3:
>                  pass       -> DMESG-WARN (ro-bdw-i7-5557U)
> Test kms_pipe_crc_basic:
>          Subgroup suspend-read-crc-pipe-a:
>                  dmesg-warn -> PASS       (ro-skl3-i5-6260u)
>                  skip       -> DMESG-WARN (ro-bdw-i7-5557U)

Both of these look like
https://bugs.freedesktop.org/show_bug.cgi?id=96614
[BAT BDW] *ERROR* failed to enable link training/failed to start channel 
equalization

.Dave.

> fi-kbl-qkkr      total:237  pass:174  dwarn:27  dfail:2   fail:7   skip:27
> fi-skl-i5-6260u  total:237  pass:189  dwarn:27  dfail:2   fail:7   skip:12
> fi-skl-i7-6700k  total:237  pass:200  dwarn:2   dfail:0   fail:9   skip:26
> ro-bdw-i5-5250u  total:237  pass:210  dwarn:2   dfail:0   fail:9   skip:16
> ro-bdw-i7-5557U  total:237  pass:210  dwarn:3   dfail:0   fail:9   skip:15
> ro-bsw-n3050     total:217  pass:170  dwarn:0   dfail:0   fail:4   skip:42
> ro-skl3-i5-6260u total:237  pass:213  dwarn:3   dfail:0   fail:9   skip:12
> fi-snb-i7-2600 failed to connect after reboot
> ro-bdw-i7-5600u failed to connect after reboot
> ro-byt-n2820 failed to connect after reboot
> ro-hsw-i3-4010u failed to connect after reboot
> ro-hsw-i7-4770r failed to connect after reboot
> ro-ilk1-i5-650 failed to connect after reboot
> ro-ilk-i7-620lm failed to connect after reboot
> ro-ivb-i7-3770 failed to connect after reboot
> ro-snb-i7-2620M failed to connect after reboot
>
> Results at /archive/results/CI_IGT_test/RO_Patchwork_1483/
>
> e8b1f0b drm-intel-nightly: 2016y-07m-13d-10h-57m-19s UTC integration manifest
> 40c374e drm/i915/guc: symbolic names for user load/submission preferences
>

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