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

["Michal Wajdeczko" <michal.wajdeczko@intel.com>]

On Tue, 10 Apr 2018 02:42:19 +0200, Jackie Li <yaodong.li@intel.com> 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>
> ---
>  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)

Can't we just return this size as positive value?
I guess wopcm size will never be larger than MAX_INT.
We can add GEM_BUG_ON to be sure.

> +{
> +	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;

You are mixing calculations with verifications here.
Focus on calculations as you have separate function that verifies values.

> +	}
> +
> +	*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

hmm, maybe we can unify somehow this verification to be able to work with
both calculated and locked values?

> *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),

you are calling calculate_min_guc_wopcm_size() twice

> +			  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 we want to support scenario where wopcm values are already locked, maybe
we should check first for them, as if they are really locked, we should
avoid calculating and printing our values...

> +	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)

hmm, either pass locked values in 'the' wopcm structure, or use i915
parameter to avoid confusion.

> +{
> +	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.

maybe simpler and more robust way would be to just use u64 for  
calculations?

> +	 */
> +	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,

calculate_min_guc_wopcm_base() called twice

> +			  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

"update" what ?

> *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;

btw, setting base then size is the correct order ;)

> 	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.

please fix typos and spaces.
btw, do we really care about different error codes?
note that -EIO has special meaning during driver load.

>   */
>  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;
>  }

as mentioned above, maybe we should start with:

	static int wopcm_init_from_registers(wopcm)
	{
		u32 base = I915_READ();
		u32 size = I915_READ();

		wopcm->guc.base = base & LOCKED ? base & MASK : 0;
		wopcm->guc.size = size & LOCKED ? size & MASK : 0;

		return wopcm->guc.base && wopcm->guc.size ? 0 : -ENODATA;
	}

then we can do everything in one pass:

	int intel_wopcm_init(wopcm)
	{
		ret = wopcm_init_from_registers(wopcm);
		if (ret)
			wopcm_finish_partitioning(wopcm);

		ret = wopcm_verify_partitions(wopcm);
		if (ret)
			return ret;

		ret = wopcm_write_registers(wopcm);
		if (ret)
			return ret;
		return 0;
	}

where "wopcm_finish_partitioning" must not try to update any non-zero
values but then can be implemented in any fashion.

(btw, I guess we don't need separate wopcm_init_hw step)

/michal
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx