From: Dave Gordon <david.s.gordon@intel.com>
To: Peter Antoine <peter.antoine@intel.com>, intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v3 1/6] drm/i915/guc: Make the GuC fw loading helper functions general
Date: Fri, 29 Jul 2016 12:18:51 +0100 [thread overview]
Message-ID: <82d2e605-87a9-ec3c-358b-206255f496d9@intel.com> (raw)
In-Reply-To: <1467815071-35665-1-git-send-email-peter.antoine@intel.com>
[-- Attachment #1: Type: text/plain, Size: 1574 bytes --]
On 06/07/16 15:24, Peter Antoine wrote:
> Rename some of the GuC fw loading code to make them more general. We
> will utilise them for HuC loading as well.
> s/intel_guc_fw/intel_uc_fw/g
> s/GUC_FIRMWARE/UC_FIRMWARE/g
>
> Struct intel_guc_fw is renamed to intel_uc_fw. Prefix of tts members,
> such as 'guc' or 'guc_fw' either is renamed to 'uc' or removed for
> same purpose.
>
> v2: rebased on top of nightly.
> reapplied the search/replace as upstream code as changed.
> v3: rebased again on drm-nightly.
>
> Signed-off-by: Alex Dai <yu.dai@intel.com>
> Signed-off-by: Peter Antoine <peter.antoine@intel.com>
> Reviewed-by: Dave Gordon <david.s.gordon@intel.com>
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 12 +--
> drivers/gpu/drm/i915/i915_guc_submission.c | 4 +-
> drivers/gpu/drm/i915/intel_guc.h | 39 ++++----
> drivers/gpu/drm/i915/intel_guc_loader.c | 146 ++++++++++++++---------------
> 4 files changed, 101 insertions(+), 100 deletions(-)
As of yesterday, the odd-numbered patches 1 & 3 no longer apply cleanly
and will need rebasing (again).
Also (as of last week's Tech Forum) any series containing more than a
single patch should preferably have a cover letter that at least gives a
summary of the patchset as a whole.
However the main problem with this patch it not what it changes, as what
it fails to change. As I previously suggested (and provided code for)
you need to change all the messages so they don't say "GuC" when we're
actually dealing with the HuC. Updated fixup-patch attached ...
.Dave.
[-- Attachment #2: v3-0007-Tweaks-to-GuC-HuC-loader-code.patch --]
[-- Type: text/x-patch, Size: 9537 bytes --]
>From 8a2e98098ff48ed1a9abec3159e630b3a8c18f64 Mon Sep 17 00:00:00 2001
From: Dave Gordon <david.s.gordon@intel.com>
Date: Tue, 28 Jun 2016 13:09:54 +0100
Subject: [PATCH v3] Tweaks to GuC/HuC loader code
Organization: Intel Corporation (UK) Ltd. - Co. Reg. #1134945 - Pipers Way, Swindon SN3 1RJ
1. Add uc_name to intel_uc_fw structure, so we can use the appropriate
name ("GuC" or "HuC") everywhere (obviously, it should match the fw_type
field).
2. Change all messages in intel_uc_fw_fetch() to use the uc_name.
3. Validate fw_type at the beginning to intel_uc_fw_fetch() rather than
midway. Is there a firmware type in the CSS header -- if so we should
use that and the relevant definitions.
4. Refactor printing the firmware fetch/load status into a macro.
Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
---
drivers/gpu/drm/i915/intel_guc.h | 3 +-
drivers/gpu/drm/i915/intel_guc_loader.c | 84 +++++++++++++++++++++------------
drivers/gpu/drm/i915/intel_huc_loader.c | 3 +-
3 files changed, 57 insertions(+), 33 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index c7b2745..c04aef8 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -106,9 +106,10 @@ enum intel_uc_fw_status {
*/
struct intel_uc_fw {
struct drm_device *uc_dev;
+ const char *uc_name;
const char *uc_fw_path;
- size_t uc_fw_size;
struct drm_i915_gem_object *uc_fw_obj;
+ size_t uc_fw_size;
enum intel_uc_fw_status fetch_status;
enum intel_uc_fw_status load_status;
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index c08b81a..b8c0df7 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -68,6 +68,15 @@
#define I915_KBL_GUC_UCODE "i915/kbl_guc_ver9_14.bin"
MODULE_FIRMWARE(I915_KBL_GUC_UCODE);
+#define DEBUG_UC_FW_STATUS(uc_fw) \
+ { \
+ DRM_DEBUG_DRIVER("%s fw status: path %s, fetch %s, load %s\n", \
+ uc_fw->uc_name, \
+ uc_fw->uc_fw_path, \
+ intel_uc_fw_status_repr(uc_fw->fetch_status), \
+ intel_uc_fw_status_repr(uc_fw->load_status)); \
+ }
+
/* User-friendly representation of an enum */
const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status)
{
@@ -156,7 +165,7 @@ static u32 get_core_family(struct drm_i915_private *dev_priv)
return GFXCORE_FAMILY_GEN9;
default:
- DRM_ERROR("GUC: unsupported core family\n");
+ DRM_ERROR("GuC: unsupported core family\n");
return GFXCORE_FAMILY_UNKNOWN;
}
}
@@ -421,10 +430,7 @@ int intel_guc_setup(struct drm_device *dev)
const char *fw_path = guc_fw->uc_fw_path;
int retries, ret, err;
- DRM_DEBUG_DRIVER("GuC fw status: path %s, fetch %s, load %s\n",
- fw_path,
- intel_uc_fw_status_repr(guc_fw->fetch_status),
- intel_uc_fw_status_repr(guc_fw->load_status));
+ DEBUG_UC_FW_STATUS(guc_fw);
/* Loading forbidden, or no firmware to load? */
if (!i915.enable_guc_loading) {
@@ -454,9 +460,7 @@ int intel_guc_setup(struct drm_device *dev)
guc_fw->load_status = UC_FIRMWARE_PENDING;
- DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n",
- intel_uc_fw_status_repr(guc_fw->fetch_status),
- intel_uc_fw_status_repr(guc_fw->load_status));
+ DEBUG_UC_FW_STATUS(guc_fw);
err = i915_guc_submission_init(dev_priv);
if (err)
@@ -493,9 +497,7 @@ int intel_guc_setup(struct drm_device *dev)
guc_fw->load_status = UC_FIRMWARE_SUCCESS;
- DRM_DEBUG_DRIVER("GuC fw status: fetch %s, load %s\n",
- intel_uc_fw_status_repr(guc_fw->fetch_status),
- intel_uc_fw_status_repr(guc_fw->load_status));
+ DEBUG_UC_FW_STATUS(guc_fw);
intel_huc_auth(dev);
@@ -563,8 +565,20 @@ void intel_uc_fw_fetch(struct drm_device *dev, struct intel_uc_fw *uc_fw)
size_t size;
int err;
- DRM_DEBUG_DRIVER("before requesting firmware: GuC fw fetch status %s\n",
- intel_uc_fw_status_repr(uc_fw->fetch_status));
+ /* Validate fw_type first of all, so we can name the uC */
+ switch (uc_fw->fw_type) {
+ case UC_FW_TYPE_GUC:
+ case UC_FW_TYPE_HUC:
+ break;
+
+ default:
+ DRM_ERROR("Unknown firmware type %d\n", uc_fw->fw_type);
+ err = -ENOEXEC;
+ goto fail;
+ }
+
+ DRM_DEBUG_DRIVER("%s fw fetch status %s before requesting firmware\n",
+ uc_fw->uc_name, intel_uc_fw_status_repr(uc_fw->fetch_status));
err = request_firmware(&fw, uc_fw->uc_fw_path, &dev->pdev->dev);
if (err)
@@ -572,12 +586,12 @@ void intel_uc_fw_fetch(struct drm_device *dev, struct intel_uc_fw *uc_fw)
if (!fw)
goto fail;
- DRM_DEBUG_DRIVER("fetch GuC fw from %s succeeded, fw %p\n",
- uc_fw->uc_fw_path, fw);
+ DRM_DEBUG_DRIVER("%s fw fetch from %s succeeded, fw %p\n",
+ uc_fw->uc_name, uc_fw->uc_fw_path, fw);
/* Check the size of the blob before examining buffer contents */
if (fw->size < sizeof(struct uc_css_header)) {
- DRM_ERROR("Firmware header is missing\n");
+ DRM_ERROR("%s firmware header is missing\n", uc_fw->uc_name);
goto fail;
}
@@ -589,7 +603,8 @@ void intel_uc_fw_fetch(struct drm_device *dev, struct intel_uc_fw *uc_fw)
css->key_size_dw - css->exponent_size_dw) * sizeof(u32);
if (uc_fw->header_size != sizeof(struct uc_css_header)) {
- DRM_ERROR("CSS header definition mismatch\n");
+ DRM_ERROR("%s firmware CSS header definition mismatch\n",
+ uc_fw->uc_name);
goto fail;
}
@@ -599,7 +614,7 @@ void intel_uc_fw_fetch(struct drm_device *dev, struct intel_uc_fw *uc_fw)
/* now RSA */
if (css->key_size_dw != UOS_RSA_SCRATCH_MAX_COUNT) {
- DRM_ERROR("RSA key size is bad\n");
+ DRM_ERROR("%s firmware RSA key size is bad\n", uc_fw->uc_name);
goto fail;
}
uc_fw->rsa_offset = uc_fw->ucode_offset + uc_fw->ucode_size;
@@ -608,7 +623,7 @@ void intel_uc_fw_fetch(struct drm_device *dev, struct intel_uc_fw *uc_fw)
/* At least, it should have header, uCode and RSA. Size of all three. */
size = uc_fw->header_size + uc_fw->ucode_size + uc_fw->rsa_size;
if (fw->size < size) {
- DRM_ERROR("Missing firmware components\n");
+ DRM_ERROR("%s firmware missing components\n", uc_fw->uc_name);
goto fail;
}
@@ -625,35 +640,40 @@ void intel_uc_fw_fetch(struct drm_device *dev, struct intel_uc_fw *uc_fw)
/* Top 32k of WOPCM is reserved (8K stack + 24k RC6 context). */
if (size > guc_wopcm_size(to_i915(dev))) {
- DRM_ERROR("Firmware is too large to fit in WOPCM\n");
+ DRM_ERROR("%s firmware is too large to fit in WOPCM\n",
+ uc_fw->uc_name);
goto fail;
}
uc_fw->major_ver_found = css->guc_sw_version >> 16;
uc_fw->minor_ver_found = css->guc_sw_version & 0xFFFF;
break;
+
case UC_FW_TYPE_HUC:
uc_fw->major_ver_found = css->huc_sw_version >> 16;
uc_fw->minor_ver_found = css->huc_sw_version & 0xFFFF;
break;
+
default:
- DRM_ERROR("Unknown firmware type %d\n", uc_fw->fw_type);
+ /*NOTREACHED*/
err = -ENOEXEC;
goto fail;
}
if (uc_fw->major_ver_found != uc_fw->major_ver_wanted ||
uc_fw->minor_ver_found < uc_fw->minor_ver_wanted) {
- DRM_ERROR("GuC firmware version %d.%d, required %d.%d\n",
+ DRM_ERROR("Found %s firmware version %d.%d, required version %d.%d\n",
+ uc_fw->uc_name,
uc_fw->major_ver_found, uc_fw->minor_ver_found,
uc_fw->major_ver_wanted, uc_fw->minor_ver_wanted);
err = -ENOEXEC;
goto fail;
}
- DRM_DEBUG_DRIVER("firmware version %d.%d OK (minimum %d.%d)\n",
- uc_fw->major_ver_found, uc_fw->minor_ver_found,
- uc_fw->major_ver_wanted, uc_fw->minor_ver_wanted);
+ DRM_DEBUG_DRIVER("%s firmware version %d.%d OK (minimum %d.%d)\n",
+ uc_fw->uc_name,
+ uc_fw->major_ver_found, uc_fw->minor_ver_found,
+ uc_fw->major_ver_wanted, uc_fw->minor_ver_wanted);
mutex_lock(&dev->struct_mutex);
obj = i915_gem_object_create_from_data(dev, fw->data, fw->size);
@@ -666,18 +686,18 @@ void intel_uc_fw_fetch(struct drm_device *dev, struct intel_uc_fw *uc_fw)
uc_fw->uc_fw_obj = obj;
uc_fw->uc_fw_size = fw->size;
- DRM_DEBUG_DRIVER("GuC fw fetch status SUCCESS, obj %p\n",
- uc_fw->uc_fw_obj);
+ DRM_DEBUG_DRIVER("%s fw fetch status SUCCESS, obj %p\n",
+ uc_fw->uc_name, uc_fw->uc_fw_obj);
release_firmware(fw);
uc_fw->fetch_status = UC_FIRMWARE_SUCCESS;
return;
fail:
- DRM_DEBUG_DRIVER("GuC fw fetch status FAIL; err %d, fw %p, obj %p\n",
- err, fw, uc_fw->uc_fw_obj);
- DRM_ERROR("Failed to fetch GuC firmware from %s (error %d)\n",
- uc_fw->uc_fw_path, err);
+ DRM_DEBUG_DRIVER("%s fw fetch status FAIL; err %d, fw %p, obj %p\n",
+ uc_fw->uc_name, err, fw, uc_fw->uc_fw_obj);
+ DRM_ERROR("Failed to fetch %s firmware from %s (error %d)\n",
+ uc_fw->uc_name, uc_fw->uc_fw_path, err);
mutex_lock(&dev->struct_mutex);
obj = uc_fw->uc_fw_obj;
@@ -730,6 +750,8 @@ void intel_guc_init(struct drm_device *dev)
}
guc_fw->uc_dev = dev;
+ guc_fw->uc_name = "GuC";
+ guc_fw->fw_type = UC_FW_TYPE_GUC;
guc_fw->uc_fw_path = fw_path;
guc_fw->fetch_status = UC_FIRMWARE_NONE;
guc_fw->load_status = UC_FIRMWARE_NONE;
diff --git a/drivers/gpu/drm/i915/intel_huc_loader.c b/drivers/gpu/drm/i915/intel_huc_loader.c
index c6d53b3..2ae0542 100644
--- a/drivers/gpu/drm/i915/intel_huc_loader.c
+++ b/drivers/gpu/drm/i915/intel_huc_loader.c
@@ -148,10 +148,11 @@ void intel_huc_init(struct drm_device *dev)
const char *fw_path = NULL;
huc_fw->uc_dev = dev;
+ huc_fw->uc_name = "HuC";
+ huc_fw->fw_type = UC_FW_TYPE_HUC;
huc_fw->uc_fw_path = NULL;
huc_fw->fetch_status = UC_FIRMWARE_NONE;
huc_fw->load_status = UC_FIRMWARE_NONE;
- huc_fw->fw_type = UC_FW_TYPE_HUC;
if (!HAS_HUC_UCODE(dev_priv))
return;
--
1.9.1
[-- Attachment #3: Type: text/plain, Size: 160 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2016-07-29 11:19 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-07-06 14:24 [PATCH v3 1/6] drm/i915/guc: Make the GuC fw loading helper functions general Peter Antoine
2016-07-06 14:24 ` [PATCH v3 2/6] drm/i915/huc: Unified css_header struct for GuC and HuC Peter Antoine
2016-07-06 20:41 ` Vivi, Rodrigo
2016-07-07 10:09 ` Peter Antoine
2016-07-06 14:24 ` [PATCH v3 3/6] drm/i915/huc: Add HuC fw loading support Peter Antoine
2016-07-29 11:29 ` Dave Gordon
2016-07-29 12:35 ` Dave Gordon
2016-07-06 14:24 ` [PATCH v3 4/6] drm/i915/huc: Add debugfs for HuC loading status check Peter Antoine
2016-07-06 14:24 ` [PATCH v3 5/6] drm/i915/huc: Support HuC authentication Peter Antoine
2016-07-29 11:33 ` Dave Gordon
2016-07-29 12:39 ` Dave Gordon
2016-07-06 14:24 ` [PATCH v3 6/6] drm/i915/huc: Add BXT HuC Loading Support Peter Antoine
2016-07-06 20:52 ` Vivi, Rodrigo
2016-07-13 8:02 ` Xiang, Haihao
2016-07-06 14:53 ` ✗ Ro.CI.BAT: warning for series starting with [v3,1/6] drm/i915/guc: Make the GuC fw loading helper functions general Patchwork
2016-07-29 11:18 ` Dave Gordon [this message]
2016-08-02 8:27 ` [PATCH v3 1/6] " Antoine, Peter
2016-07-29 11:20 ` ✗ Ro.CI.BAT: failure for series starting with [v3,1/6] drm/i915/guc: Make the GuC fw loading helper functions general (rev2) Patchwork
2016-08-11 10:49 ` [PATCH v3 1/6] drm/i915/guc: Make the GuC fw loading helper functions general Dave Gordon
2016-08-11 10:54 ` Dave Gordon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=82d2e605-87a9-ec3c-358b-206255f496d9@intel.com \
--to=david.s.gordon@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=peter.antoine@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.