[Intel-gfx] [PATCH v10 1/2] drm/i915/guc : Removing enable_guc_loading and enable_guc_submission module parameters

[Joonas Lahtinen]

+ Li (for WOPCM)

On Mon, 2017-11-27 at 11:54 -0800, Sujaritha Sundaresan wrote:
> 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 verify the HuC, which
> is every time we have a HuC (but all platforms with HuC
> have a GuC and viceversa).
> 
> The above module parameters are being replaced by a single
> enable_guc modparam.
> 
> v2: Clarifying the commit message (Anusha)
> 
> v3: Unify seq_puts messages, Re-factoring code as per review (Michal)
> 
> v4: Rebase
> 
> v5: Separating message unification into a separate patch
> 
> v6: Re-factoring code (Sagar, Michal)
>     Rebase
> 
> v7: Applying review comments (Sagar)
>     Rebase
> 
> v8: Change to NEEDS_GUC_FW (Chris)
>     Applying review comments (Michal)
>     Clarifying commit message (Joonas)
> 
> v9: Applying review comments (Michal)
> 
> v10: Introducing enable_guc modparam
> 	 Applying review comments (Michal)
> 
> Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan at intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> Cc: Oscar Mateo <oscar.mateo at intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble at intel.com>

<SNIP>

> @@ -2466,7 +2466,7 @@ static bool check_guc_submission(struct seq_file *m)
>  		seq_printf(m, "GuC submission %s\n",
>  				HAS_GUC(dev_priv) ?
>  				"not supported" :
> -				HAS_GUC_SCHED(dev_priv) ?
> +				NEEDS_GUC_LOAD(dev_priv) ?
>  				"disabled" :
>  				"failed");

I do not quite follow the logic, here. Even the old logic is reversed?

This patch doesn't seem to be on top of drm-tip, so whatever patch this
was written on top of, needs fixing too.

When sending patches, make sure they actually build on top of drm-tip
not to waste the reviewers' time.

> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3220,10 +3220,16 @@ static inline unsigned int i915_sg_segment_size(void)
>   * properties, so we have separate macros to test them.
>   */
>  #define HAS_GUC(dev_priv)	((dev_priv)->info.has_guc)
> +#define HAS_HUC(dev_priv)	(HAS_GUC(dev_priv))
>  #define HAS_GUC_CT(dev_priv)	((dev_priv)->info.has_guc_ct)
> -#define HAS_GUC_UCODE(dev_priv)	(HAS_GUC(dev_priv))
> -#define HAS_GUC_SCHED(dev_priv)	(HAS_GUC(dev_priv))
> -#define HAS_HUC_UCODE(dev_priv)	(HAS_GUC(dev_priv))
> +#define HAS_GUC_FW(dev_priv) \
> +	((dev_priv)->guc.fw.fetch_status == INTEL_UC_FIRMWARE_SUCCESS)
> +#define HAS_HUC_FW(dev_priv) \
> +	((dev_priv)->huc.fw.fetch_status == INTEL_UC_FIRMWARE_SUCCESS)

This is not a good candidate for HAS_* function as it's a rather
dynamic state. Imagine the HAS_*/IS_* functions as something you may
want to freeze at compile state.

> +
> +#define NEEDS_GUC_LOAD(dev_priv) \
> +	(HAS_GUC(dev_priv) && HAS_GUC_FW(dev_priv) && \
> +	(HAS_HUC_FW(dev_priv) || i915_modparams.enable_guc))

This even less so.

> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -316,7 +316,7 @@ static u32 default_desc_template(const struct drm_i915_private *i915,
>  	 * present or not in use we still need a small bias as ring wraparound
>  	 * at offset 0 sometimes hangs. No idea why.
>  	 */
> -	if (HAS_GUC(dev_priv) && i915_modparams.enable_guc_loading)
> +	if (NEEDS_GUC_LOAD(dev_priv))
>  		ctx->ggtt_offset_bias = GUC_WOPCM_TOP;

We should really make up our mind when we actually need to avoid the
WOPCM. According to Li, it's not bound to the usage of the
microcontrollers but rather their existence.

So the correct condition would be just HAS_GUC()?

> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -3503,7 +3503,7 @@ int i915_ggtt_probe_hw(struct drm_i915_private *dev_priv)
>  	 * currently don't have any bits spare to pass in this upper
>  	 * restriction!
>  	 */
> -	if (HAS_GUC(dev_priv) && i915_modparams.enable_guc_loading) {
> +	if (NEEDS_GUC_LOAD(dev_priv)) {
>  		ggtt->base.total = min_t(u64, ggtt->base.total, GUC_GGTT_TOP);
>  		ggtt->mappable_end = min(ggtt->mappable_end, ggtt->base.total);
>  	}

I guess same question applies here. Also, even this function was
correctly named something like intel_guc_needs_loading(), it'd have to
be much less dynamic because we're really not going to run this code
multiple times.

> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -47,38 +47,45 @@ static int __intel_uc_reset_hw(struct drm_i915_private *dev_priv)
>  	return ret;
>  }
>  
> +/*
> + * With enable_guc defined as follows:
> + *
> + * -1=auto [default]
> + *  0=disable GuC loading
> + *  1=enable GuC submission
> + *  2=enable HuC
> + *  3=enable GuC submission and HuC
> + */
> +
>  void intel_uc_sanitize_options(struct drm_i915_private *dev_priv)
>  {
> -	if (!HAS_GUC(dev_priv)) {
> -		if (i915_modparams.enable_guc_loading > 0 ||
> -		    i915_modparams.enable_guc_submission > 0)
> -			DRM_INFO("Ignoring GuC options, no hardware\n");
> -
> -		i915_modparams.enable_guc_loading = 0;
> -		i915_modparams.enable_guc_submission = 0;
> -		return;
> +	int auto_enable_guc = 0;
> +	// note that in the future this can be defined in more granular way
> +	// int auto_enable_guc = !HAS_GUC_FW(dev_priv) ? 0 :
> +	//					IS_GEN9(dev_priv) ? 1 :
> +	//                  IS_GEN10(dev_priv) ? 2:
> +	//                  3;

Please, no C++ style comments (you don't see them around, do you), and
the pseudocode seems overly detailed for pseudocode.

> +
> +	/* Making sure that our auto is well defined */
> +	GEM_BUG_ON(auto_enable_guc < 0);
> +	GEM_BUG_ON((auto_enable_guc > 0) && !HAS_GUC_FW(dev_priv));
> +	GEM_BUG_ON((auto_enable_guc & 2) && !HAS_HUC_FW(dev_priv));
> +
> +	if (i915_modparams.enable_guc < 0)
> +		i915_modparams.enable_guc = auto_enable_guc;
> +
> +	if (i915_modparams.enable_guc > 0) {
> +		if (!HAS_GUC(dev_priv) || !HAS_GUC_FW(dev_priv)) {
> +			DRM_INFO("Ignoring option enable_guc=%d - %s\n",
> +					 i915_modparams.enable_guc,
> +				     !HAS_GUC(dev_priv) ? "no GuC hardware" :
> +					 "no GuC firmware");
> +			i915_modparams.enable_guc = 0;
> +		}

The indents are way off. As this patch is already at revision 10, I'm
stopping here.

I would suggest starting with some smaller and less "controversial"
patches to get to know the codebase, coding style and other concepts.
It will make your life much more pleasant working on patches that make
actual forward progress.

It is not a good learning experience, and must be frustrating to try to
compose a patch entirely based on the review responses without having
the time to get to know the code.

So I suggest Michal or Sagar will take over this patch, giving it an
overhaul. That's the best action I can see for now as this is standing
between drm-tip and the rest of the GuC patches.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation