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

[Matt Roper <matthew.d.roper@xxxxxxxxx>]

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.

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

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