Re: [Intel-gfx] [PATCH 01/12] drm/i915: Remove bogus GEM_BUG_ON in unpark

From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: John Harrison <john.c.harrison@intel.com>,
	Intel-GFX@Lists.FreeDesktop.Org
Cc: DRI-Devel@Lists.FreeDesktop.Org
Subject: Re: [Intel-gfx] [PATCH 01/12] drm/i915: Remove bogus GEM_BUG_ON in unpark
Date: Tue, 19 Jul 2022 10:42:01 +0100	[thread overview]
Message-ID: <0337c3d4-811e-1f5a-fe26-f0e8068497b8@linux.intel.com> (raw)
In-Reply-To: <c86064c9-cbcc-cdc5-0654-0493c8d3d444@intel.com>

On 19/07/2022 01:05, John Harrison wrote:
> On 7/18/2022 05:15, Tvrtko Ursulin wrote:
>>
>> On 13/07/2022 00:31, John.C.Harrison@Intel.com wrote:
>>> From: Matthew Brost <matthew.brost@intel.com>
>>>
>>> Remove bogus GEM_BUG_ON which compared kernel context timeline seqno to
>>> seqno in memory on engine PM unpark. If a GT reset occurred these values
>>> might not match as a kernel context could be skipped. This bug was
>>> hidden by always switching to a kernel context on park (execlists
>>> requirement).
>>
>> Reset of the kernel context? Under which circumstances does that happen?
> As per description, the issue is with full GT reset.
> 
>>
>> It is unclear if the claim is this to be a general problem or the 
>> assert is only invalid with the GuC. Lack of a CI reported issue 
>> suggests it is not a generic problem?
> Currently it is not an issue because we always switch to the kernel 
> context because that's how execlists works and the entire driver is 
> fundamentally based on execlist operation. When we stop using the kernel 
> context as a (non-functional) barrier when using GuC submission, then 
> you would see an issue without this fix.

Issue is with GuC, GuC and full reset, or with full reset regardless of 
the backend?

If issue is only with GuC patch should have drm/i915/guc prefix as 
minimum. But if it actually only becomes a problem when GuC backend 
stops parking with the kernel context when I think the whole unpark code 
should be refactored in a cleaner way than just removing the one assert. 
Otherwise what is the point of leaving everything else in there?

Or if the issue is backend agnostic, *if* full reset happens to hit 
during parking, then it is different. Wouldn't that be a race with 
parking and reset which probably shouldn't happen to start with.

Regards,

Tvrtko

> 
> John.
> 
> 
>>
>> Regards,
>>
>> Tvrtko
>>
>>> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/gt/intel_engine_pm.c | 2 --
>>>   1 file changed, 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_pm.c 
>>> b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
>>> index b0a4a2dbe3ee9..fb3e1599d04ec 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_engine_pm.c
>>> +++ b/drivers/gpu/drm/i915/gt/intel_engine_pm.c
>>> @@ -68,8 +68,6 @@ static int __engine_unpark(struct intel_wakeref *wf)
>>>                ce->timeline->seqno,
>>>                READ_ONCE(*ce->timeline->hwsp_seqno),
>>>                ce->ring->emit);
>>> -        GEM_BUG_ON(ce->timeline->seqno !=
>>> -               READ_ONCE(*ce->timeline->hwsp_seqno));
>>>       }
>>>         if (engine->unpark)
> 