[Intel-gfx] [PATCH v2 5/7] drm/i915/guc: Combine enable_guc_loading|submission modparams

[Michal Wajdeczko]

On Fri, 01 Dec 2017 13:36:06 +0100, Chris Wilson  
<chris at chris-wilson.co.uk> wrote:

> Quoting Michal Wajdeczko (2017-12-01 10:33:15)
>> We currently have two module parameters that control GuC:
>> "enable_guc_loading" and "enable_guc_submission". Whenever
>> we need submission=1, we also need loading=1. We also need
>> loading=1 when we want to want to load and verify the HuC.
>>
>> Lets combine above module parameters into one "enable_guc"
>> modparam. New supported bit values are:
>>
>>  0=disable GuC (no GuC submission, no HuC)
>>  1=enable GuC submission
>>  2=enable HuC load
>>
>> Special value "-1" can be used to let driver decide what
>> option should be enabled for given platform based on
>> hardware/firmware availability or preference.
>>
>> Explicit enabling any of the GuC features makes GuC load
>> a required step, fallback to non-GuC mode will not be
>> supported.
>>
>> v2: Don't use -EIO
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>> Cc: Sagar Arun Kamble <sagar.a.kamble at intel.com>
>> Cc: Sujaritha Sundaresan <sujaritha.sundaresan at intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h    |   5 +-
>>  drivers/gpu/drm/i915/i915_params.c |  11 ++--
>>  drivers/gpu/drm/i915/i915_params.h |   3 +-
>>  drivers/gpu/drm/i915/intel_uc.c    | 100  
>> +++++++++++++++++++++----------------
>>  4 files changed, 65 insertions(+), 54 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h  
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index 2c386c7..9162a60 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -3238,8 +3238,9 @@ intel_info(const struct drm_i915_private  
>> *dev_priv)
>>  #define HAS_HUC_UCODE(dev_priv)        (HAS_GUC(dev_priv))
>>
>>  /* Having a GuC is not the same as using a GuC */
>> -#define USES_GUC(dev_priv)              
>> (i915_modparams.enable_guc_loading)
>> -#define USES_GUC_SUBMISSION(dev_priv)   
>> (i915_modparams.enable_guc_submission)
>> +#define USES_GUC(dev_priv)             (!!(i915_modparams.enable_guc >  
>> 0))
>> +#define USES_GUC_SUBMISSION(dev_priv)  (!!(i915_modparams.enable_guc &  
>> 1))
>> +#define USES_HUC(dev_priv)             (!!(i915_modparams.enable_guc &  
>> 2))
>
> * channelling Joonas
>
> Please use a magic name for each bit and so
>
> #define USE_GUC_SUBMISSION_BIT 0

I was considering that, but then I decided to follow existing code
(see USES_PPGTT* and enable_ppgtt where we use plain numbers only)

But if we want to start using magic values, then these values should
be defined in i915_params.h and rather in this way:

#define ENABLE_GUC_SUBMISSION   BIT(0)
#define ENABLE_GUC_HUC_LOAD     BIT(1)
         ^^^^^^^^^^
         this part matches modparam name

> #define USES_GUC_SUBMISSION (!!(i915_modparams.enable_guc &  
> BIT(USE_GUC_SUBMISSION_BIT)))
>
> Gah, that's so ugly.
>
> or
>
> static inline bool intel_use_guc_submission(void)

"intel_" ? maybe correct prefix should be "i915_" ?

> {
> 	return i915_modparams.enable_guc & BIT(USE_GUC_SUBMISSION_BIT);
> }

I assume that above will still be wrapped inside macro

#define USES_GUC_SUBMISSION(dev_priv) intel_uses_guc_submission()

>
> which at least stops being so shouty.
> -Chris