On Tue, 25 Feb 2025, Matt Roper <matthew.d.ro...@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.ro...@intel.com>
>> Signed-off-by: Jani Nikula <jani.nik...@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, >->reset.flags);
>> + smp_mb__after_atomic();
>> + wake_up_bit(>->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, >->reset.flags))
>> + return;
>> +
>> intel_display_reset_finish(display);
>> +
>> + clear_bit_unlock(I915_RESET_MODESET, >->reset.flags);
>> }
>>
>> static void intel_gt_reset_global(struct intel_gt *gt,
>> --
>> 2.39.5
>>
--
Jani Nikula, Intel
