On 04/19/2018 08:45 AM, Michal Wajdeczko wrote:
On Wed, 18 Apr 2018 19:01:31 +0200, Jackie Li <yaodong.li@xxxxxxxxx>
wrote:
After enabled the WOPCM write-once registers locking status checking,
reloading of the i915 module will fail with modparam enable_guc set to 3
(enable GuC and HuC firmware loading) if the module was originally
loaded
with enable_guc set to 1 (only enable GuC firmware loading). This is
because WOPCM registers were updated and locked without considering
the HuC
FW size. Since we need both GuC and HuC FW sizes to determine the final
layout of WOPCM, we should always calculate the WOPCM layout based on
the
actual sizes of the GuC and HuC firmware available for a specific
platform
if we need continue to support enable/disable HuC FW loading dynamically
with enable_guc modparam.
This patch splits uC firmware fetching into two stages. First stage
is to
fetch the firmware image and verify the firmware header. uC firmware
will
be marked as verified and this will make FW info available for following
WOPCM layout calculation. The second stage is to create a GEM object and
copy the FW data into the created GEM object which will only be
available
when GuC/HuC loading is enabled by enable_guc modparam. This will
guarantee
that the WOPCM layout will be always be calculated correctly without
making
any assumptions to the GuC and HuC firmware sizes.
v3:
- Rebase
v4:
- Renamed the new parameter add to intel_uc_fw_fetch (Michal)
Signed-off-by: Jackie Li <yaodong.li@xxxxxxxxx>
Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
Cc: Sagar Arun Kamble <sagar.a.kamble@xxxxxxxxx>
Cc: Michal Winiarski <michal.winiarski@xxxxxxxxx>
Cc: John Spotswood <john.a.spotswood@xxxxxxxxx>
Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
Reviewed-by: John Spotswood <john.a.spotswood@xxxxxxxxx>
---
drivers/gpu/drm/i915/intel_uc.c | 14 ++++----------
drivers/gpu/drm/i915/intel_uc_fw.c | 31 ++++++++++++++++++++-----------
drivers/gpu/drm/i915/intel_uc_fw.h | 7 +++++--
3 files changed, 29 insertions(+), 23 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_uc.c
b/drivers/gpu/drm/i915/intel_uc.c
index 1cffaf7..73b8f6c 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -172,11 +172,8 @@ void intel_uc_init_early(struct drm_i915_private
*i915)
sanitize_options_early(i915);
- if (USES_GUC(i915))
- intel_uc_fw_fetch(i915, &guc->fw);
-
- if (USES_HUC(i915))
- intel_uc_fw_fetch(i915, &huc->fw);
+ intel_uc_fw_fetch(i915, &guc->fw, USES_GUC(i915));
+ intel_uc_fw_fetch(i915, &huc->fw, USES_HUC(i915));
Again: I'm don't think that unconditional fetch of HuC fw is a right
choice.
We should look for other options how to support enable_guc 1-->3
scenario.
This is the real fix I can think of to support such a scenario. I think
the performance
impact here is minimal since it's only one time operation. I will think
about the
use case of HAS_HUC = 1 and only enable guc submission.
But first I suggest we need to define the expected use case (like I
mentioned in my last reply):
how to define "support enable_guc 1->3" (whether we should expect some
system reboot, or
we need guarantee 100% work with no system reboot required)? whether a
system reboot for
FW changes should be treated as code defects, etc.
}
void intel_uc_cleanup_early(struct drm_i915_private *i915)
@@ -184,11 +181,8 @@ void intel_uc_cleanup_early(struct
drm_i915_private *i915)
struct intel_guc *guc = &i915->guc;
struct intel_huc *huc = &i915->huc;
- if (USES_HUC(i915))
- intel_uc_fw_fini(&huc->fw);
-
- if (USES_GUC(i915))
- intel_uc_fw_fini(&guc->fw);
+ intel_uc_fw_fini(&huc->fw);
+ intel_uc_fw_fini(&guc->fw);
guc_free_load_err_log(guc);
}
diff --git a/drivers/gpu/drm/i915/intel_uc_fw.c
b/drivers/gpu/drm/i915/intel_uc_fw.c
index 6e8e0b5..c1fed06 100644
--- a/drivers/gpu/drm/i915/intel_uc_fw.c
+++ b/drivers/gpu/drm/i915/intel_uc_fw.c
@@ -33,11 +33,13 @@
*
* @dev_priv: device private
* @uc_fw: uC firmware
+ * @fetch: whether fetch uC firmware into GEM object or not
*
- * Fetch uC firmware into GEM obj.
+ * Fetch and verify uC firmware and copy firmware data into GEM
object if
+ * @fetch is true.
*/
void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
- struct intel_uc_fw *uc_fw)
+ struct intel_uc_fw *uc_fw, bool fetch)
{
struct pci_dev *pdev = dev_priv->drm.pdev;
struct drm_i915_gem_object *obj;
@@ -154,17 +156,24 @@ void intel_uc_fw_fetch(struct drm_i915_private
*dev_priv,
goto fail;
}
- obj = i915_gem_object_create_from_data(dev_priv, fw->data,
fw->size);
- if (IS_ERR(obj)) {
- err = PTR_ERR(obj);
- DRM_DEBUG_DRIVER("%s fw object_create err=%d\n",
- intel_uc_fw_type_repr(uc_fw->type), err);
- goto fail;
+ uc_fw->size = fw->size;
+ uc_fw->fetch_status = INTEL_UC_FIRMWARE_VERIFIED;
btw, I'm not sure that this new state is needed at all, as you don't
plan to use that fw obj if you only loaded fw blob to read header...
we will have the header info stored along with the intel_guc/intel_fw
anyway.
this state only suggests that valid FW images are available but wasn't
loaded
to the memory.
Regards,
-Jackie
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
