[PATCH 0/3] drm/i915: HuC updates

[PATCH 0/3] drm/i915: HuC updates

[Michal Wajdeczko]

Test-with: <20190519201634.24816-1-michal.wajdeczko@intel.com>

Michal Wajdeczko (3):
  drm/i915/uc: Make uc_sanitize part of gt_sanitize
  drm/i915/huc: Check HuC firmware status only once
  drm/i915/huc: Update HuC status codes

 drivers/gpu/drm/i915/gt/intel_gt_pm.c |  2 ++
 drivers/gpu/drm/i915/i915_gem_pm.c    |  1 -
 drivers/gpu/drm/i915/intel_huc.c      | 26 +++++++++++++-------------
 drivers/gpu/drm/i915/intel_huc.h      |  2 ++
 4 files changed, 17 insertions(+), 14 deletions(-)

-- 
2.19.2

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

[PATCH 1/3] drm/i915/uc: Make uc_sanitize part of gt_sanitize

[Michal Wajdeczko]

In gt_sanitize we reset whole GPU (and indirectly uC).
Make sure we don't miss to run our dedicated uC sanitize code.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/gt/intel_gt_pm.c | 2 ++
 drivers/gpu/drm/i915/i915_gem_pm.c    | 1 -
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
index ae7155f0e063..af6bcc7eabf3 100644
--- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c
+++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
@@ -114,6 +114,8 @@ void intel_gt_sanitize(struct drm_i915_private *i915, bool force)
 	if (!reset_engines(i915) && !force)
 		return;
 
+	intel_uc_sanitize(i915);
+
 	for_each_engine(engine, i915, id)
 		intel_engine_reset(engine, false);
 }
diff --git a/drivers/gpu/drm/i915/i915_gem_pm.c b/drivers/gpu/drm/i915/i915_gem_pm.c
index fa9c2ebd966a..af3a3def45d8 100644
--- a/drivers/gpu/drm/i915/i915_gem_pm.c
+++ b/drivers/gpu/drm/i915/i915_gem_pm.c
@@ -193,7 +193,6 @@ void i915_gem_suspend_late(struct drm_i915_private *i915)
 	}
 	mutex_unlock(&i915->drm.struct_mutex);
 
-	intel_uc_sanitize(i915);
 	i915_gem_sanitize(i915);
 }
 
-- 
2.19.2

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

[PATCH 2/3] drm/i915/huc: Check HuC firmware status only once

[Michal Wajdeczko]

During driver load we checked that HuC firmware was verified, and once
verified it stays verified, so there is no need to check that again.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tony Ye <tony.ye@intel.com>
---
 drivers/gpu/drm/i915/intel_huc.c | 17 ++++++-----------
 drivers/gpu/drm/i915/intel_huc.h |  2 ++
 2 files changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
index 1ff1fb015e58..aac17916e130 100644
--- a/drivers/gpu/drm/i915/intel_huc.c
+++ b/drivers/gpu/drm/i915/intel_huc.c
@@ -113,6 +113,8 @@ int intel_huc_auth(struct intel_huc *huc)
 	u32 status;
 	int ret;
 
+	GEM_BUG_ON(huc->verified);
+
 	if (huc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
 		return -ENOEXEC;
 
@@ -134,6 +136,7 @@ int intel_huc_auth(struct intel_huc *huc)
 		goto fail;
 	}
 
+	huc->verified = true;
 	return 0;
 
 fail:
@@ -147,24 +150,16 @@ int intel_huc_auth(struct intel_huc *huc)
  * intel_huc_check_status() - check HuC status
  * @huc: intel_huc structure
  *
- * This function reads status register to verify if HuC
- * firmware was successfully loaded.
- *
  * Returns: 1 if HuC firmware is loaded and verified,
  * 0 if HuC firmware is not loaded and -ENODEV if HuC
  * is not present on this platform.
  */
 int intel_huc_check_status(struct intel_huc *huc)
 {
-	struct drm_i915_private *dev_priv = huc_to_i915(huc);
-	intel_wakeref_t wakeref;
-	bool status = false;
+	struct drm_i915_private *i915 = huc_to_i915(huc);
 
-	if (!HAS_HUC(dev_priv))
+	if (!HAS_HUC(i915))
 		return -ENODEV;
 
-	with_intel_runtime_pm(dev_priv, wakeref)
-		status = I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED;
-
-	return status;
+	return huc->verified;
 }
diff --git a/drivers/gpu/drm/i915/intel_huc.h b/drivers/gpu/drm/i915/intel_huc.h
index a0c21ae02a99..8c2b6c8f179c 100644
--- a/drivers/gpu/drm/i915/intel_huc.h
+++ b/drivers/gpu/drm/i915/intel_huc.h
@@ -31,6 +31,7 @@
 struct intel_huc {
 	/* Generic uC firmware management */
 	struct intel_uc_fw fw;
+	bool verified;
 
 	/* HuC-specific additions */
 	struct i915_vma *rsa_data;
@@ -52,6 +53,7 @@ static inline void intel_huc_fini_misc(struct intel_huc *huc)
 static inline int intel_huc_sanitize(struct intel_huc *huc)
 {
 	intel_uc_fw_sanitize(&huc->fw);
+	huc->verified = false;
 	return 0;
 }
 
-- 
2.19.2

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

[PATCH 3/3] drm/i915/huc: Update HuC status codes

[Michal Wajdeczko]

Without breaking existing usage, slightly update HuC status codes
to provide more info to the clients:
 1 if HuC firmware is loaded and verified,
 0 if HuC firmware is not enabled,
 -ENOPKG if HuC firmware is not loaded,
 -ENODEV if HuC is not present on this platform.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Tony Ye <tony.ye@intel.com>
---
 drivers/gpu/drm/i915/intel_huc.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
index aac17916e130..98deb4ee60a7 100644
--- a/drivers/gpu/drm/i915/intel_huc.c
+++ b/drivers/gpu/drm/i915/intel_huc.c
@@ -150,9 +150,11 @@ int intel_huc_auth(struct intel_huc *huc)
  * intel_huc_check_status() - check HuC status
  * @huc: intel_huc structure
  *
- * Returns: 1 if HuC firmware is loaded and verified,
- * 0 if HuC firmware is not loaded and -ENODEV if HuC
- * is not present on this platform.
+ * Return:
+ * * 1 if HuC firmware is loaded and verified,
+ * * 0 if HuC firmware is not enabled,
+ * * -ENOPKG if HuC firmware is not loaded,
+ * * -ENODEV if HuC is not present on this platform.
  */
 int intel_huc_check_status(struct intel_huc *huc)
 {
@@ -161,5 +163,8 @@ int intel_huc_check_status(struct intel_huc *huc)
 	if (!HAS_HUC(i915))
 		return -ENODEV;
 
-	return huc->verified;
+	if (!USES_HUC(i915))
+		return 0;
+
+	return huc->verified ? 1 : -ENOPKG;
 }
-- 
2.19.2

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

✓ Fi.CI.BAT: success for drm/i915: HuC updates

[Patchwork]

== Series Details ==

Series: drm/i915: HuC updates
URL   : https://patchwork.freedesktop.org/series/61001/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6122 -> Patchwork_13074
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13074/

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_13074:

### IGT changes ###

#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * igt@gem_basic@bad-close:
    - {fi-cml-u}:         NOTRUN -> [INCOMPLETE][1]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13074/fi-cml-u/igt@gem_basic@bad-close.html

  
Known issues
------------

  Here are the changes found in Patchwork_13074 that come from known issues:

### IGT changes ###

#### Possible fixes ####

  * igt@i915_selftest@live_hangcheck:
    - fi-skl-iommu:       [INCOMPLETE][2] ([fdo#108602] / [fdo#108744]) -> [PASS][3]
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6122/fi-skl-iommu/igt@i915_selftest@live_hangcheck.html
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13074/fi-skl-iommu/igt@i915_selftest@live_hangcheck.html
    - {fi-icl-u3}:        [INCOMPLETE][4] ([fdo#107713] / [fdo#108569]) -> [PASS][5]
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6122/fi-icl-u3/igt@i915_selftest@live_hangcheck.html
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13074/fi-icl-u3/igt@i915_selftest@live_hangcheck.html
    - {fi-icl-dsi}:       [INCOMPLETE][6] ([fdo#107713] / [fdo#108569]) -> [PASS][7]
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6122/fi-icl-dsi/igt@i915_selftest@live_hangcheck.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13074/fi-icl-dsi/igt@i915_selftest@live_hangcheck.html

  
#### Warnings ####

  * igt@i915_selftest@live_hangcheck:
    - fi-apl-guc:         [INCOMPLETE][8] ([fdo#103927] / [fdo#110624]) -> [FAIL][9] ([fdo#110623])
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6122/fi-apl-guc/igt@i915_selftest@live_hangcheck.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13074/fi-apl-guc/igt@i915_selftest@live_hangcheck.html

  * igt@runner@aborted:
    - fi-apl-guc:         [FAIL][10] ([fdo#110624]) -> [FAIL][11] ([fdo#110622])
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6122/fi-apl-guc/igt@runner@aborted.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13074/fi-apl-guc/igt@runner@aborted.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#107724]: https://bugs.freedesktop.org/show_bug.cgi?id=107724
  [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
  [fdo#108602]: https://bugs.freedesktop.org/show_bug.cgi?id=108602
  [fdo#108744]: https://bugs.freedesktop.org/show_bug.cgi?id=108744
  [fdo#110622]: https://bugs.freedesktop.org/show_bug.cgi?id=110622
  [fdo#110623]: https://bugs.freedesktop.org/show_bug.cgi?id=110623
  [fdo#110624]: https://bugs.freedesktop.org/show_bug.cgi?id=110624


Participating hosts (52 -> 46)
------------------------------

  Additional (1): fi-cml-u 
  Missing    (7): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-byt-clapper fi-bdw-samus 


Build changes
-------------

  * IGT: IGT_5005 -> IGTPW_3002
  * Linux: CI_DRM_6122 -> Patchwork_13074

  CI_DRM_6122: 47800182935228fd8a1ee3f45fcc4cd45c0c5c54 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_3002: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3002/
  IGT_5005: adf9f435a795d692e30cd6eafe26eddf4993c8ff @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_13074: cb167841efec077c157b3e91aafbf4641ae21be1 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

cb167841efec drm/i915/huc: Update HuC status codes
f79abe3b0f74 drm/i915/huc: Check HuC firmware status only once
f649f8108abf drm/i915/uc: Make uc_sanitize part of gt_sanitize

== Logs ==

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

Re: [PATCH 1/3] drm/i915/uc: Make uc_sanitize part of gt_sanitize

[Chris Wilson]

Quoting Michal Wajdeczko (2019-05-22 20:00:55)
> In gt_sanitize we reset whole GPU (and indirectly uC).
> Make sure we don't miss to run our dedicated uC sanitize code.
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/gt/intel_gt_pm.c | 2 ++
>  drivers/gpu/drm/i915/i915_gem_pm.c    | 1 -
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm.c b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
> index ae7155f0e063..af6bcc7eabf3 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_pm.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm.c
> @@ -114,6 +114,8 @@ void intel_gt_sanitize(struct drm_i915_private *i915, bool force)
>         if (!reset_engines(i915) && !force)
>                 return;
>  
> +       intel_uc_sanitize(i915);

Better place. Still searching for better names for the conceptual layers
of this onion.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915/huc: Check HuC firmware status only once

[Chris Wilson]

Quoting Michal Wajdeczko (2019-05-22 20:00:56)
> During driver load we checked that HuC firmware was verified, and once
> verified it stays verified, so there is no need to check that again.
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tony Ye <tony.ye@intel.com>

Makes sense to me as purely a code monkey.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

I would like a second opinion from someone who knows the innards of the
HuC to confirm that indeed once verified, it remains verified. And if it
can change, we need to report the change in status to userspace (or they
just hang and the gpu + huc gets reset).
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915/huc: Update HuC status codes

[Chris Wilson]

Quoting Michal Wajdeczko (2019-05-22 20:00:57)
> Without breaking existing usage, slightly update HuC status codes
> to provide more info to the clients:
>  1 if HuC firmware is loaded and verified,
>  0 if HuC firmware is not enabled,
>  -ENOPKG if HuC firmware is not loaded,
>  -ENODEV if HuC is not present on this platform.
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tony Ye <tony.ye@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_huc.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
> index aac17916e130..98deb4ee60a7 100644
> --- a/drivers/gpu/drm/i915/intel_huc.c
> +++ b/drivers/gpu/drm/i915/intel_huc.c
> @@ -150,9 +150,11 @@ int intel_huc_auth(struct intel_huc *huc)
>   * intel_huc_check_status() - check HuC status
>   * @huc: intel_huc structure
>   *
> - * Returns: 1 if HuC firmware is loaded and verified,
> - * 0 if HuC firmware is not loaded and -ENODEV if HuC
> - * is not present on this platform.
> + * Return:
> + * * 1 if HuC firmware is loaded and verified,
> + * * 0 if HuC firmware is not enabled,
> + * * -ENOPKG if HuC firmware is not loaded,
> + * * -ENODEV if HuC is not present on this platform.
>   */
>  int intel_huc_check_status(struct intel_huc *huc)
>  {
> @@ -161,5 +163,8 @@ int intel_huc_check_status(struct intel_huc *huc)
>         if (!HAS_HUC(i915))
>                 return -ENODEV;
>  
> -       return huc->verified;
> +       if (!USES_HUC(i915))
> +               return 0;
> +
> +       return huc->verified ? 1 : -ENOPKG;

I still think EOPNOTSUPP is a better error though for the user
preventing the huc being loaded -- as opposed to the result of
verification being the non-error value.

error == unable to setup huc
0/1 == result from talking to huc

Better ask someone else for a third opinion.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915/huc: Update HuC status codes

[Michal Wajdeczko]

On Wed, 22 May 2019 22:19:47 +0200, Chris Wilson  
<chris@chris-wilson.co.uk> wrote:

> Quoting Michal Wajdeczko (2019-05-22 20:00:57)
>> Without breaking existing usage, slightly update HuC status codes
>> to provide more info to the clients:
>>  1 if HuC firmware is loaded and verified,
>>  0 if HuC firmware is not enabled,
>>  -ENOPKG if HuC firmware is not loaded,
>>  -ENODEV if HuC is not present on this platform.
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Tony Ye <tony.ye@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_huc.c | 13 +++++++++----
>>  1 file changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_huc.c  
>> b/drivers/gpu/drm/i915/intel_huc.c
>> index aac17916e130..98deb4ee60a7 100644
>> --- a/drivers/gpu/drm/i915/intel_huc.c
>> +++ b/drivers/gpu/drm/i915/intel_huc.c
>> @@ -150,9 +150,11 @@ int intel_huc_auth(struct intel_huc *huc)
>>   * intel_huc_check_status() - check HuC status
>>   * @huc: intel_huc structure
>>   *
>> - * Returns: 1 if HuC firmware is loaded and verified,
>> - * 0 if HuC firmware is not loaded and -ENODEV if HuC
>> - * is not present on this platform.
>> + * Return:
>> + * * 1 if HuC firmware is loaded and verified,
>> + * * 0 if HuC firmware is not enabled,
>> + * * -ENOPKG if HuC firmware is not loaded,
>> + * * -ENODEV if HuC is not present on this platform.
>>   */
>>  int intel_huc_check_status(struct intel_huc *huc)
>>  {
>> @@ -161,5 +163,8 @@ int intel_huc_check_status(struct intel_huc *huc)
>>         if (!HAS_HUC(i915))
>>                 return -ENODEV;
>>
>> -       return huc->verified;
>> +       if (!USES_HUC(i915))
>> +               return 0;
>> +
>> +       return huc->verified ? 1 : -ENOPKG;
>
> I still think EOPNOTSUPP is a better error though for the user
> preventing the huc being loaded -- as opposed to the result of
> verification being the non-error value.
>
> error == unable to setup huc
> 0/1 == result from talking to huc

but your 0 here overlaps with unable to setup huc error,
so from the ABI perspective, imho, is bad.

also, from media team pov, as they want to have HuC always on,
the only non-error case is when user explicitly decided otherwise.

>
> Better ask someone else for a third opinion.

Check end of the message [1] but I'm fine waiting for more opinions.

[1] https://lists.freedesktop.org/archives/intel-gfx/2019-May/199066.html

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

Re: [PATCH 2/3] drm/i915/huc: Check HuC firmware status only once

[Ye, Tony]

> 在 2019年5月23日，上午4:14，Chris Wilson <chris@chris-wilson.co.uk> 写道：
> 
> Quoting Michal Wajdeczko (2019-05-22 20:00:56)
>> During driver load we checked that HuC firmware was verified, and once
>> verified it stays verified, so there is no need to check that again.
>> 
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Tony Ye <tony.ye@intel.com>
> 
> Makes sense to me as purely a code monkey.
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> I would like a second opinion from someone who knows the innards of the
> HuC to confirm that indeed once verified, it remains verified. And if it
> can change, we need to report the change in status to userspace (or they
> just hang and the gpu + huc gets reset).
> -Chris
UMD doesn’t authenticate HuC. It only reads the authentication status. So as long as KMD/GuC verified it, it keeps verified.
Regards, -Tony
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.IGT: failure for drm/i915: HuC updates

[Patchwork]

== Series Details ==

Series: drm/i915: HuC updates
URL   : https://patchwork.freedesktop.org/series/61001/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_6122_full -> Patchwork_13074_full
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_13074_full absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_13074_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_13074_full:

### IGT changes ###

#### Possible regressions ####

  * igt@gem_exec_schedule@pi-ringfull-render:
    - shard-apl:          [PASS][1] -> [FAIL][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6122/shard-apl7/igt@gem_exec_schedule@pi-ringfull-render.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13074/shard-apl7/igt@gem_exec_schedule@pi-ringfull-render.html

  
New tests
---------

  New tests have been introduced between CI_DRM_6122_full and Patchwork_13074_full:

### New IGT tests (1) ###

  * igt@i915_huc@basic:
    - Statuses : 6 skip(s)
    - Exec time: [0.0] s

  

Known issues
------------

  Here are the changes found in Patchwork_13074_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_eio@in-flight-suspend:
    - shard-glk:          [PASS][3] -> [FAIL][4] ([fdo#110667])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6122/shard-glk7/igt@gem_eio@in-flight-suspend.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13074/shard-glk2/igt@gem_eio@in-flight-suspend.html

  * igt@gem_exec_schedule@smoketest-all:
    - shard-glk:          [PASS][5] -> [INCOMPLETE][6] ([fdo#103359] / [k.org#198133])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6122/shard-glk4/igt@gem_exec_schedule@smoketest-all.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13074/shard-glk6/igt@gem_exec_schedule@smoketest-all.html

  * igt@gem_tiled_swapping@non-threaded:
    - shard-glk:          [PASS][7] -> [DMESG-WARN][8] ([fdo#108686])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6122/shard-glk2/igt@gem_tiled_swapping@non-threaded.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13074/shard-glk6/igt@gem_tiled_swapping@non-threaded.html

  * igt@i915_suspend@fence-restore-untiled:
    - shard-apl:          [PASS][9] -> [DMESG-WARN][10] ([fdo#108566]) +2 similar issues
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6122/shard-apl1/igt@i915_suspend@fence-restore-untiled.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13074/shard-apl5/igt@i915_suspend@fence-restore-untiled.html

  * igt@kms_cursor_legacy@cursor-vs-flip-toggle:
    - shard-hsw:          [PASS][11] -> [FAIL][12] ([fdo#103355])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6122/shard-hsw4/igt@kms_cursor_legacy@cursor-vs-flip-toggle.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13074/shard-hsw6/igt@kms_cursor_legacy@cursor-vs-flip-toggle.html

  * igt@kms_flip@flip-vs-suspend-interruptible:
    - shard-snb:          [PASS][13] -> [INCOMPLETE][14] ([fdo#105411])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6122/shard-snb1/igt@kms_flip@flip-vs-suspend-interruptible.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13074/shard-snb1/igt@kms_flip@flip-vs-suspend-interruptible.html

  * igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-spr-indfb-draw-pwrite:
    - shard-glk:          [PASS][15] -> [FAIL][16] ([fdo#103167])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6122/shard-glk2/igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-spr-indfb-draw-pwrite.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13074/shard-glk3/igt@kms_frontbuffer_tracking@fbc-2p-scndscrn-spr-indfb-draw-pwrite.html

  * igt@kms_frontbuffer_tracking@psr-1p-primscrn-indfb-plflip-blt:
    - shard-skl:          [PASS][17] -> [FAIL][18] ([fdo#103167])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6122/shard-skl10/igt@kms_frontbuffer_tracking@psr-1p-primscrn-indfb-plflip-blt.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13074/shard-skl8/igt@kms_frontbuffer_tracking@psr-1p-primscrn-indfb-plflip-blt.html

  * igt@kms_plane_alpha_blend@pipe-b-coverage-7efc:
    - shard-skl:          [PASS][19] -> [FAIL][20] ([fdo#108145] / [fdo#110403])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6122/shard-skl6/igt@kms_plane_alpha_blend@pipe-b-coverage-7efc.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13074/shard-skl7/igt@kms_plane_alpha_blend@pipe-b-coverage-7efc.html

  * igt@kms_setmode@basic:
    - shard-kbl:          [PASS][21] -> [FAIL][22] ([fdo#99912])
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6122/shard-kbl6/igt@kms_setmode@basic.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13074/shard-kbl7/igt@kms_setmode@basic.html

  * igt@vgem_slow@nohang:
    - shard-kbl:          [PASS][23] -> [INCOMPLETE][24] ([fdo#103665]) +1 similar issue
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6122/shard-kbl6/igt@vgem_slow@nohang.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13074/shard-kbl1/igt@vgem_slow@nohang.html

  
#### Possible fixes ####

  * igt@gem_ctx_isolation@bcs0-none:
    - shard-glk:          [WARN][25] ([fdo#110738]) -> [PASS][26] +27 similar issues
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6122/shard-glk5/igt@gem_ctx_isolation@bcs0-none.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13074/shard-glk2/igt@gem_ctx_isolation@bcs0-none.html

  * igt@gem_ctx_isolation@bcs0-s3:
    - shard-kbl:          [WARN][27] ([fdo#110738]) -> [PASS][28] +31 similar issues
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6122/shard-kbl7/igt@gem_ctx_isolation@bcs0-s3.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13074/shard-kbl6/igt@gem_ctx_isolation@bcs0-s3.html

  * igt@gem_ctx_isolation@vcs0-dirty-create:
    - shard-apl:          [WARN][29] ([fdo#110738]) -> [PASS][30] +22 similar issues
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6122/shard-apl5/igt@gem_ctx_isolation@vcs0-dirty-create.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13074/shard-apl7/igt@gem_ctx_isolation@vcs0-dirty-create.html

  * igt@gem_ctx_isolation@vcs0-dirty-switch:
    - shard-skl:          [WARN][31] ([fdo#110738]) -> [PASS][32] +22 similar issues
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6122/shard-skl1/igt@gem_ctx_isolation@vcs0-dirty-switch.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13074/shard-skl5/igt@gem_ctx_isolation@vcs0-dirty-switch.html

  * igt@gem_pipe_control_store_loop@reused-buffer:
    - shard-hsw:          [INCOMPLETE][33] ([fdo#103540]) -> [PASS][34]
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6122/shard-hsw4/igt@gem_pipe_control_store_loop@reused-buffer.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13074/shard-hsw1/igt@gem_pipe_control_store_loop@reused-buffer.html

  * igt@gem_workarounds@suspend-resume:
    - shard-apl:          [DMESG-WARN][35] ([fdo#108566]) -> [PASS][36] +6 similar issues
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6122/shard-apl2/igt@gem_workarounds@suspend-resume.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13074/shard-apl8/igt@gem_workarounds@suspend-resume.html

  * igt@i915_pm_rpm@sysfs-read:
    - shard-skl:          [INCOMPLETE][37] ([fdo#107807]) -> [PASS][38] +2 similar issues
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6122/shard-skl5/igt@i915_pm_rpm@sysfs-read.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13074/shard-skl2/igt@i915_pm_rpm@sysfs-read.html

  * igt@kms_busy@extended-modeset-hang-oldfb-with-reset-render-a:
    - shard-glk:          [INCOMPLETE][39] ([fdo#103359] / [k.org#198133]) -> [PASS][40]
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6122/shard-glk2/igt@kms_busy@extended-modeset-hang-oldfb-with-reset-render-a.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13074/shard-glk6/igt@kms_busy@extended-modeset-hang-oldfb-with-reset-render-a.html

  * igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size:
    - shard-snb:          [INCOMPLETE][41] ([fdo#105411]) -> [PASS][42]
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6122/shard-snb1/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13074/shard-snb2/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size.html

  * igt@kms_draw_crc@draw-method-xrgb8888-pwrite-xtiled:
    - shard-skl:          [FAIL][43] ([fdo#103184] / [fdo#103232]) -> [PASS][44]
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6122/shard-skl8/igt@kms_draw_crc@draw-method-xrgb8888-pwrite-xtiled.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13074/shard-skl8/igt@kms_draw_crc@draw-method-xrgb8888-pwrite-xtiled.html

  * igt@kms_flip@modeset-vs-vblank-race-interruptible:
    - shard-kbl:          [FAIL][45] ([fdo#103060]) -> [PASS][46]
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6122/shard-kbl3/igt@kms_flip@modeset-vs-vblank-race-interruptible.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13074/shard-kbl2/igt@kms_flip@modeset-vs-vblank-race-interruptible.html

  * igt@kms_frontbuffer_tracking@psr-1p-primscrn-spr-indfb-draw-render:
    - shard-skl:          [FAIL][47] ([fdo#103167]) -> [PASS][48] +2 similar issues
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6122/shard-skl8/igt@kms_frontbuffer_tracking@psr-1p-primscrn-spr-indfb-draw-render.html
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13074/shard-skl6/igt@kms_frontbuffer_tracking@psr-1p-primscrn-spr-indfb-draw-render.html

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
    - shard-skl:          [INCOMPLETE][49] ([fdo#104108]) -> [PASS][50] +1 similar issue
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6122/shard-skl3/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b.html
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13074/shard-skl1/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b.html

  * igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes:
    - shard-snb:          [SKIP][51] ([fdo#109271]) -> [PASS][52] +1 similar issue
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6122/shard-snb2/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes.html
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13074/shard-snb1/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes.html

  * igt@kms_plane_alpha_blend@pipe-c-coverage-7efc:
    - shard-skl:          [FAIL][53] ([fdo#108145] / [fdo#110403]) -> [PASS][54]
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6122/shard-skl3/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13074/shard-skl1/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html

  
#### Warnings ####

  * igt@gem_eio@in-flight-suspend:
    - shard-kbl:          [FAIL][55] ([fdo#110667]) -> [INCOMPLETE][56] ([fdo#103665])
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6122/shard-kbl3/igt@gem_eio@in-flight-suspend.html
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13074/shard-kbl1/igt@gem_eio@in-flight-suspend.html

  * igt@gem_tiled_swapping@non-threaded:
    - shard-hsw:          [FAIL][57] ([fdo#108686]) -> [INCOMPLETE][58] ([fdo#103540])
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6122/shard-hsw2/igt@gem_tiled_swapping@non-threaded.html
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13074/shard-hsw5/igt@gem_tiled_swapping@non-threaded.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [fdo#103060]: https://bugs.freedesktop.org/show_bug.cgi?id=103060
  [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
  [fdo#103184]: https://bugs.freedesktop.org/show_bug.cgi?id=103184
  [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232
  [fdo#103355]: https://bugs.freedesktop.org/show_bug.cgi?id=103355
  [fdo#103359]: https://bugs.freedesktop.org/show_bug.cgi?id=103359
  [fdo#103540]: https://bugs.freedesktop.org/show_bug.cgi?id=103540
  [fdo#103665]: https://bugs.freedesktop.org/show_bug.cgi?id=103665
  [fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
  [fdo#105411]: https://bugs.freedesktop.org/show_bug.cgi?id=105411
  [fdo#107807]: https://bugs.freedesktop.org/show_bug.cgi?id=107807
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#108566]: https://bugs.freedesktop.org/show_bug.cgi?id=108566
  [fdo#108686]: https://bugs.freedesktop.org/show_bug.cgi?id=108686
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#110403]: https://bugs.freedesktop.org/show_bug.cgi?id=110403
  [fdo#110667]: https://bugs.freedesktop.org/show_bug.cgi?id=110667
  [fdo#110738]: https://bugs.freedesktop.org/show_bug.cgi?id=110738
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912
  [k.org#198133]: https://bugzilla.kernel.org/show_bug.cgi?id=198133


Participating hosts (9 -> 9)
------------------------------

  No changes in participating hosts


Build changes
-------------

  * IGT: IGT_5005 -> IGTPW_3002
  * Linux: CI_DRM_6122 -> Patchwork_13074

  CI_DRM_6122: 47800182935228fd8a1ee3f45fcc4cd45c0c5c54 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_3002: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_3002/
  IGT_5005: adf9f435a795d692e30cd6eafe26eddf4993c8ff @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_13074: cb167841efec077c157b3e91aafbf4641ae21be1 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

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

Re: [PATCH 3/3] drm/i915/huc: Update HuC status codes

[Joonas Lahtinen]

Quoting Michal Wajdeczko (2019-05-23 01:25:00)
> On Wed, 22 May 2019 22:19:47 +0200, Chris Wilson  
> <chris@chris-wilson.co.uk> wrote:
> 
> > Quoting Michal Wajdeczko (2019-05-22 20:00:57)
> >> Without breaking existing usage, slightly update HuC status codes
> >> to provide more info to the clients:
> >>  1 if HuC firmware is loaded and verified,
> >>  0 if HuC firmware is not enabled,
> >>  -ENOPKG if HuC firmware is not loaded,
> >>  -ENODEV if HuC is not present on this platform.
> >>
> >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> >> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >> Cc: Tony Ye <tony.ye@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_huc.c | 13 +++++++++----
> >>  1 file changed, 9 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_huc.c  
> >> b/drivers/gpu/drm/i915/intel_huc.c
> >> index aac17916e130..98deb4ee60a7 100644
> >> --- a/drivers/gpu/drm/i915/intel_huc.c
> >> +++ b/drivers/gpu/drm/i915/intel_huc.c
> >> @@ -150,9 +150,11 @@ int intel_huc_auth(struct intel_huc *huc)
> >>   * intel_huc_check_status() - check HuC status
> >>   * @huc: intel_huc structure
> >>   *
> >> - * Returns: 1 if HuC firmware is loaded and verified,
> >> - * 0 if HuC firmware is not loaded and -ENODEV if HuC
> >> - * is not present on this platform.
> >> + * Return:
> >> + * * 1 if HuC firmware is loaded and verified,
> >> + * * 0 if HuC firmware is not enabled,
> >> + * * -ENOPKG if HuC firmware is not loaded,
> >> + * * -ENODEV if HuC is not present on this platform.
> >>   */
> >>  int intel_huc_check_status(struct intel_huc *huc)
> >>  {
> >> @@ -161,5 +163,8 @@ int intel_huc_check_status(struct intel_huc *huc)
> >>         if (!HAS_HUC(i915))
> >>                 return -ENODEV;
> >>
> >> -       return huc->verified;
> >> +       if (!USES_HUC(i915))
> >> +               return 0;
> >> +
> >> +       return huc->verified ? 1 : -ENOPKG;
> >
> > I still think EOPNOTSUPP is a better error though for the user
> > preventing the huc being loaded -- as opposed to the result of
> > verification being the non-error value.
> >
> > error == unable to setup huc
> > 0/1 == result from talking to huc
> 
> but your 0 here overlaps with unable to setup huc error,
> so from the ABI perspective, imho, is bad.
> 
> also, from media team pov, as they want to have HuC always on,
> the only non-error case is when user explicitly decided otherwise.

Trying to look things from external perspective, if HUC_(AUTHENTICATION_)STATUS
is queried 1 => "authenticated", 0 => "not authenticated" makes most sense.

Both media drivers also first check for errors, then convert the return
value to boolean, so it would be compatible with that.

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

Re: [PATCH 3/3] drm/i915/huc: Update HuC status codes

[Michal Wajdeczko]

On Fri, 31 May 2019 12:13:20 +0200, Joonas Lahtinen  
<joonas.lahtinen@linux.intel.com> wrote:

> Quoting Michal Wajdeczko (2019-05-23 01:25:00)
>> On Wed, 22 May 2019 22:19:47 +0200, Chris Wilson
>> <chris@chris-wilson.co.uk> wrote:
>>
>> > Quoting Michal Wajdeczko (2019-05-22 20:00:57)
>> >> Without breaking existing usage, slightly update HuC status codes
>> >> to provide more info to the clients:
>> >>  1 if HuC firmware is loaded and verified,
>> >>  0 if HuC firmware is not enabled,
>> >>  -ENOPKG if HuC firmware is not loaded,
>> >>  -ENODEV if HuC is not present on this platform.
>> >>
>> >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> >> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> >> Cc: Tony Ye <tony.ye@intel.com>
>> >> ---
>> >>  drivers/gpu/drm/i915/intel_huc.c | 13 +++++++++----
>> >>  1 file changed, 9 insertions(+), 4 deletions(-)
>> >>
>> >> diff --git a/drivers/gpu/drm/i915/intel_huc.c
>> >> b/drivers/gpu/drm/i915/intel_huc.c
>> >> index aac17916e130..98deb4ee60a7 100644
>> >> --- a/drivers/gpu/drm/i915/intel_huc.c
>> >> +++ b/drivers/gpu/drm/i915/intel_huc.c
>> >> @@ -150,9 +150,11 @@ int intel_huc_auth(struct intel_huc *huc)
>> >>   * intel_huc_check_status() - check HuC status
>> >>   * @huc: intel_huc structure
>> >>   *
>> >> - * Returns: 1 if HuC firmware is loaded and verified,
>> >> - * 0 if HuC firmware is not loaded and -ENODEV if HuC
>> >> - * is not present on this platform.
>> >> + * Return:
>> >> + * * 1 if HuC firmware is loaded and verified,
>> >> + * * 0 if HuC firmware is not enabled,
>> >> + * * -ENOPKG if HuC firmware is not loaded,
>> >> + * * -ENODEV if HuC is not present on this platform.
>> >>   */
>> >>  int intel_huc_check_status(struct intel_huc *huc)
>> >>  {
>> >> @@ -161,5 +163,8 @@ int intel_huc_check_status(struct intel_huc *huc)
>> >>         if (!HAS_HUC(i915))
>> >>                 return -ENODEV;
>> >>
>> >> -       return huc->verified;
>> >> +       if (!USES_HUC(i915))
>> >> +               return 0;
>> >> +
>> >> +       return huc->verified ? 1 : -ENOPKG;
>> >
>> > I still think EOPNOTSUPP is a better error though for the user
>> > preventing the huc being loaded -- as opposed to the result of
>> > verification being the non-error value.
>> >
>> > error == unable to setup huc
>> > 0/1 == result from talking to huc
>>
>> but your 0 here overlaps with unable to setup huc error,
>> so from the ABI perspective, imho, is bad.
>>
>> also, from media team pov, as they want to have HuC always on,
>> the only non-error case is when user explicitly decided otherwise.
>
> Trying to look things from external perspective, if  
> HUC_(AUTHENTICATION_)STATUS
> is queried 1 => "authenticated", 0 => "not authenticated" makes most  
> sense.

If you want to treat that as "authenticated" status then I fully agree.

But authentication status is just a one technical detail how we load
the firmware on hardware. Users mainly want to check if HuC was correctly
enabled on their system, so this is more like HUC_(ENABLED_)STATUS.

Then we have 1 => "enabled/active" and 0 => "disabled"

>
> Both media drivers also first check for errors, then convert the return
> value to boolean, so it would be compatible with that.

Both solutions are compatible with current media drivers implementations,
we don't want to break ABI, just to make it more useful.

Now these drivers just checks for success of the ioctl() call alone and
ignore any reported error since we don't provide any meaningful codes
why HuC is not working. We forced them to check dmesg for any hint.

Providing clear notation of 1(active) 0(disabled) and set of possible
errno codes why not explicitly disabled HuC is not working well, will,
imho, be much more useful than 1(authenticated) and 0(not authenticated,
dunno why)

Again, proposed here approach is aligned with Tony's opinion.

Regards,
Michal

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