[Intel-gfx] [RFC 0/4] Add ops to intel_uc

[Intel-gfx] [RFC 0/4] Add ops to intel_uc

[Michal Wajdeczko]

Instead of spreading multiple conditionals across the uC code
to find out current mode of uC operation, start using predefined
set of function pointers that reflect that mode.

Michal Wajdeczko (4):
  drm/i915/uc: Add ops to intel_uc
  drm/i915/uc: Add init_fw/fini_fw to to intel_uc_ops
  drm/i915/uc: Add init/fini to to intel_uc_ops
  drm/i915/uc: Add sanitize to to intel_uc_ops

 drivers/gpu/drm/i915/gt/uc/intel_uc.c | 83 ++++++++++++++++++---------
 drivers/gpu/drm/i915/gt/uc/intel_uc.h | 64 ++++++++++++++++++---
 2 files changed, 113 insertions(+), 34 deletions(-)

-- 
2.19.2

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

[Intel-gfx] [RFC 1/4] drm/i915/uc: Add ops to intel_uc

[Michal Wajdeczko]

Instead of spreading multiple conditionals across the uC code
to find out current mode of uC operation, start using predefined
set of function pointers that reflect that mode.

Begin with pair of init_hw/fini_hw functions that are responsible
for uC hardware initialization and cleanup.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_uc.c | 49 +++++++++++++++++++++++----
 drivers/gpu/drm/i915/gt/uc/intel_uc.h | 23 +++++++++++--
 2 files changed, 63 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
index c6519066a0f6..e3d1359f9719 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
@@ -12,6 +12,10 @@
 
 #include "i915_drv.h"
 
+extern const struct intel_uc_ops uc_ops_none;
+extern const struct intel_uc_ops uc_ops_off;
+extern const struct intel_uc_ops uc_ops_on;
+
 /* Reset GuC providing us with fresh state for both GuC and HuC.
  */
 static int __intel_uc_reset_hw(struct intel_uc *uc)
@@ -89,6 +93,13 @@ void intel_uc_init_early(struct intel_uc *uc)
 	intel_huc_init_early(&uc->huc);
 
 	__confirm_options(uc);
+
+	if (intel_uc_uses_guc(uc))
+		uc->ops = &uc_ops_on;
+	else if (intel_uc_supports_guc(uc))
+		uc->ops = &uc_ops_off;
+	else
+		uc->ops = &uc_ops_none;
 }
 
 void intel_uc_driver_late_release(struct intel_uc *uc)
@@ -413,24 +424,36 @@ static bool uc_is_wopcm_locked(struct intel_uc *uc)
 	       (intel_uncore_read(uncore, DMA_GUC_WOPCM_OFFSET) & GUC_WOPCM_OFFSET_VALID);
 }
 
-int intel_uc_init_hw(struct intel_uc *uc)
+static int __uc_check_hw(struct intel_uc *uc)
+{
+	GEM_BUG_ON(!intel_uc_supports_guc(uc));
+
+	/*
+	 * We can silently continue without GuC only if it was never enabled
+	 * before on this system after reboot, otherwise we risk GPU hangs.
+	 * To check if GuC was loaded before we look at WOPCM registers.
+	 */
+	if (uc_is_wopcm_locked(uc))
+		return -EIO;
+
+	return 0;
+}
+
+static int __uc_init_hw(struct intel_uc *uc)
 {
 	struct drm_i915_private *i915 = uc_to_gt(uc)->i915;
 	struct intel_guc *guc = &uc->guc;
 	struct intel_huc *huc = &uc->huc;
 	int ret, attempts;
 
-	if (!intel_uc_supports_guc(uc))
-		return 0;
+	GEM_BUG_ON(!intel_uc_supports_guc(uc));
+	GEM_BUG_ON(!intel_uc_uses_guc(uc));
 
 	/*
 	 * We can silently continue without GuC only if it was never enabled
 	 * before on this system after reboot, otherwise we risk GPU hangs.
 	 * To check if GuC was loaded before we look at WOPCM registers.
 	 */
-	if (!intel_uc_uses_guc(uc) && !uc_is_wopcm_locked(uc))
-		return 0;
-
 	if (!intel_uc_fw_is_available(&guc->fw)) {
 		ret = uc_is_wopcm_locked(uc) ||
 		      intel_uc_fw_is_overridden(&guc->fw) ||
@@ -528,7 +551,7 @@ int intel_uc_init_hw(struct intel_uc *uc)
 	return -EIO;
 }
 
-void intel_uc_fini_hw(struct intel_uc *uc)
+static void __uc_fini_hw(struct intel_uc *uc)
 {
 	struct intel_guc *guc = &uc->guc;
 
@@ -628,3 +651,15 @@ int intel_uc_runtime_resume(struct intel_uc *uc)
 	 */
 	return __uc_resume(uc, true);
 }
+
+const struct intel_uc_ops uc_ops_none = {
+};
+
+const struct intel_uc_ops uc_ops_off = {
+	.init_hw = __uc_check_hw,
+};
+
+const struct intel_uc_ops uc_ops_on = {
+	.init_hw = __uc_init_hw,
+	.fini_hw = __uc_fini_hw,
+};
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.h b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
index 527995c21196..36643e17a09e 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
@@ -10,7 +10,15 @@
 #include "intel_huc.h"
 #include "i915_params.h"
 
+struct intel_uc;
+
+struct intel_uc_ops {
+	int (*init_hw)(struct intel_uc *uc);
+	void (*fini_hw)(struct intel_uc *uc);
+};
+
 struct intel_uc {
+	struct intel_uc_ops const *ops;
 	struct intel_guc guc;
 	struct intel_huc huc;
 
@@ -25,8 +33,6 @@ void intel_uc_fetch_firmwares(struct intel_uc *uc);
 void intel_uc_cleanup_firmwares(struct intel_uc *uc);
 void intel_uc_sanitize(struct intel_uc *uc);
 void intel_uc_init(struct intel_uc *uc);
-int intel_uc_init_hw(struct intel_uc *uc);
-void intel_uc_fini_hw(struct intel_uc *uc);
 void intel_uc_fini(struct intel_uc *uc);
 void intel_uc_reset_prepare(struct intel_uc *uc);
 void intel_uc_suspend(struct intel_uc *uc);
@@ -64,4 +70,17 @@ static inline bool intel_uc_uses_huc(struct intel_uc *uc)
 	return intel_huc_is_enabled(&uc->huc);
 }
 
+static inline int intel_uc_init_hw(struct intel_uc *uc)
+{
+	if (uc->ops->init_hw)
+		return uc->ops->init_hw(uc);
+	return 0;
+}
+
+static inline void intel_uc_fini_hw(struct intel_uc *uc)
+{
+	if (uc->ops->fini_hw)
+		uc->ops->fini_hw(uc);
+}
+
 #endif
-- 
2.19.2

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

[Intel-gfx] [RFC 2/4] drm/i915/uc: Add init_fw/fini_fw to to intel_uc_ops

[Michal Wajdeczko]

Firmware fetching and cleanup steps are only meaningful if we are
running with uC enabled. Make these functions part of the uc_ops.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_uc.c | 12 ++++++------
 drivers/gpu/drm/i915/gt/uc/intel_uc.h | 16 ++++++++++++++--
 2 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
index e3d1359f9719..6c4554e27d07 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
@@ -276,13 +276,12 @@ static void guc_disable_communication(struct intel_guc *guc)
 	DRM_INFO("GuC communication disabled\n");
 }
 
-void intel_uc_fetch_firmwares(struct intel_uc *uc)
+static void __uc_fetch_firmwares(struct intel_uc *uc)
 {
 	struct drm_i915_private *i915 = uc_to_gt(uc)->i915;
 	int err;
 
-	if (!intel_uc_uses_guc(uc))
-		return;
+	GEM_BUG_ON(!intel_uc_uses_guc(uc));
 
 	err = intel_uc_fw_fetch(&uc->guc.fw, i915);
 	if (err)
@@ -292,10 +291,9 @@ void intel_uc_fetch_firmwares(struct intel_uc *uc)
 		intel_uc_fw_fetch(&uc->huc.fw, i915);
 }
 
-void intel_uc_cleanup_firmwares(struct intel_uc *uc)
+static void __uc_cleanup_firmwares(struct intel_uc *uc)
 {
-	if (!intel_uc_uses_guc(uc))
-		return;
+	GEM_BUG_ON(!intel_uc_uses_guc(uc));
 
 	if (intel_uc_uses_huc(uc))
 		intel_uc_fw_cleanup_fetch(&uc->huc.fw);
@@ -660,6 +658,8 @@ const struct intel_uc_ops uc_ops_off = {
 };
 
 const struct intel_uc_ops uc_ops_on = {
+	.init_fw = __uc_fetch_firmwares,
+	.fini_fw = __uc_cleanup_firmwares,
 	.init_hw = __uc_init_hw,
 	.fini_hw = __uc_fini_hw,
 };
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.h b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
index 36643e17a09e..02068585f6ac 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
@@ -13,6 +13,8 @@
 struct intel_uc;
 
 struct intel_uc_ops {
+	void (*init_fw)(struct intel_uc *uc);
+	void (*fini_fw)(struct intel_uc *uc);
 	int (*init_hw)(struct intel_uc *uc);
 	void (*fini_hw)(struct intel_uc *uc);
 };
@@ -29,8 +31,6 @@ struct intel_uc {
 void intel_uc_init_early(struct intel_uc *uc);
 void intel_uc_driver_late_release(struct intel_uc *uc);
 void intel_uc_init_mmio(struct intel_uc *uc);
-void intel_uc_fetch_firmwares(struct intel_uc *uc);
-void intel_uc_cleanup_firmwares(struct intel_uc *uc);
 void intel_uc_sanitize(struct intel_uc *uc);
 void intel_uc_init(struct intel_uc *uc);
 void intel_uc_fini(struct intel_uc *uc);
@@ -70,6 +70,18 @@ static inline bool intel_uc_uses_huc(struct intel_uc *uc)
 	return intel_huc_is_enabled(&uc->huc);
 }
 
+static inline void intel_uc_fetch_firmwares(struct intel_uc *uc)
+{
+	if (uc->ops->init_fw)
+		uc->ops->init_fw(uc);
+}
+
+static inline void intel_uc_cleanup_firmwares(struct intel_uc *uc)
+{
+	if (uc->ops->fini_fw)
+		uc->ops->fini_fw(uc);
+}
+
 static inline int intel_uc_init_hw(struct intel_uc *uc)
 {
 	if (uc->ops->init_hw)
-- 
2.19.2

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

[Intel-gfx] [RFC 3/4] drm/i915/uc: Add init/fini to to intel_uc_ops

[Michal Wajdeczko]

uC preparation and cleanup steps are only meaningful if we are
running with uC enabled. Make these functions part of the uc_ops.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_uc.c | 12 ++++++------
 drivers/gpu/drm/i915/gt/uc/intel_uc.h | 16 ++++++++++++++--
 2 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
index 6c4554e27d07..c8b399d61166 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
@@ -301,14 +301,13 @@ static void __uc_cleanup_firmwares(struct intel_uc *uc)
 	intel_uc_fw_cleanup_fetch(&uc->guc.fw);
 }
 
-void intel_uc_init(struct intel_uc *uc)
+static void __uc_init(struct intel_uc *uc)
 {
 	struct intel_guc *guc = &uc->guc;
 	struct intel_huc *huc = &uc->huc;
 	int ret;
 
-	if (!intel_uc_uses_guc(uc))
-		return;
+	GEM_BUG_ON(!intel_uc_uses_guc(uc));
 
 	/* XXX: GuC submission is unavailable for now */
 	GEM_BUG_ON(intel_uc_supports_guc_submission(uc));
@@ -323,12 +322,11 @@ void intel_uc_init(struct intel_uc *uc)
 		intel_huc_init(huc);
 }
 
-void intel_uc_fini(struct intel_uc *uc)
+static void __uc_fini(struct intel_uc *uc)
 {
 	struct intel_guc *guc = &uc->guc;
 
-	if (!intel_uc_uses_guc(uc))
-		return;
+	GEM_BUG_ON(!intel_uc_uses_guc(uc));
 
 	if (intel_uc_uses_huc(uc))
 		intel_huc_fini(&uc->huc);
@@ -660,6 +658,8 @@ const struct intel_uc_ops uc_ops_off = {
 const struct intel_uc_ops uc_ops_on = {
 	.init_fw = __uc_fetch_firmwares,
 	.fini_fw = __uc_cleanup_firmwares,
+	.init = __uc_init,
+	.fini = __uc_fini,
 	.init_hw = __uc_init_hw,
 	.fini_hw = __uc_fini_hw,
 };
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.h b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
index 02068585f6ac..2bd8326130f1 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
@@ -15,6 +15,8 @@ struct intel_uc;
 struct intel_uc_ops {
 	void (*init_fw)(struct intel_uc *uc);
 	void (*fini_fw)(struct intel_uc *uc);
+	void (*init)(struct intel_uc *uc);
+	void (*fini)(struct intel_uc *uc);
 	int (*init_hw)(struct intel_uc *uc);
 	void (*fini_hw)(struct intel_uc *uc);
 };
@@ -32,8 +34,6 @@ void intel_uc_init_early(struct intel_uc *uc);
 void intel_uc_driver_late_release(struct intel_uc *uc);
 void intel_uc_init_mmio(struct intel_uc *uc);
 void intel_uc_sanitize(struct intel_uc *uc);
-void intel_uc_init(struct intel_uc *uc);
-void intel_uc_fini(struct intel_uc *uc);
 void intel_uc_reset_prepare(struct intel_uc *uc);
 void intel_uc_suspend(struct intel_uc *uc);
 void intel_uc_runtime_suspend(struct intel_uc *uc);
@@ -82,6 +82,18 @@ static inline void intel_uc_cleanup_firmwares(struct intel_uc *uc)
 		uc->ops->fini_fw(uc);
 }
 
+static inline void intel_uc_init(struct intel_uc *uc)
+{
+	if (uc->ops->init)
+		uc->ops->init(uc);
+}
+
+static inline void intel_uc_fini(struct intel_uc *uc)
+{
+	if (uc->ops->fini)
+		uc->ops->fini(uc);
+}
+
 static inline int intel_uc_init_hw(struct intel_uc *uc)
 {
 	if (uc->ops->init_hw)
-- 
2.19.2

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

[Intel-gfx] [RFC 4/4] drm/i915/uc: Add sanitize to to intel_uc_ops

[Michal Wajdeczko]

uC sanitization is only meaningful if we are running with uC present
or enabled. Make this function part of the uc_ops.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
---
 drivers/gpu/drm/i915/gt/uc/intel_uc.c | 10 ++--------
 drivers/gpu/drm/i915/gt/uc/intel_uc.h |  9 ++++++++-
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
index c8b399d61166..634ef2fa34f1 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
@@ -349,14 +349,6 @@ static int __uc_sanitize(struct intel_uc *uc)
 	return __intel_uc_reset_hw(uc);
 }
 
-void intel_uc_sanitize(struct intel_uc *uc)
-{
-	if (!intel_uc_supports_guc(uc))
-		return;
-
-	__uc_sanitize(uc);
-}
-
 /* Initialize and verify the uC regs related to uC positioning in WOPCM */
 static int uc_init_wopcm(struct intel_uc *uc)
 {
@@ -652,10 +644,12 @@ const struct intel_uc_ops uc_ops_none = {
 };
 
 const struct intel_uc_ops uc_ops_off = {
+	.sanitize = __intel_uc_reset_hw,
 	.init_hw = __uc_check_hw,
 };
 
 const struct intel_uc_ops uc_ops_on = {
+	.sanitize = __uc_sanitize,
 	.init_fw = __uc_fetch_firmwares,
 	.fini_fw = __uc_cleanup_firmwares,
 	.init = __uc_init,
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.h b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
index 2bd8326130f1..3410d35f8b0c 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
@@ -19,6 +19,7 @@ struct intel_uc_ops {
 	void (*fini)(struct intel_uc *uc);
 	int (*init_hw)(struct intel_uc *uc);
 	void (*fini_hw)(struct intel_uc *uc);
+	int (*sanitize)(struct intel_uc *uc);
 };
 
 struct intel_uc {
@@ -33,7 +34,6 @@ struct intel_uc {
 void intel_uc_init_early(struct intel_uc *uc);
 void intel_uc_driver_late_release(struct intel_uc *uc);
 void intel_uc_init_mmio(struct intel_uc *uc);
-void intel_uc_sanitize(struct intel_uc *uc);
 void intel_uc_reset_prepare(struct intel_uc *uc);
 void intel_uc_suspend(struct intel_uc *uc);
 void intel_uc_runtime_suspend(struct intel_uc *uc);
@@ -107,4 +107,11 @@ static inline void intel_uc_fini_hw(struct intel_uc *uc)
 		uc->ops->fini_hw(uc);
 }
 
+static inline int intel_uc_sanitize(struct intel_uc *uc)
+{
+	if (uc->ops->sanitize)
+		return uc->ops->sanitize(uc);
+	return 0;
+}
+
 #endif
-- 
2.19.2

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

Re: [Intel-gfx] [RFC 2/4] drm/i915/uc: Add init_fw/fini_fw to to intel_uc_ops

[Chris Wilson]

Quoting Michal Wajdeczko (2019-12-10 20:47:42)
>  const struct intel_uc_ops uc_ops_on = {
> +       .init_fw = __uc_fetch_firmwares,
> +       .fini_fw = __uc_cleanup_firmwares,

Whitespace between the pairs, or fullname _firmware?

>         .init_hw = __uc_init_hw,
>         .fini_hw = __uc_fini_hw,

It did require a double take :)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC 1/4] drm/i915/uc: Add ops to intel_uc

[Chris Wilson]

Quoting Michal Wajdeczko (2019-12-10 20:47:41)
> @@ -628,3 +651,15 @@ int intel_uc_runtime_resume(struct intel_uc *uc)
>          */
>         return __uc_resume(uc, true);
>  }
> +
> +const struct intel_uc_ops uc_ops_none = {
> +};
> +
> +const struct intel_uc_ops uc_ops_off = {
> +       .init_hw = __uc_check_hw,
> +};
> +
> +const struct intel_uc_ops uc_ops_on = {
> +       .init_hw = __uc_init_hw,
> +       .fini_hw = __uc_fini_hw,
> +};

No externs in the headers, so should these be static?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC 4/4] drm/i915/uc: Add sanitize to to intel_uc_ops

[Chris Wilson]

Quoting Michal Wajdeczko (2019-12-10 20:47:44)
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.h b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
> index 2bd8326130f1..3410d35f8b0c 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
> @@ -19,6 +19,7 @@ struct intel_uc_ops {
>         void (*fini)(struct intel_uc *uc);
>         int (*init_hw)(struct intel_uc *uc);
>         void (*fini_hw)(struct intel_uc *uc);
> +       int (*sanitize)(struct intel_uc *uc);

In the order of ops, first?

The series looks sane. Looks a bit overkill, but if you can convince
Daniele of your vision, fine by me.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC 1/4] drm/i915/uc: Add ops to intel_uc

[Michal Wajdeczko]

On Tue, 10 Dec 2019 21:55:13 +0100, Chris Wilson  
<chris@chris-wilson.co.uk> wrote:

> Quoting Michal Wajdeczko (2019-12-10 20:47:41)
>> @@ -628,3 +651,15 @@ int intel_uc_runtime_resume(struct intel_uc *uc)
>>          */
>>         return __uc_resume(uc, true);
>>  }
>> +
>> +const struct intel_uc_ops uc_ops_none = {
>> +};
>> +
>> +const struct intel_uc_ops uc_ops_off = {
>> +       .init_hw = __uc_check_hw,
>> +};
>> +
>> +const struct intel_uc_ops uc_ops_on = {
>> +       .init_hw = __uc_init_hw,
>> +       .fini_hw = __uc_fini_hw,
>> +};
>
> No externs in the headers, so should these be static?

but then forwards from top of the file will not work.
and early_init will have to be moved here as well.
doable, but wanted to minimize diffs during rfc phase.

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

Re: [Intel-gfx] [RFC 1/4] drm/i915/uc: Add ops to intel_uc

[Chris Wilson]

Quoting Michal Wajdeczko (2019-12-10 21:02:30)
> On Tue, 10 Dec 2019 21:55:13 +0100, Chris Wilson  
> <chris@chris-wilson.co.uk> wrote:
> 
> > Quoting Michal Wajdeczko (2019-12-10 20:47:41)
> >> @@ -628,3 +651,15 @@ int intel_uc_runtime_resume(struct intel_uc *uc)
> >>          */
> >>         return __uc_resume(uc, true);
> >>  }
> >> +
> >> +const struct intel_uc_ops uc_ops_none = {
> >> +};
> >> +
> >> +const struct intel_uc_ops uc_ops_off = {
> >> +       .init_hw = __uc_check_hw,
> >> +};
> >> +
> >> +const struct intel_uc_ops uc_ops_on = {
> >> +       .init_hw = __uc_init_hw,
> >> +       .fini_hw = __uc_fini_hw,
> >> +};
> >
> > No externs in the headers, so should these be static?
> 
> but then forwards from top of the file will not work.
> and early_init will have to be moved here as well.
> doable, but wanted to minimize diffs during rfc phase.

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

[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for Add ops to intel_uc

[Patchwork]

== Series Details ==

Series: Add ops to intel_uc
URL   : https://patchwork.freedesktop.org/series/70716/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
954921ae8651 drm/i915/uc: Add ops to intel_uc
-:27: WARNING:AVOID_EXTERNS: externs should be avoided in .c files
#27: FILE: drivers/gpu/drm/i915/gt/uc/intel_uc.c:16:
+extern const struct intel_uc_ops uc_ops_off;

-:28: WARNING:AVOID_EXTERNS: externs should be avoided in .c files
#28: FILE: drivers/gpu/drm/i915/gt/uc/intel_uc.c:17:
+extern const struct intel_uc_ops uc_ops_on;

total: 0 errors, 2 warnings, 0 checks, 128 lines checked
32700bafdc26 drm/i915/uc: Add init_fw/fini_fw to to intel_uc_ops
b5bf30f4e353 drm/i915/uc: Add init/fini to to intel_uc_ops
fe773089ddeb drm/i915/uc: Add sanitize to to intel_uc_ops

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

[Intel-gfx] ✗ Fi.CI.BAT: failure for Add ops to intel_uc

[Patchwork]

== Series Details ==

Series: Add ops to intel_uc
URL   : https://patchwork.freedesktop.org/series/70716/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_7534 -> Patchwork_15680
====================================================

Summary
-------

  **FAILURE**

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

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15680/index.html

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

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

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live_blt:
    - fi-hsw-4770r:       NOTRUN -> [DMESG-FAIL][1]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15680/fi-hsw-4770r/igt@i915_selftest@live_blt.html

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

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

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live_blt:
    - fi-ivb-3770:        [PASS][2] -> [DMESG-FAIL][3] ([i915#725])
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7534/fi-ivb-3770/igt@i915_selftest@live_blt.html
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15680/fi-ivb-3770/igt@i915_selftest@live_blt.html
    - fi-hsw-4770:        [PASS][4] -> [DMESG-FAIL][5] ([i915#725])
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7534/fi-hsw-4770/igt@i915_selftest@live_blt.html
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15680/fi-hsw-4770/igt@i915_selftest@live_blt.html

  
#### Possible fixes ####

  * igt@gem_ctx_create@basic-files:
    - {fi-tgl-u}:         [INCOMPLETE][6] ([fdo#111735]) -> [PASS][7]
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7534/fi-tgl-u/igt@gem_ctx_create@basic-files.html
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15680/fi-tgl-u/igt@gem_ctx_create@basic-files.html

  
#### Warnings ####

  * igt@i915_selftest@live_gem_contexts:
    - fi-hsw-peppy:       [INCOMPLETE][8] ([i915#694]) -> [DMESG-FAIL][9] ([i915#722])
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7534/fi-hsw-peppy/igt@i915_selftest@live_gem_contexts.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15680/fi-hsw-peppy/igt@i915_selftest@live_gem_contexts.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy:
    - fi-kbl-x1275:       [DMESG-WARN][10] ([i915#62] / [i915#92] / [i915#95]) -> [DMESG-WARN][11] ([i915#62] / [i915#92]) +6 similar issues
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7534/fi-kbl-x1275/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15680/fi-kbl-x1275/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-legacy.html

  * igt@kms_pipe_crc_basic@read-crc-pipe-b:
    - fi-kbl-x1275:       [DMESG-WARN][12] ([i915#62] / [i915#92]) -> [DMESG-WARN][13] ([i915#62] / [i915#92] / [i915#95]) +3 similar issues
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7534/fi-kbl-x1275/igt@kms_pipe_crc_basic@read-crc-pipe-b.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_15680/fi-kbl-x1275/igt@kms_pipe_crc_basic@read-crc-pipe-b.html

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

  [fdo#111735]: https://bugs.freedesktop.org/show_bug.cgi?id=111735
  [i915#62]: https://gitlab.freedesktop.org/drm/intel/issues/62
  [i915#694]: https://gitlab.freedesktop.org/drm/intel/issues/694
  [i915#707]: https://gitlab.freedesktop.org/drm/intel/issues/707
  [i915#722]: https://gitlab.freedesktop.org/drm/intel/issues/722
  [i915#725]: https://gitlab.freedesktop.org/drm/intel/issues/725
  [i915#92]: https://gitlab.freedesktop.org/drm/intel/issues/92
  [i915#95]: https://gitlab.freedesktop.org/drm/intel/issues/95


Participating hosts (53 -> 46)
------------------------------

  Additional (2): fi-hsw-4770r fi-kbl-7560u 
  Missing    (9): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-kbl-7500u fi-ctg-p8600 fi-tgl-y fi-byt-clapper fi-bdw-samus 


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

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_7534 -> Patchwork_15680

  CI-20190529: 20190529
  CI_DRM_7534: 66a6222c16abb82d6a4480b0a7ff8e375cc6a3a6 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5342: 3e43fb3fa97ce25819444d63848439acf6e397b5 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_15680: fe773089ddebef6d9c956bb7fbcecb912202c2c0 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

fe773089ddeb drm/i915/uc: Add sanitize to to intel_uc_ops
b5bf30f4e353 drm/i915/uc: Add init/fini to to intel_uc_ops
32700bafdc26 drm/i915/uc: Add init_fw/fini_fw to to intel_uc_ops
954921ae8651 drm/i915/uc: Add ops to intel_uc

== Logs ==

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

Re: [Intel-gfx] [RFC 4/4] drm/i915/uc: Add sanitize to to intel_uc_ops

[Daniele Ceraolo Spurio]

On 12/10/19 1:00 PM, Chris Wilson wrote:
> Quoting Michal Wajdeczko (2019-12-10 20:47:44)
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.h b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
>> index 2bd8326130f1..3410d35f8b0c 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
>> @@ -19,6 +19,7 @@ struct intel_uc_ops {
>>          void (*fini)(struct intel_uc *uc);
>>          int (*init_hw)(struct intel_uc *uc);
>>          void (*fini_hw)(struct intel_uc *uc);
>> +       int (*sanitize)(struct intel_uc *uc);
> 
> In the order of ops, first?
> 
> The series looks sane. Looks a bit overkill, but if you can convince
> Daniele of your vision, fine by me.
> -Chris
> 

I've had a quick chat with Michal about this and, when also considering 
some of the upcoming stuff, it does definitely make sense to me to 
switch to ops.

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

Re: [Intel-gfx] [RFC 1/4] drm/i915/uc: Add ops to intel_uc

[Daniele Ceraolo Spurio]

On 12/10/19 12:47 PM, Michal Wajdeczko wrote:
> Instead of spreading multiple conditionals across the uC code
> to find out current mode of uC operation, start using predefined
> set of function pointers that reflect that mode.
> 
> Begin with pair of init_hw/fini_hw functions that are responsible
> for uC hardware initialization and cleanup.
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> ---
>   drivers/gpu/drm/i915/gt/uc/intel_uc.c | 49 +++++++++++++++++++++++----
>   drivers/gpu/drm/i915/gt/uc/intel_uc.h | 23 +++++++++++--
>   2 files changed, 63 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> index c6519066a0f6..e3d1359f9719 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> @@ -12,6 +12,10 @@
>   
>   #include "i915_drv.h"
>   
> +extern const struct intel_uc_ops uc_ops_none;
> +extern const struct intel_uc_ops uc_ops_off;
> +extern const struct intel_uc_ops uc_ops_on;
> +
>   /* Reset GuC providing us with fresh state for both GuC and HuC.
>    */
>   static int __intel_uc_reset_hw(struct intel_uc *uc)
> @@ -89,6 +93,13 @@ void intel_uc_init_early(struct intel_uc *uc)
>   	intel_huc_init_early(&uc->huc);
>   
>   	__confirm_options(uc);
> +
> +	if (intel_uc_uses_guc(uc))
> +		uc->ops = &uc_ops_on;
> +	else if (intel_uc_supports_guc(uc))
> +		uc->ops = &uc_ops_off;
> +	else
> +		uc->ops = &uc_ops_none;
>   }
>   
>   void intel_uc_driver_late_release(struct intel_uc *uc)
> @@ -413,24 +424,36 @@ static bool uc_is_wopcm_locked(struct intel_uc *uc)
>   	       (intel_uncore_read(uncore, DMA_GUC_WOPCM_OFFSET) & GUC_WOPCM_OFFSET_VALID);
>   }
>   
> -int intel_uc_init_hw(struct intel_uc *uc)
> +static int __uc_check_hw(struct intel_uc *uc)
> +{
> +	GEM_BUG_ON(!intel_uc_supports_guc(uc));
> +
> +	/*
> +	 * We can silently continue without GuC only if it was never enabled
> +	 * before on this system after reboot, otherwise we risk GPU hangs.
> +	 * To check if GuC was loaded before we look at WOPCM registers.
> +	 */
> +	if (uc_is_wopcm_locked(uc))
> +		return -EIO;
> +
> +	return 0;
> +}
> +
> +static int __uc_init_hw(struct intel_uc *uc)
>   {
>   	struct drm_i915_private *i915 = uc_to_gt(uc)->i915;
>   	struct intel_guc *guc = &uc->guc;
>   	struct intel_huc *huc = &uc->huc;
>   	int ret, attempts;
>   
> -	if (!intel_uc_supports_guc(uc))
> -		return 0;
> +	GEM_BUG_ON(!intel_uc_supports_guc(uc));
> +	GEM_BUG_ON(!intel_uc_uses_guc(uc));
>   
>   	/*
>   	 * We can silently continue without GuC only if it was never enabled
>   	 * before on this system after reboot, otherwise we risk GPU hangs.
>   	 * To check if GuC was loaded before we look at WOPCM registers.
>   	 */
> -	if (!intel_uc_uses_guc(uc) && !uc_is_wopcm_locked(uc))
> -		return 0;
> -
>   	if (!intel_uc_fw_is_available(&guc->fw)) {
>   		ret = uc_is_wopcm_locked(uc) ||
>   		      intel_uc_fw_is_overridden(&guc->fw) ||
> @@ -528,7 +551,7 @@ int intel_uc_init_hw(struct intel_uc *uc)
>   	return -EIO;
>   }
>   
> -void intel_uc_fini_hw(struct intel_uc *uc)
> +static void __uc_fini_hw(struct intel_uc *uc)
>   {
>   	struct intel_guc *guc = &uc->guc;
>   
> @@ -628,3 +651,15 @@ int intel_uc_runtime_resume(struct intel_uc *uc)
>   	 */
>   	return __uc_resume(uc, true);
>   }
> +
> +const struct intel_uc_ops uc_ops_none = {
> +};
> +
> +const struct intel_uc_ops uc_ops_off = {
> +	.init_hw = __uc_check_hw,
> +};
> +

I'm not sold on having both uc_ops_off and uc_ops_none and I'd prefer 
them to be merged into one.
AFAICS, the only things you  do in uc_ops_off are __intel_uc_reset_hw 
and __uc_check_hw. __intel_uc_reset_hw shouldn't be needed, we do a GT 
reset when we come up and we're not touching the microcontrollers if 
intel_uc_uses_guc is false, so there should be no need to reset them. 
for __uc_check_hw, we can add an early return if !intel_uc_supports_guc 
so it is callable on all platforms and add it to uc_ops_none.

Daniele

> +const struct intel_uc_ops uc_ops_on = {
> +	.init_hw = __uc_init_hw,
> +	.fini_hw = __uc_fini_hw,
> +};
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.h b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
> index 527995c21196..36643e17a09e 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
> @@ -10,7 +10,15 @@
>   #include "intel_huc.h"
>   #include "i915_params.h"
>   
> +struct intel_uc;
> +
> +struct intel_uc_ops {
> +	int (*init_hw)(struct intel_uc *uc);
> +	void (*fini_hw)(struct intel_uc *uc);
> +};
> +
>   struct intel_uc {
> +	struct intel_uc_ops const *ops;
>   	struct intel_guc guc;
>   	struct intel_huc huc;
>   
> @@ -25,8 +33,6 @@ void intel_uc_fetch_firmwares(struct intel_uc *uc);
>   void intel_uc_cleanup_firmwares(struct intel_uc *uc);
>   void intel_uc_sanitize(struct intel_uc *uc);
>   void intel_uc_init(struct intel_uc *uc);
> -int intel_uc_init_hw(struct intel_uc *uc);
> -void intel_uc_fini_hw(struct intel_uc *uc);
>   void intel_uc_fini(struct intel_uc *uc);
>   void intel_uc_reset_prepare(struct intel_uc *uc);
>   void intel_uc_suspend(struct intel_uc *uc);
> @@ -64,4 +70,17 @@ static inline bool intel_uc_uses_huc(struct intel_uc *uc)
>   	return intel_huc_is_enabled(&uc->huc);
>   }
>   
> +static inline int intel_uc_init_hw(struct intel_uc *uc)
> +{
> +	if (uc->ops->init_hw)
> +		return uc->ops->init_hw(uc);
> +	return 0;
> +}
> +
> +static inline void intel_uc_fini_hw(struct intel_uc *uc)
> +{
> +	if (uc->ops->fini_hw)
> +		uc->ops->fini_hw(uc);
> +}
> +
>   #endif
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC 1/4] drm/i915/uc: Add ops to intel_uc

[Michal Wajdeczko]

On Thu, 12 Dec 2019 01:23:33 +0100, Daniele Ceraolo Spurio  
<daniele.ceraolospurio@intel.com> wrote:

>
>
> On 12/10/19 12:47 PM, Michal Wajdeczko wrote:
>> Instead of spreading multiple conditionals across the uC code
>> to find out current mode of uC operation, start using predefined
>> set of function pointers that reflect that mode.
>>  Begin with pair of init_hw/fini_hw functions that are responsible
>> for uC hardware initialization and cleanup.
>>  Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>> ---
>>   drivers/gpu/drm/i915/gt/uc/intel_uc.c | 49 +++++++++++++++++++++++----
>>   drivers/gpu/drm/i915/gt/uc/intel_uc.h | 23 +++++++++++--
>>   2 files changed, 63 insertions(+), 9 deletions(-)
>>  diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c  
>> b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> index c6519066a0f6..e3d1359f9719 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>> @@ -12,6 +12,10 @@
>>     #include "i915_drv.h"
>>   +extern const struct intel_uc_ops uc_ops_none;
>> +extern const struct intel_uc_ops uc_ops_off;
>> +extern const struct intel_uc_ops uc_ops_on;
>> +
>>   /* Reset GuC providing us with fresh state for both GuC and HuC.
>>    */
>>   static int __intel_uc_reset_hw(struct intel_uc *uc)
>> @@ -89,6 +93,13 @@ void intel_uc_init_early(struct intel_uc *uc)
>>   	intel_huc_init_early(&uc->huc);
>>     	__confirm_options(uc);
>> +
>> +	if (intel_uc_uses_guc(uc))
>> +		uc->ops = &uc_ops_on;
>> +	else if (intel_uc_supports_guc(uc))
>> +		uc->ops = &uc_ops_off;
>> +	else
>> +		uc->ops = &uc_ops_none;
>>   }
>>     void intel_uc_driver_late_release(struct intel_uc *uc)
>> @@ -413,24 +424,36 @@ static bool uc_is_wopcm_locked(struct intel_uc  
>> *uc)
>>   	       (intel_uncore_read(uncore, DMA_GUC_WOPCM_OFFSET) &  
>> GUC_WOPCM_OFFSET_VALID);
>>   }
>>   -int intel_uc_init_hw(struct intel_uc *uc)
>> +static int __uc_check_hw(struct intel_uc *uc)
>> +{
>> +	GEM_BUG_ON(!intel_uc_supports_guc(uc));
>> +
>> +	/*
>> +	 * We can silently continue without GuC only if it was never enabled
>> +	 * before on this system after reboot, otherwise we risk GPU hangs.
>> +	 * To check if GuC was loaded before we look at WOPCM registers.
>> +	 */
>> +	if (uc_is_wopcm_locked(uc))
>> +		return -EIO;
>> +
>> +	return 0;
>> +}
>> +
>> +static int __uc_init_hw(struct intel_uc *uc)
>>   {
>>   	struct drm_i915_private *i915 = uc_to_gt(uc)->i915;
>>   	struct intel_guc *guc = &uc->guc;
>>   	struct intel_huc *huc = &uc->huc;
>>   	int ret, attempts;
>>   -	if (!intel_uc_supports_guc(uc))
>> -		return 0;
>> +	GEM_BUG_ON(!intel_uc_supports_guc(uc));
>> +	GEM_BUG_ON(!intel_uc_uses_guc(uc));
>>     	/*
>>   	 * We can silently continue without GuC only if it was never enabled
>>   	 * before on this system after reboot, otherwise we risk GPU hangs.
>>   	 * To check if GuC was loaded before we look at WOPCM registers.
>>   	 */
>> -	if (!intel_uc_uses_guc(uc) && !uc_is_wopcm_locked(uc))
>> -		return 0;
>> -
>>   	if (!intel_uc_fw_is_available(&guc->fw)) {
>>   		ret = uc_is_wopcm_locked(uc) ||
>>   		      intel_uc_fw_is_overridden(&guc->fw) ||
>> @@ -528,7 +551,7 @@ int intel_uc_init_hw(struct intel_uc *uc)
>>   	return -EIO;
>>   }
>>   -void intel_uc_fini_hw(struct intel_uc *uc)
>> +static void __uc_fini_hw(struct intel_uc *uc)
>>   {
>>   	struct intel_guc *guc = &uc->guc;
>>   @@ -628,3 +651,15 @@ int intel_uc_runtime_resume(struct intel_uc *uc)
>>   	 */
>>   	return __uc_resume(uc, true);
>>   }
>> +
>> +const struct intel_uc_ops uc_ops_none = {
>> +};
>> +
>> +const struct intel_uc_ops uc_ops_off = {
>> +	.init_hw = __uc_check_hw,
>> +};
>> +
>
> I'm not sold on having both uc_ops_off and uc_ops_none and I'd prefer  
> them to be merged into one.
> AFAICS, the only things you  do in uc_ops_off are __intel_uc_reset_hw  
> and __uc_check_hw. __intel_uc_reset_hw shouldn't be needed, we do a GT  
> reset when we come up and we're not touching the microcontrollers if  
> intel_uc_uses_guc is false, so there should be no need to reset them.  
> for __uc_check_hw, we can add an early return if !intel_uc_supports_guc  
> so it is callable on all platforms and add it to uc_ops_none.

I can drop uc_reset_hw from ops.off, but I would keep both off|none
separate as otherwise we are blurring the idea of having dedicated
ops without runtime checks (next step of such "merging/simplification"
approach would be temptation to add more checks into existing ops,
rather then preparing dedicated one). Note that we may soon add
ops to other components and we should be consistent on usage model.

Michal

>
> Daniele
>
>> +const struct intel_uc_ops uc_ops_on = {
>> +	.init_hw = __uc_init_hw,
>> +	.fini_hw = __uc_fini_hw,
>> +};
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.h  
>> b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
>> index 527995c21196..36643e17a09e 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
>> @@ -10,7 +10,15 @@
>>   #include "intel_huc.h"
>>   #include "i915_params.h"
>>   +struct intel_uc;
>> +
>> +struct intel_uc_ops {
>> +	int (*init_hw)(struct intel_uc *uc);
>> +	void (*fini_hw)(struct intel_uc *uc);
>> +};
>> +
>>   struct intel_uc {
>> +	struct intel_uc_ops const *ops;
>>   	struct intel_guc guc;
>>   	struct intel_huc huc;
>>   @@ -25,8 +33,6 @@ void intel_uc_fetch_firmwares(struct intel_uc *uc);
>>   void intel_uc_cleanup_firmwares(struct intel_uc *uc);
>>   void intel_uc_sanitize(struct intel_uc *uc);
>>   void intel_uc_init(struct intel_uc *uc);
>> -int intel_uc_init_hw(struct intel_uc *uc);
>> -void intel_uc_fini_hw(struct intel_uc *uc);
>>   void intel_uc_fini(struct intel_uc *uc);
>>   void intel_uc_reset_prepare(struct intel_uc *uc);
>>   void intel_uc_suspend(struct intel_uc *uc);
>> @@ -64,4 +70,17 @@ static inline bool intel_uc_uses_huc(struct intel_uc  
>> *uc)
>>   	return intel_huc_is_enabled(&uc->huc);
>>   }
>>   +static inline int intel_uc_init_hw(struct intel_uc *uc)
>> +{
>> +	if (uc->ops->init_hw)
>> +		return uc->ops->init_hw(uc);
>> +	return 0;
>> +}
>> +
>> +static inline void intel_uc_fini_hw(struct intel_uc *uc)
>> +{
>> +	if (uc->ops->fini_hw)
>> +		uc->ops->fini_hw(uc);
>> +}
>> +
>>   #endif
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC 1/4] drm/i915/uc: Add ops to intel_uc

[Daniele Ceraolo Spurio]

On 12/12/19 1:53 PM, Michal Wajdeczko wrote:
> On Thu, 12 Dec 2019 01:23:33 +0100, Daniele Ceraolo Spurio 
> <daniele.ceraolospurio@intel.com> wrote:
> 
>>
>>
>> On 12/10/19 12:47 PM, Michal Wajdeczko wrote:
>>> Instead of spreading multiple conditionals across the uC code
>>> to find out current mode of uC operation, start using predefined
>>> set of function pointers that reflect that mode.
>>>  Begin with pair of init_hw/fini_hw functions that are responsible
>>> for uC hardware initialization and cleanup.
>>>  Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/gt/uc/intel_uc.c | 49 +++++++++++++++++++++++----
>>>   drivers/gpu/drm/i915/gt/uc/intel_uc.h | 23 +++++++++++--
>>>   2 files changed, 63 insertions(+), 9 deletions(-)
>>>  diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c 
>>> b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>>> index c6519066a0f6..e3d1359f9719 100644
>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>>> @@ -12,6 +12,10 @@
>>>     #include "i915_drv.h"
>>>   +extern const struct intel_uc_ops uc_ops_none;
>>> +extern const struct intel_uc_ops uc_ops_off;
>>> +extern const struct intel_uc_ops uc_ops_on;
>>> +
>>>   /* Reset GuC providing us with fresh state for both GuC and HuC.
>>>    */
>>>   static int __intel_uc_reset_hw(struct intel_uc *uc)
>>> @@ -89,6 +93,13 @@ void intel_uc_init_early(struct intel_uc *uc)
>>>       intel_huc_init_early(&uc->huc);
>>>         __confirm_options(uc);
>>> +
>>> +    if (intel_uc_uses_guc(uc))
>>> +        uc->ops = &uc_ops_on;
>>> +    else if (intel_uc_supports_guc(uc))
>>> +        uc->ops = &uc_ops_off;
>>> +    else
>>> +        uc->ops = &uc_ops_none;
>>>   }
>>>     void intel_uc_driver_late_release(struct intel_uc *uc)
>>> @@ -413,24 +424,36 @@ static bool uc_is_wopcm_locked(struct intel_uc 
>>> *uc)
>>>              (intel_uncore_read(uncore, DMA_GUC_WOPCM_OFFSET) & 
>>> GUC_WOPCM_OFFSET_VALID);
>>>   }
>>>   -int intel_uc_init_hw(struct intel_uc *uc)
>>> +static int __uc_check_hw(struct intel_uc *uc)
>>> +{
>>> +    GEM_BUG_ON(!intel_uc_supports_guc(uc));
>>> +
>>> +    /*
>>> +     * We can silently continue without GuC only if it was never 
>>> enabled
>>> +     * before on this system after reboot, otherwise we risk GPU hangs.
>>> +     * To check if GuC was loaded before we look at WOPCM registers.
>>> +     */
>>> +    if (uc_is_wopcm_locked(uc))
>>> +        return -EIO;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int __uc_init_hw(struct intel_uc *uc)
>>>   {
>>>       struct drm_i915_private *i915 = uc_to_gt(uc)->i915;
>>>       struct intel_guc *guc = &uc->guc;
>>>       struct intel_huc *huc = &uc->huc;
>>>       int ret, attempts;
>>>   -    if (!intel_uc_supports_guc(uc))
>>> -        return 0;
>>> +    GEM_BUG_ON(!intel_uc_supports_guc(uc));
>>> +    GEM_BUG_ON(!intel_uc_uses_guc(uc));
>>>         /*
>>>        * We can silently continue without GuC only if it was never 
>>> enabled
>>>        * before on this system after reboot, otherwise we risk GPU 
>>> hangs.
>>>        * To check if GuC was loaded before we look at WOPCM registers.
>>>        */
>>> -    if (!intel_uc_uses_guc(uc) && !uc_is_wopcm_locked(uc))
>>> -        return 0;
>>> -
>>>       if (!intel_uc_fw_is_available(&guc->fw)) {
>>>           ret = uc_is_wopcm_locked(uc) ||
>>>                 intel_uc_fw_is_overridden(&guc->fw) ||
>>> @@ -528,7 +551,7 @@ int intel_uc_init_hw(struct intel_uc *uc)
>>>       return -EIO;
>>>   }
>>>   -void intel_uc_fini_hw(struct intel_uc *uc)
>>> +static void __uc_fini_hw(struct intel_uc *uc)
>>>   {
>>>       struct intel_guc *guc = &uc->guc;
>>>   @@ -628,3 +651,15 @@ int intel_uc_runtime_resume(struct intel_uc *uc)
>>>        */
>>>       return __uc_resume(uc, true);
>>>   }
>>> +
>>> +const struct intel_uc_ops uc_ops_none = {
>>> +};
>>> +
>>> +const struct intel_uc_ops uc_ops_off = {
>>> +    .init_hw = __uc_check_hw,
>>> +};
>>> +
>>
>> I'm not sold on having both uc_ops_off and uc_ops_none and I'd prefer 
>> them to be merged into one.
>> AFAICS, the only things you  do in uc_ops_off are __intel_uc_reset_hw 
>> and __uc_check_hw. __intel_uc_reset_hw shouldn't be needed, we do a GT 
>> reset when we come up and we're not touching the microcontrollers if 
>> intel_uc_uses_guc is false, so there should be no need to reset them. 
>> for __uc_check_hw, we can add an early return if 
>> !intel_uc_supports_guc so it is callable on all platforms and add it 
>> to uc_ops_none.
> 
> I can drop uc_reset_hw from ops.off, but I would keep both off|none
> separate as otherwise we are blurring the idea of having dedicated
> ops without runtime checks (next step of such "merging/simplification"
> approach would be temptation to add more checks into existing ops,
> rather then preparing dedicated one). Note that we may soon add
> ops to other components and we should be consistent on usage model.

Having a dedicated set of ops just to avoid 1 runtime check seems 
overkill in the other direction to me. AFAICS We only care about the 
difference between !supported and !enabled is the init flow because the 
HW can go in a weird state if we switch from GuC enabled to disabled, so 
I doubt similar checks could proliferate since the 2 cases should be 
identical in all other paths. Or do you have a more concrete example of 
another place where the difference matters?

Daniele

> 
> Michal
> 
>>
>> Daniele
>>
>>> +const struct intel_uc_ops uc_ops_on = {
>>> +    .init_hw = __uc_init_hw,
>>> +    .fini_hw = __uc_fini_hw,
>>> +};
>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.h 
>>> b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
>>> index 527995c21196..36643e17a09e 100644
>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h
>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
>>> @@ -10,7 +10,15 @@
>>>   #include "intel_huc.h"
>>>   #include "i915_params.h"
>>>   +struct intel_uc;
>>> +
>>> +struct intel_uc_ops {
>>> +    int (*init_hw)(struct intel_uc *uc);
>>> +    void (*fini_hw)(struct intel_uc *uc);
>>> +};
>>> +
>>>   struct intel_uc {
>>> +    struct intel_uc_ops const *ops;
>>>       struct intel_guc guc;
>>>       struct intel_huc huc;
>>>   @@ -25,8 +33,6 @@ void intel_uc_fetch_firmwares(struct intel_uc *uc);
>>>   void intel_uc_cleanup_firmwares(struct intel_uc *uc);
>>>   void intel_uc_sanitize(struct intel_uc *uc);
>>>   void intel_uc_init(struct intel_uc *uc);
>>> -int intel_uc_init_hw(struct intel_uc *uc);
>>> -void intel_uc_fini_hw(struct intel_uc *uc);
>>>   void intel_uc_fini(struct intel_uc *uc);
>>>   void intel_uc_reset_prepare(struct intel_uc *uc);
>>>   void intel_uc_suspend(struct intel_uc *uc);
>>> @@ -64,4 +70,17 @@ static inline bool intel_uc_uses_huc(struct 
>>> intel_uc *uc)
>>>       return intel_huc_is_enabled(&uc->huc);
>>>   }
>>>   +static inline int intel_uc_init_hw(struct intel_uc *uc)
>>> +{
>>> +    if (uc->ops->init_hw)
>>> +        return uc->ops->init_hw(uc);
>>> +    return 0;
>>> +}
>>> +
>>> +static inline void intel_uc_fini_hw(struct intel_uc *uc)
>>> +{
>>> +    if (uc->ops->fini_hw)
>>> +        uc->ops->fini_hw(uc);
>>> +}
>>> +
>>>   #endif
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx