From: Nirmoy Das <nirmoy.das@intel.com>
To: John Harrison <john.c.harrison@intel.com>,
<intel-gfx@lists.freedesktop.org>
Cc: <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH 3/3] drm/i915: Fix gt reset with GuC submission disabled
Date: Fri, 19 Apr 2024 11:46:50 +0200 [thread overview]
Message-ID: <11526f05-7931-413b-904f-57de2c501f2c@intel.com> (raw)
In-Reply-To: <13043328-20e9-4e6f-bf52-8e275af4015f@intel.com>
Hi John,
On 4/19/2024 1:38 AM, John Harrison wrote:
> On 4/18/2024 10:10, Nirmoy Das wrote:
>> Currently intel_gt_reset() happens as follows:
>>
>> reset_prepare() ---> Sends GDRST to GuC, GuC is in GS_MIA_IN_RESET
>> do_reset()
>> intel_gt_reset_all_engines()
>> *_engine_reset_prepare() -->RESET_CTL expects running GuC
> Not technically correct. There is no direct connection between
> RESET_CTL and GuC.
>
>> *_reset_engines()
>> intel_gt_init_hw() --> GuC comes out of GS_MIA_IN_RESET with FW loaded.
>>
>> Fix the issue by sanitizing the GuC only after resetting requested
>> engines and before intel_gt_init_hw().
> You never actually state what the issue is.
>
> The problem is that there is a dedicated CSB FIFO going to GuC (and
> nothing else has access to it). If that FIFO fills up, the hardware
> will block on the next context switch until there is space. If no-one
> (i.e. GuC) is draining it, that means the system is effectively hung.
> If an engine is reset whilst actively executing a context, a CSB entry
> will be sent to say that the context has gone idle. Thus if you reset
> a very busy system and start with killing GuC before killing the
> engines and only then re-enabling GuC, you run the risk of generating
> more CSB entries than will fit in the FIFO and deadlocking. Whereas,
> if the system is idle then you can reset the engines as much as you
> like while GuC is dead and it won't be a problem.
I wasn't sure if I could talk about internal details so kept it to
minimal. I will borrow above explanation and resend :)
>
>>
>> Note intel_uc_reset_finish() and intel_uc_reset() are nop when
>> guc submission is disabled.
>>
>> Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
>> ---
>> drivers/gpu/drm/i915/gt/intel_reset.c | 16 ++++++++++++++--
>> 1 file changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c
>> b/drivers/gpu/drm/i915/gt/intel_reset.c
>> index 6504e8ba9c58..bd166f5aca4b 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_reset.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_reset.c
>> @@ -907,8 +907,17 @@ static intel_engine_mask_t reset_prepare(struct
>> intel_gt *gt)
>> intel_engine_mask_t awake = 0;
>> enum intel_engine_id id;
>> - /* For GuC mode, ensure submission is disabled before stopping
>> ring */
>> - intel_uc_reset_prepare(>->uc);
>> + /**
>> + * For GuC mode with submission enabled, ensure submission
>> + * is disabled before stopping ring.
>> + *
>> + * For GuC mode with submission disabled, ensure that GuC is not
>> + * sanitized, do that at the end in reset_finish(). reset_prepare()
>> + * is followed by engine reset which in this mode requires GuC to
>> + * be functional to process engine reset events.
> -> to process any CSB FIFO entries generated by the resets.
I will add this.
Thanks,
Nirmoy
>
> John.
>
>> + */
>> + if (intel_uc_uses_guc_submission(>->uc))
>> + intel_uc_reset_prepare(>->uc);
>> for_each_engine(engine, gt, id) {
>> if (intel_engine_pm_get_if_awake(engine))
>> @@ -1255,6 +1264,9 @@ void intel_gt_reset(struct intel_gt *gt,
>> intel_overlay_reset(gt->i915);
>> + /* sanitize uC after engine reset */
>> + if (!intel_uc_uses_guc_submission(>->uc))
>> + intel_uc_reset_prepare(>->uc);
>> /*
>> * Next we need to restore the context, but we don't use those
>> * yet either...
>
next prev parent reply other threads:[~2024-04-19 9:47 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-18 17:10 [PATCH 1/3] drm/i915: Refactor confusing __intel_gt_reset() Nirmoy Das
2024-04-18 17:10 ` [PATCH 2/3] drm/i915 Rename intel_engine_reset to intel_gt_engine_recover Nirmoy Das
2024-04-18 23:27 ` John Harrison
2024-04-19 8:48 ` Nirmoy Das
2024-04-18 17:10 ` [PATCH 3/3] drm/i915: Fix gt reset with GuC submission disabled Nirmoy Das
2024-04-18 23:38 ` John Harrison
2024-04-19 9:46 ` Nirmoy Das [this message]
2024-04-18 20:09 ` ✗ Fi.CI.SPARSE: warning for series starting with [1/3] drm/i915: Refactor confusing __intel_gt_reset() Patchwork
2024-04-18 20:16 ` ✗ Fi.CI.BAT: failure " Patchwork
2024-04-18 21:46 ` ✗ Fi.CI.SPARSE: warning for series starting with [1/3] drm/i915: Refactor confusing __intel_gt_reset() (rev2) Patchwork
2024-04-18 21:56 ` ✗ Fi.CI.BAT: failure " Patchwork
2024-04-18 23:27 ` [PATCH 1/3] drm/i915: Refactor confusing __intel_gt_reset() John Harrison
2024-04-19 8:44 ` Nirmoy Das
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=11526f05-7931-413b-904f-57de2c501f2c@intel.com \
--to=nirmoy.das@intel.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=intel-gfx@lists.freedesktop.org \
--cc=john.c.harrison@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 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 read-only IMAP folder(s) and NNTP newsgroup(s).