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 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 68160C43603 for ; Thu, 12 Dec 2019 21:53:47 +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 41B22214D8 for ; Thu, 12 Dec 2019 21:53:47 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 41B22214D8 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 DC55F6E20F; Thu, 12 Dec 2019 21:53:46 +0000 (UTC) Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by gabe.freedesktop.org (Postfix) with ESMTPS id 7B3DF6E20F for ; Thu, 12 Dec 2019 21:53:45 +0000 (UTC) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 12 Dec 2019 13:53:45 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.69,307,1571727600"; d="scan'208";a="226074762" Received: from irvmail001.ir.intel.com ([163.33.26.43]) by orsmga002.jf.intel.com with ESMTP; 12 Dec 2019 13:53:43 -0800 Received: from mwajdecz-mobl1.ger.corp.intel.com (mwajdecz-mobl1.ger.corp.intel.com [10.249.150.184]) by irvmail001.ir.intel.com (8.14.3/8.13.6/MailSET/Hub) with ESMTP id xBCLrgB7016198; Thu, 12 Dec 2019 21:53:42 GMT To: intel-gfx@lists.freedesktop.org, "Daniele Ceraolo Spurio" References: <20191210204744.65276-1-michal.wajdeczko@intel.com> <20191210204744.65276-2-michal.wajdeczko@intel.com> <0623f63e-372d-1047-896c-379e428ec6da@intel.com> Date: Thu, 12 Dec 2019 22:53:41 +0100 MIME-Version: 1.0 From: "Michal Wajdeczko" Message-ID: In-Reply-To: <0623f63e-372d-1047-896c-379e428ec6da@intel.com> User-Agent: Opera Mail/1.0 (Win32) Subject: Re: [Intel-gfx] [RFC 1/4] drm/i915/uc: Add ops to intel_uc 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 Thu, 12 Dec 2019 01:23:33 +0100, Daniele Ceraolo Spurio wrote: > > > On 12/10/19 12:47 PM, Michal Wajdeczko wrote: >> Instead of spreading multiple conditionals across the uC code >> to find out current mode of uC operation, start using predefined >> set of function pointers that reflect that mode. >> Begin with pair of init_hw/fini_hw functions that are responsible >> for uC hardware initialization and cleanup. >> Signed-off-by: Michal Wajdeczko >> Cc: Joonas Lahtinen >> Cc: Chris Wilson >> Cc: Daniele Ceraolo Spurio >> --- >> drivers/gpu/drm/i915/gt/uc/intel_uc.c | 49 +++++++++++++++++++++++---- >> drivers/gpu/drm/i915/gt/uc/intel_uc.h | 23 +++++++++++-- >> 2 files changed, 63 insertions(+), 9 deletions(-) >> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c >> b/drivers/gpu/drm/i915/gt/uc/intel_uc.c >> index c6519066a0f6..e3d1359f9719 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c >> @@ -12,6 +12,10 @@ >> #include "i915_drv.h" >> +extern const struct intel_uc_ops uc_ops_none; >> +extern const struct intel_uc_ops uc_ops_off; >> +extern const struct intel_uc_ops uc_ops_on; >> + >> /* Reset GuC providing us with fresh state for both GuC and HuC. >> */ >> static int __intel_uc_reset_hw(struct intel_uc *uc) >> @@ -89,6 +93,13 @@ void intel_uc_init_early(struct intel_uc *uc) >> intel_huc_init_early(&uc->huc); >> __confirm_options(uc); >> + >> + if (intel_uc_uses_guc(uc)) >> + uc->ops = &uc_ops_on; >> + else if (intel_uc_supports_guc(uc)) >> + uc->ops = &uc_ops_off; >> + else >> + uc->ops = &uc_ops_none; >> } >> void intel_uc_driver_late_release(struct intel_uc *uc) >> @@ -413,24 +424,36 @@ static bool uc_is_wopcm_locked(struct intel_uc >> *uc) >> (intel_uncore_read(uncore, DMA_GUC_WOPCM_OFFSET) & >> GUC_WOPCM_OFFSET_VALID); >> } >> -int intel_uc_init_hw(struct intel_uc *uc) >> +static int __uc_check_hw(struct intel_uc *uc) >> +{ >> + GEM_BUG_ON(!intel_uc_supports_guc(uc)); >> + >> + /* >> + * We can silently continue without GuC only if it was never enabled >> + * before on this system after reboot, otherwise we risk GPU hangs. >> + * To check if GuC was loaded before we look at WOPCM registers. >> + */ >> + if (uc_is_wopcm_locked(uc)) >> + return -EIO; >> + >> + return 0; >> +} >> + >> +static int __uc_init_hw(struct intel_uc *uc) >> { >> struct drm_i915_private *i915 = uc_to_gt(uc)->i915; >> struct intel_guc *guc = &uc->guc; >> struct intel_huc *huc = &uc->huc; >> int ret, attempts; >> - if (!intel_uc_supports_guc(uc)) >> - return 0; >> + GEM_BUG_ON(!intel_uc_supports_guc(uc)); >> + GEM_BUG_ON(!intel_uc_uses_guc(uc)); >> /* >> * We can silently continue without GuC only if it was never enabled >> * before on this system after reboot, otherwise we risk GPU hangs. >> * To check if GuC was loaded before we look at WOPCM registers. >> */ >> - if (!intel_uc_uses_guc(uc) && !uc_is_wopcm_locked(uc)) >> - return 0; >> - >> if (!intel_uc_fw_is_available(&guc->fw)) { >> ret = uc_is_wopcm_locked(uc) || >> intel_uc_fw_is_overridden(&guc->fw) || >> @@ -528,7 +551,7 @@ int intel_uc_init_hw(struct intel_uc *uc) >> return -EIO; >> } >> -void intel_uc_fini_hw(struct intel_uc *uc) >> +static void __uc_fini_hw(struct intel_uc *uc) >> { >> struct intel_guc *guc = &uc->guc; >> @@ -628,3 +651,15 @@ int intel_uc_runtime_resume(struct intel_uc *uc) >> */ >> return __uc_resume(uc, true); >> } >> + >> +const struct intel_uc_ops uc_ops_none = { >> +}; >> + >> +const struct intel_uc_ops uc_ops_off = { >> + .init_hw = __uc_check_hw, >> +}; >> + > > I'm not sold on having both uc_ops_off and uc_ops_none and I'd prefer > them to be merged into one. > AFAICS, the only things you do in uc_ops_off are __intel_uc_reset_hw > and __uc_check_hw. __intel_uc_reset_hw shouldn't be needed, we do a GT > reset when we come up and we're not touching the microcontrollers if > intel_uc_uses_guc is false, so there should be no need to reset them. > for __uc_check_hw, we can add an early return if !intel_uc_supports_guc > so it is callable on all platforms and add it to uc_ops_none. I can drop uc_reset_hw from ops.off, but I would keep both off|none separate as otherwise we are blurring the idea of having dedicated ops without runtime checks (next step of such "merging/simplification" approach would be temptation to add more checks into existing ops, rather then preparing dedicated one). Note that we may soon add ops to other components and we should be consistent on usage model. Michal > > Daniele > >> +const struct intel_uc_ops uc_ops_on = { >> + .init_hw = __uc_init_hw, >> + .fini_hw = __uc_fini_hw, >> +}; >> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.h >> b/drivers/gpu/drm/i915/gt/uc/intel_uc.h >> index 527995c21196..36643e17a09e 100644 >> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h >> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h >> @@ -10,7 +10,15 @@ >> #include "intel_huc.h" >> #include "i915_params.h" >> +struct intel_uc; >> + >> +struct intel_uc_ops { >> + int (*init_hw)(struct intel_uc *uc); >> + void (*fini_hw)(struct intel_uc *uc); >> +}; >> + >> struct intel_uc { >> + struct intel_uc_ops const *ops; >> struct intel_guc guc; >> struct intel_huc huc; >> @@ -25,8 +33,6 @@ void intel_uc_fetch_firmwares(struct intel_uc *uc); >> void intel_uc_cleanup_firmwares(struct intel_uc *uc); >> void intel_uc_sanitize(struct intel_uc *uc); >> void intel_uc_init(struct intel_uc *uc); >> -int intel_uc_init_hw(struct intel_uc *uc); >> -void intel_uc_fini_hw(struct intel_uc *uc); >> void intel_uc_fini(struct intel_uc *uc); >> void intel_uc_reset_prepare(struct intel_uc *uc); >> void intel_uc_suspend(struct intel_uc *uc); >> @@ -64,4 +70,17 @@ static inline bool intel_uc_uses_huc(struct intel_uc >> *uc) >> return intel_huc_is_enabled(&uc->huc); >> } >> +static inline int intel_uc_init_hw(struct intel_uc *uc) >> +{ >> + if (uc->ops->init_hw) >> + return uc->ops->init_hw(uc); >> + return 0; >> +} >> + >> +static inline void intel_uc_fini_hw(struct intel_uc *uc) >> +{ >> + if (uc->ops->fini_hw) >> + uc->ops->fini_hw(uc); >> +} >> + >> #endif _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx