[Intel-gfx] [PATCH 1/2] drm/i915: Add code to accept valid locked WOPCM register values

[Michał Winiarski]

On Tue, Mar 20, 2018 at 04:18:46PM -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.
> 
> 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: Michał Winiarski <michal.winiarski at intel.com>
> Cc: John Spotswood <john.a.spotswood at intel.com>
> ---
>  drivers/gpu/drm/i915/intel_wopcm.c | 183 +++++++++++++++++++++++++++++++------
>  1 file changed, 157 insertions(+), 26 deletions(-)

[snip]

>  /**
>   * intel_wopcm_init() - Initialize the WOPCM structure.
>   * @wopcm: pointer to intel_wopcm.
> @@ -155,10 +202,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);
> @@ -175,30 +220,17 @@ 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);
> -		return -E2BIG;
> -	}
> -
> -	guc_wopcm_size = wopcm->size - guc_wopcm_base - ctx_rsvd;
> -	guc_wopcm_size &= GUC_WOPCM_SIZE_MASK;
> +	guc_wopcm_base = calculate_min_guc_wopcm_base(huc_fw_size);

We already discussed this elsewhere, but just to have this part available
for wider audience:

huc_fw_size is unknown (there may be no huc firmware present) - which means
we're still not able to handle module reload from guc-without-huc into
guc-with-huc

Question remains whether we want to support this scenario, or whether we should
tie GuC and HuC together and refuse to operate early on if either one is not
present. We could remove a lot of code this way...

> +	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_size / 1024);
>  
> -	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 -E2BIG;
> -	}
> -
> -	err = check_hw_restriction(i915, guc_wopcm_base, guc_wopcm_size,
> -				   huc_fw_size);
> +	err = verify_calculated_values(i915, guc_fw_size, huc_fw_size,
> +				       guc_wopcm_base, guc_wopcm_size);

Ok - so we're calculating the values in init...

>  	if (err)
>  		return err;
>  
> @@ -208,6 +240,92 @@ int intel_wopcm_init(struct intel_wopcm *wopcm)
>  	return 0;
>  }
>  

[snip]

>  static inline int write_and_verify(struct drm_i915_private *dev_priv,
>  				   i915_reg_t reg, u32 val, u32 mask,
>  				   u32 locked_bit)
> @@ -231,7 +349,8 @@ 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)
>  {
> @@ -247,27 +366,39 @@ int intel_wopcm_init_hw(struct intel_wopcm *wopcm)
>  	GEM_BUG_ON(!wopcm->guc.size);
>  	GEM_BUG_ON(!wopcm->guc.base);
>  
> +	err = verify_locked_values_and_update(wopcm);
> +	if (err) {
> +		DRM_ERROR("WOPCM registers are locked with invalid values.\n");
> +		goto err_out;
> +	}
> +

...Only to throw everything out if the regs are locked in init_hw?

Since we've changed the logic, I think we should refactor a bit by changing
the ordering.
We could start from checking the regs and partition ourselves if the values are
not locked.

We can then have a common codepath for verification.

>  	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)
> +	if (err) {
> +		DRM_ERROR("Failed to lock GUC_WOPCM_SIZE with %#x.\n",
> +			  wopcm->guc.size);
>  		goto err_out;
> +	}
>  
>  	huc_agent = USES_HUC(dev_priv) ? HUC_LOADING_AGENT_GUC : 0;
>  	mask = GUC_WOPCM_OFFSET_MASK | GUC_WOPCM_OFFSET_VALID | huc_agent;
>  	err = write_and_verify(dev_priv, DMA_GUC_WOPCM_OFFSET,
>  			       wopcm->guc.base | huc_agent, mask,
>  			       GUC_WOPCM_OFFSET_VALID);
> -	if (err)
> +	if (err) {
> +		DRM_ERROR("Failed to lock DMA_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");

Functionally I think it's a step in the right direction, but I think we should
try even harder to not ever display this error msg.

-Michał
>  
>  	return err;
>  }
> -- 
> 2.7.4
>