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

[Dave Gordon <david.s.gordon@intel.com>]

[-- 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