diff mbox series

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

Message ID 3af6452fb882a17279018c1f1516545634136139.1740481927.git.jani.nikula@intel.com (mailing list archive)
State New
Headers show
Series drm/i915: display reset cleanups | expand

Commit Message

Jani Nikula Feb. 25, 2025, 11:14 a.m. UTC 
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@intel.com>
Signed-off-by: Jani Nikula <jani.nikula@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(-)

Comments

Matt Roper Feb. 25, 2025, 8:35 p.m. UTC | #1 
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@intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@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.

> -	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 Feb. 26, 2025, 10:38 a.m. UTC | #2 
On Tue, 25 Feb 2025, Matt Roper <matthew.d.roper@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@intel.com>
>> Signed-off-by: Jani Nikula <jani.nikula@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?

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
>>
Matt Roper Feb. 26, 2025, 11:46 p.m. UTC | #3 
On Wed, Feb 26, 2025 at 12:38:40PM +0200, Jani Nikula wrote:
> On Tue, 25 Feb 2025, Matt Roper <matthew.d.roper@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@intel.com>
> >> Signed-off-by: Jani Nikula <jani.nikula@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
diff mbox series

Patch

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. */
-	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. */
+	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,