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

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

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