Re: [Intel-gfx] [PATCH 11/15] drm/i915/huc: track delayed HuC load with a fence

2022-07-05 Thread Teres Alexis, Alan Previn
Other than the one nit below, everything looks in order as per internal 
development and reviews...
but really wish we had the nit added - so we have a single location we can 
build on to get all
the various stages of gsc vs pxp vs huc operation sequences across hw gens (at 
least the first 
gen when it changes route).


Reviewed-by: Alan Previn 



diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.h 
b/drivers/gpu/drm/i915/gt/uc/intel_huc.h
> +enum intel_huc_delayed_load_status {
> + INTEL_HUC_WAITING_ON_GSC = 0,
> + INTEL_HUC_WAITING_ON_PXP,
> + INTEL_HUC_DELAYED_LOAD_ERROR,
> +};
> +
>  struct intel_huc {
>   /* Generic uC firmware management */
>   struct intel_uc_fw fw;
> @@ -20,17 +28,28 @@ struct intel_huc {
>   u32 mask;
>   u32 value;
>   } status;
> +
> + struct {
> + struct i915_sw_fence fence;
> + struct hrtimer timer;
> + struct notifier_block nb;
> + enum intel_huc_delayed_load_status status;
> + } delayed_load;
>  };


NIT: I really wish we had a state machine diagram for the huc load status
because of the prior, this and upcoming hw design progression - will be nice to
have a single place to look at the various flows (also for without
delayed-huc-loading case). Similiar to the diagram of what we have above
the definition of enum intel_uc_fw_status.



[Intel-gfx] [PATCH 11/15] drm/i915/huc: track delayed HuC load with a fence

2022-06-09 Thread Daniele Ceraolo Spurio
Given that HuC load is delayed on DG2, this patch adds support for a fence
that can be used to wait for load completion. No waiters are added in this
patch (they're coming up in the next one), to keep the focus of the
patch on the tracking logic.

The full HuC loading flow on boot DG2 is as follows:
1) i915 exports the GSC as an aux device;
2) the mei-gsc driver is loaded on the aux device;
3) the mei-pxp component is loaded;
4) mei-pxp calls back into i915 and we load the HuC.

Between steps 1 and 2 there can be several seconds of gap, mainly due to
the kernel doing other work during the boot.
The resume flow is slightly different, because we don't need to
re-expose or re-probe the aux device, so we go directly to step 3 once
i915 and mei-gsc have completed their resume flow.

Here's an example of the boot timing, captured with some logs added to
i915:

[   17.908307] [drm] adding GSC device
[   17.915717] [drm] i915 probe done
[   22.282917] [drm] mei-gsc bound
[   22.938153] [drm] HuC authenticated

Also to note is that if something goes wrong during GSC HW init the
mei-gsc driver will still bind, but steps 3 and 4 will not happen.

The status tracking is done by registering a bus_notifier to receive a
callback when the mei-gsc driver binds, with a large enough timeout to
account for delays. Once mei-gsc is bound, we switch to a smaller
timeout to wait for the mei-pxp component to load.
The fence is signalled on HuC load complete or if anything goes wrong in
any of the tracking steps. Timeout are enforced via hrtimer callbacks.

Signed-off-by: Daniele Ceraolo Spurio 
---
 drivers/gpu/drm/i915/gt/intel_gsc.c|  22 ++-
 drivers/gpu/drm/i915/gt/uc/intel_huc.c | 198 +
 drivers/gpu/drm/i915/gt/uc/intel_huc.h |  19 +++
 3 files changed, 236 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_gsc.c 
b/drivers/gpu/drm/i915/gt/intel_gsc.c
index 4d87519d5773..2868673e6213 100644
--- a/drivers/gpu/drm/i915/gt/intel_gsc.c
+++ b/drivers/gpu/drm/i915/gt/intel_gsc.c
@@ -154,8 +154,14 @@ static void gsc_destroy_one(struct drm_i915_private *i915,
struct intel_gsc_intf *intf = &gsc->intf[intf_id];
 
if (intf->adev) {
-   auxiliary_device_delete(&intf->adev->aux_dev);
-   auxiliary_device_uninit(&intf->adev->aux_dev);
+   struct auxiliary_device *aux_dev = &intf->adev->aux_dev;
+
+   if (intf_id == 0)
+   
intel_huc_unregister_gsc_notifier(&gsc_to_gt(gsc)->uc.huc,
+ aux_dev->dev.bus);
+
+   auxiliary_device_delete(aux_dev);
+   auxiliary_device_uninit(aux_dev);
intf->adev = NULL;
}
 
@@ -255,14 +261,24 @@ static void gsc_init_one(struct drm_i915_private *i915,
goto fail;
}
 
+   intf->adev = adev; /* needed by the notifier */
+
+   if (intf_id == 0)
+   intel_huc_register_gsc_notifier(&gsc_to_gt(gsc)->uc.huc,
+   aux_dev->dev.bus);
+
ret = auxiliary_device_add(aux_dev);
if (ret < 0) {
drm_err(&i915->drm, "gsc aux add failed %d\n", ret);
+   if (intf_id == 0)
+   
intel_huc_unregister_gsc_notifier(&gsc_to_gt(gsc)->uc.huc,
+ aux_dev->dev.bus);
+   intf->adev = NULL;
+
/* adev will be freed with the put_device() and .release 
sequence */
auxiliary_device_uninit(aux_dev);
goto fail;
}
-   intf->adev = adev;
 
return;
 fail:
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c 
b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
index a57d77c6f818..075ec97b459d 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
@@ -10,6 +10,8 @@
 #include "intel_huc.h"
 #include "i915_drv.h"
 
+#include 
+
 /**
  * DOC: HuC
  *
@@ -42,6 +44,164 @@
  * HuC-specific commands.
  */
 
+/*
+ * MEI-GSC load is an async process. The probing of the exposed aux device
+ * (see intel_gsc.c) usually happens a few seconds after i915 probe, depending
+ * on when the kernel schedules it. Unless something goes terribly wrong, we're
+ * guaranteed for this to happen during boot, so the big timeout is a safety 
net
+ * that we never expect to need.
+ * MEI-PXP + HuC load usually takes ~300ms, but if the GSC needs to be resumed
+ * and/or reset, this can take longer.
+ */
+#define GSC_INIT_TIMEOUT_MS 1
+#define PXP_INIT_TIMEOUT_MS 2000
+
+static int sw_fence_dummy_notify(struct i915_sw_fence *sf,
+enum i915_sw_fence_notify state)
+{
+   return NOTIFY_DONE;
+}
+
+static void __delayed_huc_load_complete(struct intel_huc *huc)
+{
+   if (!i915_sw_fence_done(&huc->delayed_load.fence))
+   i915_sw_fence_complete(&huc->delayed_load.fence);
+}
+
+static void delaye