From: John Harrison <John.C.Harrison@Intel.com>
To: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>,
intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH v3 06/10] drm/i915/uc: Improve tracking of uC init status
Date: Thu, 13 Feb 2020 17:23:19 -0800 [thread overview]
Message-ID: <0a52e81d-063c-5819-df40-46847a894838@Intel.com> (raw)
In-Reply-To: <6e4c0794-1a42-a4a8-a620-511ee5b86a10@intel.com>
On 2/13/2020 16:21, Daniele Ceraolo Spurio wrote:
> On 2/13/20 4:04 PM, John Harrison wrote:
>> On 2/13/2020 15:44, Daniele Ceraolo Spurio wrote:
>>> On 2/13/20 3:36 PM, John Harrison wrote:
>>>> On 2/11/2020 16:31, 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)
>>>>>
>>>>> v3: s/wants_huc/uses_huc in uc_init_wopcm
>>>>>
>>>>> Signed-off-by: Daniele Ceraolo Spurio
>>>>> <daniele.ceraolospurio@intel.com>
>>>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>>>>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>>>> Cc: John Harrison <John.C.Harrison@Intel.com>
>>>>> Cc: Matthew Brost <matthew.brost@intel.com>
>>>>> Reviewed-by: Fernando Pacheco <fernando.pacheco@intel.com> #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 | 21 +++++++++++---------
>>>>> drivers/gpu/drm/i915/gt/uc/intel_uc.h | 24
>>>>> ++++++++++++++++++++++-
>>>>> 5 files changed, 50 insertions(+), 13 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
>>>>> b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
>>>>> index 668b067b71e2..b51adaac8752 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..5825a6c3e0a0 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));
>>>>> @@ -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:
>>>>> + * - Not supported: not available in HW and/or firmware not defined.
>>>>> + * - Supported: available in HW and firmware defined.
>>>>> + * - Wanted: supported + enabled in modparam.
>>>>> + * - In use: wanted + firmware found on the system and
>>>>> successfully fetched.
>>>> Should be another line for 'Running'? I guess the definition of
>>>> 'is_running' comes from elsewhere but it would make sense to
>>>> include it in the description here.
>>>
>>> "Running" belongs to a different set of states, indicating the
>>> current state of the uC. The ones added here are about what we plan
>>> to do with the GuC, not what the current state is, so I disagree
>>> that it makes sense to add "running" here.
>>>
>>>>
>>>> Also, it would be good to include the actual function names
>>>> themselves. That way if someone searches on 'intel_uc_uses_guc',
>>>> for example, they will find this description. Especially as
>>>> searching for them by either text or symbol will not find the
>>>> definition!
>>>>
>>>
>>> How do you want them added? We don't usually do that for
>>> autogenerated functions (there is an example of that further down in
>>> this file).
>>>
>>> Daniele
>>>
>> An example of documentation? Or an example of more autogenerated
>> functions? The intel_uc.h currently in the tree has no documentation
>> in it at all! I was just thinking that I personally get frustrated
>> when I try to search for a function definition and can't find
>> anything because it is some fancy autogenerated thing with no
>> documentation at all. If you feel that including the expanded names
>> is too verbose then fine, it's hardly a show stopper issue. At least
>> you have added some documentation about what the various states mean!
>>
>
> I meant to ask how did you want them documented. There isn't a 1:1
> mapping between the function and the states (4 theoretical states, but
> 3 actual recorded states/functions) and the names are subject to
> change as well, which is difficult to mirror in detached
> documentation. And yes, it does also feel a bit too verbose to me :P
Just ship it then.
Reviewed-by: John Harrison <John.C.Harrison@Intel.com>
>
> Daniele
>
>> John.
>>
>>>> With that tweak:
>>>> Reviewed-by: John Harrison <John.C.Harrison@Intel.com>
>>>>
>>>>
>>>>> + */
>>>>> +
>>>>> #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
next prev parent reply other threads:[~2020-02-14 1:23 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-02-12 0:31 [Intel-gfx] [PATCH v3 00/10] Commit early to GuC Daniele Ceraolo Spurio
2020-02-12 0:31 ` [Intel-gfx] [PATCH v3 01/10] drm/i915/debugfs: Pass guc_log struct to i915_guc_log_info Daniele Ceraolo Spurio
2020-02-12 1:16 ` Andi Shyti
2020-02-12 0:31 ` [Intel-gfx] [PATCH v3 02/10] drm/i915/guc: Kill USES_GUC macro Daniele Ceraolo Spurio
2020-02-12 1:16 ` Andi Shyti
2020-02-12 0:31 ` [Intel-gfx] [PATCH v3 03/10] drm/i915/guc: Kill USES_GUC_SUBMISSION macro Daniele Ceraolo Spurio
2020-02-12 1:19 ` Andi Shyti
2020-02-12 0:31 ` [Intel-gfx] [PATCH v3 04/10] drm/i915/uc: Update the FW status on injected fetch error Daniele Ceraolo Spurio
2020-02-12 0:31 ` [Intel-gfx] [PATCH v3 05/10] drm/i915/uc: autogenerate uC checker functions Daniele Ceraolo Spurio
2020-02-12 1:38 ` Andi Shyti
2020-02-12 0:31 ` [Intel-gfx] [PATCH v3 06/10] drm/i915/uc: Improve tracking of uC init status Daniele Ceraolo Spurio
2020-02-13 23:36 ` John Harrison
2020-02-13 23:44 ` Daniele Ceraolo Spurio
2020-02-14 0:04 ` John Harrison
2020-02-14 0:21 ` Daniele Ceraolo Spurio
2020-02-14 1:23 ` John Harrison [this message]
2020-02-12 0:31 ` [Intel-gfx] [PATCH v3 07/10] drm/i915/guc: Apply new uC status tracking to GuC submission as well Daniele Ceraolo Spurio
2020-02-13 23:42 ` John Harrison
2020-02-12 0:31 ` [Intel-gfx] [PATCH v3 08/10] drm/i915/uc: Abort early on uc_init failure Daniele Ceraolo Spurio
2020-02-12 1:47 ` Andi Shyti
2020-02-12 21:47 ` Daniele Ceraolo Spurio
2020-02-12 0:31 ` [Intel-gfx] [PATCH v3 09/10] drm/i915/uc: consolidate firmware cleanup Daniele Ceraolo Spurio
2020-02-13 23:44 ` John Harrison
2020-02-13 23:48 ` Daniele Ceraolo Spurio
2020-02-12 0:31 ` [Intel-gfx] [PATCH v3 10/10] HAX: drm/i915: default to enable_guc=2 Daniele Ceraolo Spurio
2020-02-12 21:29 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for Commit early to GuC (rev3) Patchwork
2020-02-12 21:52 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2020-02-15 16:06 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork
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=0a52e81d-063c-5819-df40-46847a894838@Intel.com \
--to=john.c.harrison@intel.com \
--cc=daniele.ceraolospurio@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).