Re: [PATCH v3 3/4] drm/i915: Add code to accept valid locked WOPCM register values

[John Spotswood <john.a.spotswood@intel.com>]

On Mon, 2018-04-09 at 17:42 -0700, Jackie Li wrote:
> In current code, we only compare the locked WOPCM register values
> with the
> calculated values. However, we can continue loading GuC/HuC firmware
> if the
> locked (or partially locked) values were valid for current GuC/HuC
> firmware
> sizes.
> 
> This patch added a new code path to verify whether the locked
> register
> values can be used for GuC/HuC firmware loading, it will recalculate
> the
> verify the new values if these registers were partially locked, so
> that we
> won't fail the GuC/HuC firmware loading even if the locked register
> values
> are different from the calculated ones.
> 
> v2:
>  - Update WOPCM register only if it's not locked
> 
> Signed-off-by: Jackie Li <yaodong.li@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Cc: John Spotswood <john.a.spotswood@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Reviewed-by: John Spotswood <john.a.spotswood@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_wopcm.c | 217
> +++++++++++++++++++++++++++++++------
>  1 file changed, 185 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_wopcm.c
> b/drivers/gpu/drm/i915/intel_wopcm.c
> index b1c08ca..fa8d2be 100644
> --- a/drivers/gpu/drm/i915/intel_wopcm.c
> +++ b/drivers/gpu/drm/i915/intel_wopcm.c
> @@ -140,6 +140,53 @@ static inline int check_hw_restriction(struct
> drm_i915_private *i915,
>  	return err;
>  }
>  
> +static inline u32 calculate_min_guc_wopcm_base(u32 huc_fw_size)
> +{
> +	return ALIGN(huc_fw_size + WOPCM_RESERVED_SIZE,
> +		     GUC_WOPCM_OFFSET_ALIGNMENT);
> +}
> +
> +static inline u32 calculate_min_guc_wopcm_size(u32 guc_fw_size)
> +{
> +	return guc_fw_size + GUC_WOPCM_RESERVED +
> GUC_WOPCM_STACK_RESERVED;
> +}
> +
> +static inline int calculate_max_guc_wopcm_size(struct intel_wopcm
> *wopcm,
> +					       u32 guc_wopcm_base,
> +					       u32 *guc_wopcm_size)
> +{
> +	struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
> +	u32 ctx_rsvd = context_reserved_size(i915);
> +
> +	if (guc_wopcm_base >= wopcm->size ||
> +	    (guc_wopcm_base + ctx_rsvd) >= wopcm->size) {
> +		DRM_ERROR("GuC WOPCM base (%uKiB) is too big.\n",
> +			  guc_wopcm_base / 1024);
> +		return -E2BIG;
> +	}
> +
> +	*guc_wopcm_size =
> +		(wopcm->size - guc_wopcm_base - ctx_rsvd) &
> GUC_WOPCM_SIZE_MASK;
> +
> +	return 0;
> +}
> +
> +static inline int verify_calculated_values(struct drm_i915_private
> *i915,
> +					   u32 guc_fw_size, u32
> huc_fw_size,
> +					   u32 guc_wopcm_base,
> +					   u32 guc_wopcm_size)
> +{
> +	if (guc_wopcm_size <
> calculate_min_guc_wopcm_size(guc_fw_size)) {
> +		DRM_ERROR("Need %uKiB WOPCM for GuC FW, %uKiB
> available.\n",
> +			  calculate_min_guc_wopcm_size(guc_fw_size),
> +			  guc_wopcm_size / 1024);
> +		return -E2BIG;
> +	}
> +
> +	return check_hw_restriction(i915, guc_wopcm_base,
> guc_wopcm_size,
> +				    huc_fw_size);
> +}
> +
>  /**
>   * intel_wopcm_init() - Initialize the WOPCM structure.
>   * @wopcm: pointer to intel_wopcm.
> @@ -157,10 +204,8 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
>  	struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
>  	u32 guc_fw_size = intel_uc_fw_get_upload_size(&i915-
> >guc.fw);
>  	u32 huc_fw_size = intel_uc_fw_get_upload_size(&i915-
> >huc.fw);
> -	u32 ctx_rsvd = context_reserved_size(i915);
>  	u32 guc_wopcm_base;
>  	u32 guc_wopcm_size;
> -	u32 guc_wopcm_rsvd;
>  	int err;
>  
>  	GEM_BUG_ON(!wopcm->size);
> @@ -177,35 +222,121 @@ int intel_wopcm_init(struct intel_wopcm
> *wopcm)
>  		return -E2BIG;
>  	}
>  
> -	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);
> +	guc_wopcm_base = calculate_min_guc_wopcm_base(huc_fw_size);
> +	err = calculate_max_guc_wopcm_size(wopcm, guc_wopcm_base,
> +					   &guc_wopcm_size);
> +	if (err)
> +		return err;
> +
> +	DRM_DEBUG_DRIVER("Calculated GuC WOPCM Region: [%uKiB,
> %uKiB)\n",
> +			 guc_wopcm_base / 1024,
> +			 (guc_wopcm_base + guc_wopcm_size) / 1024);
> +
> +	err = verify_calculated_values(i915, guc_fw_size,
> huc_fw_size,
> +				       guc_wopcm_base,
> guc_wopcm_size);
> +	if (err)
> +		return err;
> +
> +	wopcm->guc.base = guc_wopcm_base;
> +	wopcm->guc.size = guc_wopcm_size;
> +
> +	return 0;
> +}
> +
> +static inline int verify_locked_values(struct intel_wopcm *wopcm,
> +				       u32 guc_wopcm_base, u32
> guc_wopcm_size)
> +{
> +	struct drm_i915_private *i915 = wopcm_to_i915(wopcm);
> +	u32 guc_fw_size = intel_uc_fw_get_upload_size(&i915-
> >guc.fw);
> +	u32 huc_fw_size = intel_uc_fw_get_upload_size(&i915-
> >huc.fw);
> +	u32 ctx_rsvd = context_reserved_size(i915);
> +
> +	/*
> +	 * Locked GuC WOPCM base and size could be any values which
> are large
> +	 * enough to lead to a wrap around after performing add
> operations.
> +	 */
> +	if (guc_wopcm_base >= wopcm->size) {
> +		DRM_ERROR("Locked base value %uKiB >= WOPCM size
> %uKiB.\n",
> +			  guc_wopcm_base / 1024,
> +			  wopcm->size / 1024);
>  		return -E2BIG;
>  	}
>  
> -	guc_wopcm_size = wopcm->size - guc_wopcm_base - ctx_rsvd;
> -	guc_wopcm_size &= GUC_WOPCM_SIZE_MASK;
> +	if (guc_wopcm_size >= wopcm->size) {
> +		DRM_ERROR("Locked size value %uKiB >= WOPCM size
> %uKiB.\n",
> +			  guc_wopcm_base / 1024,
> +			  wopcm->size / 1024);
> +		return -E2BIG;
> +	}
>  
> -	DRM_DEBUG_DRIVER("Calculated GuC WOPCM Region: [%uKiB,
> %uKiB)\n",
> -			 guc_wopcm_base / 1024, guc_wopcm_size /
> 1024);
> +	if (guc_wopcm_base + guc_wopcm_size + ctx_rsvd > wopcm-
> >size) {
> +		DRM_ERROR("Need %uKiB WOPCM in total, %uKiB
> available.\n",
> +			  (guc_wopcm_base + guc_wopcm_size +
> ctx_rsvd) / 1024,
> +			  (wopcm->size) / 1024);
> +		return -E2BIG;
> +	}
>  
> -	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);
> +	if (guc_wopcm_base <
> calculate_min_guc_wopcm_base(huc_fw_size)) {
> +		DRM_ERROR("Need %uKiB WOPCM for HuC FW, %uKiB
> available.\n",
> +			  calculate_min_guc_wopcm_base(huc_fw_size)
> / 1024,
> +			  guc_wopcm_base / 1024);
>  		return -E2BIG;
>  	}
>  
> -	err = check_hw_restriction(i915, guc_wopcm_base,
> guc_wopcm_size,
> -				   huc_fw_size);
> +	return verify_calculated_values(i915, guc_fw_size,
> huc_fw_size,
> +					guc_wopcm_base,
> guc_wopcm_size);
> +}
> +
> +static inline int verify_locked_values_and_update(struct intel_wopcm
> *wopcm,
> +						  bool
> *update_size_reg,
> +						  bool
> *update_offset_reg)
> +{
> +	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 size_val = I915_READ(GUC_WOPCM_SIZE);
> +	u32 offset_val = I915_READ(DMA_GUC_WOPCM_OFFSET);
> +	bool offset_reg_locked = offset_val &
> GUC_WOPCM_OFFSET_VALID;
> +	bool size_reg_locked = size_val & GUC_WOPCM_SIZE_LOCKED;
> +	u32 guc_wopcm_base = offset_val & GUC_WOPCM_OFFSET_MASK;
> +	u32 guc_wopcm_size = size_val & GUC_WOPCM_SIZE_MASK;
> +	int err;
> +
> +	*update_size_reg = !size_reg_locked;
> +	*update_offset_reg = !offset_reg_locked;
> +
> +	if (!offset_reg_locked && !size_reg_locked)
> +		return 0;
> +
> +	if (guc_wopcm_base == wopcm->guc.base &&
> +	    guc_wopcm_size == wopcm->guc.size)
> +		return 0;
> +
> +	if (USES_HUC(dev_priv) && offset_reg_locked &&
> +	    !(offset_val & HUC_LOADING_AGENT_GUC)) {
> +		DRM_ERROR("HUC_LOADING_AGENT_GUC isn't locked for
> USES_HUC.\n");
> +		return -EIO;
> +	}
> +
> +	if (!offset_reg_locked)
> +		guc_wopcm_base =
> calculate_min_guc_wopcm_base(huc_fw_size);
> +
> +	if (!size_reg_locked) {
> +		err = calculate_max_guc_wopcm_size(wopcm,
> guc_wopcm_base,
> +						   &guc_wopcm_size);
> +		if (err)
> +			return err;
> +	}
> +
> +	DRM_DEBUG_DRIVER("Recalculated GuC WOPCM Region: [%uKiB,
> %uKiB)\n",
> +			 guc_wopcm_base / 1024,
> +			 (guc_wopcm_base + guc_wopcm_size) / 1024);
> +
> +	err = verify_locked_values(wopcm, guc_wopcm_base,
> guc_wopcm_size);
>  	if (err)
>  		return err;
>  
> -	wopcm->guc.base = guc_wopcm_base;
>  	wopcm->guc.size = guc_wopcm_size;
> +	wopcm->guc.base = guc_wopcm_base;
>  
>  	return 0;
>  }
> @@ -233,11 +364,14 @@ static inline int write_and_verify(struct
> drm_i915_private *dev_priv,
>   * will verify the register values to make sure the registers are
> locked with
>   * correct values.
>   *
> - * Return: 0 on success. -EIO if registers were locked with
> incorrect values.
> + * Return: 0 on success. Minus error code if registered were locked
> with
> + * incorrect values.-EIO if registers failed to lock with correct
> values.
>   */
>  int intel_wopcm_init_hw(struct intel_wopcm *wopcm)
>  {
>  	struct drm_i915_private *dev_priv = wopcm_to_i915(wopcm);
> +	bool update_size_reg;
> +	bool update_offset_reg;
>  	int err;
>  
>  	if (!USES_GUC(dev_priv))
> @@ -247,27 +381,46 @@ int intel_wopcm_init_hw(struct intel_wopcm
> *wopcm)
>  	GEM_BUG_ON(!wopcm->guc.size);
>  	GEM_BUG_ON(!wopcm->guc.base);
>  
> -	err = write_and_verify(dev_priv, GUC_WOPCM_SIZE, wopcm-
> >guc.size,
> -			       GUC_WOPCM_SIZE_MASK |
> GUC_WOPCM_SIZE_LOCKED,
> -			       GUC_WOPCM_SIZE_LOCKED);
> -	if (err)
> +	err = verify_locked_values_and_update(wopcm,
> &update_size_reg,
> +					      &update_offset_reg);
> +	if (err) {
> +		DRM_ERROR("WOPCM registers are locked with invalid
> values.\n");
>  		goto err_out;
> +	}
>  
> -	err = write_and_verify(dev_priv, DMA_GUC_WOPCM_OFFSET,
> -			       wopcm->guc.base |
> HUC_LOADING_AGENT_GUC,
> -			       GUC_WOPCM_OFFSET_MASK |
> HUC_LOADING_AGENT_GUC |
> -			       GUC_WOPCM_OFFSET_VALID,
> -			       GUC_WOPCM_OFFSET_VALID);
> -	if (err)
> -		goto err_out;
> +	if (update_size_reg) {
> +		err = write_and_verify(dev_priv, GUC_WOPCM_SIZE,
> +				       wopcm->guc.size,
> +				       GUC_WOPCM_SIZE_MASK |
> +				       GUC_WOPCM_SIZE_LOCKED,
> +				       GUC_WOPCM_SIZE_LOCKED);
> +		if (err) {
> +			DRM_ERROR("Failed to GuC WOPCM size with
> %#x.\n",
> +				  wopcm->guc.size);
> +			goto err_out;
> +		}
> +	}
>  
> +	if (update_offset_reg) {
> +		err = write_and_verify(dev_priv,
> DMA_GUC_WOPCM_OFFSET,
> +				       wopcm->guc.base |
> HUC_LOADING_AGENT_GUC,
> +				       GUC_WOPCM_OFFSET_MASK |
> +				       HUC_LOADING_AGENT_GUC |
> +				       GUC_WOPCM_OFFSET_VALID,
> +				       GUC_WOPCM_OFFSET_VALID);
> +		if (err) {
> +			DRM_ERROR("Failed to GuC WOPCM offset with
> %#x.\n",
> +				  wopcm->guc.base);
> +			goto err_out;
> +		}
> +	}
>  	return 0;
>  
>  err_out:
> -	DRM_ERROR("Failed to init WOPCM registers:\n");
>  	DRM_ERROR("DMA_GUC_WOPCM_OFFSET=%#x\n",
>  		  I915_READ(DMA_GUC_WOPCM_OFFSET));
>  	DRM_ERROR("GUC_WOPCM_SIZE=%#x\n",
> I915_READ(GUC_WOPCM_SIZE));
> +	DRM_ERROR("A system reboot is required.\n");
>  
>  	return err;
>  }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx