From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 6D579C3524B for ; Tue, 4 Feb 2020 17:54:17 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 50E5E2084E for ; Tue, 4 Feb 2020 17:54:17 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 50E5E2084E Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=intel.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=intel-gfx-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 0AF2F6E89C; Tue, 4 Feb 2020 17:54:17 +0000 (UTC) Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by gabe.freedesktop.org (Postfix) with ESMTPS id D4B546E89C for ; Tue, 4 Feb 2020 17:54:15 +0000 (UTC) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga006.jf.intel.com ([10.7.209.51]) by fmsmga105.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 04 Feb 2020 09:54:14 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.70,402,1574150400"; d="scan'208";a="235251468" Received: from irvmail001.ir.intel.com ([163.33.26.43]) by orsmga006.jf.intel.com with ESMTP; 04 Feb 2020 09:54:12 -0800 Received: from mwajdecz-mobl1.ger.corp.intel.com (mwajdecz-mobl1.ger.corp.intel.com [172.28.174.138]) by irvmail001.ir.intel.com (8.14.3/8.13.6/MailSET/Hub) with ESMTP id 014HsBlc018810; Tue, 4 Feb 2020 17:54:11 GMT To: intel-gfx@lists.freedesktop.org, "Daniele Ceraolo Spurio" References: <20200203232838.14822-1-daniele.ceraolospurio@intel.com> <20200203232838.14822-7-daniele.ceraolospurio@intel.com> Date: Tue, 04 Feb 2020 18:54:11 +0100 MIME-Version: 1.0 From: "Michal Wajdeczko" Message-ID: In-Reply-To: <20200203232838.14822-7-daniele.ceraolospurio@intel.com> User-Agent: Opera Mail/1.0 (Win32) Subject: Re: [Intel-gfx] [PATCH v2 06/10] drm/i915/uc: Improve tracking of uC init status X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed"; DelSp="yes" Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" On Tue, 04 Feb 2020 00:28:34 +0100, Daniele Ceraolo Spurio wrote: > To be able to setup GuC submission functions during engine init we need > to commit to using GuC as soon as possible. > Currently, the only thing that can stop us from using the > microcontrollers once we've fetched the blobs is a fundamental > error (e.g. OOM); given that if we hit such an error we can't really > fall-back to anything, we can "officialize" the FW fetching completion > as the moment at which we're committing to using GuC. > > To better differentiate this case, the uses_guc check, which indicates > that GuC is supported and was selected in modparam, is renamed to > wants_guc and a new uses_guc is introduced to represent the case were > we're committed to using the GuC. Note that uses_guc does still not imply > that the blob is actually loaded on the HW (is_running is the check for > that). Also, since we need to have attempted the fetch for the result > of uses_guc to be meaningful, we need to make sure we've moved away > from INTEL_UC_FIRMWARE_SELECTED. > > All the GuC changes have been mirrored on the HuC for coherency. > > v2: split fetch return changes and new macros to their own patches, > support HuC only if GuC is wanted, improve "used" state > description (Michal) > > Signed-off-by: Daniele Ceraolo Spurio > Cc: Chris Wilson > Cc: Tvrtko Ursulin > Cc: Michal Wajdeczko > Cc: John Harrison > Cc: Matthew Brost > Reviewed-by: Fernando Pacheco #v1 > --- > drivers/gpu/drm/i915/gt/uc/intel_guc.h | 8 +++++++- > drivers/gpu/drm/i915/gt/uc/intel_huc.h | 8 +++++++- > drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c | 2 +- > drivers/gpu/drm/i915/gt/uc/intel_uc.c | 23 ++++++++++++---------- > drivers/gpu/drm/i915/gt/uc/intel_uc.h | 24 ++++++++++++++++++++++- > 5 files changed, 51 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h > b/drivers/gpu/drm/i915/gt/uc/intel_guc.h > index 7ca9e5159f05..f6b33745ae0b 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h > @@ -143,11 +143,17 @@ static inline bool intel_guc_is_supported(struct > intel_guc *guc) > return intel_uc_fw_is_supported(&guc->fw); > } > -static inline bool intel_guc_is_enabled(struct intel_guc *guc) > +static inline bool intel_guc_is_wanted(struct intel_guc *guc) > { > return intel_uc_fw_is_enabled(&guc->fw); > } > +static inline bool intel_guc_is_used(struct intel_guc *guc) > +{ > + GEM_BUG_ON(__intel_uc_fw_status(&guc->fw) == > INTEL_UC_FIRMWARE_SELECTED); > + return intel_uc_fw_is_available(&guc->fw); > +} > + > static inline bool intel_guc_is_fw_running(struct intel_guc *guc) > { > return intel_uc_fw_is_running(&guc->fw); > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.h > b/drivers/gpu/drm/i915/gt/uc/intel_huc.h > index 644c059fe01d..a40b9cfc6c22 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.h > +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.h > @@ -41,11 +41,17 @@ static inline bool intel_huc_is_supported(struct > intel_huc *huc) > return intel_uc_fw_is_supported(&huc->fw); > } > -static inline bool intel_huc_is_enabled(struct intel_huc *huc) > +static inline bool intel_huc_is_wanted(struct intel_huc *huc) > { > return intel_uc_fw_is_enabled(&huc->fw); > } > +static inline bool intel_huc_is_used(struct intel_huc *huc) > +{ > + GEM_BUG_ON(__intel_uc_fw_status(&huc->fw) == > INTEL_UC_FIRMWARE_SELECTED); > + return intel_uc_fw_is_available(&huc->fw); > +} > + > static inline bool intel_huc_is_authenticated(struct intel_huc *huc) > { > return intel_uc_fw_is_running(&huc->fw); > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c > b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c > index eee193bf2cc4..9cdf4cbe691c 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c > @@ -20,7 +20,7 @@ void intel_huc_fw_init_early(struct intel_huc *huc) > struct drm_i915_private *i915 = gt->i915; > intel_uc_fw_init_early(&huc->fw, INTEL_UC_FW_TYPE_HUC, > - intel_uc_uses_guc(uc), > + intel_uc_wants_guc(uc), > INTEL_INFO(i915)->platform, INTEL_REVID(i915)); > } > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c > b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > index affc4d6f9ead..654d7c0c757a 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c > +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c > @@ -48,17 +48,17 @@ static void __confirm_options(struct intel_uc *uc) > DRM_DEV_DEBUG_DRIVER(i915->drm.dev, > "enable_guc=%d (guc:%s submission:%s huc:%s)\n", > i915_modparams.enable_guc, > - yesno(intel_uc_uses_guc(uc)), > + yesno(intel_uc_wants_guc(uc)), > yesno(intel_uc_uses_guc_submission(uc)), > - yesno(intel_uc_uses_huc(uc))); > + yesno(intel_uc_wants_huc(uc))); > if (i915_modparams.enable_guc == -1) > return; > if (i915_modparams.enable_guc == 0) { > - GEM_BUG_ON(intel_uc_uses_guc(uc)); > + GEM_BUG_ON(intel_uc_wants_guc(uc)); > GEM_BUG_ON(intel_uc_uses_guc_submission(uc)); > - GEM_BUG_ON(intel_uc_uses_huc(uc)); > + GEM_BUG_ON(intel_uc_wants_huc(uc)); > return; > } > @@ -93,7 +93,7 @@ void intel_uc_init_early(struct intel_uc *uc) > __confirm_options(uc); > - if (intel_uc_uses_guc(uc)) > + if (intel_uc_wants_guc(uc)) > uc->ops = &uc_ops_on; > else > uc->ops = &uc_ops_off; > @@ -257,13 +257,13 @@ static void __uc_fetch_firmwares(struct intel_uc > *uc) > { > int err; > - GEM_BUG_ON(!intel_uc_uses_guc(uc)); > + GEM_BUG_ON(!intel_uc_wants_guc(uc)); > err = intel_uc_fw_fetch(&uc->guc.fw); > if (err) > return; > - if (intel_uc_uses_huc(uc)) > + if (intel_uc_wants_huc(uc)) > intel_uc_fw_fetch(&uc->huc.fw); > } > @@ -279,7 +279,10 @@ static void __uc_init(struct intel_uc *uc) > struct intel_huc *huc = &uc->huc; > int ret; > - GEM_BUG_ON(!intel_uc_uses_guc(uc)); > + GEM_BUG_ON(!intel_uc_wants_guc(uc)); > + > + if (!intel_uc_uses_guc(uc)) > + return; > /* XXX: GuC submission is unavailable for now */ > GEM_BUG_ON(intel_uc_supports_guc_submission(uc)); > @@ -322,7 +325,7 @@ static int uc_init_wopcm(struct intel_uc *uc) > struct intel_uncore *uncore = gt->uncore; > u32 base = intel_wopcm_guc_base(>->i915->wopcm); > u32 size = intel_wopcm_guc_size(>->i915->wopcm); > - u32 huc_agent = intel_uc_uses_huc(uc) ? HUC_LOADING_AGENT_GUC : 0; > + u32 huc_agent = intel_uc_wants_huc(uc) ? HUC_LOADING_AGENT_GUC : 0; > u32 mask; > int err; > @@ -402,7 +405,7 @@ static int __uc_init_hw(struct intel_uc *uc) > int ret, attempts; > GEM_BUG_ON(!intel_uc_supports_guc(uc)); > - GEM_BUG_ON(!intel_uc_uses_guc(uc)); > + GEM_BUG_ON(!intel_uc_wants_guc(uc)); > if (!intel_uc_fw_is_available(&guc->fw)) { > ret = __uc_check_hw(uc) || > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.h > b/drivers/gpu/drm/i915/gt/uc/intel_uc.h > index 2ce993ceb60a..a41aaf353f88 100644 > --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h > +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h > @@ -40,6 +40,27 @@ void intel_uc_runtime_suspend(struct intel_uc *uc); > int intel_uc_resume(struct intel_uc *uc); > int intel_uc_runtime_resume(struct intel_uc *uc); > +/* > + * We need to know as early as possible if we're going to use GuC or > not to > + * take the correct setup paths. Additionally, once we've started > loading the > + * GuC, it is unsafe to keep executing without it because some parts of > the HW, > + * a subset of which is not cleaned on GT reset, will start expecting > the GuC FW > + * to be running. > + * To solve both these requirements, we commit to using the > microcontrollers if > + * the relevant modparam is set and the blobs are found on the system. > At this > + * stage, the only thing that can stop us from attempting to load the > blobs on > + * the HW and use them is a fundamental issue (e.g. no memory for our > + * structures); if we hit such a problem during driver load we're > broken even > + * without GuC, so there is no point in trying to fall back. > + * > + * Given the above, we can be in one of 4 states, with the last one > implying > + * we're committed to using the microcontroller: nit: maybe we should document below state names in capitals as: NOT_SUPPORTED, WANTED, IN_USE, ... > + * - Not supported: not available in HW and/or firmware not defined. > + * - Supported: available in HW and firmware defined. > + * - Wanted: supported + enabled in modparam. hmm, we are checking modparam during fw path selection, thus for me there is no difference between SUPPORTED and WANTED, what I missed? maybe we only need 3 states: NOT_SUPPORTED, WANTED, IN_USE. > + * - In use: wanted + firmware found on the system and successfully > fetched. In what state we will be after unsuccessful fetch ? still WANTED ? > + */ > + > #define __uc_state_checker(x, state, required) \ > static inline bool intel_uc_##state##_##x(struct intel_uc *uc) \ > { \ > @@ -48,7 +69,8 @@ static inline bool intel_uc_##state##_##x(struct > intel_uc *uc) \ > #define uc_state_checkers(x) \ > __uc_state_checker(x, supports, supported) \ > -__uc_state_checker(x, uses, enabled) > +__uc_state_checker(x, wants, wanted) \ > +__uc_state_checker(x, uses, used) > uc_state_checkers(guc); > uc_state_checkers(huc); _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx