Re: [PATCH v2 6/8] drm/i915/reset: move gt related stuff out of display reset

[Jani Nikula <jani.nikula@xxxxxxxxx>]

On Tue, 25 Feb 2025, Matt Roper <matthew.d.roper@xxxxxxxxx> wrote:
> On Tue, Feb 25, 2025 at 01:14:20PM +0200, Jani Nikula wrote:
>> Move the checks for whether display reset is needed as well as
>> I915_RESET_MODESET flag handling to gt side of things.
>> 
>> Cc: Matt Roper <matthew.d.roper@xxxxxxxxx>
>> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx>
>> ---
>>  .../drm/i915/display/intel_display_reset.c    | 15 --------------
>>  drivers/gpu/drm/i915/gt/intel_reset.c         | 20 +++++++++++++++++++
>>  2 files changed, 20 insertions(+), 15 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/intel_display_reset.c b/drivers/gpu/drm/i915/display/intel_display_reset.c
>> index b7962f90c21c..362436cd280f 100644
>> --- a/drivers/gpu/drm/i915/display/intel_display_reset.c
>> +++ b/drivers/gpu/drm/i915/display/intel_display_reset.c
>> @@ -37,15 +37,6 @@ void intel_display_reset_prepare(struct intel_display *display)
>>  	if (!HAS_DISPLAY(display))
>>  		return;
>>  
>> -	/* reset doesn't touch the display */
>> -	if (!intel_display_reset_test(display) &&
>> -	    !gpu_reset_clobbers_display(display))
>> -		return;
>> -
>> -	/* We have a modeset vs reset deadlock, defensively unbreak it. */
>
> Doesn't this comment more accurately apply to the 'if' condition below
> rather than to the flag updates and wakeup we do before that?  Assuming
> I'm understanding correctly, it seems like the comment should stay here
> and not move to the other file --- saying "We have a ... deadlock" is
> only true if we still have a pending pin after we've done that other
> stuff.  The unbreaking part (by wedging) is still located here too.

I'm... not sure.

Commit d59cf7bb73f3 ("drm/i915/display: Use dma_fence interfaces instead
of i915_sw_fence") seems relevant. We no longer have anyone waiting on
I915_RESET_MODESET, and I think we could probably remove the bit from
reset flags altogether, and handle this locally in
intel_gt_reset_global(). Right?

BR,
Jani.

>
>> -	set_bit(I915_RESET_MODESET, &to_gt(dev_priv)->reset.flags);
>> -	smp_mb__after_atomic();
>> -	wake_up_bit(&to_gt(dev_priv)->reset.flags, I915_RESET_MODESET);
>>  	if (atomic_read(&display->restore.pending_fb_pin)) {
>>  		drm_dbg_kms(display->drm,
>>  			    "Modeset potentially stuck, unbreaking through wedging\n");
>> @@ -99,10 +90,6 @@ void intel_display_reset_finish(struct intel_display *display)
>>  	if (!HAS_DISPLAY(display))
>>  		return;
>>  
>> -	/* reset doesn't touch the display */
>> -	if (!test_bit(I915_RESET_MODESET, &to_gt(i915)->reset.flags))
>> -		return;
>> -
>>  	state = fetch_and_zero(&display->restore.modeset_state);
>>  	if (!state)
>>  		goto unlock;
>> @@ -140,6 +127,4 @@ void intel_display_reset_finish(struct intel_display *display)
>>  	drm_modeset_drop_locks(ctx);
>>  	drm_modeset_acquire_fini(ctx);
>>  	mutex_unlock(&display->drm->mode_config.mutex);
>> -
>> -	clear_bit_unlock(I915_RESET_MODESET, &to_gt(i915)->reset.flags);
>>  }
>> diff --git a/drivers/gpu/drm/i915/gt/intel_reset.c b/drivers/gpu/drm/i915/gt/intel_reset.c
>> index d424ffb43aa7..62590ed91cf2 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_reset.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_reset.c
>> @@ -1400,11 +1400,25 @@ int intel_engine_reset(struct intel_engine_cs *engine, const char *msg)
>>  	return err;
>>  }
>>  
>> +static bool gt_reset_clobbers_display(struct intel_gt *gt)
>> +{
>> +	return intel_gt_gpu_reset_clobbers_display(gt) && intel_has_gpu_reset(gt);
>> +}
>> +
>>  static void display_reset_prepare(struct intel_gt *gt)
>>  {
>>  	struct drm_i915_private *i915 = gt->i915;
>>  	struct intel_display *display = &i915->display;
>>  
>> +	/* reset doesn't touch the display */
>> +	if (!intel_display_reset_test(display) && !gt_reset_clobbers_display(gt))
>> +		return;
>> +
>> +	/* We have a modeset vs reset deadlock, defensively unbreak it. */
>
> As noted above, this seems inaccurate.  We're just doing the stuff
> necessary to check whether we have a deadlock here.
>
>
> Matt
>
>> +	set_bit(I915_RESET_MODESET, &gt->reset.flags);
>> +	smp_mb__after_atomic();
>> +	wake_up_bit(&gt->reset.flags, I915_RESET_MODESET);
>> +
>>  	intel_display_reset_prepare(display);
>>  }
>>  
>> @@ -1413,7 +1427,13 @@ static void display_reset_finish(struct intel_gt *gt)
>>  	struct drm_i915_private *i915 = gt->i915;
>>  	struct intel_display *display = &i915->display;
>>  
>> +	/* reset doesn't touch the display */
>> +	if (!test_bit(I915_RESET_MODESET, &gt->reset.flags))
>> +		return;
>> +
>>  	intel_display_reset_finish(display);
>> +
>> +	clear_bit_unlock(I915_RESET_MODESET, &gt->reset.flags);
>>  }
>>  
>>  static void intel_gt_reset_global(struct intel_gt *gt,
>> -- 
>> 2.39.5
>> 

-- 
Jani Nikula, Intel