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

[Chris Wilson]

Quoting Michal Wajdeczko (2017-12-01 14:09:31)
> 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)

enable_ppgtt is on my list to kill. If vgpu didn't conspire against us,
it would be simpler!

> 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

Reasonable.

> > #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()

Why? The compiler will dce functions just as well as macros; the age of
the macro is long past, so the question is just a matter of how much is
it worth continuing to cargo-cult a pattern that is long past requirement.

Even its placement in i915_drv.h is up for debate. Maintaining the
status quo is a valid argument, we just need a good reason to switch
patterns (splitting up X-thousand lines of code into manageable chunks
with consistent forward-facing interfaces is one such :).
-Chris