Re: [PATCH v2 2/2] drm/i915: Fix gt reset with GuC submission is disabled

[Nirmoy Das <nirmoy.das@linux.intel.com>]

Hi Andi,

On 4/23/2024 11:32 AM, Andi Shyti wrote:
> Hi Nirmoy,
>
> On Mon, Apr 22, 2024 at 10:19:51PM +0200, Nirmoy Das wrote:
>> Currently intel_gt_reset() kills the GuC and then resets requested
>> engines. This is problematic because there is a dedicated CSB FIFO
>> which only GuC can access and if that FIFO fills up, the hardware
>> will block on the next context switch until there is space 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 reset happens on a very busy system then
>> killing GuC before killing the engines will lead to deadlock because
>> of filled up CSB FIFO.
> is this a fix?

I went quite far back in the commit logs, and it appears to me that 
we've always been using the current reset flow.

I believe we don't perform a GT reset immediately after sending a number 
of requests, which is what the current failed test is doing.

So, I don't think there will be any visible impact on the user with the 
current flow.

>
>> To address this issue, the GuC should be killed only after resetting
>> the requested engines and before calling intel_gt_init_hw().
>>
>> v2: Improve commit message(John)
>>
>> Cc: John Harrison <john.c.harrison@intel.com>
>> 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 b1393863ca9b..6161f7a3ff70 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_reset.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_reset.c
>> @@ -879,8 +879,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(&gt->uc);
>> +	/**
>> +	 * For GuC mode with submission enabled, ensure submission
>> +	 * is disabled before stopping ring.
> nit: "stopping *the* ring"
Will fix it while merging if I don't have to resend this again.
>
>> +	 *
>> +	 * For GuC mode with submission disabled, ensure that GuC is not
>> +	 * sanitized, do that after engine reset. reset_prepare()
>> +	 * is followed by engine reset which in this mode requires GuC to
>> +	 * process any CSB FIFO entries generated by the resets.
>> +	 */
>> +	if (intel_uc_uses_guc_submission(&gt->uc))
>> +		intel_uc_reset_prepare(&gt->uc);
>>   
>>   	for_each_engine(engine, gt, id) {
>>   		if (intel_engine_pm_get_if_awake(engine))
>> @@ -1227,6 +1236,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(&gt->uc))
>> +		intel_uc_reset_prepare(&gt->uc);
> Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>

Thanks,

Nirmoy

> Thanks,
> Andi