| Message ID | 577B9EEE.8060103@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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