Re: [Intel-gfx] [PATCH 2/2] drm/i915/huc: Adjust HuC state accordingly after GuC fetch error

2020-07-07 Thread Daniele Ceraolo Spurio



On 7/7/2020 2:52 PM, Michał Winiarski wrote:

From: Michał Winiarski 

Firmware "Selected" state is a transient state - we don't expect to see
it after finishing driver probe, we even have asserts sprinkled over
i915 to confirm whether that's the case.
Unfortunately - we don't handle the transition out of "Selected" in case
of GuC fetch error, leading those asserts to fire when calling
"intel_huc_is_used()".

Reported-by: Marcin Bernatowicz 
Signed-off-by: Michał Winiarski 
Cc: Chris Wilson 
Cc: Daniele Ceraolo Spurio 
Cc: Marcin Bernatowicz 
Cc: Michal Wajdeczko 
---
  drivers/gpu/drm/i915/gt/uc/intel_uc.c | 8 +++-
  1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c 
b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
index 1c2d6358826c..993e9755f317 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
@@ -267,8 +267,14 @@ static void __uc_fetch_firmwares(struct intel_uc *uc)
GEM_BUG_ON(!intel_uc_wants_guc(uc));
  
  	err = intel_uc_fw_fetch(>guc.fw);

-   if (err)
+   if (err) {
+   /* Make sure we transition out of transient "SELECTED" state */
+   if (intel_uc_wants_huc(uc))
+   intel_uc_fw_change_status(>huc.fw,
+ INTEL_UC_FIRMWARE_ERROR);


I think that a debug message saying that we're disabling HuC because GuC 
FW was not found would be useful to make it clear that the error is not 
related to the HuC blob itself.



+
return;
+   }
  
  	if (intel_uc_wants_huc(uc))

intel_uc_fw_fetch(>huc.fw);


It looks like this function could use a bit of rework for better onion 
unwinding because if we fail to fetch the HuC and we don't want GuC 
submission we should disable the GuC.

That's not an issue with this patch, so with the added debug log:

Reviewed-by: Daniele Ceraolo Spurio

Daniele


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


[Intel-gfx] [PATCH 2/2] drm/i915/huc: Adjust HuC state accordingly after GuC fetch error

2020-07-07 Thread Michał Winiarski
From: Michał Winiarski 

Firmware "Selected" state is a transient state - we don't expect to see
it after finishing driver probe, we even have asserts sprinkled over
i915 to confirm whether that's the case.
Unfortunately - we don't handle the transition out of "Selected" in case
of GuC fetch error, leading those asserts to fire when calling
"intel_huc_is_used()".

Reported-by: Marcin Bernatowicz 
Signed-off-by: Michał Winiarski 
Cc: Chris Wilson 
Cc: Daniele Ceraolo Spurio 
Cc: Marcin Bernatowicz 
Cc: Michal Wajdeczko 
---
 drivers/gpu/drm/i915/gt/uc/intel_uc.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c 
b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
index 1c2d6358826c..993e9755f317 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
@@ -267,8 +267,14 @@ static void __uc_fetch_firmwares(struct intel_uc *uc)
GEM_BUG_ON(!intel_uc_wants_guc(uc));
 
err = intel_uc_fw_fetch(>guc.fw);
-   if (err)
+   if (err) {
+   /* Make sure we transition out of transient "SELECTED" state */
+   if (intel_uc_wants_huc(uc))
+   intel_uc_fw_change_status(>huc.fw,
+ INTEL_UC_FIRMWARE_ERROR);
+
return;
+   }
 
if (intel_uc_wants_huc(uc))
intel_uc_fw_fetch(>huc.fw);
-- 
2.27.0

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