[Intel-gfx] [PATCH 1/2] drm/i915: implemented dynamic WOPCM partition.

[Joonas Lahtinen]

On Fri, 2017-11-03 at 17:18 -0700, Jackie Li wrote:
> Static WOPCM partitioning would lead to GuC loading failure
> if the GuC/HuC firmware size exceeded current static 512KB
> partition boundary.
> 
> This patch enabled the dynamical calculation of the WOPCM aperture
> used by GuC/HuC firmware. GuC WOPCM offset was set to
> HuC size + reserved WOPCM size. GuC WOPCM size was set to
> total WOPCM size - GuC WOPCM offset - RC6CTX size.
> 
> Signed-off-by: Jackie Li <yaodong.li at intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble at intel.com>
> Cc: Sujaritha Sundaresan <sujaritha.sundaresan at intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> Cc: John Spotswood <john.a.spotswood at intel.com>
> Cc: Oscar Mateo <oscar.mateo at intel.com>

<SNIP>

> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -623,6 +623,15 @@ static void i915_gem_fini(struct drm_i915_private *dev_priv)
>  	WARN_ON(!list_empty(&dev_priv->contexts.list));
>  }
>  
> +static void i915_wopcm_init(struct drm_i915_private *dev_priv)

Use "struct drm_i915_private *i915" when introducing new code that is
not fiddling with MMIOs.

> +{
> +	struct intel_wopcm_info *wopcm = &dev_priv->wopcm;
> +
> +	wopcm->size = WOPCM_DEFAULT_SIZE;
> +
> +	DRM_DEBUG_DRIVER("WOPCM size: %dKB\n", wopcm->size >> 10);
> +}
> +
>  static int i915_load_modeset_init(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(dev);
> @@ -670,6 +679,12 @@ static int i915_load_modeset_init(struct drm_device *dev)
>  	if (ret)
>  		goto cleanup_irq;
>  
> +	/*
> +	 * Get the wopcm memory info.
> +	 * NOTE: this need to be called before init FW.
> +	 */

Useless comments. Comments should always answer question "why?", not
"what?". And "why?" only needs answering when the code is more obscure
than this. So when the code reads clearly and you don't need comments
inside the function bodies.

> +	i915_wopcm_init(dev_priv);
> +
>  	intel_uc_init_fw(dev_priv);

WOPCM is the reserved memory for the uC's. Why couldn't it be inside
the *_uc_* functions? It's only relevant when you have the uCs.

> +++ b/drivers/gpu/drm/i915/intel_guc.c
> @@ -337,7 +337,7 @@ int intel_guc_resume(struct drm_i915_private *dev_priv)
>   * This is a wrapper to create an object for use with the GuC. In order to
>   * use it inside the GuC, an object needs to be pinned lifetime, so we allocate
>   * both some backing storage and a range inside the Global GTT. We must pin
> - * it in the GGTT somewhere other than than [0, GUC_WOPCM_TOP) because that
> + * it in the GGTT somewhere other than than [0, guc wopcm_top) because that
>   * range is reserved inside GuC.
>   *
>   * Return:	A i915_vma if successful, otherwise an ERR_PTR.
> @@ -358,7 +358,8 @@ struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size)
>  		goto err;
>  
>  	ret = i915_vma_pin(vma, 0, PAGE_SIZE,
> -			   PIN_GLOBAL | PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
> +			   PIN_GLOBAL | PIN_OFFSET_BIAS |
> +			   intel_guc_wopcm_top(dev_priv));

Could read just as "guc->wopcm.top"? Because we're not going to change
it runtime, we could avoid a function. It's a one-off programmable
register after all.

> @@ -373,11 +374,42 @@ struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size)
>  
>  u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv)
>  {
> -	u32 wopcm_size = GUC_WOPCM_TOP;
> +	struct intel_wopcm_partition *wp = &dev_priv->wopcm_partition;
>  
> -	/* On BXT, the top of WOPCM is reserved for RC6 context */
> -	if (IS_GEN9_LP(dev_priv))
> -		wopcm_size -= BXT_GUC_WOPCM_RC6_RESERVED;
> +	GEM_BUG_ON(!wp->guc_wopcm_size);
>  
> -	return wopcm_size;
> +	return wp->guc_wopcm_size;
> +}
> +
> +u32 intel_guc_wopcm_top(struct drm_i915_private *dev_priv)
> +{
> +	struct intel_wopcm_partition *wp = &dev_priv->wopcm_partition;
> +
> +	GEM_BUG_ON(!dev_priv->wopcm.size);
> +
> +	return wp->guc_wopcm_top ? wp->guc_wopcm_top : dev_priv->wopcm.size;
> +}
> +
> +u32 intel_guc_wopcm_offset(struct drm_i915_private *dev_priv)
> +{
> +	struct intel_wopcm_partition *wp = &dev_priv->wopcm_partition;
> +
> +	GEM_BUG_ON(!wp->guc_wopcm_size);
> +
> +	return wp->guc_wopcm_offset;
> +}
> +
> +/*
> + * GuC does not allow any gfx GGTT address that falls into range [0, WOPCM_TOP),
> + * which is reserved for Boot ROM, SRAM and WOPCM. All gfx objects
> + * used by GuC is pinned with PIN_OFFSET_BIAS along with top of WOPCM.
> + */
> +u32 guc_ggtt_offset(struct i915_vma *vma)
> +{
> +	struct drm_i915_private *dev_priv = vma->vm->i915;
> +	u32 offset = i915_ggtt_offset(vma);
> +
> +	GEM_BUG_ON(offset < intel_guc_wopcm_top(dev_priv));
> +	GEM_BUG_ON(range_overflows_t(u64, offset, vma->size, GUC_GGTT_TOP));
> +	return offset;
>  }

Function prefixes and parameters are not following the convention.
There also needs to be proper kerneldoc for newly exposed functions.

Then, to the big question, why not address this the way of just once
calculating the hole? Then simply using the calculated values just like
*stolen_reserved* properties.

Apart from the high level questions, please sync with somebody local to
your office about the i915 coding style conventions, it'll more
efficient to go through rest of the code in more interactive manner for
a better learning experience.

We should try to avoid making even the simplest operations a function
call (especially one that is not inline). And as far as I understand,
the register can be written exactly once so we are not expecting that
dynamic operation.

While going through the code, please also implement the feature of
reading the WOPCM register and going with that value, if it's been
written to already when driver is initializing. We could be the second
driver initialization after 'rmmod i915' for example.

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