Re: [PATCH 2/5] drm/i915/wopcm: Check WOPCM layout separately from calculations

[Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>]

On 8/15/19 2:48 PM, Michal Wajdeczko wrote:
> We can do WOPCM partitioning using rough estimates and limits
> and perform detailed check as separate step.
> 
> v2: oops! s/max/min
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/intel_wopcm.c | 105 ++++++++++++++++++++---------
>   1 file changed, 74 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_wopcm.c b/drivers/gpu/drm/i915/intel_wopcm.c
> index 2975e00f57f5..39f2764ca3a8 100644
> --- a/drivers/gpu/drm/i915/intel_wopcm.c
> +++ b/drivers/gpu/drm/i915/intel_wopcm.c
> @@ -87,7 +87,8 @@ void intel_wopcm_init_early(struct intel_wopcm *wopcm)
>   	else
>   		wopcm->size = GEN9_WOPCM_SIZE;
>   
> -	DRM_DEBUG_DRIVER("WOPCM size: %uKiB\n", wopcm->size / 1024);
> +	DRM_DEV_DEBUG_DRIVER(i915->drm.dev, "WOPCM: size %uKiB\n",
> +			     wopcm->size / SZ_1K);
>   }
>   
>   static inline u32 context_reserved_size(struct drm_i915_private *i915)
> @@ -138,9 +139,9 @@ static inline int gen9_check_huc_fw_fits(u32 guc_wopcm_size, u32 huc_fw_size)
>   	return 0;
>   }
>   
> -static inline int check_hw_restriction(struct drm_i915_private *i915,
> -				       u32 guc_wopcm_base, u32 guc_wopcm_size,
> -				       u32 huc_fw_size)
> +static inline bool check_hw_restrictions(struct drm_i915_private *i915,
> +					 u32 guc_wopcm_base, u32 guc_wopcm_size,
> +					 u32 huc_fw_size)
>   {
>   	int err = 0;
>   
> @@ -151,7 +152,64 @@ static inline int check_hw_restriction(struct drm_i915_private *i915,
>   	    (IS_GEN(i915, 9) || IS_CNL_REVID(i915, CNL_REVID_A0, CNL_REVID_A0)))
>   		err = gen9_check_huc_fw_fits(guc_wopcm_size, huc_fw_size);
>   
> -	return err;
> +	return !err;
> +}
> +
> +static inline bool __check_layout(struct drm_i915_private *i915, u32 wopcm_size,
> +				  u32 guc_wopcm_base, u32 guc_wopcm_size,
> +				  u32 guc_fw_size, u32 huc_fw_size)
> +{
> +	const u32 ctx_rsvd = context_reserved_size(i915);
> +	u32 size;
> +
> +	if (unlikely(guc_wopcm_base > wopcm_size)) {
> +		dev_err(i915->drm.dev,
> +			"WOPCM: invalid GuC region base: %uK > %uK\n",
> +			guc_wopcm_base / SZ_1K, wopcm_size / SZ_1K);
> +		return false;
> +	}
> +
> +	size = wopcm_size - ctx_rsvd;
> +	if (unlikely(guc_wopcm_base > size)) {
> +		dev_err(i915->drm.dev,
> +			"WOPCM: invalid GuC region base: %uK > %uK\n",
> +			guc_wopcm_base / SZ_1K, size / SZ_1K);
> +		return false;
> +	}
> +
> +	if (unlikely(guc_wopcm_size > wopcm_size)) {
> +		dev_err(i915->drm.dev,
> +			"WOPCM: invalid GuC region size: %uK > %uK\n",
> +			guc_wopcm_size / SZ_1K, wopcm_size / SZ_1K);
> +		return false;
> +	}
> +
> +	size = wopcm_size - guc_wopcm_base - ctx_rsvd;
> +	if (unlikely(guc_wopcm_size > size)) {
> +		dev_err(i915->drm.dev,
> +			"WOPCM: invalid GuC region size: %uK > %uK\n",
> +			guc_wopcm_size / SZ_1K, size / SZ_1K);
> +		return false;
> +	}


I think we can consolidate all the checks above in just:

wopcm_guc_max = wopcm_size - ctx_rsvd;
if (range_overflows(guc_wopcm_base, guc_wopcm_size, wopcm_guc_max)
		return false;


> +
> +	size = guc_fw_size + GUC_WOPCM_RESERVED + GUC_WOPCM_STACK_RESERVED;
> +	if (unlikely(guc_wopcm_size < size)) {
> +		dev_err(i915->drm.dev, "WOPCM: no space for %s: %uK < %uK\n",
> +			intel_uc_fw_type_repr(INTEL_UC_FW_TYPE_GUC),
> +			guc_wopcm_size / SZ_1K, size / SZ_1K);
> +		return false;
> +	}
> +
> +	size = huc_fw_size + WOPCM_RESERVED_SIZE;
> +	if (unlikely(guc_wopcm_base < size)) {
> +		dev_err(i915->drm.dev, "WOPCM: no space for %s: %uK < %uK\n",
> +			intel_uc_fw_type_repr(INTEL_UC_FW_TYPE_HUC),
> +			guc_wopcm_base / SZ_1K, size / SZ_1K);
> +		return false;
> +	}
> +
> +	return check_hw_restrictions(i915, guc_wopcm_base, guc_wopcm_size,
> +				     huc_fw_size);
>   }
>   
>   /**
> @@ -172,8 +230,6 @@ void intel_wopcm_init(struct intel_wopcm *wopcm)
>   	u32 ctx_rsvd = context_reserved_size(i915);
>   	u32 guc_wopcm_base;
>   	u32 guc_wopcm_size;
> -	u32 guc_wopcm_rsvd;
> -	int err;
>   
>   	if (!guc_fw_size)
>   		return;
> @@ -183,39 +239,26 @@ void intel_wopcm_init(struct intel_wopcm *wopcm)
>   	GEM_BUG_ON(wopcm->guc.size);
>   	GEM_BUG_ON(guc_fw_size >= wopcm->size);
>   	GEM_BUG_ON(huc_fw_size >= wopcm->size);
> +	GEM_BUG_ON(ctx_rsvd + WOPCM_RESERVED_SIZE >= wopcm->size);
>   
>   	if (i915_inject_probe_failure(i915))
>   		return;
>   
>   	guc_wopcm_base = ALIGN(huc_fw_size + WOPCM_RESERVED_SIZE,
>   			       GUC_WOPCM_OFFSET_ALIGNMENT);
> -	if ((guc_wopcm_base + ctx_rsvd) >= wopcm->size) {
> -		DRM_ERROR("GuC WOPCM base (%uKiB) is too big.\n",
> -			  guc_wopcm_base / 1024);
> -		return;
> -	}
> -
> +	guc_wopcm_base = min(wopcm->size - ctx_rsvd, guc_wopcm_base);

This line confused me quite a bit until we chatted on IM about it. maybe 
add a comment, e.g.:

/*
  * we want to keep all the checks in the same place to be able to re-use
  * them when we find locked values in WOPCM so we don't validate
  * guc_wopcm_base here, but we still need to clamp it to make sure the
  * following math is sane.
  */

Also, with my suggestion for consolidation above, for the checks we 
always care about wopcm->size - ctx_rsvd, so maybe store that in a local 
var to use it here and below and pass that into __check_layout().

Daniele

>   	guc_wopcm_size = wopcm->size - guc_wopcm_base - ctx_rsvd;
>   	guc_wopcm_size &= GUC_WOPCM_SIZE_MASK;
>   
> -	DRM_DEBUG_DRIVER("Calculated GuC WOPCM Region: [%uKiB, %uKiB)\n",
> -			 guc_wopcm_base / 1024, guc_wopcm_size / 1024);
> +	DRM_DEV_DEBUG_DRIVER(i915->drm.dev,
> +			     "Calculated GuC WOPCM Region: [%uKiB, %uKiB)\n",
> +			     guc_wopcm_base / SZ_1K, guc_wopcm_size / SZ_1K);
>   
> -	guc_wopcm_rsvd = GUC_WOPCM_RESERVED + GUC_WOPCM_STACK_RESERVED;
> -	if ((guc_fw_size + guc_wopcm_rsvd) > guc_wopcm_size) {
> -		DRM_ERROR("Need %uKiB WOPCM for GuC, %uKiB available.\n",
> -			  (guc_fw_size + guc_wopcm_rsvd) / 1024,
> -			  guc_wopcm_size / 1024);
> -		return;
> +	if (__check_layout(i915, wopcm->size, guc_wopcm_base, guc_wopcm_size,
> +			   guc_fw_size, huc_fw_size)) {
> +		wopcm->guc.base = guc_wopcm_base;
> +		wopcm->guc.size = guc_wopcm_size;
> +		GEM_BUG_ON(!wopcm->guc.base);
> +		GEM_BUG_ON(!wopcm->guc.size);
>   	}
> -
> -	err = check_hw_restriction(i915, guc_wopcm_base, guc_wopcm_size,
> -				   huc_fw_size);
> -	if (err)
> -		return;
> -
> -	wopcm->guc.base = guc_wopcm_base;
> -	wopcm->guc.size = guc_wopcm_size;
> -	GEM_BUG_ON(!wopcm->guc.base);
> -	GEM_BUG_ON(!wopcm->guc.size);
>   }
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx