[Intel-gfx] [PATCH 5/8] drm/i915/guc: Use GuC FW size and HW restrictions to determine WOPCM partition

[Yaodong Li]

On 03/23/2018 05:34 AM, Michał Winiarski wrote:
> We need GuC to load HuC, but it's also possible for GuC to operate on
> its own. We don't know what the size of HuC FW may be, so, not wanting
> to make any platform-specific hardcoded guesses, we assume that its size
> is 0... Which is a very bad approximation.
> This has a very unfortunate consequence - once we've booted with GuC and
> no HuC, we'll never be able to load HuC (unless we reboot, since the
> registers are locked).
> Rather than using unknown values in our computations - let's partition
> based on GuC size.
we can do this based on known GuC and HuC sizes without
any assumption on FW sizes :)
please also refer to: https://patchwork.freedesktop.org/series/40541/
> We have one HW restriction where we're using HuC size (GuC size needs to
> be roughly similar to HuC size - which may be unknown at this point),
> luckily, another HW restriction makes it very unlikely to run in any
> sort of issues in this case.
>
> Signed-off-by: Michał Winiarski <michal.winiarski at intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Jackie Li <yaodong.li at intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> ---
>   drivers/gpu/drm/i915/intel_wopcm.c | 60 +++++++++++++++++++++-----------------
>   1 file changed, 34 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_wopcm.c b/drivers/gpu/drm/i915/intel_wopcm.c
> index 52841d340002..295d302e97b9 100644
> --- a/drivers/gpu/drm/i915/intel_wopcm.c
> +++ b/drivers/gpu/drm/i915/intel_wopcm.c
> @@ -167,7 +167,22 @@ static int check_ctx_rsvd_fits(struct intel_wopcm *wopcm, u32 ctx_rsvd)
>   	return 0;
>   }
>   
> -static int wopcm_check_hw_restrictions(struct intel_wopcm *wopcm)
> +static inline void
> +__guc_region_grow(struct intel_wopcm *wopcm, u32 size)
> +{
> +	/*
> +	 * We're growing guc region in the direction of lower addresses.
> +	 * We need to use multiples of base alignment, because it has more
> +	 * strict alignment rules.
> +	 */
> +	size = DIV_ROUND_UP(size, 2);
A bit confused here. Don't we just want to push the base downward for
*size* bytes?
> +	size = ALIGN(size, GUC_WOPCM_OFFSET_ALIGNMENT);
Hmm, I think we only need align it to 4K boundary.
> +
> +	wopcm->guc.base -= size;
> +	wopcm->guc.size += size;
> +}
> +
> +static void wopcm_adjust_for_hw_restrictions(struct intel_wopcm *wopcm)
>   {
>   	struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
>   	u32 huc_fw_size = intel_uc_fw_get_upload_size(&i915->huc.fw);
> @@ -177,22 +192,18 @@ static int wopcm_check_hw_restrictions(struct intel_wopcm *wopcm)
>   		size = gen9_size_for_dword_gap_restriction(wopcm->guc.base,
>   							   wopcm->guc.size);
>   		if (size)
> -			goto err;
> +			__guc_region_grow(wopcm, size);
>   
>   		size = gen9_size_for_huc_restriction(wopcm->guc.size,
>   						     huc_fw_size);
Note that huc_fw_size might be zero in enable_guc=1 (GuC only) case.
Once the registers were locked, enable_guc=3 may still fail since 
huc_fw_size
may still large enough to break the restriction we want to enforce that:
hw_fw_size + 16KB > guc_fw_size.
>   		if (size)
> -			goto err;
> -	}
> -
> -	return 0;
> +			__guc_region_grow(wopcm, size);
>   
> -err:
> -	DRM_ERROR("GuC WOPCM size %uKiB is too small. %uKiB more needed.\n",
> -		  wopcm->guc.size / 1024,
> -		  size / 1024);
> -
> -	return -E2BIG;
> +		GEM_BUG_ON(gen9_size_for_dword_gap_restriction(wopcm->guc.base,
> +							       wopcm->guc.size));
> +		GEM_BUG_ON(gen9_size_for_huc_restriction(wopcm->guc.size,
> +							 huc_fw_size));
> +	}
>   }
>   
>   static bool wopcm_check_components_fit(struct intel_wopcm *wopcm)
> @@ -218,21 +229,16 @@ static bool wopcm_check_components_fit(struct intel_wopcm *wopcm)
>   	return 0;
>   }
>   
> -static int wopcm_guc_init(struct intel_wopcm *wopcm)
> +static int wopcm_guc_region_init(struct intel_wopcm *wopcm)
>   {
>   	struct drm_i915_private *dev_priv = wopcm_to_i915(wopcm);
> -	u32 huc_fw_size = intel_uc_fw_get_upload_size(&dev_priv->huc.fw);
> +	u32 guc_fw_size = intel_uc_fw_get_upload_size(&dev_priv->guc.fw);
>   	u32 ctx_rsvd = context_reserved_size(dev_priv);
>   
> -	wopcm->guc.base = ALIGN_DOWN(huc_fw_size_in_wopcm(huc_fw_size),
> -				     GUC_WOPCM_OFFSET_ALIGNMENT);
> +	wopcm->guc.size = guc_fw_size_in_wopcm(guc_fw_size);
>   
> -	wopcm->guc.size = ALIGN(wopcm->size - wopcm->guc.base - ctx_rsvd,
> -				PAGE_SIZE);
> -
> -	DRM_DEBUG_DRIVER("GuC WOPCM Region: [%uKiB, %uKiB)\n",
> -			 wopcm->guc.base / 1024,
> -			 (wopcm->guc.base + wopcm->guc.size) / 1024);
> +	wopcm->guc.base = ALIGN_DOWN(wopcm->size - ctx_rsvd - wopcm->guc.size,
> +				     GUC_WOPCM_OFFSET_ALIGNMENT);
guc.size + ctx_rsvd > wopcm->size check?
>   
>   	return 0;
>   }
> @@ -255,18 +261,20 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
>   
>   	GEM_BUG_ON(!wopcm->size);
>   
> -	err = wopcm_guc_init(wopcm);
> +	err = wopcm_guc_region_init(wopcm);
Again, wopcm will contain invalid data on failure. But maybe we can
go with it now :)

Regards,
-Jackie