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

[Matt Roper]

On Wed, Feb 26, 2025 at 12:38:40PM +0200, Jani Nikula wrote:
> On Tue, 25 Feb 2025, Matt Roper <matthew.d.roper at intel.com> 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 at intel.com>
> >> Signed-off-by: Jani Nikula <jani.nikula at intel.com>
> >> ---
> >>  .../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?

Yeah, I believe you're right.


Matt

> 
> 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

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation