From patchwork Fri Jul 29 11:18:51 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dave Gordon X-Patchwork-Id: 9252443 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 2C34160757 for ; Fri, 29 Jul 2016 11:19:19 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 1A45B27E66 for ; Fri, 29 Jul 2016 11:19:19 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 0EAC227F95; Fri, 29 Jul 2016 11:19:19 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.2 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_MED autolearn=ham version=3.3.1 Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 34AC127E66 for ; Fri, 29 Jul 2016 11:19:18 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id AB15D6E990; Fri, 29 Jul 2016 11:19:14 +0000 (UTC) X-Original-To: intel-gfx@lists.freedesktop.org Delivered-To: intel-gfx@lists.freedesktop.org Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by gabe.freedesktop.org (Postfix) with ESMTP id 608796E2A8 for ; Fri, 29 Jul 2016 11:19:11 +0000 (UTC) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga101.fm.intel.com with ESMTP; 29 Jul 2016 04:18:53 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.28,439,1464678000"; d="scan'208,223";a="1026195581" Received: from dsgordon-linux2.isw.intel.com (HELO [10.102.226.88]) ([10.102.226.88]) by orsmga002.jf.intel.com with ESMTP; 29 Jul 2016 04:18:51 -0700 To: Peter Antoine , intel-gfx@lists.freedesktop.org References: <1467815071-35665-1-git-send-email-peter.antoine@intel.com> From: Dave Gordon Organization: Intel Corporation (UK) Ltd. - Co. Reg. #1134945 - Pipers Way, Swindon SN3 1RJ Message-ID: <82d2e605-87a9-ec3c-358b-206255f496d9@intel.com> Date: Fri, 29 Jul 2016 12:18:51 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <1467815071-35665-1-git-send-email-peter.antoine@intel.com> Subject: Re: [Intel-gfx] [PATCH v3 1/6] drm/i915/guc: Make the GuC fw loading helper functions general X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" X-Virus-Scanned: ClamAV using ClamSMTP 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 > Signed-off-by: Peter Antoine > Reviewed-by: Dave Gordon > --- > 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. From 8a2e98098ff48ed1a9abec3159e630b3a8c18f64 Mon Sep 17 00:00:00 2001 From: Dave Gordon 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 --- 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