[PATCH v3 1/4] drm/i915: Always do WOPCM partitioning based on real firmware sizes

[PATCH v3 1/4] drm/i915: Always do WOPCM partitioning based on real firmware sizes

[Jackie Li]

After enabled the WOPCM write-once registers locking status checking,
reloading of the i915 module will fail with modparam enable_guc set to 3
(enable GuC and HuC firmware loading) if the module was originally loaded
with enable_guc set to 1 (only enable GuC firmware loading). This is
because WOPCM registers were updated and locked without considering the HuC
FW size. Since we need both GuC and HuC FW sizes to determine the final
layout of WOPCM, we should always calculate the WOPCM layout based on the
actual sizes of the GuC and HuC firmware available for a specific platform
if we need continue to support enable/disable HuC FW loading dynamically
with enable_guc modparam.

This patch splits uC firmware fetching into two stages. First stage is to
fetch the firmware image and verify the firmware header. uC firmware will
be marked as verified and this will make FW info available for following
WOPCM layout calculation. The second stage is to create a GEM object and
copy the FW data into the created GEM object which will only be available
when GuC/HuC loading is enabled by enable_guc modparam. This will guarantee
that the WOPCM layout will be always be calculated correctly without making
any assumptions to the GuC and HuC firmware sizes.

v3:
 - Rebase

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_uc.c    | 14 ++++----------
 drivers/gpu/drm/i915/intel_uc_fw.c | 31 ++++++++++++++++++++-----------
 drivers/gpu/drm/i915/intel_uc_fw.h |  7 +++++--
 3 files changed, 29 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 1cffaf7..73b8f6c 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -172,11 +172,8 @@ void intel_uc_init_early(struct drm_i915_private *i915)
 
 	sanitize_options_early(i915);
 
-	if (USES_GUC(i915))
-		intel_uc_fw_fetch(i915, &guc->fw);
-
-	if (USES_HUC(i915))
-		intel_uc_fw_fetch(i915, &huc->fw);
+	intel_uc_fw_fetch(i915, &guc->fw, USES_GUC(i915));
+	intel_uc_fw_fetch(i915, &huc->fw, USES_HUC(i915));
 }
 
 void intel_uc_cleanup_early(struct drm_i915_private *i915)
@@ -184,11 +181,8 @@ void intel_uc_cleanup_early(struct drm_i915_private *i915)
 	struct intel_guc *guc = &i915->guc;
 	struct intel_huc *huc = &i915->huc;
 
-	if (USES_HUC(i915))
-		intel_uc_fw_fini(&huc->fw);
-
-	if (USES_GUC(i915))
-		intel_uc_fw_fini(&guc->fw);
+	intel_uc_fw_fini(&huc->fw);
+	intel_uc_fw_fini(&guc->fw);
 
 	guc_free_load_err_log(guc);
 }
diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c b/drivers/gpu/drm/i915/intel_uc_fw.c
index 6e8e0b5..a9cb900 100644
--- a/drivers/gpu/drm/i915/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/intel_uc_fw.c
@@ -33,11 +33,13 @@
  *
  * @dev_priv: device private
  * @uc_fw: uC firmware
+ * @copy_to_obj: whether fetch uC firmware into GEM object or not
  *
- * Fetch uC firmware into GEM obj.
+ * Fetch and verify uC firmware and copy firmware data into GEM object if
+ * @copy_to_obj is true.
  */
 void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
-		       struct intel_uc_fw *uc_fw)
+		       struct intel_uc_fw *uc_fw, bool copy_to_obj)
 {
 	struct pci_dev *pdev = dev_priv->drm.pdev;
 	struct drm_i915_gem_object *obj;
@@ -154,17 +156,24 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
 		goto fail;
 	}
 
-	obj = i915_gem_object_create_from_data(dev_priv, fw->data, fw->size);
-	if (IS_ERR(obj)) {
-		err = PTR_ERR(obj);
-		DRM_DEBUG_DRIVER("%s fw object_create err=%d\n",
-				 intel_uc_fw_type_repr(uc_fw->type), err);
-		goto fail;
+	uc_fw->size = fw->size;
+	uc_fw->fetch_status = INTEL_UC_FIRMWARE_VERIFIED;
+
+	if (copy_to_obj) {
+		obj = i915_gem_object_create_from_data(dev_priv, fw->data,
+						       fw->size);
+		if (IS_ERR(obj)) {
+			err = PTR_ERR(obj);
+			DRM_DEBUG_DRIVER("%s fw object_create err=%d\n",
+					 intel_uc_fw_type_repr(uc_fw->type),
+					 err);
+			goto fail;
+		}
+
+		uc_fw->obj = obj;
+		uc_fw->fetch_status = INTEL_UC_FIRMWARE_SUCCESS;
 	}
 
-	uc_fw->obj = obj;
-	uc_fw->size = fw->size;
-	uc_fw->fetch_status = INTEL_UC_FIRMWARE_SUCCESS;
 	DRM_DEBUG_DRIVER("%s fw fetch %s\n",
 			 intel_uc_fw_type_repr(uc_fw->type),
 			 intel_uc_fw_status_repr(uc_fw->fetch_status));
diff --git a/drivers/gpu/drm/i915/intel_uc_fw.h b/drivers/gpu/drm/i915/intel_uc_fw.h
index dc33b12..4e7ecc8 100644
--- a/drivers/gpu/drm/i915/intel_uc_fw.h
+++ b/drivers/gpu/drm/i915/intel_uc_fw.h
@@ -36,6 +36,7 @@ enum intel_uc_fw_status {
 	INTEL_UC_FIRMWARE_FAIL = -1,
 	INTEL_UC_FIRMWARE_NONE = 0,
 	INTEL_UC_FIRMWARE_PENDING,
+	INTEL_UC_FIRMWARE_VERIFIED,
 	INTEL_UC_FIRMWARE_SUCCESS
 };
 
@@ -84,6 +85,8 @@ const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status)
 		return "NONE";
 	case INTEL_UC_FIRMWARE_PENDING:
 		return "PENDING";
+	case INTEL_UC_FIRMWARE_VERIFIED:
+		return "VERIFIED";
 	case INTEL_UC_FIRMWARE_SUCCESS:
 		return "SUCCESS";
 	}
@@ -131,14 +134,14 @@ static inline void intel_uc_fw_sanitize(struct intel_uc_fw *uc_fw)
  */
 static inline u32 intel_uc_fw_get_upload_size(struct intel_uc_fw *uc_fw)
 {
-	if (uc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
+	if (uc_fw->fetch_status < INTEL_UC_FIRMWARE_VERIFIED)
 		return 0;
 
 	return uc_fw->header_size + uc_fw->ucode_size;
 }
 
 void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
-		       struct intel_uc_fw *uc_fw);
+		       struct intel_uc_fw *uc_fw, bool copy_to_obj);
 int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
 		       int (*xfer)(struct intel_uc_fw *uc_fw,
 				   struct i915_vma *vma));
-- 
2.7.4

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

[PATCH v3 2/4] drm/i915: Always set HUC_LOADING_AGENT_GUC bit in WOPCM offset register

[Jackie Li]

The enable_guc modparam is used to enable/disable GuC/HuC FW uploading
dynamcially during i915 module loading. If WOPCM offset register was locked
without having HUC_LOADING_AGENT_GUC bit set to 1, the module reloading
with both GuC and HuC FW will fail since we need to set this bit to 1 for
HuC FW uploading.

Since HUC_LOADING_AGENT_GUC bit has no impact on GuC FW uploading, this
patch updates the register updating code to make sure the WOPCM offset
register is always locked with HUC_LOADING_AGENT_GUC bit set to 1 which
will guarantee successful uploading of both GuC and HuC FW. We will further
take care of the locked values in the following enhancement patch.

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 | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_wopcm.c b/drivers/gpu/drm/i915/intel_wopcm.c
index 74bf76f..b1c08ca 100644
--- a/drivers/gpu/drm/i915/intel_wopcm.c
+++ b/drivers/gpu/drm/i915/intel_wopcm.c
@@ -238,8 +238,6 @@ static inline int write_and_verify(struct drm_i915_private *dev_priv,
 int intel_wopcm_init_hw(struct intel_wopcm *wopcm)
 {
 	struct drm_i915_private *dev_priv = wopcm_to_i915(wopcm);
-	u32 huc_agent;
-	u32 mask;
 	int err;
 
 	if (!USES_GUC(dev_priv))
@@ -255,10 +253,10 @@ int intel_wopcm_init_hw(struct intel_wopcm *wopcm)
 	if (err)
 		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,
+			       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;
-- 
2.7.4

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

Re: [PATCH v3 2/4] drm/i915: Always set HUC_LOADING_AGENT_GUC bit in WOPCM offset register

[John Spotswood]

On Mon, 2018-04-09 at 17:42 -0700, Jackie Li wrote:
> The enable_guc modparam is used to enable/disable GuC/HuC FW
> uploading
> dynamcially during i915 module loading. If WOPCM offset register was
> locked
> without having HUC_LOADING_AGENT_GUC bit set to 1, the module
> reloading
> with both GuC and HuC FW will fail since we need to set this bit to 1
> for
> HuC FW uploading.
> 
> Since HUC_LOADING_AGENT_GUC bit has no impact on GuC FW uploading,
> this
> patch updates the register updating code to make sure the WOPCM
> offset
> register is always locked with HUC_LOADING_AGENT_GUC bit set to 1
> which
> will guarantee successful uploading of both GuC and HuC FW. We will
> further
> take care of the locked values in the following enhancement patch.
> 
> 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 | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_wopcm.c
> b/drivers/gpu/drm/i915/intel_wopcm.c
> index 74bf76f..b1c08ca 100644
> --- a/drivers/gpu/drm/i915/intel_wopcm.c
> +++ b/drivers/gpu/drm/i915/intel_wopcm.c
> @@ -238,8 +238,6 @@ static inline int write_and_verify(struct
> drm_i915_private *dev_priv,
>  int intel_wopcm_init_hw(struct intel_wopcm *wopcm)
>  {
>  	struct drm_i915_private *dev_priv = wopcm_to_i915(wopcm);
> -	u32 huc_agent;
> -	u32 mask;
>  	int err;
>  
>  	if (!USES_GUC(dev_priv))
> @@ -255,10 +253,10 @@ int intel_wopcm_init_hw(struct intel_wopcm
> *wopcm)
>  	if (err)
>  		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,
> +			       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;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 2/4] drm/i915: Always set HUC_LOADING_AGENT_GUC bit in WOPCM offset register

[Michal Wajdeczko]

On Tue, 10 Apr 2018 02:42:18 +0200, Jackie Li <yaodong.li@intel.com> wrote:

> The enable_guc modparam is used to enable/disable GuC/HuC FW uploading
> dynamcially during i915 module loading. If WOPCM offset register was
   ^^^^
typo

> locked
> without having HUC_LOADING_AGENT_GUC bit set to 1, the module reloading
> with both GuC and HuC FW will fail since we need to set this bit to 1 for
> HuC FW uploading.
>
> Since HUC_LOADING_AGENT_GUC bit has no impact on GuC FW uploading, this
> patch updates the register updating code to make sure the WOPCM offset
> register is always locked with HUC_LOADING_AGENT_GUC bit set to 1 which
> will guarantee successful uploading of both GuC and HuC FW. We will  
> further
> take care of the locked values in the following enhancement patch.
>
> 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 | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_wopcm.c  
> b/drivers/gpu/drm/i915/intel_wopcm.c
> index 74bf76f..b1c08ca 100644
> --- a/drivers/gpu/drm/i915/intel_wopcm.c
> +++ b/drivers/gpu/drm/i915/intel_wopcm.c
> @@ -238,8 +238,6 @@ static inline int write_and_verify(struct  
> drm_i915_private *dev_priv,
>  int intel_wopcm_init_hw(struct intel_wopcm *wopcm)
>  {
>  	struct drm_i915_private *dev_priv = wopcm_to_i915(wopcm);
> -	u32 huc_agent;
> -	u32 mask;
>  	int err;
> 	if (!USES_GUC(dev_priv))
> @@ -255,10 +253,10 @@ int intel_wopcm_init_hw(struct intel_wopcm *wopcm)
>  	if (err)
>  		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,
> +			       wopcm->guc.base | HUC_LOADING_AGENT_GUC,
> +			       GUC_WOPCM_OFFSET_MASK | HUC_LOADING_AGENT_GUC |
> +			       GUC_WOPCM_OFFSET_VALID,

while we can unconditionally set HUC_AGENT bit, there is no need to verify
it unless we are using HuC, so we can consider leaving old mask intact.

/m

>  			       GUC_WOPCM_OFFSET_VALID);
>  	if (err)
>  		goto err_out;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 2/4] drm/i915: Always set HUC_LOADING_AGENT_GUC bit in WOPCM offset register

[Yaodong Li]

On 04/13/2018 07:26 PM, Michal Wajdeczko wrote:
> On Tue, 10 Apr 2018 02:42:18 +0200, Jackie Li <yaodong.li@intel.com> 
> wrote:
>
>> The enable_guc modparam is used to enable/disable GuC/HuC FW uploading
>> dynamcially during i915 module loading. If WOPCM offset register was
>   ^^^^
> typo
>
D'oh! I really need a tool for this! Thanks, will fix it.
>> locked
>> without having HUC_LOADING_AGENT_GUC bit set to 1, the module reloading
>> with both GuC and HuC FW will fail since we need to set this bit to 1 
>> for
>> HuC FW uploading.
>>
>> Since HUC_LOADING_AGENT_GUC bit has no impact on GuC FW uploading, this
>> patch updates the register updating code to make sure the WOPCM offset
>> register is always locked with HUC_LOADING_AGENT_GUC bit set to 1 which
>> will guarantee successful uploading of both GuC and HuC FW. We will 
>> further
>> take care of the locked values in the following enhancement patch.
>>
>> 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 | 8 +++-----
>>  1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_wopcm.c 
>> b/drivers/gpu/drm/i915/intel_wopcm.c
>> index 74bf76f..b1c08ca 100644
>> --- a/drivers/gpu/drm/i915/intel_wopcm.c
>> +++ b/drivers/gpu/drm/i915/intel_wopcm.c
>> @@ -238,8 +238,6 @@ static inline int write_and_verify(struct 
>> drm_i915_private *dev_priv,
>>  int intel_wopcm_init_hw(struct intel_wopcm *wopcm)
>>  {
>>      struct drm_i915_private *dev_priv = wopcm_to_i915(wopcm);
>> -    u32 huc_agent;
>> -    u32 mask;
>>      int err;
>>     if (!USES_GUC(dev_priv))
>> @@ -255,10 +253,10 @@ int intel_wopcm_init_hw(struct intel_wopcm *wopcm)
>>      if (err)
>>          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,
>> +                   wopcm->guc.base | HUC_LOADING_AGENT_GUC,
>> +                   GUC_WOPCM_OFFSET_MASK | HUC_LOADING_AGENT_GUC |
>> +                   GUC_WOPCM_OFFSET_VALID,
>
> while we can unconditionally set HUC_AGENT bit, there is no need to 
> verify
> it unless we are using HuC, so we can consider leaving old mask intact.
The idea is to verify the written values are exactly we want, so I think 
it better
to keep doing it in this way.

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

Re: [PATCH v3 2/4] drm/i915: Always set HUC_LOADING_AGENT_GUC bit in WOPCM offset register

[Michal Wajdeczko]

On Mon, 16 Apr 2018 19:43:52 +0200, Yaodong Li <yaodong.li@intel.com>  
wrote:

> On 04/13/2018 07:26 PM, Michal Wajdeczko wrote:
>> On Tue, 10 Apr 2018 02:42:18 +0200, Jackie Li <yaodong.li@intel.com>  
>> wrote:
>>
>>> The enable_guc modparam is used to enable/disable GuC/HuC FW uploading
>>> dynamcially during i915 module loading. If WOPCM offset register was
>>   ^^^^
>> typo
>>
> D'oh! I really need a tool for this! Thanks, will fix it.
>>> locked
>>> without having HUC_LOADING_AGENT_GUC bit set to 1, the module reloading
>>> with both GuC and HuC FW will fail since we need to set this bit to 1  
>>> for
>>> HuC FW uploading.
>>>
>>> Since HUC_LOADING_AGENT_GUC bit has no impact on GuC FW uploading, this
>>> patch updates the register updating code to make sure the WOPCM offset
>>> register is always locked with HUC_LOADING_AGENT_GUC bit set to 1 which
>>> will guarantee successful uploading of both GuC and HuC FW. We will  
>>> further
>>> take care of the locked values in the following enhancement patch.
>>>
>>> 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 | 8 +++-----
>>>  1 file changed, 3 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_wopcm.c  
>>> b/drivers/gpu/drm/i915/intel_wopcm.c
>>> index 74bf76f..b1c08ca 100644
>>> --- a/drivers/gpu/drm/i915/intel_wopcm.c
>>> +++ b/drivers/gpu/drm/i915/intel_wopcm.c
>>> @@ -238,8 +238,6 @@ static inline int write_and_verify(struct  
>>> drm_i915_private *dev_priv,
>>>  int intel_wopcm_init_hw(struct intel_wopcm *wopcm)
>>>  {
>>>      struct drm_i915_private *dev_priv = wopcm_to_i915(wopcm);
>>> -    u32 huc_agent;
>>> -    u32 mask;
>>>      int err;
>>>     if (!USES_GUC(dev_priv))
>>> @@ -255,10 +253,10 @@ int intel_wopcm_init_hw(struct intel_wopcm  
>>> *wopcm)
>>>      if (err)
>>>          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,
>>> +                   wopcm->guc.base | HUC_LOADING_AGENT_GUC,
>>> +                   GUC_WOPCM_OFFSET_MASK | HUC_LOADING_AGENT_GUC |
>>> +                   GUC_WOPCM_OFFSET_VALID,
>>
>> while we can unconditionally set HUC_AGENT bit, there is no need to  
>> verify
>> it unless we are using HuC, so we can consider leaving old mask intact.
> The idea is to verify the written values are exactly we want, so I think  
> it better
> to keep doing it in this way.

Hmm, but then instead of being more flexible, you're unnecessary  
restricting
yourself to require HUC_AGENT bit, even if you don't need it - recall the
theoretical scenario with bad bios that already locked that register.

This seems to be little inconsistent with earlier patch where you try to
support much more different scenario (from no HuC fw to use HuC fw)

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

Re: [PATCH v3 2/4] drm/i915: Always set HUC_LOADING_AGENT_GUC bit in WOPCM offset register

[Yaodong Li]

On 04/19/2018 08:52 AM, Michal Wajdeczko wrote:
> On Mon, 16 Apr 2018 19:43:52 +0200, Yaodong Li <yaodong.li@intel.com> 
> wrote:
>
>> On 04/13/2018 07:26 PM, Michal Wajdeczko wrote:
>>> On Tue, 10 Apr 2018 02:42:18 +0200, Jackie Li <yaodong.li@intel.com> 
>>> wrote:
>>>
>>>> The enable_guc modparam is used to enable/disable GuC/HuC FW uploading
>>>> dynamcially during i915 module loading. If WOPCM offset register was
>>>   ^^^^
>>> typo
>>>
>> D'oh! I really need a tool for this! Thanks, will fix it.
>>>> locked
>>>> without having HUC_LOADING_AGENT_GUC bit set to 1, the module 
>>>> reloading
>>>> with both GuC and HuC FW will fail since we need to set this bit to 
>>>> 1 for
>>>> HuC FW uploading.
>>>>
>>>> Since HUC_LOADING_AGENT_GUC bit has no impact on GuC FW uploading, 
>>>> this
>>>> patch updates the register updating code to make sure the WOPCM offset
>>>> register is always locked with HUC_LOADING_AGENT_GUC bit set to 1 
>>>> which
>>>> will guarantee successful uploading of both GuC and HuC FW. We will 
>>>> further
>>>> take care of the locked values in the following enhancement patch.
>>>>
>>>> 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 | 8 +++-----
>>>>  1 file changed, 3 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_wopcm.c 
>>>> b/drivers/gpu/drm/i915/intel_wopcm.c
>>>> index 74bf76f..b1c08ca 100644
>>>> --- a/drivers/gpu/drm/i915/intel_wopcm.c
>>>> +++ b/drivers/gpu/drm/i915/intel_wopcm.c
>>>> @@ -238,8 +238,6 @@ static inline int write_and_verify(struct 
>>>> drm_i915_private *dev_priv,
>>>>  int intel_wopcm_init_hw(struct intel_wopcm *wopcm)
>>>>  {
>>>>      struct drm_i915_private *dev_priv = wopcm_to_i915(wopcm);
>>>> -    u32 huc_agent;
>>>> -    u32 mask;
>>>>      int err;
>>>>     if (!USES_GUC(dev_priv))
>>>> @@ -255,10 +253,10 @@ int intel_wopcm_init_hw(struct intel_wopcm 
>>>> *wopcm)
>>>>      if (err)
>>>>          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,
>>>> +                   wopcm->guc.base | HUC_LOADING_AGENT_GUC,
>>>> +                   GUC_WOPCM_OFFSET_MASK | HUC_LOADING_AGENT_GUC |
>>>> +                   GUC_WOPCM_OFFSET_VALID,
>>>
>>> while we can unconditionally set HUC_AGENT bit, there is no need to 
>>> verify
>>> it unless we are using HuC, so we can consider leaving old mask intact.
>> The idea is to verify the written values are exactly we want, so I 
>> think it better
>> to keep doing it in this way.
>
> Hmm, but then instead of being more flexible, you're unnecessary 
> restricting
> yourself to require HUC_AGENT bit, even if you don't need it - recall the
> theoretical scenario with bad bios that already locked that register.
>
Hmm. Actually my thought is pretty simple here. we want to always set this
bit so we always keep checking it. For the fault bios handling, my 
thought is
if this reg was locked without HUC_AGENT bit when USES_HUC is true. we will
return error - the 3/3 patch is taking care of this.

> This seems to be little inconsistent with earlier patch where you try to
> support much more different scenario (from no HuC fw to use HuC fw)
My 1/3 patch is trying to support enable_guc=1->3->1 without any FW changes
on the FS while work as an enhancement patch to handle more complicated
cases for the theoretical scenario - faulty bios.

Regards,
-Jackie

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

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

[Jackie Li]

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)
+{
+	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;
 }
-- 
2.7.4

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

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

[John Spotswood]

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

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

[Michal Wajdeczko]

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

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

[Yaodong Li]

On 04/13/2018 09:20 PM, Michal Wajdeczko wrote:
> 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?
I actually thought about that. However, since the verification based on 
the assumption
that the unlocked reg value could be any numbers so it puts more checks 
on these values
which are unnecessary during the calculation. I've made the responding 
check common
enough to be reused by this verification code and the calculation code.
>
>> *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
Will update it.
>
>> +              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...
I think we shall not access registers in early init (by i915_driver_load 
design purpose)?
and the reason we need to put the calculation and locking check + reg 
update into
separate functions is we only want to calculate the values for once and 
locking check
and reg update should be done every time hw init is called.

The purpose of this patch is only to take care of worst cases that BIOS 
had locked these
regs and we still want give it a try if we could go with these values. 
and it should be very
unlikely.
>
>> +    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.
we are trying to keep the wopcm structure clean (only contains valid 
values).
>
>> +{
>> +    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?
Hmm. These values are for 32-bit regs, so even with u64 variables we 
still need
similar checks to keep them to be 32-bit values. e.g. we need guarantee 
that
wopcm->size is within 32-bit boundary.
>
>> +     */
>> +    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.
>
I think is better to return different err codes to distinguish different 
errors.

Will EIO will be handled different by i915_driver_load? Another 
candidate would
be EBUSY.
>>   */
>>  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)
As I mentioned above, the main reason to this separation is because
we only expect one time calculation while the locking check and reg update
may happen every time hw init was called. e.g. resume.

Thanks,
-Jackie

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

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

[Michal Wajdeczko]

On Mon, 16 Apr 2018 20:43:39 +0200, Yaodong Li <yaodong.li@intel.com>  
wrote:

> On 04/13/2018 09:20 PM, Michal Wajdeczko wrote:
>> 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?
> I actually thought about that. However, since the verification based on  
> the assumption
> that the unlocked reg value could be any numbers so it puts more checks  
> on these values
> which are unnecessary during the calculation.

maybe some checks would be "unnecessary" but they still should be valid.
and code reuse is nice benefit that sacrifice that few extra checks.

> I've made the responding check common
> enough to be reused by this verification code and the calculation code.
>>
>>> *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
> Will update it.
>>
>>> +              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...
> I think we shall not access registers in early init (by i915_driver_load  
> design purpose)?

but we don't have to do it in early, intel_wopmc_init[_hw]() should be ok

> and the reason we need to put the calculation and locking check + reg  
> update into
> separate functions is we only want to calculate the values for once and  
> locking check
> and reg update should be done every time hw init is called.
>
> The purpose of this patch is only to take care of worst cases that BIOS  
> had locked these
> regs and we still want give it a try if we could go with these values.  
> and it should be very
> unlikely.

hmm, I was assuming that you want to cover the driver reload scenario
loaded with different guc/huc fw sizes, which is likely scenario during
debugging (see your patch 1 motivation)

>>
>>> +    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.
> we are trying to keep the wopcm structure clean (only contains valid  
> values).
>>
>>> +{
>>> +    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?
> Hmm. These values are for 32-bit regs, so even with u64 variables we  
> still need
> similar checks to keep them to be 32-bit values. e.g. we need guarantee  
> that
> wopcm->size is within 32-bit boundary.

I was referring to use u64 vars for calculations, not inside wopcm.
and we already have many checks so we should catch values over 32b

Just checking guc_base/size against wopcm size may be insufficient:

	wopcm->size    = 0xF0000000;
	guc_wopcm_base = 0x80000000;
	guc_wopcm_size = 0x80000000;

>>
>>> +     */
>>> +    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.
>>
> I think is better to return different err codes to distinguish different  
> errors.
>
> Will EIO will be handled different by i915_driver_load?

yes, it is handled differently.

> Another candidate would
> be EBUSY.
>>>   */
>>>  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)
> As I mentioned above, the main reason to this separation is because
> we only expect one time calculation while the locking check and reg  
> update
> may happen every time hw init was called. e.g. resume.
>
> Thanks,
> -Jackie
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Yaodong Li]

On 04/19/2018 09:37 AM, Michal Wajdeczko wrote:
> On Mon, 16 Apr 2018 20:43:39 +0200, Yaodong Li <yaodong.li@intel.com> 
> wrote:
>
>> On 04/13/2018 09:20 PM, Michal Wajdeczko wrote:
>>> 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?
>> I actually thought about that. However, since the verification based 
>> on the assumption
>> that the unlocked reg value could be any numbers so it puts more 
>> checks on these values
>> which are unnecessary during the calculation.
>
> maybe some checks would be "unnecessary" but they still should be valid.
> and code reuse is nice benefit that sacrifice that few extra checks.
Hmm, actually there's no duplicated code here. But I agree that it will 
be clearer to
have single place for the verification - it makes sense too:). So will 
choose to
sacrifice a little bit time here for unnecessary checks.
>
>> I've made the responding check common
>> enough to be reused by this verification code and the calculation code.
>>>
>>>> *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
>> Will update it.
>>>
>>>> +              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...
>> I think we shall not access registers in early init (by 
>> i915_driver_load design purpose)?
>
> but we don't have to do it in early, intel_wopmc_init[_hw]() should be ok
if we want to do so then intel_wopcm_init_hw should be the right place. 
Just that these changes
is only for handling the worst cases that rarely happens and it is 
unlikely to be called. The benefit
to distinguish early init and hw init is that we can do the calculation 
without powering on the HW
which should be expected as a common use case.
>
>> and the reason we need to put the calculation and locking check + reg 
>> update into
>> separate functions is we only want to calculate the values for once 
>> and locking check
>> and reg update should be done every time hw init is called.
>>
>> The purpose of this patch is only to take care of worst cases that 
>> BIOS had locked these
>> regs and we still want give it a try if we could go with these 
>> values. and it should be very
>> unlikely.
>
> hmm, I was assuming that you want to cover the driver reload scenario
> loaded with different guc/huc fw sizes, which is likely scenario during
> debugging (see your patch 1 motivation)
Yes, this is one of the motivations too. e.g. without this patch, we 
will have to
reboot the system even if for use case such as we are trying to reload a 
smaller
guc fw. This patch will enable such scenarios when it's possible.
>
>>>
>>>> +    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.
>> we are trying to keep the wopcm structure clean (only contains valid 
>> values).
>>>
>>>> +{
>>>> +    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?
>> Hmm. These values are for 32-bit regs, so even with u64 variables we 
>> still need
>> similar checks to keep them to be 32-bit values. e.g. we need 
>> guarantee that
>> wopcm->size is within 32-bit boundary.
>
> I was referring to use u64 vars for calculations, not inside wopcm.
> and we already have many checks so we should catch values over 32b
>
> Just checking guc_base/size against wopcm size may be insufficient:
>
>     wopcm->size    = 0xF0000000;
>     guc_wopcm_base = 0x80000000;
>     guc_wopcm_size = 0x80000000;
>
my v4 patch did some simplification to these checks.
>>>
>>>> +     */
>>>> +    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.
>>>
>> I think is better to return different err codes to distinguish 
>> different errors.
>>
>> Will EIO will be handled different by i915_driver_load?
>
> yes, it is handled differently.
Can you tell any negative impact if EIO was used? I can switch to EBUSY 
instead
but I think it's more accurate to use EIO for errors like reg is locked 
without HUC_AGENT
bit set when using HUC.

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

[PATCH v3 4/4] HAX enable guc for CI

[Jackie Li]

Signed-off-by: Jackie Li <yaodong.li@intel.com>
---
 drivers/gpu/drm/i915/i915_params.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index c963603..53037b5 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -47,7 +47,7 @@ struct drm_printer;
 	param(int, disable_power_well, -1) \
 	param(int, enable_ips, 1) \
 	param(int, invert_brightness, 0) \
-	param(int, enable_guc, 0) \
+	param(int, enable_guc, -1) \
 	param(int, guc_log_level, -1) \
 	param(char *, guc_firmware_path, NULL) \
 	param(char *, huc_firmware_path, NULL) \
-- 
2.7.4

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

✓ Fi.CI.BAT: success for series starting with [v3,1/4] drm/i915: Always do WOPCM partitioning based on real firmware sizes

[Patchwork]

== Series Details ==

Series: series starting with [v3,1/4] drm/i915: Always do WOPCM partitioning based on real firmware sizes
URL   : https://patchwork.freedesktop.org/series/41409/
State : success

== Summary ==

Series 41409v1 series starting with [v3,1/4] drm/i915: Always do WOPCM partitioning based on real firmware sizes
https://patchwork.freedesktop.org/api/1.0/series/41409/revisions/1/mbox/

---- Known issues:

Test gem_exec_suspend:
        Subgroup basic-s3:
                dmesg-warn -> PASS       (fi-glk-j4005) fdo#103359
Test gem_mmap_gtt:
        Subgroup basic-small-bo-tiledx:
                fail       -> PASS       (fi-gdg-551) fdo#102575
Test kms_flip:
        Subgroup basic-flip-vs-wf_vblank:
                fail       -> PASS       (fi-glk-j4005) fdo#100368
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-c:
                pass       -> INCOMPLETE (fi-bxt-dsi) fdo#103927

fdo#103359 https://bugs.freedesktop.org/show_bug.cgi?id=103359
fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#103927 https://bugs.freedesktop.org/show_bug.cgi?id=103927

fi-bdw-5557u     total:285  pass:264  dwarn:0   dfail:0   fail:0   skip:21  time:433s
fi-bdw-gvtdvm    total:285  pass:261  dwarn:0   dfail:0   fail:0   skip:24  time:443s
fi-blb-e6850     total:285  pass:220  dwarn:1   dfail:0   fail:0   skip:64  time:380s
fi-bsw-n3050     total:285  pass:239  dwarn:0   dfail:0   fail:0   skip:46  time:538s
fi-bwr-2160      total:285  pass:180  dwarn:0   dfail:0   fail:0   skip:105 time:298s
fi-bxt-dsi       total:243  pass:216  dwarn:0   dfail:0   fail:0   skip:26 
fi-bxt-j4205     total:285  pass:256  dwarn:0   dfail:0   fail:0   skip:29  time:513s
fi-byt-j1900     total:285  pass:250  dwarn:0   dfail:0   fail:0   skip:35  time:520s
fi-byt-n2820     total:285  pass:246  dwarn:0   dfail:0   fail:0   skip:39  time:507s
fi-cfl-8700k     total:285  pass:257  dwarn:0   dfail:0   fail:0   skip:28  time:411s
fi-cfl-s3        total:285  pass:259  dwarn:0   dfail:0   fail:0   skip:26  time:561s
fi-cfl-u         total:285  pass:259  dwarn:0   dfail:0   fail:0   skip:26  time:515s
fi-cnl-y3        total:285  pass:259  dwarn:0   dfail:0   fail:0   skip:26  time:585s
fi-elk-e7500     total:285  pass:226  dwarn:0   dfail:0   fail:0   skip:59  time:422s
fi-gdg-551       total:285  pass:177  dwarn:0   dfail:0   fail:0   skip:108 time:315s
fi-glk-1         total:285  pass:257  dwarn:0   dfail:0   fail:0   skip:28  time:535s
fi-glk-j4005     total:285  pass:256  dwarn:0   dfail:0   fail:0   skip:29  time:483s
fi-hsw-4770      total:285  pass:258  dwarn:0   dfail:0   fail:0   skip:27  time:408s
fi-ilk-650       total:285  pass:225  dwarn:0   dfail:0   fail:0   skip:60  time:421s
fi-ivb-3520m     total:285  pass:256  dwarn:0   dfail:0   fail:0   skip:29  time:474s
fi-ivb-3770      total:285  pass:252  dwarn:0   dfail:0   fail:0   skip:33  time:437s
fi-kbl-7500u     total:285  pass:260  dwarn:1   dfail:0   fail:0   skip:24  time:473s
fi-kbl-7567u     total:285  pass:265  dwarn:0   dfail:0   fail:0   skip:20  time:464s
fi-kbl-r         total:285  pass:258  dwarn:0   dfail:0   fail:0   skip:27  time:505s
fi-pnv-d510      total:285  pass:220  dwarn:1   dfail:0   fail:0   skip:64  time:672s
fi-skl-6260u     total:285  pass:265  dwarn:0   dfail:0   fail:0   skip:20  time:448s
fi-skl-6600u     total:285  pass:258  dwarn:0   dfail:0   fail:0   skip:27  time:534s
fi-skl-6700k2    total:285  pass:261  dwarn:0   dfail:0   fail:0   skip:24  time:505s
fi-skl-6770hq    total:285  pass:265  dwarn:0   dfail:0   fail:0   skip:20  time:491s
fi-skl-guc       total:285  pass:257  dwarn:0   dfail:0   fail:0   skip:28  time:432s
fi-skl-gvtdvm    total:285  pass:262  dwarn:0   dfail:0   fail:0   skip:23  time:444s
fi-snb-2520m     total:285  pass:245  dwarn:0   dfail:0   fail:0   skip:40  time:576s
fi-snb-2600      total:285  pass:245  dwarn:0   dfail:0   fail:0   skip:40  time:404s

617cdf0bd4fd2cb0dcc64ddf07fbb56572ba800a drm-tip: 2018y-04m-09d-19h-55m-54s UTC integration manifest
9f8948d7bfa4 HAX enable guc for CI
c17662cc72f9 drm/i915: Add code to accept valid locked WOPCM register values
8cd29217af34 drm/i915: Always set HUC_LOADING_AGENT_GUC bit in WOPCM offset register
79d7faa4b092 drm/i915: Always do WOPCM partitioning based on real firmware sizes

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8649/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.IGT: failure for series starting with [v3,1/4] drm/i915: Always do WOPCM partitioning based on real firmware sizes

[Patchwork]

== Series Details ==

Series: series starting with [v3,1/4] drm/i915: Always do WOPCM partitioning based on real firmware sizes
URL   : https://patchwork.freedesktop.org/series/41409/
State : failure

== Summary ==

---- Possible new issues:

Test gem_eio:
        Subgroup execbuf:
                pass       -> INCOMPLETE (shard-apl)
        Subgroup throttle:
                pass       -> INCOMPLETE (shard-apl)
Test kms_cursor_legacy:
        Subgroup pipe-c-torture-move:
                dmesg-warn -> PASS       (shard-hsw)
Test kms_plane:
        Subgroup pixel-format-pipe-b-planes:
                dmesg-warn -> PASS       (shard-hsw)
Test perf:
        Subgroup gen8-unprivileged-single-ctx-counters:
                pass       -> FAIL       (shard-apl)
Test pm_rpm:
        Subgroup debugfs-read:
                pass       -> DMESG-WARN (shard-apl)

---- Known issues:

Test drv_missed_irq:
                pass       -> SKIP       (shard-apl) fdo#103199
Test kms_flip:
        Subgroup 2x-plain-flip-ts-check-interruptible:
                pass       -> FAIL       (shard-hsw) fdo#100368
        Subgroup dpms-vs-vblank-race:
                fail       -> PASS       (shard-hsw) fdo#103060

fdo#103199 https://bugs.freedesktop.org/show_bug.cgi?id=103199
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060

shard-apl        total:2586 pass:1774 dwarn:2   dfail:0   fail:11  skip:796 time:11920s
shard-hsw        total:2680 pass:1785 dwarn:1   dfail:0   fail:2   skip:891 time:11327s
Blacklisted hosts:
shard-kbl        total:2467 pass:1789 dwarn:21  dfail:0   fail:10  skip:641 time:7348s
shard-snb        total:2680 pass:1377 dwarn:1   dfail:0   fail:3   skip:1299 time:6880s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8649/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 1/4] drm/i915: Always do WOPCM partitioning based on real firmware sizes

[John Spotswood]

On Mon, 2018-04-09 at 17:42 -0700, Jackie Li wrote:
> After enabled the WOPCM write-once registers locking status checking,
> reloading of the i915 module will fail with modparam enable_guc set
> to 3
> (enable GuC and HuC firmware loading) if the module was originally
> loaded
> with enable_guc set to 1 (only enable GuC firmware loading). This is
> because WOPCM registers were updated and locked without considering
> the HuC
> FW size. Since we need both GuC and HuC FW sizes to determine the
> final
> layout of WOPCM, we should always calculate the WOPCM layout based on
> the
> actual sizes of the GuC and HuC firmware available for a specific
> platform
> if we need continue to support enable/disable HuC FW loading
> dynamically
> with enable_guc modparam.
> 
> This patch splits uC firmware fetching into two stages. First stage
> is to
> fetch the firmware image and verify the firmware header. uC firmware
> will
> be marked as verified and this will make FW info available for
> following
> WOPCM layout calculation. The second stage is to create a GEM object
> and
> copy the FW data into the created GEM object which will only be
> available
> when GuC/HuC loading is enabled by enable_guc modparam. This will
> guarantee
> that the WOPCM layout will be always be calculated correctly without
> making
> any assumptions to the GuC and HuC firmware sizes.
> 
> v3:
>  - Rebase
> 
> 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_uc.c    | 14 ++++----------
>  drivers/gpu/drm/i915/intel_uc_fw.c | 31 ++++++++++++++++++++------
> -----
>  drivers/gpu/drm/i915/intel_uc_fw.h |  7 +++++--
>  3 files changed, 29 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uc.c
> b/drivers/gpu/drm/i915/intel_uc.c
> index 1cffaf7..73b8f6c 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -172,11 +172,8 @@ void intel_uc_init_early(struct drm_i915_private
> *i915)
>  
>  	sanitize_options_early(i915);
>  
> -	if (USES_GUC(i915))
> -		intel_uc_fw_fetch(i915, &guc->fw);
> -
> -	if (USES_HUC(i915))
> -		intel_uc_fw_fetch(i915, &huc->fw);
> +	intel_uc_fw_fetch(i915, &guc->fw, USES_GUC(i915));
> +	intel_uc_fw_fetch(i915, &huc->fw, USES_HUC(i915));
>  }
>  
>  void intel_uc_cleanup_early(struct drm_i915_private *i915)
> @@ -184,11 +181,8 @@ void intel_uc_cleanup_early(struct
> drm_i915_private *i915)
>  	struct intel_guc *guc = &i915->guc;
>  	struct intel_huc *huc = &i915->huc;
>  
> -	if (USES_HUC(i915))
> -		intel_uc_fw_fini(&huc->fw);
> -
> -	if (USES_GUC(i915))
> -		intel_uc_fw_fini(&guc->fw);
> +	intel_uc_fw_fini(&huc->fw);
> +	intel_uc_fw_fini(&guc->fw);
>  
>  	guc_free_load_err_log(guc);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c
> b/drivers/gpu/drm/i915/intel_uc_fw.c
> index 6e8e0b5..a9cb900 100644
> --- a/drivers/gpu/drm/i915/intel_uc_fw.c
> +++ b/drivers/gpu/drm/i915/intel_uc_fw.c
> @@ -33,11 +33,13 @@
>   *
>   * @dev_priv: device private
>   * @uc_fw: uC firmware
> + * @copy_to_obj: whether fetch uC firmware into GEM object or not
>   *
> - * Fetch uC firmware into GEM obj.
> + * Fetch and verify uC firmware and copy firmware data into GEM
> object if
> + * @copy_to_obj is true.
>   */
>  void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
> -		       struct intel_uc_fw *uc_fw)
> +		       struct intel_uc_fw *uc_fw, bool copy_to_obj)
>  {
>  	struct pci_dev *pdev = dev_priv->drm.pdev;
>  	struct drm_i915_gem_object *obj;
> @@ -154,17 +156,24 @@ void intel_uc_fw_fetch(struct drm_i915_private
> *dev_priv,
>  		goto fail;
>  	}
>  
> -	obj = i915_gem_object_create_from_data(dev_priv, fw->data,
> fw->size);
> -	if (IS_ERR(obj)) {
> -		err = PTR_ERR(obj);
> -		DRM_DEBUG_DRIVER("%s fw object_create err=%d\n",
> -				 intel_uc_fw_type_repr(uc_fw->type), 
> err);
> -		goto fail;
> +	uc_fw->size = fw->size;
> +	uc_fw->fetch_status = INTEL_UC_FIRMWARE_VERIFIED;
> +
> +	if (copy_to_obj) {
> +		obj = i915_gem_object_create_from_data(dev_priv, fw-
> >data,
> +						       fw->size);
> +		if (IS_ERR(obj)) {
> +			err = PTR_ERR(obj);
> +			DRM_DEBUG_DRIVER("%s fw object_create
> err=%d\n",
> +					 intel_uc_fw_type_repr(uc_fw
> ->type),
> +					 err);
> +			goto fail;
> +		}
> +
> +		uc_fw->obj = obj;
> +		uc_fw->fetch_status = INTEL_UC_FIRMWARE_SUCCESS;
>  	}
>  
> -	uc_fw->obj = obj;
> -	uc_fw->size = fw->size;
> -	uc_fw->fetch_status = INTEL_UC_FIRMWARE_SUCCESS;
>  	DRM_DEBUG_DRIVER("%s fw fetch %s\n",
>  			 intel_uc_fw_type_repr(uc_fw->type),
>  			 intel_uc_fw_status_repr(uc_fw-
> >fetch_status));
> diff --git a/drivers/gpu/drm/i915/intel_uc_fw.h
> b/drivers/gpu/drm/i915/intel_uc_fw.h
> index dc33b12..4e7ecc8 100644
> --- a/drivers/gpu/drm/i915/intel_uc_fw.h
> +++ b/drivers/gpu/drm/i915/intel_uc_fw.h
> @@ -36,6 +36,7 @@ enum intel_uc_fw_status {
>  	INTEL_UC_FIRMWARE_FAIL = -1,
>  	INTEL_UC_FIRMWARE_NONE = 0,
>  	INTEL_UC_FIRMWARE_PENDING,
> +	INTEL_UC_FIRMWARE_VERIFIED,
>  	INTEL_UC_FIRMWARE_SUCCESS
>  };
>  
> @@ -84,6 +85,8 @@ const char *intel_uc_fw_status_repr(enum
> intel_uc_fw_status status)
>  		return "NONE";
>  	case INTEL_UC_FIRMWARE_PENDING:
>  		return "PENDING";
> +	case INTEL_UC_FIRMWARE_VERIFIED:
> +		return "VERIFIED";
>  	case INTEL_UC_FIRMWARE_SUCCESS:
>  		return "SUCCESS";
>  	}
> @@ -131,14 +134,14 @@ static inline void intel_uc_fw_sanitize(struct
> intel_uc_fw *uc_fw)
>   */
>  static inline u32 intel_uc_fw_get_upload_size(struct intel_uc_fw
> *uc_fw)
>  {
> -	if (uc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
> +	if (uc_fw->fetch_status < INTEL_UC_FIRMWARE_VERIFIED)
>  		return 0;
>  
>  	return uc_fw->header_size + uc_fw->ucode_size;
>  }
>  
>  void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
> -		       struct intel_uc_fw *uc_fw);
> +		       struct intel_uc_fw *uc_fw, bool copy_to_obj);
>  int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
>  		       int (*xfer)(struct intel_uc_fw *uc_fw,
>  				   struct i915_vma *vma));
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 1/4] drm/i915: Always do WOPCM partitioning based on real firmware sizes

[Michal Wajdeczko]

On Tue, 10 Apr 2018 02:42:17 +0200, Jackie Li <yaodong.li@intel.com> wrote:

> After enabled the WOPCM write-once registers locking status checking,
> reloading of the i915 module will fail with modparam enable_guc set to 3
> (enable GuC and HuC firmware loading) if the module was originally loaded
> with enable_guc set to 1 (only enable GuC firmware loading).

Is this frequent and required scenario ? or just for debug/development ?

> This is
> because WOPCM registers were updated and locked without considering the  
> HuC
> FW size. Since we need both GuC and HuC FW sizes to determine the final
> layout of WOPCM, we should always calculate the WOPCM layout based on the
> actual sizes of the GuC and HuC firmware available for a specific  
> platform
> if we need continue to support enable/disable HuC FW loading dynamically
> with enable_guc modparam.
>
> This patch splits uC firmware fetching into two stages. First stage is to
> fetch the firmware image and verify the firmware header. uC firmware will
> be marked as verified and this will make FW info available for following
> WOPCM layout calculation. The second stage is to create a GEM object and
> copy the FW data into the created GEM object which will only be available
> when GuC/HuC loading is enabled by enable_guc modparam. This will  
> guarantee
> that the WOPCM layout will be always be calculated correctly without  
> making
> any assumptions to the GuC and HuC firmware sizes.

You are also assuming that on reload exactly the same GuC/HuC firmwares
will bee used as in initial run. This will make this useless for debug/
development scenarios, where custom fw are likely to be specified.

If we want to support enable_guc=1->3->1 scenarios for debug/dev then
maybe more flexible will be other approach that makes allocations from
the other end as proposed in [1]

[1] https://patchwork.freedesktop.org/patch/212471/

>
> v3:
>  - Rebase
>
> 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_uc.c    | 14 ++++----------
>  drivers/gpu/drm/i915/intel_uc_fw.c | 31 ++++++++++++++++++++-----------
>  drivers/gpu/drm/i915/intel_uc_fw.h |  7 +++++--
>  3 files changed, 29 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_uc.c  
> b/drivers/gpu/drm/i915/intel_uc.c
> index 1cffaf7..73b8f6c 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -172,11 +172,8 @@ void intel_uc_init_early(struct drm_i915_private  
> *i915)
> 	sanitize_options_early(i915);
> -	if (USES_GUC(i915))
> -		intel_uc_fw_fetch(i915, &guc->fw);
> -
> -	if (USES_HUC(i915))
> -		intel_uc_fw_fetch(i915, &huc->fw);
> +	intel_uc_fw_fetch(i915, &guc->fw, USES_GUC(i915));
> +	intel_uc_fw_fetch(i915, &huc->fw, USES_HUC(i915));

Hmm, side effect of those unconditional fetches might be unwanted warnings
about missing firmwares (on configs with disabled guc) as well as extended
driver load time.

Do we really need to support this corner case enable_guc=1->3 at all costs?

/michal

>  }
> void intel_uc_cleanup_early(struct drm_i915_private *i915)
> @@ -184,11 +181,8 @@ void intel_uc_cleanup_early(struct drm_i915_private  
> *i915)
>  	struct intel_guc *guc = &i915->guc;
>  	struct intel_huc *huc = &i915->huc;
> -	if (USES_HUC(i915))
> -		intel_uc_fw_fini(&huc->fw);
> -
> -	if (USES_GUC(i915))
> -		intel_uc_fw_fini(&guc->fw);
> +	intel_uc_fw_fini(&huc->fw);
> +	intel_uc_fw_fini(&guc->fw);
> 	guc_free_load_err_log(guc);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c  
> b/drivers/gpu/drm/i915/intel_uc_fw.c
> index 6e8e0b5..a9cb900 100644
> --- a/drivers/gpu/drm/i915/intel_uc_fw.c
> +++ b/drivers/gpu/drm/i915/intel_uc_fw.c
> @@ -33,11 +33,13 @@
>   *
>   * @dev_priv: device private
>   * @uc_fw: uC firmware
> + * @copy_to_obj: whether fetch uC firmware into GEM object or not

s/copy_to_obj/fetch

>   *
> - * Fetch uC firmware into GEM obj.
> + * Fetch and verify uC firmware and copy firmware data into GEM object  
> if
> + * @copy_to_obj is true.
>   */
>  void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
> -		       struct intel_uc_fw *uc_fw)
> +		       struct intel_uc_fw *uc_fw, bool copy_to_obj)
>  {
>  	struct pci_dev *pdev = dev_priv->drm.pdev;
>  	struct drm_i915_gem_object *obj;
> @@ -154,17 +156,24 @@ void intel_uc_fw_fetch(struct drm_i915_private  
> *dev_priv,
>  		goto fail;
>  	}
> -	obj = i915_gem_object_create_from_data(dev_priv, fw->data, fw->size);
> -	if (IS_ERR(obj)) {
> -		err = PTR_ERR(obj);
> -		DRM_DEBUG_DRIVER("%s fw object_create err=%d\n",
> -				 intel_uc_fw_type_repr(uc_fw->type), err);
> -		goto fail;
> +	uc_fw->size = fw->size;
> +	uc_fw->fetch_status = INTEL_UC_FIRMWARE_VERIFIED;
> +
> +	if (copy_to_obj) {
> +		obj = i915_gem_object_create_from_data(dev_priv, fw->data,
> +						       fw->size);
> +		if (IS_ERR(obj)) {
> +			err = PTR_ERR(obj);
> +			DRM_DEBUG_DRIVER("%s fw object_create err=%d\n",
> +					 intel_uc_fw_type_repr(uc_fw->type),
> +					 err);
> +			goto fail;
> +		}
> +
> +		uc_fw->obj = obj;
> +		uc_fw->fetch_status = INTEL_UC_FIRMWARE_SUCCESS;
>  	}
> -	uc_fw->obj = obj;
> -	uc_fw->size = fw->size;
> -	uc_fw->fetch_status = INTEL_UC_FIRMWARE_SUCCESS;
>  	DRM_DEBUG_DRIVER("%s fw fetch %s\n",
>  			 intel_uc_fw_type_repr(uc_fw->type),
>  			 intel_uc_fw_status_repr(uc_fw->fetch_status));
> diff --git a/drivers/gpu/drm/i915/intel_uc_fw.h  
> b/drivers/gpu/drm/i915/intel_uc_fw.h
> index dc33b12..4e7ecc8 100644
> --- a/drivers/gpu/drm/i915/intel_uc_fw.h
> +++ b/drivers/gpu/drm/i915/intel_uc_fw.h
> @@ -36,6 +36,7 @@ enum intel_uc_fw_status {
>  	INTEL_UC_FIRMWARE_FAIL = -1,
>  	INTEL_UC_FIRMWARE_NONE = 0,
>  	INTEL_UC_FIRMWARE_PENDING,
> +	INTEL_UC_FIRMWARE_VERIFIED,
>  	INTEL_UC_FIRMWARE_SUCCESS
>  };
> @@ -84,6 +85,8 @@ const char *intel_uc_fw_status_repr(enum  
> intel_uc_fw_status status)
>  		return "NONE";
>  	case INTEL_UC_FIRMWARE_PENDING:
>  		return "PENDING";
> +	case INTEL_UC_FIRMWARE_VERIFIED:
> +		return "VERIFIED";
>  	case INTEL_UC_FIRMWARE_SUCCESS:
>  		return "SUCCESS";
>  	}
> @@ -131,14 +134,14 @@ static inline void intel_uc_fw_sanitize(struct  
> intel_uc_fw *uc_fw)
>   */
>  static inline u32 intel_uc_fw_get_upload_size(struct intel_uc_fw *uc_fw)
>  {
> -	if (uc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
> +	if (uc_fw->fetch_status < INTEL_UC_FIRMWARE_VERIFIED)
>  		return 0;
> 	return uc_fw->header_size + uc_fw->ucode_size;
>  }
> void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
> -		       struct intel_uc_fw *uc_fw);
> +		       struct intel_uc_fw *uc_fw, bool copy_to_obj);
>  int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
>  		       int (*xfer)(struct intel_uc_fw *uc_fw,
>  				   struct i915_vma *vma));
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 1/4] drm/i915: Always do WOPCM partitioning based on real firmware sizes

[Yaodong Li]

On 04/13/2018 07:15 PM, Michal Wajdeczko wrote:
> On Tue, 10 Apr 2018 02:42:17 +0200, Jackie Li <yaodong.li@intel.com> 
> wrote:
>
>> After enabled the WOPCM write-once registers locking status checking,
>> reloading of the i915 module will fail with modparam enable_guc set to 3
>> (enable GuC and HuC firmware loading) if the module was originally 
>> loaded
>> with enable_guc set to 1 (only enable GuC firmware loading).
>
> Is this frequent and required scenario ? or just for debug/development ?
>
My understanding is this should be a nice to have feature and mainly for 
debugging.
>> This is
>> because WOPCM registers were updated and locked without considering 
>> the HuC
>> FW size. Since we need both GuC and HuC FW sizes to determine the final
>> layout of WOPCM, we should always calculate the WOPCM layout based on 
>> the
>> actual sizes of the GuC and HuC firmware available for a specific 
>> platform
>> if we need continue to support enable/disable HuC FW loading dynamically
>> with enable_guc modparam.
>>
>> This patch splits uC firmware fetching into two stages. First stage 
>> is to
>> fetch the firmware image and verify the firmware header. uC firmware 
>> will
>> be marked as verified and this will make FW info available for following
>> WOPCM layout calculation. The second stage is to create a GEM object and
>> copy the FW data into the created GEM object which will only be 
>> available
>> when GuC/HuC loading is enabled by enable_guc modparam. This will 
>> guarantee
>> that the WOPCM layout will be always be calculated correctly without 
>> making
>> any assumptions to the GuC and HuC firmware sizes.
>
> You are also assuming that on reload exactly the same GuC/HuC firmwares
> will bee used as in initial run. This will make this useless for debug/
> development scenarios, where custom fw are likely to be specified.
>
This patch is mainly for providing a real fix to support 
enable_guc=1->3->1 use case.
It based on the fact that it is inevitable that sometimes we need to 
reboot the system
if the status of the fw was changed on the file system.
I am not sure how often we switch between different HuC FW with 
different sizes?
> If we want to support enable_guc=1->3->1 scenarios for debug/dev then
> maybe more flexible will be other approach that makes allocations from
> the other end as proposed in [1]
>
> [1] https://patchwork.freedesktop.org/patch/212471/
Actually, I do think this might be one of the options, and I've also put 
some comments on this
series. The main concern I have is it still make assumption on the GuC 
FW size and may
run into the same issue if the GuC FW failed to meet the requirement.
and for debugging purpose it would have the same possible for different 
GuC FW debugging.
>
>>
>> v3:
>>  - Rebase
>>
>> 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_uc.c    | 14 ++++----------
>>  drivers/gpu/drm/i915/intel_uc_fw.c | 31 ++++++++++++++++++++-----------
>>  drivers/gpu/drm/i915/intel_uc_fw.h |  7 +++++--
>>  3 files changed, 29 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_uc.c 
>> b/drivers/gpu/drm/i915/intel_uc.c
>> index 1cffaf7..73b8f6c 100644
>> --- a/drivers/gpu/drm/i915/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>> @@ -172,11 +172,8 @@ void intel_uc_init_early(struct drm_i915_private 
>> *i915)
>>     sanitize_options_early(i915);
>> -    if (USES_GUC(i915))
>> -        intel_uc_fw_fetch(i915, &guc->fw);
>> -
>> -    if (USES_HUC(i915))
>> -        intel_uc_fw_fetch(i915, &huc->fw);
>> +    intel_uc_fw_fetch(i915, &guc->fw, USES_GUC(i915));
>> +    intel_uc_fw_fetch(i915, &huc->fw, USES_HUC(i915));
>
> Hmm, side effect of those unconditional fetches might be unwanted 
> warnings
> about missing firmwares (on configs with disabled guc) as well as 
> extended
> driver load time.
Hmm, if HAS_GUC is false then fw path would be NULL. The fetch will 
return directly.
>
> Do we really need to support this corner case enable_guc=1->3 at all 
> costs?
I think this is the real solution for this issue (with no assumption). 
However, we do
need to decide whether we should support such a corner case which is 
mainly for
debugging.
>
> /michal
>
>>  }
>> void intel_uc_cleanup_early(struct drm_i915_private *i915)
>> @@ -184,11 +181,8 @@ void intel_uc_cleanup_early(struct 
>> drm_i915_private *i915)
>>      struct intel_guc *guc = &i915->guc;
>>      struct intel_huc *huc = &i915->huc;
>> -    if (USES_HUC(i915))
>> -        intel_uc_fw_fini(&huc->fw);
>> -
>> -    if (USES_GUC(i915))
>> -        intel_uc_fw_fini(&guc->fw);
>> +    intel_uc_fw_fini(&huc->fw);
>> +    intel_uc_fw_fini(&guc->fw);
>>     guc_free_load_err_log(guc);
>>  }
>> diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c 
>> b/drivers/gpu/drm/i915/intel_uc_fw.c
>> index 6e8e0b5..a9cb900 100644
>> --- a/drivers/gpu/drm/i915/intel_uc_fw.c
>> +++ b/drivers/gpu/drm/i915/intel_uc_fw.c
>> @@ -33,11 +33,13 @@
>>   *
>>   * @dev_priv: device private
>>   * @uc_fw: uC firmware
>> + * @copy_to_obj: whether fetch uC firmware into GEM object or not
>
> s/copy_to_obj/fetch
sure.
>
>>   *
>> - * Fetch uC firmware into GEM obj.
>> + * Fetch and verify uC firmware and copy firmware data into GEM 
>> object if
>> + * @copy_to_obj is true.
>>   */
>>  void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
>> -               struct intel_uc_fw *uc_fw)
>> +               struct intel_uc_fw *uc_fw, bool copy_to_obj)
>>  {
>>      struct pci_dev *pdev = dev_priv->drm.pdev;
>>      struct drm_i915_gem_object *obj;
>> @@ -154,17 +156,24 @@ void intel_uc_fw_fetch(struct drm_i915_private 
>> *dev_priv,
>>          goto fail;
>>      }
>> -    obj = i915_gem_object_create_from_data(dev_priv, fw->data, 
>> fw->size);
>> -    if (IS_ERR(obj)) {
>> -        err = PTR_ERR(obj);
>> -        DRM_DEBUG_DRIVER("%s fw object_create err=%d\n",
>> -                 intel_uc_fw_type_repr(uc_fw->type), err);
>> -        goto fail;
>> +    uc_fw->size = fw->size;
>> +    uc_fw->fetch_status = INTEL_UC_FIRMWARE_VERIFIED;
>> +
>> +    if (copy_to_obj) {
>> +        obj = i915_gem_object_create_from_data(dev_priv, fw->data,
>> +                               fw->size);
>> +        if (IS_ERR(obj)) {
>> +            err = PTR_ERR(obj);
>> +            DRM_DEBUG_DRIVER("%s fw object_create err=%d\n",
>> +                     intel_uc_fw_type_repr(uc_fw->type),
>> +                     err);
>> +            goto fail;
>> +        }
>> +
>> +        uc_fw->obj = obj;
>> +        uc_fw->fetch_status = INTEL_UC_FIRMWARE_SUCCESS;
>>      }
>> -    uc_fw->obj = obj;
>> -    uc_fw->size = fw->size;
>> -    uc_fw->fetch_status = INTEL_UC_FIRMWARE_SUCCESS;
>>      DRM_DEBUG_DRIVER("%s fw fetch %s\n",
>>               intel_uc_fw_type_repr(uc_fw->type),
>>               intel_uc_fw_status_repr(uc_fw->fetch_status));
>> diff --git a/drivers/gpu/drm/i915/intel_uc_fw.h 
>> b/drivers/gpu/drm/i915/intel_uc_fw.h
>> index dc33b12..4e7ecc8 100644
>> --- a/drivers/gpu/drm/i915/intel_uc_fw.h
>> +++ b/drivers/gpu/drm/i915/intel_uc_fw.h
>> @@ -36,6 +36,7 @@ enum intel_uc_fw_status {
>>      INTEL_UC_FIRMWARE_FAIL = -1,
>>      INTEL_UC_FIRMWARE_NONE = 0,
>>      INTEL_UC_FIRMWARE_PENDING,
>> +    INTEL_UC_FIRMWARE_VERIFIED,
>>      INTEL_UC_FIRMWARE_SUCCESS
>>  };
>> @@ -84,6 +85,8 @@ const char *intel_uc_fw_status_repr(enum 
>> intel_uc_fw_status status)
>>          return "NONE";
>>      case INTEL_UC_FIRMWARE_PENDING:
>>          return "PENDING";
>> +    case INTEL_UC_FIRMWARE_VERIFIED:
>> +        return "VERIFIED";
>>      case INTEL_UC_FIRMWARE_SUCCESS:
>>          return "SUCCESS";
>>      }
>> @@ -131,14 +134,14 @@ static inline void intel_uc_fw_sanitize(struct 
>> intel_uc_fw *uc_fw)
>>   */
>>  static inline u32 intel_uc_fw_get_upload_size(struct intel_uc_fw 
>> *uc_fw)
>>  {
>> -    if (uc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
>> +    if (uc_fw->fetch_status < INTEL_UC_FIRMWARE_VERIFIED)
>>          return 0;
>>     return uc_fw->header_size + uc_fw->ucode_size;
>>  }
>> void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
>> -               struct intel_uc_fw *uc_fw);
>> +               struct intel_uc_fw *uc_fw, bool copy_to_obj);
>>  int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
>>                 int (*xfer)(struct intel_uc_fw *uc_fw,
>>                     struct i915_vma *vma));

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

Re: [PATCH v3 1/4] drm/i915: Always do WOPCM partitioning based on real firmware sizes

[Michal Wajdeczko]

On Mon, 16 Apr 2018 19:28:04 +0200, Yaodong Li <yaodong.li@intel.com>  
wrote:

> On 04/13/2018 07:15 PM, Michal Wajdeczko wrote:
>> On Tue, 10 Apr 2018 02:42:17 +0200, Jackie Li <yaodong.li@intel.com>  
>> wrote:
>>
>>> After enabled the WOPCM write-once registers locking status checking,
>>> reloading of the i915 module will fail with modparam enable_guc set to  
>>> 3
>>> (enable GuC and HuC firmware loading) if the module was originally  
>>> loaded
>>> with enable_guc set to 1 (only enable GuC firmware loading).
>>
>> Is this frequent and required scenario ? or just for debug/development ?
>>
> My understanding is this should be a nice to have feature and mainly for  
> debugging.
>>> This is
>>> because WOPCM registers were updated and locked without considering  
>>> the HuC
>>> FW size. Since we need both GuC and HuC FW sizes to determine the final
>>> layout of WOPCM, we should always calculate the WOPCM layout based on  
>>> the
>>> actual sizes of the GuC and HuC firmware available for a specific  
>>> platform
>>> if we need continue to support enable/disable HuC FW loading  
>>> dynamically
>>> with enable_guc modparam.
>>>
>>> This patch splits uC firmware fetching into two stages. First stage is  
>>> to
>>> fetch the firmware image and verify the firmware header. uC firmware  
>>> will
>>> be marked as verified and this will make FW info available for  
>>> following
>>> WOPCM layout calculation. The second stage is to create a GEM object  
>>> and
>>> copy the FW data into the created GEM object which will only be  
>>> available
>>> when GuC/HuC loading is enabled by enable_guc modparam. This will  
>>> guarantee
>>> that the WOPCM layout will be always be calculated correctly without  
>>> making
>>> any assumptions to the GuC and HuC firmware sizes.
>>
>> You are also assuming that on reload exactly the same GuC/HuC firmwares
>> will bee used as in initial run. This will make this useless for debug/
>> development scenarios, where custom fw are likely to be specified.
>>
> This patch is mainly for providing a real fix to support  
> enable_guc=1->3->1 use case.
> It based on the fact that it is inevitable that sometimes we need to  
> reboot the system
> if the status of the fw was changed on the file system.

What do you mean by "status of the fw was changed on the file system" ?
* change of the fw binary/version/size, or
* change from not-present to present ?

> I am not sure how often we switch between different HuC FW with  
> different sizes?

Just above you said that you need this "mainly for debugging" so
I would expect that then different fw sizes are expected.

>> If we want to support enable_guc=1->3->1 scenarios for debug/dev then
>> maybe more flexible will be other approach that makes allocations from
>> the other end as proposed in [1]
>>
>> [1] https://patchwork.freedesktop.org/patch/212471/
> Actually, I do think this might be one of the options, and I've also put  
> some comments on this
> series. The main concern I have is it still make assumption on the GuC  
> FW size and may

But in enable_guc=1-->3 scenario, I would assume that the only difference
will be HuC fw (as with enable=1 we already loaded GuC)

If you want just to test different GuC fws, then it is different scenario
as then enable_guc will always be = 1.

> run into the same issue if the GuC FW failed to meet the requirement.
> and for debugging purpose it would have the same possible for different  
> GuC FW debugging.
>>
>>>
>>> v3:
>>>  - Rebase
>>>
>>> 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_uc.c    | 14 ++++----------
>>>  drivers/gpu/drm/i915/intel_uc_fw.c | 31  
>>> ++++++++++++++++++++-----------
>>>  drivers/gpu/drm/i915/intel_uc_fw.h |  7 +++++--
>>>  3 files changed, 29 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_uc.c  
>>> b/drivers/gpu/drm/i915/intel_uc.c
>>> index 1cffaf7..73b8f6c 100644
>>> --- a/drivers/gpu/drm/i915/intel_uc.c
>>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>>> @@ -172,11 +172,8 @@ void intel_uc_init_early(struct drm_i915_private  
>>> *i915)
>>>     sanitize_options_early(i915);
>>> -    if (USES_GUC(i915))
>>> -        intel_uc_fw_fetch(i915, &guc->fw);
>>> -
>>> -    if (USES_HUC(i915))
>>> -        intel_uc_fw_fetch(i915, &huc->fw);
>>> +    intel_uc_fw_fetch(i915, &guc->fw, USES_GUC(i915));
>>> +    intel_uc_fw_fetch(i915, &huc->fw, USES_HUC(i915));
>>
>> Hmm, side effect of those unconditional fetches might be unwanted  
>> warnings
>> about missing firmwares (on configs with disabled guc) as well as  
>> extended
>> driver load time.
> Hmm, if HAS_GUC is false then fw path would be NULL. The fetch will  
> return directly.

I was referring to scenario when on platform with HAS_HUC and with
enable_guc=1 (just submission, no HuC) we will try to fetch HuC fw
(that may not be present at all) and then drop it as don't need it.

>>
>> Do we really need to support this corner case enable_guc=1->3 at all  
>> costs?
> I think this is the real solution for this issue (with no assumption).  
> However, we do
> need to decide whether we should support such a corner case which is  
> mainly for
> debugging.

I'm repeating here Joonas' earlier statement:

"Then just require a reboot if after that partitioning,
  changing the parameter causes the FW not to fit"


>>
>> /michal
>>
>>>  }
>>> void intel_uc_cleanup_early(struct drm_i915_private *i915)
>>> @@ -184,11 +181,8 @@ void intel_uc_cleanup_early(struct  
>>> drm_i915_private *i915)
>>>      struct intel_guc *guc = &i915->guc;
>>>      struct intel_huc *huc = &i915->huc;
>>> -    if (USES_HUC(i915))
>>> -        intel_uc_fw_fini(&huc->fw);
>>> -
>>> -    if (USES_GUC(i915))
>>> -        intel_uc_fw_fini(&guc->fw);
>>> +    intel_uc_fw_fini(&huc->fw);
>>> +    intel_uc_fw_fini(&guc->fw);
>>>     guc_free_load_err_log(guc);
>>>  }
>>> diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c  
>>> b/drivers/gpu/drm/i915/intel_uc_fw.c
>>> index 6e8e0b5..a9cb900 100644
>>> --- a/drivers/gpu/drm/i915/intel_uc_fw.c
>>> +++ b/drivers/gpu/drm/i915/intel_uc_fw.c
>>> @@ -33,11 +33,13 @@
>>>   *
>>>   * @dev_priv: device private
>>>   * @uc_fw: uC firmware
>>> + * @copy_to_obj: whether fetch uC firmware into GEM object or not
>>
>> s/copy_to_obj/fetch
> sure.
>>
>>>   *
>>> - * Fetch uC firmware into GEM obj.
>>> + * Fetch and verify uC firmware and copy firmware data into GEM  
>>> object if
>>> + * @copy_to_obj is true.
>>>   */
>>>  void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
>>> -               struct intel_uc_fw *uc_fw)
>>> +               struct intel_uc_fw *uc_fw, bool copy_to_obj)
>>>  {
>>>      struct pci_dev *pdev = dev_priv->drm.pdev;
>>>      struct drm_i915_gem_object *obj;
>>> @@ -154,17 +156,24 @@ void intel_uc_fw_fetch(struct drm_i915_private  
>>> *dev_priv,
>>>          goto fail;
>>>      }
>>> -    obj = i915_gem_object_create_from_data(dev_priv, fw->data,  
>>> fw->size);
>>> -    if (IS_ERR(obj)) {
>>> -        err = PTR_ERR(obj);
>>> -        DRM_DEBUG_DRIVER("%s fw object_create err=%d\n",
>>> -                 intel_uc_fw_type_repr(uc_fw->type), err);
>>> -        goto fail;
>>> +    uc_fw->size = fw->size;
>>> +    uc_fw->fetch_status = INTEL_UC_FIRMWARE_VERIFIED;
>>> +
>>> +    if (copy_to_obj) {
>>> +        obj = i915_gem_object_create_from_data(dev_priv, fw->data,
>>> +                               fw->size);
>>> +        if (IS_ERR(obj)) {
>>> +            err = PTR_ERR(obj);
>>> +            DRM_DEBUG_DRIVER("%s fw object_create err=%d\n",
>>> +                     intel_uc_fw_type_repr(uc_fw->type),
>>> +                     err);
>>> +            goto fail;
>>> +        }
>>> +
>>> +        uc_fw->obj = obj;
>>> +        uc_fw->fetch_status = INTEL_UC_FIRMWARE_SUCCESS;
>>>      }
>>> -    uc_fw->obj = obj;
>>> -    uc_fw->size = fw->size;
>>> -    uc_fw->fetch_status = INTEL_UC_FIRMWARE_SUCCESS;
>>>      DRM_DEBUG_DRIVER("%s fw fetch %s\n",
>>>               intel_uc_fw_type_repr(uc_fw->type),
>>>               intel_uc_fw_status_repr(uc_fw->fetch_status));
>>> diff --git a/drivers/gpu/drm/i915/intel_uc_fw.h  
>>> b/drivers/gpu/drm/i915/intel_uc_fw.h
>>> index dc33b12..4e7ecc8 100644
>>> --- a/drivers/gpu/drm/i915/intel_uc_fw.h
>>> +++ b/drivers/gpu/drm/i915/intel_uc_fw.h
>>> @@ -36,6 +36,7 @@ enum intel_uc_fw_status {
>>>      INTEL_UC_FIRMWARE_FAIL = -1,
>>>      INTEL_UC_FIRMWARE_NONE = 0,
>>>      INTEL_UC_FIRMWARE_PENDING,
>>> +    INTEL_UC_FIRMWARE_VERIFIED,
>>>      INTEL_UC_FIRMWARE_SUCCESS
>>>  };
>>> @@ -84,6 +85,8 @@ const char *intel_uc_fw_status_repr(enum  
>>> intel_uc_fw_status status)
>>>          return "NONE";
>>>      case INTEL_UC_FIRMWARE_PENDING:
>>>          return "PENDING";
>>> +    case INTEL_UC_FIRMWARE_VERIFIED:
>>> +        return "VERIFIED";
>>>      case INTEL_UC_FIRMWARE_SUCCESS:
>>>          return "SUCCESS";
>>>      }
>>> @@ -131,14 +134,14 @@ static inline void intel_uc_fw_sanitize(struct  
>>> intel_uc_fw *uc_fw)
>>>   */
>>>  static inline u32 intel_uc_fw_get_upload_size(struct intel_uc_fw  
>>> *uc_fw)
>>>  {
>>> -    if (uc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
>>> +    if (uc_fw->fetch_status < INTEL_UC_FIRMWARE_VERIFIED)
>>>          return 0;
>>>     return uc_fw->header_size + uc_fw->ucode_size;
>>>  }
>>> void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
>>> -               struct intel_uc_fw *uc_fw);
>>> +               struct intel_uc_fw *uc_fw, bool copy_to_obj);
>>>  int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
>>>                 int (*xfer)(struct intel_uc_fw *uc_fw,
>>>                     struct i915_vma *vma));
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3 1/4] drm/i915: Always do WOPCM partitioning based on real firmware sizes

[Yaodong Li]

On 04/19/2018 08:31 AM, Michal Wajdeczko wrote:
> On Mon, 16 Apr 2018 19:28:04 +0200, Yaodong Li <yaodong.li@intel.com> 
> wrote:
>
>> On 04/13/2018 07:15 PM, Michal Wajdeczko wrote:
>>> On Tue, 10 Apr 2018 02:42:17 +0200, Jackie Li <yaodong.li@intel.com> 
>>> wrote:
>>>
>>>> After enabled the WOPCM write-once registers locking status checking,
>>>> reloading of the i915 module will fail with modparam enable_guc set 
>>>> to 3
>>>> (enable GuC and HuC firmware loading) if the module was originally 
>>>> loaded
>>>> with enable_guc set to 1 (only enable GuC firmware loading).
>>>
>>> Is this frequent and required scenario ? or just for 
>>> debug/development ?
>>>
>> My understanding is this should be a nice to have feature and mainly 
>> for debugging.
>>>> This is
>>>> because WOPCM registers were updated and locked without considering 
>>>> the HuC
>>>> FW size. Since we need both GuC and HuC FW sizes to determine the 
>>>> final
>>>> layout of WOPCM, we should always calculate the WOPCM layout based 
>>>> on the
>>>> actual sizes of the GuC and HuC firmware available for a specific 
>>>> platform
>>>> if we need continue to support enable/disable HuC FW loading 
>>>> dynamically
>>>> with enable_guc modparam.
>>>>
>>>> This patch splits uC firmware fetching into two stages. First stage 
>>>> is to
>>>> fetch the firmware image and verify the firmware header. uC 
>>>> firmware will
>>>> be marked as verified and this will make FW info available for 
>>>> following
>>>> WOPCM layout calculation. The second stage is to create a GEM 
>>>> object and
>>>> copy the FW data into the created GEM object which will only be 
>>>> available
>>>> when GuC/HuC loading is enabled by enable_guc modparam. This will 
>>>> guarantee
>>>> that the WOPCM layout will be always be calculated correctly 
>>>> without making
>>>> any assumptions to the GuC and HuC firmware sizes.
>>>
>>> You are also assuming that on reload exactly the same GuC/HuC firmwares
>>> will bee used as in initial run. This will make this useless for debug/
>>> development scenarios, where custom fw are likely to be specified.
>>>
>> This patch is mainly for providing a real fix to support 
>> enable_guc=1->3->1 use case.
>> It based on the fact that it is inevitable that sometimes we need to 
>> reboot the system
>> if the status of the fw was changed on the file system.
>
> What do you mean by "status of the fw was changed on the file system" ?
> * change of the fw binary/version/size, or
> * change from not-present to present ?
I think it should include all of the presence changes, file updates.
>
>> I am not sure how often we switch between different HuC FW with 
>> different sizes?
>
> Just above you said that you need this "mainly for debugging" so
> I would expect that then different fw sizes are expected.
>
>>> If we want to support enable_guc=1->3->1 scenarios for debug/dev then
>>> maybe more flexible will be other approach that makes allocations from
>>> the other end as proposed in [1]
>>>
>>> [1] https://patchwork.freedesktop.org/patch/212471/
>> Actually, I do think this might be one of the options, and I've also 
>> put some comments on this
>> series. The main concern I have is it still make assumption on the 
>> GuC FW size and may
>
> But in enable_guc=1-->3 scenario, I would assume that the only difference
> will be HuC fw (as with enable=1 we already loaded GuC)
Hmm, my main concern to the current "from the end" solution is it makes 
assumption on
the GuC FW size in order to meet the HW restriction.
>
> If you want just to test different GuC fws, then it is different scenario
> as then enable_guc will always be = 1.
>
what I mean is the "from the end" approach will lead to the same issue 
for different GuC FW sizes - we
may have to reboot the system for GuC FW debugging (different GuC FW 
sizes) even if enable_guc is always
set to 1. However, with the current "from the beginning" way we won't 
run into such problems
for GuC FW debugging (since it's already used the max available space). 
Thus I think we should
define the enable_guc = 1->3->1 as following:
we would support use of enable_guc=1->3->1 correctly without system 
reboot for the present FWs. A system
reboot will be expected (but not necessarily happen if we found current 
partition works for the new FWs)
for any FW changes (including add/remove/update).

if we decide to drop the support for enable_guc=1->3->1 since it's only 
for debugging purpose then we should
expect a system reboot for either "from the end" or "from the beginning" 
solutions since we cannot 100% have
this issue (changing FW without a system boot) solved. Therefore, the 
require of system reboot should not be
a bug when it comes to FW updating.

>> run into the same issue if the GuC FW failed to meet the requirement.
>> and for debugging purpose it would have the same possible for 
>> different GuC FW debugging.
>>>
>>>>
>>>> v3:
>>>>  - Rebase
>>>>
>>>> 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_uc.c    | 14 ++++----------
>>>>  drivers/gpu/drm/i915/intel_uc_fw.c | 31 
>>>> ++++++++++++++++++++-----------
>>>>  drivers/gpu/drm/i915/intel_uc_fw.h |  7 +++++--
>>>>  3 files changed, 29 insertions(+), 23 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_uc.c 
>>>> b/drivers/gpu/drm/i915/intel_uc.c
>>>> index 1cffaf7..73b8f6c 100644
>>>> --- a/drivers/gpu/drm/i915/intel_uc.c
>>>> +++ b/drivers/gpu/drm/i915/intel_uc.c
>>>> @@ -172,11 +172,8 @@ void intel_uc_init_early(struct 
>>>> drm_i915_private *i915)
>>>>     sanitize_options_early(i915);
>>>> -    if (USES_GUC(i915))
>>>> -        intel_uc_fw_fetch(i915, &guc->fw);
>>>> -
>>>> -    if (USES_HUC(i915))
>>>> -        intel_uc_fw_fetch(i915, &huc->fw);
>>>> +    intel_uc_fw_fetch(i915, &guc->fw, USES_GUC(i915));
>>>> +    intel_uc_fw_fetch(i915, &huc->fw, USES_HUC(i915));
>>>
>>> Hmm, side effect of those unconditional fetches might be unwanted 
>>> warnings
>>> about missing firmwares (on configs with disabled guc) as well as 
>>> extended
>>> driver load time.
>> Hmm, if HAS_GUC is false then fw path would be NULL. The fetch will 
>> return directly.
>
> I was referring to scenario when on platform with HAS_HUC and with
> enable_guc=1 (just submission, no HuC) we will try to fetch HuC fw
> (that may not be present at all) and then drop it as don't need it.
>
I think there are two scenarios here for this specific case - a platform 
with HAS_HUC = 1  and only GuC submission is needed:
0) No HuC FW available - We should expect a system reboot for adding new FW.
1) If HuC FW is present - always get the FW header info in order to 
support possible enable_guc=1->3->1.

IMHO, the problem we have here is that we need to define the use case 
precisely. e.g. whether we shall support
enable_guc=1->3->1 flawlessly? and whether we shall support dynamic HuC 
FW sizes for debugging rather than
supporting dynamic GuC FW sizes for debugging purpose?
>>>
>>> Do we really need to support this corner case enable_guc=1->3 at all 
>>> costs?
>> I think this is the real solution for this issue (with no 
>> assumption). However, we do
>> need to decide whether we should support such a corner case which is 
>> mainly for
>> debugging.
>
> I'm repeating here Joonas' earlier statement:
>
> "Then just require a reboot if after that partitioning,
>  changing the parameter causes the FW not to fit"
>
That's my thought too:)

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

[PATCH v3 1/4] drm/i915/uc: Use correct error code for GuC initialization failure

[Michal Wajdeczko]

Since commit 6ca9a2beb54a ("drm/i915: Unwind i915_gem_init() failure")
we believed that we correctly handle all errors encountered during
GuC initialization, including special one that indicates request to
run driver with disabled GPU submission (-EIO).

Unfortunately since commit 121981fafe69 ("drm/i915/guc: Combine
enable_guc_loading|submission modparams") we stopped using that
error code to avoid unwanted fallback to execlist submission mode.

In result any GuC initialization failure was treated as non-recoverable
error leading to driver load abort, so we could not even read related
GuC error log to investigate cause of the problem.

Fix that by always returning -EIO on uC hardware related failure.

v2: don't allow -EIO from uc_init
    don't call uc_fini[_misc] on -EIO
    mark guc fw as failed on hw init failure
    prepare uc_fini_hw to run after earlier -EIO

v3: update comments (Sagar)
    use sanitize functions on failure in init_hw (Michal)
    and also sanitize guc/huc fw in fini_hw (Michal)

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michal Winiarski <michal.winiarski@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c    | 17 ++++++++++-------
 drivers/gpu/drm/i915/intel_guc.h   |  5 +++++
 drivers/gpu/drm/i915/intel_uc.c    | 18 ++++++++++++++----
 drivers/gpu/drm/i915/intel_uc_fw.h |  5 +++++
 4 files changed, 34 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 05b0724..8eed87d 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -5338,8 +5338,10 @@ int i915_gem_init(struct drm_i915_private *dev_priv)
 	intel_init_gt_powersave(dev_priv);
 
 	ret = intel_uc_init(dev_priv);
-	if (ret)
+	if (ret) {
+		GEM_BUG_ON(ret == -EIO);
 		goto err_pm;
+	}
 
 	ret = i915_gem_init_hw(dev_priv);
 	if (ret)
@@ -5386,7 +5388,8 @@ int i915_gem_init(struct drm_i915_private *dev_priv)
 	i915_gem_contexts_lost(dev_priv);
 	intel_uc_fini_hw(dev_priv);
 err_uc_init:
-	intel_uc_fini(dev_priv);
+	if (ret != -EIO)
+		intel_uc_fini(dev_priv);
 err_pm:
 	if (ret != -EIO) {
 		intel_cleanup_gt_powersave(dev_priv);
@@ -5400,15 +5403,15 @@ int i915_gem_init(struct drm_i915_private *dev_priv)
 	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
 	mutex_unlock(&dev_priv->drm.struct_mutex);
 
-	intel_uc_fini_misc(dev_priv);
-
-	if (ret != -EIO)
+	if (ret != -EIO) {
+		intel_uc_fini_misc(dev_priv);
 		i915_gem_cleanup_userptr(dev_priv);
+	}
 
 	if (ret == -EIO) {
 		/*
-		 * Allow engine initialisation to fail by marking the GPU as
-		 * wedged. But we only want to do this where the GPU is angry,
+		 * Allow engines or uC initialization to fail by marking the GPU
+		 * as wedged. But we only want to do this when the GPU is angry,
 		 * for all other failure, such as an allocation failure, bail.
 		 */
 		if (!i915_terminally_wedged(&dev_priv->gpu_error)) {
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index d878160..faa9e01 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -139,4 +139,9 @@ static inline int intel_guc_sanitize(struct intel_guc *guc)
 	return 0;
 }
 
+static inline bool intel_guc_is_loaded(struct intel_guc *guc)
+{
+	return intel_uc_fw_is_loaded(&guc->fw);
+}
+
 #endif
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 9d5ffd7..f89acf4 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -447,15 +447,20 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	 * Note that there is no fallback as either user explicitly asked for
 	 * the GuC or driver default option was to run with the GuC enabled.
 	 */
-	if (GEM_WARN_ON(ret == -EIO))
-		ret = -EINVAL;
-
 	dev_err(dev_priv->drm.dev, "GuC initialization failed %d\n", ret);
-	return ret;
+
+	/* Sanitize GuC/HuC to avoid clean-up on wedged */
+	intel_huc_sanitize(huc);
+	intel_guc_sanitize(guc);
+	GEM_BUG_ON(intel_guc_is_loaded(guc));
+
+	/* We want to disable GPU submission but keep KMS alive */
+	return -EIO;
 }
 
 void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
 {
+	struct intel_huc *huc = &dev_priv->huc;
 	struct intel_guc *guc = &dev_priv->guc;
 
 	if (!USES_GUC(dev_priv))
@@ -463,10 +468,15 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
 
 	GEM_BUG_ON(!HAS_GUC(dev_priv));
 
+	if (!intel_guc_is_loaded(guc))
+		return;
+
 	if (USES_GUC_SUBMISSION(dev_priv))
 		intel_guc_submission_disable(guc);
 
 	guc_disable_communication(guc);
+	intel_huc_sanitize(huc);
+	intel_guc_sanitize(guc);
 
 	if (USES_GUC_SUBMISSION(dev_priv))
 		gen9_disable_guc_interrupts(dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_uc_fw.h b/drivers/gpu/drm/i915/intel_uc_fw.h
index 2601521..ff9ce9a 100644
--- a/drivers/gpu/drm/i915/intel_uc_fw.h
+++ b/drivers/gpu/drm/i915/intel_uc_fw.h
@@ -121,6 +121,11 @@ static inline void intel_uc_fw_sanitize(struct intel_uc_fw *uc_fw)
 		uc_fw->load_status = INTEL_UC_FIRMWARE_PENDING;
 }
 
+static inline bool intel_uc_fw_is_loaded(struct intel_uc_fw *uc_fw)
+{
+	return uc_fw->load_status == INTEL_UC_FIRMWARE_SUCCESS;
+}
+
 void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
 		       struct intel_uc_fw *uc_fw);
 int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
-- 
1.9.1

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

[PATCH v3 4/4] HAX: Enable GuC for CI

[Michal Wajdeczko]

v2: except running with HYPERVISOR

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
---
 drivers/gpu/drm/i915/i915_params.h | 2 +-
 drivers/gpu/drm/i915/intel_uc.c    | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index 430f5f9..3deae1e 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -47,7 +47,7 @@
 	param(int, disable_power_well, -1) \
 	param(int, enable_ips, 1) \
 	param(int, invert_brightness, 0) \
-	param(int, enable_guc, 0) \
+	param(int, enable_guc, -1) \
 	param(int, guc_log_level, 0) \
 	param(char *, guc_firmware_path, NULL) \
 	param(char *, huc_firmware_path, NULL) \
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index c22155b..22af945 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -63,6 +63,8 @@ static int __get_platform_enable_guc(struct drm_i915_private *i915)
 		enable_guc |= ENABLE_GUC_LOAD_HUC;
 
 	/* Any platform specific fine-tuning can be done here */
+	if (boot_cpu_has(X86_FEATURE_HYPERVISOR))
+		enable_guc = 0;
 
 	return enable_guc;
 }
-- 
1.9.1

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