[PATCH v2] drm/i915/huc: Don't try to check HuC status if it's not loaded

[PATCH v2] drm/i915/huc: Don't try to check HuC status if it's not loaded

[Michal Wajdeczko]

If we never attempted to load HuC firmware, or we already wedged
or reset GuC/HuC, then there is no reason to wake up the device
to check one bit in the register that will be for sure cleared.

v2: check if HuC was enabled; subtle change in ABI
    reuse hus_is_load helper

Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
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 | 20 ++++++++++++--------
 drivers/gpu/drm/i915/intel_huc.h |  5 +++++
 2 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
index 1ff1fb015e58..bfdebec1cfc8 100644
--- a/drivers/gpu/drm/i915/intel_huc.c
+++ b/drivers/gpu/drm/i915/intel_huc.c
@@ -113,7 +113,7 @@ int intel_huc_auth(struct intel_huc *huc)
 	u32 status;
 	int ret;
 
-	if (huc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
+	if (!intel_huc_is_loaded(huc))
 		return -ENOEXEC;
 
 	ret = intel_guc_auth_huc(guc,
@@ -150,21 +150,25 @@ int intel_huc_auth(struct intel_huc *huc)
  * 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.
+ * Returns: 1 if HuC firmware is loaded and verified, 0 if HuC firmware is not
+ * enabled, -ENOPKG 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;
+	bool verified = false;
 
 	if (!HAS_HUC(dev_priv))
 		return -ENODEV;
 
-	with_intel_runtime_pm(dev_priv, wakeref)
-		status = I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED;
+	if (!USES_HUC(dev_priv))
+		return 0;
 
-	return status;
+	if (intel_huc_is_loaded(huc))
+		with_intel_runtime_pm(dev_priv, wakeref)
+			verified = I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED;
+
+	return verified ? 1 : -ENOPKG;
 }
diff --git a/drivers/gpu/drm/i915/intel_huc.h b/drivers/gpu/drm/i915/intel_huc.h
index a0c21ae02a99..cde3d303718d 100644
--- a/drivers/gpu/drm/i915/intel_huc.h
+++ b/drivers/gpu/drm/i915/intel_huc.h
@@ -44,6 +44,11 @@ void intel_huc_fini(struct intel_huc *huc);
 int intel_huc_auth(struct intel_huc *huc);
 int intel_huc_check_status(struct intel_huc *huc);
 
+static inline bool intel_huc_is_loaded(struct intel_huc *huc)
+{
+	return intel_uc_fw_is_loaded(&huc->fw);
+}
+
 static inline void intel_huc_fini_misc(struct intel_huc *huc)
 {
 	intel_uc_fw_cleanup_fetch(&huc->fw);
-- 
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: Don't try to check HuC status if it's not loaded (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915/huc: Don't try to check HuC status if it's not loaded (rev2)
URL   : https://patchwork.freedesktop.org/series/60807/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6097 -> Patchwork_13039
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

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

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

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

### IGT changes ###

#### Issues hit ####

  * igt@i915_module_load@reload:
    - fi-blb-e6850:       [PASS][1] -> [INCOMPLETE][2] ([fdo#107718])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/fi-blb-e6850/igt@i915_module_load@reload.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13039/fi-blb-e6850/igt@i915_module_load@reload.html

  * igt@i915_pm_rpm@module-reload:
    - fi-skl-6770hq:      [PASS][3] -> [FAIL][4] ([fdo#108511])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/fi-skl-6770hq/igt@i915_pm_rpm@module-reload.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13039/fi-skl-6770hq/igt@i915_pm_rpm@module-reload.html

  * igt@i915_selftest@live_execlists:
    - fi-apl-guc:         [PASS][5] -> [INCOMPLETE][6] ([fdo#103927] / [fdo#109720])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/fi-apl-guc/igt@i915_selftest@live_execlists.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13039/fi-apl-guc/igt@i915_selftest@live_execlists.html

  
#### Possible fixes ####

  * igt@amdgpu/amd_basic@userptr:
    - fi-kbl-8809g:       [DMESG-WARN][7] ([fdo#108965]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/fi-kbl-8809g/igt@amdgpu/amd_basic@userptr.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13039/fi-kbl-8809g/igt@amdgpu/amd_basic@userptr.html

  * igt@i915_selftest@live_hangcheck:
    - fi-skl-iommu:       [INCOMPLETE][9] ([fdo#108602] / [fdo#108744]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/fi-skl-iommu/igt@i915_selftest@live_hangcheck.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13039/fi-skl-iommu/igt@i915_selftest@live_hangcheck.html
    - fi-apl-guc:         [FAIL][11] ([fdo#110623]) -> [PASS][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/fi-apl-guc/igt@i915_selftest@live_hangcheck.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13039/fi-apl-guc/igt@i915_selftest@live_hangcheck.html

  
#### Warnings ####

  * igt@runner@aborted:
    - fi-apl-guc:         [FAIL][13] ([fdo#110622]) -> [FAIL][14] ([fdo#108622] / [fdo#109720])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/fi-apl-guc/igt@runner@aborted.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13039/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#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
  [fdo#108511]: https://bugs.freedesktop.org/show_bug.cgi?id=108511
  [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
  [fdo#108602]: https://bugs.freedesktop.org/show_bug.cgi?id=108602
  [fdo#108622]: https://bugs.freedesktop.org/show_bug.cgi?id=108622
  [fdo#108744]: https://bugs.freedesktop.org/show_bug.cgi?id=108744
  [fdo#108965]: https://bugs.freedesktop.org/show_bug.cgi?id=108965
  [fdo#109720]: https://bugs.freedesktop.org/show_bug.cgi?id=109720
  [fdo#110622]: https://bugs.freedesktop.org/show_bug.cgi?id=110622
  [fdo#110623]: https://bugs.freedesktop.org/show_bug.cgi?id=110623


Participating hosts (52 -> 42)
------------------------------

  Additional (1): fi-pnv-d510 
  Missing    (11): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-bwr-2160 fi-ctg-p8600 fi-gdg-551 fi-icl-u3 fi-byt-clapper fi-bdw-samus fi-kbl-r 


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

  * Linux: CI_DRM_6097 -> Patchwork_13039

  CI_DRM_6097: 3f2d6a47d9eec66594887b1e9718bc1a29aa6a77 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4996: 6fe5d254ec1b9b47d61408e1b49a7339876bf1e7 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_13039: d259d605e926a0ef7554219c2d8d13ab735b8228 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

d259d605e926 drm/i915/huc: Don't try to check HuC status if it's not loaded

== Logs ==

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

✓ Fi.CI.IGT: success for drm/i915/huc: Don't try to check HuC status if it's not loaded (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915/huc: Don't try to check HuC status if it's not loaded (rev2)
URL   : https://patchwork.freedesktop.org/series/60807/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_6097_full -> Patchwork_13039_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_mmap_gtt@forked-big-copy:
    - shard-iclb:         [PASS][1] -> [TIMEOUT][2] ([fdo#109673])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/shard-iclb3/igt@gem_mmap_gtt@forked-big-copy.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13039/shard-iclb6/igt@gem_mmap_gtt@forked-big-copy.html

  * igt@i915_pm_rpm@basic-pci-d3-state:
    - shard-skl:          [PASS][3] -> [INCOMPLETE][4] ([fdo#107807])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/shard-skl5/igt@i915_pm_rpm@basic-pci-d3-state.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13039/shard-skl3/igt@i915_pm_rpm@basic-pci-d3-state.html

  * igt@kms_cursor_legacy@flip-vs-cursor-legacy:
    - shard-snb:          [PASS][5] -> [SKIP][6] ([fdo#109271])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/shard-snb6/igt@kms_cursor_legacy@flip-vs-cursor-legacy.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13039/shard-snb2/igt@kms_cursor_legacy@flip-vs-cursor-legacy.html

  * igt@kms_draw_crc@draw-method-xrgb2101010-render-untiled:
    - shard-skl:          [PASS][7] -> [FAIL][8] ([fdo#103184] / [fdo#103232])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/shard-skl2/igt@kms_draw_crc@draw-method-xrgb2101010-render-untiled.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13039/shard-skl10/igt@kms_draw_crc@draw-method-xrgb2101010-render-untiled.html

  * igt@kms_flip@flip-vs-suspend-interruptible:
    - shard-hsw:          [PASS][9] -> [INCOMPLETE][10] ([fdo#103540])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/shard-hsw1/igt@kms_flip@flip-vs-suspend-interruptible.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13039/shard-hsw5/igt@kms_flip@flip-vs-suspend-interruptible.html

  * igt@kms_frontbuffer_tracking@fbc-rgb565-draw-pwrite:
    - shard-iclb:         [PASS][11] -> [FAIL][12] ([fdo#103167]) +3 similar issues
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/shard-iclb3/igt@kms_frontbuffer_tracking@fbc-rgb565-draw-pwrite.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13039/shard-iclb3/igt@kms_frontbuffer_tracking@fbc-rgb565-draw-pwrite.html

  * igt@kms_frontbuffer_tracking@psr-1p-offscren-pri-shrfb-draw-mmap-cpu:
    - shard-skl:          [PASS][13] -> [FAIL][14] ([fdo#103167])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/shard-skl2/igt@kms_frontbuffer_tracking@psr-1p-offscren-pri-shrfb-draw-mmap-cpu.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13039/shard-skl10/igt@kms_frontbuffer_tracking@psr-1p-offscren-pri-shrfb-draw-mmap-cpu.html

  * igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes:
    - shard-apl:          [PASS][15] -> [DMESG-WARN][16] ([fdo#108566]) +2 similar issues
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/shard-apl3/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13039/shard-apl2/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes.html

  * igt@kms_plane_lowres@pipe-a-tiling-x:
    - shard-iclb:         [PASS][17] -> [FAIL][18] ([fdo#103166])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/shard-iclb6/igt@kms_plane_lowres@pipe-a-tiling-x.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13039/shard-iclb7/igt@kms_plane_lowres@pipe-a-tiling-x.html

  * igt@kms_psr@psr2_dpms:
    - shard-iclb:         [PASS][19] -> [SKIP][20] ([fdo#109441]) +1 similar issue
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/shard-iclb2/igt@kms_psr@psr2_dpms.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13039/shard-iclb7/igt@kms_psr@psr2_dpms.html

  
#### Possible fixes ####

  * igt@gem_tiled_swapping@non-threaded:
    - shard-glk:          [DMESG-WARN][21] ([fdo#108686]) -> [PASS][22]
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/shard-glk7/igt@gem_tiled_swapping@non-threaded.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13039/shard-glk5/igt@gem_tiled_swapping@non-threaded.html

  * igt@i915_pm_rpm@debugfs-read:
    - shard-skl:          [INCOMPLETE][23] ([fdo#107807]) -> [PASS][24]
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/shard-skl3/igt@i915_pm_rpm@debugfs-read.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13039/shard-skl4/igt@i915_pm_rpm@debugfs-read.html

  * igt@i915_pm_rpm@gem-execbuf:
    - shard-skl:          [INCOMPLETE][25] ([fdo#107803] / [fdo#107807]) -> [PASS][26]
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/shard-skl4/igt@i915_pm_rpm@gem-execbuf.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13039/shard-skl2/igt@i915_pm_rpm@gem-execbuf.html

  * igt@kms_cursor_legacy@2x-long-cursor-vs-flip-atomic:
    - shard-hsw:          [FAIL][27] ([fdo#105767]) -> [PASS][28]
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/shard-hsw1/igt@kms_cursor_legacy@2x-long-cursor-vs-flip-atomic.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13039/shard-hsw5/igt@kms_cursor_legacy@2x-long-cursor-vs-flip-atomic.html

  * igt@kms_dp_dsc@basic-dsc-enable-edp:
    - shard-iclb:         [SKIP][29] ([fdo#109349]) -> [PASS][30]
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/shard-iclb6/igt@kms_dp_dsc@basic-dsc-enable-edp.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13039/shard-iclb2/igt@kms_dp_dsc@basic-dsc-enable-edp.html

  * igt@kms_flip@2x-flip-vs-expired-vblank:
    - shard-glk:          [FAIL][31] ([fdo#102887]) -> [PASS][32]
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/shard-glk8/igt@kms_flip@2x-flip-vs-expired-vblank.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13039/shard-glk5/igt@kms_flip@2x-flip-vs-expired-vblank.html

  * igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-cur-indfb-draw-render:
    - shard-iclb:         [FAIL][33] ([fdo#103167]) -> [PASS][34] +1 similar issue
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/shard-iclb8/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-cur-indfb-draw-render.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13039/shard-iclb2/igt@kms_frontbuffer_tracking@fbcpsr-1p-primscrn-cur-indfb-draw-render.html

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b:
    - shard-apl:          [DMESG-WARN][35] ([fdo#108566]) -> [PASS][36] +2 similar issues
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/shard-apl2/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13039/shard-apl8/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b.html

  * igt@kms_plane_alpha_blend@pipe-c-coverage-7efc:
    - shard-skl:          [FAIL][37] ([fdo#108145] / [fdo#110403]) -> [PASS][38]
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/shard-skl5/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13039/shard-skl4/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html

  * igt@kms_plane_lowres@pipe-a-tiling-y:
    - shard-iclb:         [FAIL][39] ([fdo#103166]) -> [PASS][40]
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/shard-iclb5/igt@kms_plane_lowres@pipe-a-tiling-y.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13039/shard-iclb5/igt@kms_plane_lowres@pipe-a-tiling-y.html

  * igt@kms_psr@psr2_sprite_blt:
    - shard-iclb:         [SKIP][41] ([fdo#109441]) -> [PASS][42] +1 similar issue
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/shard-iclb8/igt@kms_psr@psr2_sprite_blt.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13039/shard-iclb2/igt@kms_psr@psr2_sprite_blt.html

  * igt@kms_setmode@basic:
    - shard-kbl:          [FAIL][43] ([fdo#99912]) -> [PASS][44]
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/shard-kbl7/igt@kms_setmode@basic.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13039/shard-kbl2/igt@kms_setmode@basic.html

  * igt@kms_sysfs_edid_timing:
    - shard-iclb:         [FAIL][45] ([fdo#100047]) -> [PASS][46]
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/shard-iclb2/igt@kms_sysfs_edid_timing.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13039/shard-iclb7/igt@kms_sysfs_edid_timing.html

  * igt@syncobj_wait@invalid-signal-illegal-handle:
    - shard-iclb:         [INCOMPLETE][47] ([fdo#107713]) -> [PASS][48]
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/shard-iclb7/igt@syncobj_wait@invalid-signal-illegal-handle.html
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13039/shard-iclb7/igt@syncobj_wait@invalid-signal-illegal-handle.html

  
#### Warnings ####

  * igt@gem_mmap_gtt@forked-big-copy-xy:
    - shard-iclb:         [INCOMPLETE][49] ([fdo#107713] / [fdo#109100]) -> [TIMEOUT][50] ([fdo#109673])
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/shard-iclb3/igt@gem_mmap_gtt@forked-big-copy-xy.html
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13039/shard-iclb6/igt@gem_mmap_gtt@forked-big-copy-xy.html

  * igt@i915_pm_rpm@pc8-residency:
    - shard-skl:          [INCOMPLETE][51] ([fdo#107807]) -> [SKIP][52] ([fdo#109271])
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_6097/shard-skl4/igt@i915_pm_rpm@pc8-residency.html
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_13039/shard-skl9/igt@i915_pm_rpm@pc8-residency.html

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

  [fdo#100047]: https://bugs.freedesktop.org/show_bug.cgi?id=100047
  [fdo#102887]: https://bugs.freedesktop.org/show_bug.cgi?id=102887
  [fdo#103166]: https://bugs.freedesktop.org/show_bug.cgi?id=103166
  [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#103540]: https://bugs.freedesktop.org/show_bug.cgi?id=103540
  [fdo#105767]: https://bugs.freedesktop.org/show_bug.cgi?id=105767
  [fdo#107713]: https://bugs.freedesktop.org/show_bug.cgi?id=107713
  [fdo#107803]: https://bugs.freedesktop.org/show_bug.cgi?id=107803
  [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#109100]: https://bugs.freedesktop.org/show_bug.cgi?id=109100
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109349]: https://bugs.freedesktop.org/show_bug.cgi?id=109349
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#109673]: https://bugs.freedesktop.org/show_bug.cgi?id=109673
  [fdo#110403]: https://bugs.freedesktop.org/show_bug.cgi?id=110403
  [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912


Participating hosts (10 -> 10)
------------------------------

  No changes in participating hosts


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

  * Linux: CI_DRM_6097 -> Patchwork_13039

  CI_DRM_6097: 3f2d6a47d9eec66594887b1e9718bc1a29aa6a77 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4996: 6fe5d254ec1b9b47d61408e1b49a7339876bf1e7 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_13039: d259d605e926a0ef7554219c2d8d13ab735b8228 @ 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_13039/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915/huc: Don't try to check HuC status if it's not loaded

[Chris Wilson]

Quoting Michal Wajdeczko (2019-05-19 22:50:43)
> If we never attempted to load HuC firmware, or we already wedged
> or reset GuC/HuC, then there is no reason to wake up the device
> to check one bit in the register that will be for sure cleared.
> 
> v2: check if HuC was enabled; subtle change in ABI
>     reuse hus_is_load helper
> 
> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> 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 | 20 ++++++++++++--------
>  drivers/gpu/drm/i915/intel_huc.h |  5 +++++
>  2 files changed, 17 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
> index 1ff1fb015e58..bfdebec1cfc8 100644
> --- a/drivers/gpu/drm/i915/intel_huc.c
> +++ b/drivers/gpu/drm/i915/intel_huc.c
> @@ -113,7 +113,7 @@ int intel_huc_auth(struct intel_huc *huc)
>         u32 status;
>         int ret;
>  
> -       if (huc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
> +       if (!intel_huc_is_loaded(huc))
>                 return -ENOEXEC;
>  
>         ret = intel_guc_auth_huc(guc,
> @@ -150,21 +150,25 @@ int intel_huc_auth(struct intel_huc *huc)
>   * 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.
> + * Returns: 1 if HuC firmware is loaded and verified, 0 if HuC firmware is not
> + * enabled, -ENOPKG 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;
> +       bool verified = false;
>  
>         if (!HAS_HUC(dev_priv))
>                 return -ENODEV;
>  
> -       with_intel_runtime_pm(dev_priv, wakeref)
> -               status = I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED;
> +       if (!USES_HUC(dev_priv))
> +               return 0;

Hmm, EOPNOTSUPP here for the user disabled case?

Then

if (!intel_huc_is_loaded(huc))
	return -ENOPKG;

bool verified = false;
with_intel_runtime_pm(dev_priv, wakeref)
	verified = I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED;
return verified.

That keeps it a bit flatter with the special casing separate and the 0/1
result as given by the register status. Then if negative, we know one of
the preconditions for using HuC is not available, and if 0 something went
wrong in mmio setup.

Does that make sense?

> -       return status;
> +       if (intel_huc_is_loaded(huc))
> +               with_intel_runtime_pm(dev_priv, wakeref)
> +                       verified = I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED;
> +
> +       return verified ? 1 : -ENOPKG;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_huc.h b/drivers/gpu/drm/i915/intel_huc.h
> index a0c21ae02a99..cde3d303718d 100644
> --- a/drivers/gpu/drm/i915/intel_huc.h
> +++ b/drivers/gpu/drm/i915/intel_huc.h
> @@ -44,6 +44,11 @@ void intel_huc_fini(struct intel_huc *huc);
>  int intel_huc_auth(struct intel_huc *huc);
>  int intel_huc_check_status(struct intel_huc *huc);
>  
> +static inline bool intel_huc_is_loaded(struct intel_huc *huc)
> +{
> +       return intel_uc_fw_is_loaded(&huc->fw);
> +}
> +
>  static inline void intel_huc_fini_misc(struct intel_huc *huc)
>  {
>         intel_uc_fw_cleanup_fetch(&huc->fw);
> -- 
> 2.19.2
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915/huc: Don't try to check HuC status if it's not loaded

[Michal Wajdeczko]

On Mon, 20 May 2019 11:35:26 +0200, Chris Wilson  
<chris@chris-wilson.co.uk> wrote:

> Quoting Michal Wajdeczko (2019-05-19 22:50:43)
>> If we never attempted to load HuC firmware, or we already wedged
>> or reset GuC/HuC, then there is no reason to wake up the device
>> to check one bit in the register that will be for sure cleared.
>>
>> v2: check if HuC was enabled; subtle change in ABI
>>     reuse hus_is_load helper
>>
>> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
>> 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 | 20 ++++++++++++--------
>>  drivers/gpu/drm/i915/intel_huc.h |  5 +++++
>>  2 files changed, 17 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_huc.c  
>> b/drivers/gpu/drm/i915/intel_huc.c
>> index 1ff1fb015e58..bfdebec1cfc8 100644
>> --- a/drivers/gpu/drm/i915/intel_huc.c
>> +++ b/drivers/gpu/drm/i915/intel_huc.c
>> @@ -113,7 +113,7 @@ int intel_huc_auth(struct intel_huc *huc)
>>         u32 status;
>>         int ret;
>>
>> -       if (huc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
>> +       if (!intel_huc_is_loaded(huc))
>>                 return -ENOEXEC;
>>
>>         ret = intel_guc_auth_huc(guc,
>> @@ -150,21 +150,25 @@ int intel_huc_auth(struct intel_huc *huc)
>>   * 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.
>> + * Returns: 1 if HuC firmware is loaded and verified, 0 if HuC  
>> firmware is not
>> + * enabled, -ENOPKG 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;
>> +       bool verified = false;
>>
>>         if (!HAS_HUC(dev_priv))
>>                 return -ENODEV;
>>
>> -       with_intel_runtime_pm(dev_priv, wakeref)
>> -               status = I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED;
>> +       if (!USES_HUC(dev_priv))
>> +               return 0;
>
> Hmm, EOPNOTSUPP here for the user disabled case?

I'm not sure that user disabled case shall be reported as error,
since it perfectly fits into legacy ABI 0="not loaded".
The only small change is that now 0="not loaded (not enabled by user)"

>
> Then
>
> if (!intel_huc_is_loaded(huc))
> 	return -ENOPKG;
>
> bool verified = false;
> with_intel_runtime_pm(dev_priv, wakeref)
> 	verified = I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED;
> return verified.
>
> That keeps it a bit flatter with the special casing separate and the 0/1
> result as given by the register status. Then if negative, we know one of
> the preconditions for using HuC is not available, and if 0 something went
> wrong in mmio setup.
>
> Does that make sense?

IMHO, if something went wrong it is better to report it as error rather
then weak 0 status. Compare:

+---------------+-------+-------+-------+
  no hardware     -ENODEV -ENODEV -ENODEV
  fw disabled        0       0    -ENOTSUP
  fw not loaded      0    -ENOPKG -ENOPKG
  fw not verified*   0    -ENOPKG    0
  fw verified*       1       1       1
+---------------+-------+-------+-------+
*) registry access

Note that in your case, fw verification problem can be reported both
as -ENOPKG or as 0 (former when verification fails at load time, latter
as result of runtime reg read). Very likely we will never see 0 at all,
since probably that can't change once fw was verified at load time.
That might be viewed as undesired ABI change.

>
>> -       return status;
>> +       if (intel_huc_is_loaded(huc))
>> +               with_intel_runtime_pm(dev_priv, wakeref)
>> +                       verified = I915_READ(HUC_STATUS2) &  
>> HUC_FW_VERIFIED;
>> +
>> +       return verified ? 1 : -ENOPKG;
>>  }
>> diff --git a/drivers/gpu/drm/i915/intel_huc.h  
>> b/drivers/gpu/drm/i915/intel_huc.h
>> index a0c21ae02a99..cde3d303718d 100644
>> --- a/drivers/gpu/drm/i915/intel_huc.h
>> +++ b/drivers/gpu/drm/i915/intel_huc.h
>> @@ -44,6 +44,11 @@ void intel_huc_fini(struct intel_huc *huc);
>>  int intel_huc_auth(struct intel_huc *huc);
>>  int intel_huc_check_status(struct intel_huc *huc);
>>
>> +static inline bool intel_huc_is_loaded(struct intel_huc *huc)
>> +{
>> +       return intel_uc_fw_is_loaded(&huc->fw);
>> +}
>> +
>>  static inline void intel_huc_fini_misc(struct intel_huc *huc)
>>  {
>>         intel_uc_fw_cleanup_fetch(&huc->fw);
>> --
>> 2.19.2
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915/huc: Don't try to check HuC status if it's not loaded

[Chris Wilson]

Quoting Michal Wajdeczko (2019-05-20 11:24:37)
> On Mon, 20 May 2019 11:35:26 +0200, Chris Wilson  
> <chris@chris-wilson.co.uk> wrote:
> 
> > Quoting Michal Wajdeczko (2019-05-19 22:50:43)
> >> If we never attempted to load HuC firmware, or we already wedged
> >> or reset GuC/HuC, then there is no reason to wake up the device
> >> to check one bit in the register that will be for sure cleared.
> >>
> >> v2: check if HuC was enabled; subtle change in ABI
> >>     reuse hus_is_load helper
> >>
> >> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> >> 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 | 20 ++++++++++++--------
> >>  drivers/gpu/drm/i915/intel_huc.h |  5 +++++
> >>  2 files changed, 17 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_huc.c  
> >> b/drivers/gpu/drm/i915/intel_huc.c
> >> index 1ff1fb015e58..bfdebec1cfc8 100644
> >> --- a/drivers/gpu/drm/i915/intel_huc.c
> >> +++ b/drivers/gpu/drm/i915/intel_huc.c
> >> @@ -113,7 +113,7 @@ int intel_huc_auth(struct intel_huc *huc)
> >>         u32 status;
> >>         int ret;
> >>
> >> -       if (huc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
> >> +       if (!intel_huc_is_loaded(huc))
> >>                 return -ENOEXEC;
> >>
> >>         ret = intel_guc_auth_huc(guc,
> >> @@ -150,21 +150,25 @@ int intel_huc_auth(struct intel_huc *huc)
> >>   * 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.
> >> + * Returns: 1 if HuC firmware is loaded and verified, 0 if HuC  
> >> firmware is not
> >> + * enabled, -ENOPKG 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;
> >> +       bool verified = false;
> >>
> >>         if (!HAS_HUC(dev_priv))
> >>                 return -ENODEV;
> >>
> >> -       with_intel_runtime_pm(dev_priv, wakeref)
> >> -               status = I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED;
> >> +       if (!USES_HUC(dev_priv))
> >> +               return 0;
> >
> > Hmm, EOPNOTSUPP here for the user disabled case?
> 
> I'm not sure that user disabled case shall be reported as error,
> since it perfectly fits into legacy ABI 0="not loaded".
> The only small change is that now 0="not loaded (not enabled by user)"

The only requirement for the intel-media driver seems to "HUC_STATUS > 0".
That's our only user, so I think we have the liberty to redefine <=0 as
befits error reporting.

> 
> >
> > Then
> >
> > if (!intel_huc_is_loaded(huc))
> >       return -ENOPKG;
> >
> > bool verified = false;
> > with_intel_runtime_pm(dev_priv, wakeref)
> >       verified = I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED;
> > return verified.
> >
> > That keeps it a bit flatter with the special casing separate and the 0/1
> > result as given by the register status. Then if negative, we know one of
> > the preconditions for using HuC is not available, and if 0 something went
> > wrong in mmio setup.
> >
> > Does that make sense?
> 
> IMHO, if something went wrong it is better to report it as error rather
> then weak 0 status. Compare:

I disagree, <0 is the weak case. 0 is the HW reports something went
wrong (and only that). 1 is the HW reports all went well.

If we have more ways we can report the HW went wrong, sure expand that
into extra error codes, but I don't see that. And if it's in the regs,
why are not exporting them via reg_read_ioctl?

> +---------------+-------+-------+-------+
>   no hardware     -ENODEV -ENODEV -ENODEV
>   fw disabled        0       0    -ENOTSUP
>   fw not loaded      0    -ENOPKG -ENOPKG
>   fw not verified*   0    -ENOPKG    0
>   fw verified*       1       1       1
> +---------------+-------+-------+-------+
> *) registry access
> 
> Note that in your case, fw verification problem can be reported both
> as -ENOPKG or as 0 (former when verification fails at load time, latter
> as result of runtime reg read). Very likely we will never see 0 at all,
> since probably that can't change once fw was verified at load time.

I see more distinction for ENODEV/ENOTSUP/ENOPKG/0/1. I am not getting
your point that we have conflation here, as to me it looks more
distinct, and it is clear when we report the value as given by the
register and when we give an interpreted value for a driver error.

> That might be viewed as undesired ABI change.

grep says it is fine, so I'm happy that no one else cares (as can be
seen by the lack of igt, no one has looked hard enough into how to
distinguish the API failures ;)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915/huc: Don't try to check HuC status if it's not loaded

[Michal Wajdeczko]

On Mon, 20 May 2019 12:44:43 +0200, Chris Wilson  
<chris@chris-wilson.co.uk> wrote:

> Quoting Michal Wajdeczko (2019-05-20 11:24:37)
>> On Mon, 20 May 2019 11:35:26 +0200, Chris Wilson
>> <chris@chris-wilson.co.uk> wrote:
>>
>> > Quoting Michal Wajdeczko (2019-05-19 22:50:43)
>> >> If we never attempted to load HuC firmware, or we already wedged
>> >> or reset GuC/HuC, then there is no reason to wake up the device
>> >> to check one bit in the register that will be for sure cleared.
>> >>
>> >> v2: check if HuC was enabled; subtle change in ABI
>> >>     reuse hus_is_load helper
>> >>
>> >> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
>> >> 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 | 20 ++++++++++++--------
>> >>  drivers/gpu/drm/i915/intel_huc.h |  5 +++++
>> >>  2 files changed, 17 insertions(+), 8 deletions(-)
>> >>
>> >> diff --git a/drivers/gpu/drm/i915/intel_huc.c
>> >> b/drivers/gpu/drm/i915/intel_huc.c
>> >> index 1ff1fb015e58..bfdebec1cfc8 100644
>> >> --- a/drivers/gpu/drm/i915/intel_huc.c
>> >> +++ b/drivers/gpu/drm/i915/intel_huc.c
>> >> @@ -113,7 +113,7 @@ int intel_huc_auth(struct intel_huc *huc)
>> >>         u32 status;
>> >>         int ret;
>> >>
>> >> -       if (huc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
>> >> +       if (!intel_huc_is_loaded(huc))
>> >>                 return -ENOEXEC;
>> >>
>> >>         ret = intel_guc_auth_huc(guc,
>> >> @@ -150,21 +150,25 @@ int intel_huc_auth(struct intel_huc *huc)
>> >>   * 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.
>> >> + * Returns: 1 if HuC firmware is loaded and verified, 0 if HuC
>> >> firmware is not
>> >> + * enabled, -ENOPKG 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;
>> >> +       bool verified = false;
>> >>
>> >>         if (!HAS_HUC(dev_priv))
>> >>                 return -ENODEV;
>> >>
>> >> -       with_intel_runtime_pm(dev_priv, wakeref)
>> >> -               status = I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED;
>> >> +       if (!USES_HUC(dev_priv))
>> >> +               return 0;
>> >
>> > Hmm, EOPNOTSUPP here for the user disabled case?
>>
>> I'm not sure that user disabled case shall be reported as error,
>> since it perfectly fits into legacy ABI 0="not loaded".
>> The only small change is that now 0="not loaded (not enabled by user)"
>
> The only requirement for the intel-media driver seems to "HUC_STATUS >  
> 0".
> That's our only user, so I think we have the liberty to redefine <=0 as
> befits error reporting.
>
>>
>> >
>> > Then
>> >
>> > if (!intel_huc_is_loaded(huc))
>> >       return -ENOPKG;
>> >
>> > bool verified = false;
>> > with_intel_runtime_pm(dev_priv, wakeref)
>> >       verified = I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED;
>> > return verified.
>> >
>> > That keeps it a bit flatter with the special casing separate and the  
>> 0/1
>> > result as given by the register status. Then if negative, we know one  
>> of
>> > the preconditions for using HuC is not available, and if 0 something  
>> went
>> > wrong in mmio setup.
>> >
>> > Does that make sense?
>>
>> IMHO, if something went wrong it is better to report it as error rather
>> then weak 0 status. Compare:
>
> I disagree, <0 is the weak case. 0 is the HW reports something went
> wrong (and only that). 1 is the HW reports all went well.

But I was assuming that we're defining ABI here, not simply exposing  
unified
register value. Since we want 1 to report all ok, and we can fail for many
reasons (no hw, no fw blob, wrong fw, bad signature, ...) we should at  
least
try to match all these failures into error code (today just ENOPKG, but  
later
we can extend into ENOEXEC=bad fw, ENOSPC=can't fit into WOPCM, ...).

On other hand, at least for me, user decision to disable HuC should not
be treated the same way as other real failures, so 0 seems like perfect  
match.

Then we'll have: 1 = enabled, 0 = disabled, <0 = problem

>
> If we have more ways we can report the HW went wrong, sure expand that
> into extra error codes, but I don't see that. And if it's in the regs,
> why are not exporting them via reg_read_ioctl?
>
>> +---------------+-------+-------+-------+
>>   no hardware     -ENODEV -ENODEV -ENODEV
>>   fw disabled        0       0    -ENOTSUP
>>   fw not loaded      0    -ENOPKG -ENOPKG
>>   fw not verified*   0    -ENOPKG    0
>>   fw verified*       1       1       1
>> +---------------+-------+-------+-------+
>> *) registry access
>>
>> Note that in your case, fw verification problem can be reported both
>> as -ENOPKG or as 0 (former when verification fails at load time, latter
>> as result of runtime reg read). Very likely we will never see 0 at all,
>> since probably that can't change once fw was verified at load time.
>
> I see more distinction for ENODEV/ENOTSUP/ENOPKG/0/1. I am not getting
> your point that we have conflation here, as to me it looks more

Note that we are verifying HuC firmware right after it was loaded into
hardware using exactly the same register as here (see intel_huc_auth),
and if this fails we immediately change fw status to FAIL, so we never
go beyond huc_is_loaded point.

Assuming that once fw is verified it can't fail (to be verified later)
then your codes will be only ENODEV/ENOTSUP/ENOPKG/1 (no 0 value)

> distinct, and it is clear when we report the value as given by the
> register and when we give an interpreted value for a driver error.

Note that your initial idea was to reuse driver maintained fw status
instead of waking up device just to check register again (we don't do
that now as we need someone to confirm/verify that we can really skip
reg read). Use 0/1 for ABI as register values does not fit here, imho.

There is also a risk that we will report some known failures as <0
error codes, while other, captured by hw as just 0.

>
>> That might be viewed as undesired ABI change.
>
> grep says it is fine, so I'm happy that no one else cares (as can be
> seen by the lack of igt, no one has looked hard enough into how to
> distinguish the API failures ;)

it's always hard to come out from gray area into solid spec ;)

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

Re: [PATCH v2] drm/i915/huc: Don't try to check HuC status if it's not loaded

[Ye, Tony]

There are two users of I915_PARAM_HUC_STATUS, the intel-media driver and 
the legacy intel-vaapi-driver.

Both drivers check the return value like this:

     has_huc = !! ret_value;

So the ABI change will break the existing stack because negative values 
are treated as 1, won't it?

On 5/20/2019 8:16 PM, Michal Wajdeczko wrote:

> On Mon, 20 May 2019 12:44:43 +0200, Chris Wilson 
> <chris@chris-wilson.co.uk> wrote:
>
>> Quoting Michal Wajdeczko (2019-05-20 11:24:37)
>>> On Mon, 20 May 2019 11:35:26 +0200, Chris Wilson
>>> <chris@chris-wilson.co.uk> wrote:
>>>
>>> > Quoting Michal Wajdeczko (2019-05-19 22:50:43)
>>> >> If we never attempted to load HuC firmware, or we already wedged
>>> >> or reset GuC/HuC, then there is no reason to wake up the device
>>> >> to check one bit in the register that will be for sure cleared.
>>> >>
>>> >> v2: check if HuC was enabled; subtle change in ABI
>>> >>     reuse hus_is_load helper
>>> >>
>>> >> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> >> 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 | 20 ++++++++++++--------
>>> >>  drivers/gpu/drm/i915/intel_huc.h |  5 +++++
>>> >>  2 files changed, 17 insertions(+), 8 deletions(-)
>>> >>
>>> >> diff --git a/drivers/gpu/drm/i915/intel_huc.c
>>> >> b/drivers/gpu/drm/i915/intel_huc.c
>>> >> index 1ff1fb015e58..bfdebec1cfc8 100644
>>> >> --- a/drivers/gpu/drm/i915/intel_huc.c
>>> >> +++ b/drivers/gpu/drm/i915/intel_huc.c
>>> >> @@ -113,7 +113,7 @@ int intel_huc_auth(struct intel_huc *huc)
>>> >>         u32 status;
>>> >>         int ret;
>>> >>
>>> >> -       if (huc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
>>> >> +       if (!intel_huc_is_loaded(huc))
>>> >>                 return -ENOEXEC;
>>> >>
>>> >>         ret = intel_guc_auth_huc(guc,
>>> >> @@ -150,21 +150,25 @@ int intel_huc_auth(struct intel_huc *huc)
>>> >>   * 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.
>>> >> + * Returns: 1 if HuC firmware is loaded and verified, 0 if HuC
>>> >> firmware is not
>>> >> + * enabled, -ENOPKG 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;
>>> >> +       bool verified = false;
>>> >>
>>> >>         if (!HAS_HUC(dev_priv))
>>> >>                 return -ENODEV;
>>> >>
>>> >> -       with_intel_runtime_pm(dev_priv, wakeref)
>>> >> -               status = I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED;
>>> >> +       if (!USES_HUC(dev_priv))
>>> >> +               return 0;
>>> >
>>> > Hmm, EOPNOTSUPP here for the user disabled case?
>>>
>>> I'm not sure that user disabled case shall be reported as error,
>>> since it perfectly fits into legacy ABI 0="not loaded".
>>> The only small change is that now 0="not loaded (not enabled by user)"
>>
>> The only requirement for the intel-media driver seems to "HUC_STATUS 
>> > 0".
>> That's our only user, so I think we have the liberty to redefine <=0 as
>> befits error reporting.
>>
>>>
>>> >
>>> > Then
>>> >
>>> > if (!intel_huc_is_loaded(huc))
>>> >       return -ENOPKG;
>>> >
>>> > bool verified = false;
>>> > with_intel_runtime_pm(dev_priv, wakeref)
>>> >       verified = I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED;
>>> > return verified.
>>> >
>>> > That keeps it a bit flatter with the special casing separate and 
>>> the 0/1
>>> > result as given by the register status. Then if negative, we know 
>>> one of
>>> > the preconditions for using HuC is not available, and if 0 
>>> something went
>>> > wrong in mmio setup.
>>> >
>>> > Does that make sense?
>>>
>>> IMHO, if something went wrong it is better to report it as error rather
>>> then weak 0 status. Compare:
>>
>> I disagree, <0 is the weak case. 0 is the HW reports something went
>> wrong (and only that). 1 is the HW reports all went well.
>
> But I was assuming that we're defining ABI here, not simply exposing 
> unified
> register value. Since we want 1 to report all ok, and we can fail for 
> many
> reasons (no hw, no fw blob, wrong fw, bad signature, ...) we should at 
> least
> try to match all these failures into error code (today just ENOPKG, 
> but later
> we can extend into ENOEXEC=bad fw, ENOSPC=can't fit into WOPCM, ...).
>
> On other hand, at least for me, user decision to disable HuC should not
> be treated the same way as other real failures, so 0 seems like 
> perfect match.
>
> Then we'll have: 1 = enabled, 0 = disabled, <0 = problem
>
>>
>> If we have more ways we can report the HW went wrong, sure expand that
>> into extra error codes, but I don't see that. And if it's in the regs,
>> why are not exporting them via reg_read_ioctl?
>>
>>> +---------------+-------+-------+-------+
>>>   no hardware     -ENODEV -ENODEV -ENODEV
>>>   fw disabled        0       0    -ENOTSUP
>>>   fw not loaded      0    -ENOPKG -ENOPKG
>>>   fw not verified*   0    -ENOPKG    0
>>>   fw verified*       1       1       1
>>> +---------------+-------+-------+-------+
>>> *) registry access
>>>
>>> Note that in your case, fw verification problem can be reported both
>>> as -ENOPKG or as 0 (former when verification fails at load time, latter
>>> as result of runtime reg read). Very likely we will never see 0 at all,
>>> since probably that can't change once fw was verified at load time.
>>
>> I see more distinction for ENODEV/ENOTSUP/ENOPKG/0/1. I am not getting
>> your point that we have conflation here, as to me it looks more
>
> Note that we are verifying HuC firmware right after it was loaded into
> hardware using exactly the same register as here (see intel_huc_auth),
> and if this fails we immediately change fw status to FAIL, so we never
> go beyond huc_is_loaded point.
>
> Assuming that once fw is verified it can't fail (to be verified later)
> then your codes will be only ENODEV/ENOTSUP/ENOPKG/1 (no 0 value)
>
>> distinct, and it is clear when we report the value as given by the
>> register and when we give an interpreted value for a driver error.
>
> Note that your initial idea was to reuse driver maintained fw status
> instead of waking up device just to check register again (we don't do
> that now as we need someone to confirm/verify that we can really skip
> reg read). Use 0/1 for ABI as register values does not fit here, imho.
>
> There is also a risk that we will report some known failures as <0
> error codes, while other, captured by hw as just 0.
>
>>
>>> That might be viewed as undesired ABI change.
>>
>> grep says it is fine, so I'm happy that no one else cares (as can be
>> seen by the lack of igt, no one has looked hard enough into how to
>> distinguish the API failures ;)
>
> it's always hard to come out from gray area into solid spec ;)
>
>> -Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915/huc: Don't try to check HuC status if it's not loaded

[Michal Wajdeczko]

On Tue, 21 May 2019 13:04:12 +0200, Ye, Tony <tony.ye@intel.com> wrote:

> There are two users of I915_PARAM_HUC_STATUS, the intel-media driver and  
> the legacy intel-vaapi-driver.
>
> Both drivers check the return value like this:
>
>      has_huc = !! ret_value;
>
> So the ABI change will break the existing stack because negative values  
> are treated as 1, won't it?

No, since negative errors returned by i915 intel_huc_check_status()  
function
will be only used to indicate ioctl() call failure and then stored in  
errno.
Note that your "ret_value" will be modified only when ioctl is successful.

Michal

>
> On 5/20/2019 8:16 PM, Michal Wajdeczko wrote:
>
>> On Mon, 20 May 2019 12:44:43 +0200, Chris Wilson  
>> <chris@chris-wilson.co.uk> wrote:
>>
>>> Quoting Michal Wajdeczko (2019-05-20 11:24:37)
>>>> On Mon, 20 May 2019 11:35:26 +0200, Chris Wilson
>>>> <chris@chris-wilson.co.uk> wrote:
>>>>
>>>> > Quoting Michal Wajdeczko (2019-05-19 22:50:43)
>>>> >> If we never attempted to load HuC firmware, or we already wedged
>>>> >> or reset GuC/HuC, then there is no reason to wake up the device
>>>> >> to check one bit in the register that will be for sure cleared.
>>>> >>
>>>> >> v2: check if HuC was enabled; subtle change in ABI
>>>> >>     reuse hus_is_load helper
>>>> >>
>>>> >> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
>>>> >> 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 | 20 ++++++++++++--------
>>>> >>  drivers/gpu/drm/i915/intel_huc.h |  5 +++++
>>>> >>  2 files changed, 17 insertions(+), 8 deletions(-)
>>>> >>
>>>> >> diff --git a/drivers/gpu/drm/i915/intel_huc.c
>>>> >> b/drivers/gpu/drm/i915/intel_huc.c
>>>> >> index 1ff1fb015e58..bfdebec1cfc8 100644
>>>> >> --- a/drivers/gpu/drm/i915/intel_huc.c
>>>> >> +++ b/drivers/gpu/drm/i915/intel_huc.c
>>>> >> @@ -113,7 +113,7 @@ int intel_huc_auth(struct intel_huc *huc)
>>>> >>         u32 status;
>>>> >>         int ret;
>>>> >>
>>>> >> -       if (huc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
>>>> >> +       if (!intel_huc_is_loaded(huc))
>>>> >>                 return -ENOEXEC;
>>>> >>
>>>> >>         ret = intel_guc_auth_huc(guc,
>>>> >> @@ -150,21 +150,25 @@ int intel_huc_auth(struct intel_huc *huc)
>>>> >>   * 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.
>>>> >> + * Returns: 1 if HuC firmware is loaded and verified, 0 if HuC
>>>> >> firmware is not
>>>> >> + * enabled, -ENOPKG 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;
>>>> >> +       bool verified = false;
>>>> >>
>>>> >>         if (!HAS_HUC(dev_priv))
>>>> >>                 return -ENODEV;
>>>> >>
>>>> >> -       with_intel_runtime_pm(dev_priv, wakeref)
>>>> >> -               status = I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED;
>>>> >> +       if (!USES_HUC(dev_priv))
>>>> >> +               return 0;
>>>> >
>>>> > Hmm, EOPNOTSUPP here for the user disabled case?
>>>>
>>>> I'm not sure that user disabled case shall be reported as error,
>>>> since it perfectly fits into legacy ABI 0="not loaded".
>>>> The only small change is that now 0="not loaded (not enabled by user)"
>>>
>>> The only requirement for the intel-media driver seems to "HUC_STATUS >  
>>> 0".
>>> That's our only user, so I think we have the liberty to redefine <=0 as
>>> befits error reporting.
>>>
>>>>
>>>> >
>>>> > Then
>>>> >
>>>> > if (!intel_huc_is_loaded(huc))
>>>> >       return -ENOPKG;
>>>> >
>>>> > bool verified = false;
>>>> > with_intel_runtime_pm(dev_priv, wakeref)
>>>> >       verified = I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED;
>>>> > return verified.
>>>> >
>>>> > That keeps it a bit flatter with the special casing separate and  
>>>> the 0/1
>>>> > result as given by the register status. Then if negative, we know  
>>>> one of
>>>> > the preconditions for using HuC is not available, and if 0  
>>>> something went
>>>> > wrong in mmio setup.
>>>> >
>>>> > Does that make sense?
>>>>
>>>> IMHO, if something went wrong it is better to report it as error  
>>>> rather
>>>> then weak 0 status. Compare:
>>>
>>> I disagree, <0 is the weak case. 0 is the HW reports something went
>>> wrong (and only that). 1 is the HW reports all went well.
>>
>> But I was assuming that we're defining ABI here, not simply exposing  
>> unified
>> register value. Since we want 1 to report all ok, and we can fail for  
>> many
>> reasons (no hw, no fw blob, wrong fw, bad signature, ...) we should at  
>> least
>> try to match all these failures into error code (today just ENOPKG, but  
>> later
>> we can extend into ENOEXEC=bad fw, ENOSPC=can't fit into WOPCM, ...).
>>
>> On other hand, at least for me, user decision to disable HuC should not
>> be treated the same way as other real failures, so 0 seems like perfect  
>> match.
>>
>> Then we'll have: 1 = enabled, 0 = disabled, <0 = problem
>>
>>>
>>> If we have more ways we can report the HW went wrong, sure expand that
>>> into extra error codes, but I don't see that. And if it's in the regs,
>>> why are not exporting them via reg_read_ioctl?
>>>
>>>> +---------------+-------+-------+-------+
>>>>   no hardware     -ENODEV -ENODEV -ENODEV
>>>>   fw disabled        0       0    -ENOTSUP
>>>>   fw not loaded      0    -ENOPKG -ENOPKG
>>>>   fw not verified*   0    -ENOPKG    0
>>>>   fw verified*       1       1       1
>>>> +---------------+-------+-------+-------+
>>>> *) registry access
>>>>
>>>> Note that in your case, fw verification problem can be reported both
>>>> as -ENOPKG or as 0 (former when verification fails at load time,  
>>>> latter
>>>> as result of runtime reg read). Very likely we will never see 0 at  
>>>> all,
>>>> since probably that can't change once fw was verified at load time.
>>>
>>> I see more distinction for ENODEV/ENOTSUP/ENOPKG/0/1. I am not getting
>>> your point that we have conflation here, as to me it looks more
>>
>> Note that we are verifying HuC firmware right after it was loaded into
>> hardware using exactly the same register as here (see intel_huc_auth),
>> and if this fails we immediately change fw status to FAIL, so we never
>> go beyond huc_is_loaded point.
>>
>> Assuming that once fw is verified it can't fail (to be verified later)
>> then your codes will be only ENODEV/ENOTSUP/ENOPKG/1 (no 0 value)
>>
>>> distinct, and it is clear when we report the value as given by the
>>> register and when we give an interpreted value for a driver error.
>>
>> Note that your initial idea was to reuse driver maintained fw status
>> instead of waking up device just to check register again (we don't do
>> that now as we need someone to confirm/verify that we can really skip
>> reg read). Use 0/1 for ABI as register values does not fit here, imho.
>>
>> There is also a risk that we will report some known failures as <0
>> error codes, while other, captured by hw as just 0.
>>
>>>
>>>> That might be viewed as undesired ABI change.
>>>
>>> grep says it is fine, so I'm happy that no one else cares (as can be
>>> seen by the lack of igt, no one has looked hard enough into how to
>>> distinguish the API failures ;)
>>
>> it's always hard to come out from gray area into solid spec ;)
>>
>>> -Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915/huc: Don't try to check HuC status if it's not loaded

[Ye, Tony]

On 5/21/2019 7:08 PM, Michal Wajdeczko wrote:
> On Tue, 21 May 2019 13:04:12 +0200, Ye, Tony <tony.ye@intel.com> wrote:
>
>> There are two users of I915_PARAM_HUC_STATUS, the intel-media driver 
>> and the legacy intel-vaapi-driver.
>>
>> Both drivers check the return value like this:
>>
>>      has_huc = !! ret_value;
>>
>> So the ABI change will break the existing stack because negative 
>> values are treated as 1, won't it?
>
> No, since negative errors returned by i915 intel_huc_check_status() 
> function
> will be only used to indicate ioctl() call failure and then stored in 
> errno.
> Note that your "ret_value" will be modified only when ioctl is 
> successful.
>
> Michal
Yeah, you are right.
>
>>
>> On 5/20/2019 8:16 PM, Michal Wajdeczko wrote:
>>
>>> On Mon, 20 May 2019 12:44:43 +0200, Chris Wilson 
>>> <chris@chris-wilson.co.uk> wrote:
>>>
>>>> Quoting Michal Wajdeczko (2019-05-20 11:24:37)
>>>>> On Mon, 20 May 2019 11:35:26 +0200, Chris Wilson
>>>>> <chris@chris-wilson.co.uk> wrote:
>>>>>
>>>>> > Quoting Michal Wajdeczko (2019-05-19 22:50:43)
>>>>> >> If we never attempted to load HuC firmware, or we already wedged
>>>>> >> or reset GuC/HuC, then there is no reason to wake up the device
>>>>> >> to check one bit in the register that will be for sure cleared.
>>>>> >>
>>>>> >> v2: check if HuC was enabled; subtle change in ABI
>>>>> >>     reuse hus_is_load helper
>>>>> >>
>>>>> >> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
>>>>> >> 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 | 20 ++++++++++++--------
>>>>> >>  drivers/gpu/drm/i915/intel_huc.h |  5 +++++
>>>>> >>  2 files changed, 17 insertions(+), 8 deletions(-)
>>>>> >>
>>>>> >> diff --git a/drivers/gpu/drm/i915/intel_huc.c
>>>>> >> b/drivers/gpu/drm/i915/intel_huc.c
>>>>> >> index 1ff1fb015e58..bfdebec1cfc8 100644
>>>>> >> --- a/drivers/gpu/drm/i915/intel_huc.c
>>>>> >> +++ b/drivers/gpu/drm/i915/intel_huc.c
>>>>> >> @@ -113,7 +113,7 @@ int intel_huc_auth(struct intel_huc *huc)
>>>>> >>         u32 status;
>>>>> >>         int ret;
>>>>> >>
>>>>> >> -       if (huc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
>>>>> >> +       if (!intel_huc_is_loaded(huc))
>>>>> >>                 return -ENOEXEC;
>>>>> >>
>>>>> >>         ret = intel_guc_auth_huc(guc,
>>>>> >> @@ -150,21 +150,25 @@ int intel_huc_auth(struct intel_huc *huc)
>>>>> >>   * 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.
>>>>> >> + * Returns: 1 if HuC firmware is loaded and verified, 0 if HuC
>>>>> >> firmware is not
>>>>> >> + * enabled, -ENOPKG 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;
>>>>> >> +       bool verified = false;
>>>>> >>
>>>>> >>         if (!HAS_HUC(dev_priv))
>>>>> >>                 return -ENODEV;
>>>>> >>
>>>>> >> -       with_intel_runtime_pm(dev_priv, wakeref)
>>>>> >> -               status = I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED;
>>>>> >> +       if (!USES_HUC(dev_priv))
>>>>> >> +               return 0;
>>>>> >
>>>>> > Hmm, EOPNOTSUPP here for the user disabled case?
>>>>>
>>>>> I'm not sure that user disabled case shall be reported as error,
>>>>> since it perfectly fits into legacy ABI 0="not loaded".
>>>>> The only small change is that now 0="not loaded (not enabled by 
>>>>> user)"
>>>>
>>>> The only requirement for the intel-media driver seems to 
>>>> "HUC_STATUS > 0".
>>>> That's our only user, so I think we have the liberty to redefine 
>>>> <=0 as
>>>> befits error reporting.
>>>>
>>>>>
>>>>> >
>>>>> > Then
>>>>> >
>>>>> > if (!intel_huc_is_loaded(huc))
>>>>> >       return -ENOPKG;
>>>>> >
>>>>> > bool verified = false;
>>>>> > with_intel_runtime_pm(dev_priv, wakeref)
>>>>> >       verified = I915_READ(HUC_STATUS2) & HUC_FW_VERIFIED;
>>>>> > return verified.
>>>>> >
>>>>> > That keeps it a bit flatter with the special casing separate and 
>>>>> the 0/1
>>>>> > result as given by the register status. Then if negative, we 
>>>>> know one of
>>>>> > the preconditions for using HuC is not available, and if 0 
>>>>> something went
>>>>> > wrong in mmio setup.
>>>>> >
>>>>> > Does that make sense?
>>>>>
>>>>> IMHO, if something went wrong it is better to report it as error 
>>>>> rather
>>>>> then weak 0 status. Compare:
>>>>
>>>> I disagree, <0 is the weak case. 0 is the HW reports something went
>>>> wrong (and only that). 1 is the HW reports all went well.
>>>
>>> But I was assuming that we're defining ABI here, not simply exposing 
>>> unified
>>> register value. Since we want 1 to report all ok, and we can fail 
>>> for many
>>> reasons (no hw, no fw blob, wrong fw, bad signature, ...) we should 
>>> at least
>>> try to match all these failures into error code (today just ENOPKG, 
>>> but later
>>> we can extend into ENOEXEC=bad fw, ENOSPC=can't fit into WOPCM, ...).
>>>
>>> On other hand, at least for me, user decision to disable HuC should not
>>> be treated the same way as other real failures, so 0 seems like 
>>> perfect match.
>>>
>>> Then we'll have: 1 = enabled, 0 = disabled, <0 = problem
>>>
>>>>
>>>> If we have more ways we can report the HW went wrong, sure expand that
>>>> into extra error codes, but I don't see that. And if it's in the regs,
>>>> why are not exporting them via reg_read_ioctl?
>>>>
>>>>> +---------------+-------+-------+-------+
>>>>>   no hardware     -ENODEV -ENODEV -ENODEV
>>>>>   fw disabled        0       0    -ENOTSUP
>>>>>   fw not loaded      0    -ENOPKG -ENOPKG
>>>>>   fw not verified*   0    -ENOPKG    0
>>>>>   fw verified*       1       1       1
>>>>> +---------------+-------+-------+-------+
>>>>> *) registry access
>>>>>
>>>>> Note that in your case, fw verification problem can be reported both
>>>>> as -ENOPKG or as 0 (former when verification fails at load time, 
>>>>> latter
>>>>> as result of runtime reg read). Very likely we will never see 0 at 
>>>>> all,
>>>>> since probably that can't change once fw was verified at load time.
>>>>
>>>> I see more distinction for ENODEV/ENOTSUP/ENOPKG/0/1. I am not getting
>>>> your point that we have conflation here, as to me it looks more
>>>
>>> Note that we are verifying HuC firmware right after it was loaded into
>>> hardware using exactly the same register as here (see intel_huc_auth),
>>> and if this fails we immediately change fw status to FAIL, so we never
>>> go beyond huc_is_loaded point.
>>>
>>> Assuming that once fw is verified it can't fail (to be verified later)
>>> then your codes will be only ENODEV/ENOTSUP/ENOPKG/1 (no 0 value)
>>>
>>>> distinct, and it is clear when we report the value as given by the
>>>> register and when we give an interpreted value for a driver error.
>>>
>>> Note that your initial idea was to reuse driver maintained fw status
>>> instead of waking up device just to check register again (we don't do
>>> that now as we need someone to confirm/verify that we can really skip
>>> reg read). Use 0/1 for ABI as register values does not fit here, imho.
>>>
>>> There is also a risk that we will report some known failures as <0
>>> error codes, while other, captured by hw as just 0.
>>>
>>>>
>>>>> That might be viewed as undesired ABI change.
>>>>
>>>> grep says it is fine, so I'm happy that no one else cares (as can be
>>>> seen by the lack of igt, no one has looked hard enough into how to
>>>> distinguish the API failures ;)

 From UMD perspective, when HuC is not working as expected, usually we 
look into the kernel log and i915_huc_load_status debugfs to find out 
why it's not working. It will be helpful to know the reason of the 
failure from the error code. The typically failures we encountered are 
"HuC FW not loaded (FW not built into initramfs)" and "HuC FW 
authentication failed".

And it looks clearer to me if we can define 0 as "disabled by user" to 
distinguish it from other errors like ENODEV, ENOPKG and "not 
authenticated".

Regards, --Tony

>>>
>>> it's always hard to come out from gray area into solid spec ;)
>>>
>>>> -Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915/huc: Don't try to check HuC status if it's not loaded

[Joonas Lahtinen]

Quoting Ye, Tony (2019-05-22 14:32:41)
>  From UMD perspective, when HuC is not working as expected, usually we 
> look into the kernel log and i915_huc_load_status debugfs to find out 
> why it's not working. It will be helpful to know the reason of the 
> failure from the error code. The typically failures we encountered are 
> "HuC FW not loaded (FW not built into initramfs)" and "HuC FW 
> authentication failed".
> 
> And it looks clearer to me if we can define 0 as "disabled by user" to 
> distinguish it from other errors like ENODEV, ENOPKG and "not 
> authenticated".

Suggestion by Chris for 0/1 for huc_status makes sense to me to reflect if
HuC authentication was succesful or not. Mostly because the name of the
debugfs and func is huc_status. It also nicely maps to a register.

One could have huc_enabled which would then collapse to easy 0/1, but that'd
be bit redundant.

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

Re: [PATCH v2] drm/i915/huc: Don't try to check HuC status if it's not loaded

[Michal Wajdeczko]

On Fri, 24 May 2019 15:10:58 +0200, Joonas Lahtinen  
<joonas.lahtinen@linux.intel.com> wrote:

> Quoting Ye, Tony (2019-05-22 14:32:41)
>>  From UMD perspective, when HuC is not working as expected, usually we
>> look into the kernel log and i915_huc_load_status debugfs to find out
>> why it's not working. It will be helpful to know the reason of the
>> failure from the error code. The typically failures we encountered are
>> "HuC FW not loaded (FW not built into initramfs)" and "HuC FW
>> authentication failed".
>>
>> And it looks clearer to me if we can define 0 as "disabled by user" to
>> distinguish it from other errors like ENODEV, ENOPKG and "not
>> authenticated".
>
> Suggestion by Chris for 0/1 for huc_status makes sense to me to reflect  
> if
> HuC authentication was succesful or not. Mostly because the name of the
> debugfs and func is huc_status. It also nicely maps to a register.

But this entry is not limited to "huc authentication status", as then
we should even not try to introduce new errors, but return 0 to match
register.

On other hand, under normal conditions, we expect HuC to be authenticated
and running or explicitly disabled by the user. Other cases are unexpected
error conditions. I would expect from the ABI that these error conditions
will be reported as errors. For me ABI is something more than reg value.

>
> One could have huc_enabled which would then collapse to easy 0/1, but  
> that'd
> be bit redundant.

that's why we wanted to provide more details in new error codes for
existing GETPARAM, without breaking current usages.

Michal

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

Re: [PATCH v2] drm/i915/huc: Don't try to check HuC status if it's not loaded

[Michal Wajdeczko]

On Fri, 24 May 2019 15:29:22 +0200, Michal Wajdeczko  
<michal.wajdeczko@intel.com> wrote:

> On Fri, 24 May 2019 15:10:58 +0200, Joonas Lahtinen  
> <joonas.lahtinen@linux.intel.com> wrote:
>
>> Quoting Ye, Tony (2019-05-22 14:32:41)
>>>  From UMD perspective, when HuC is not working as expected, usually we
>>> look into the kernel log and i915_huc_load_status debugfs to find out
>>> why it's not working. It will be helpful to know the reason of the
>>> failure from the error code. The typically failures we encountered are
>>> "HuC FW not loaded (FW not built into initramfs)" and "HuC FW
>>> authentication failed".
>>>
>>> And it looks clearer to me if we can define 0 as "disabled by user" to
>>> distinguish it from other errors like ENODEV, ENOPKG and "not
>>> authenticated".
>>
>> Suggestion by Chris for 0/1 for huc_status makes sense to me to reflect  
>> if
>> HuC authentication was succesful or not. Mostly because the name of the
>> debugfs and func is huc_status.

one more thought on debugfs: we are dumping there raw register value,
not just single bit that holds authentication status (and auth bit is 7),
so I'm not sure why do you want to link that value with getparam value?

Michal

>> It also nicely maps to a register.
>
> But this entry is not limited to "huc authentication status", as then
> we should even not try to introduce new errors, but return 0 to match
> register.
>
> On other hand, under normal conditions, we expect HuC to be authenticated
> and running or explicitly disabled by the user. Other cases are  
> unexpected
> error conditions. I would expect from the ABI that these error conditions
> will be reported as errors. For me ABI is something more than reg value.
>
>>
>> One could have huc_enabled which would then collapse to easy 0/1, but  
>> that'd
>> be bit redundant.
>
> that's why we wanted to provide more details in new error codes for
> existing GETPARAM, without breaking current usages.
>
> Michal
>
>>
>> Regards, Joonas
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx