On Wed, Feb 26, 2025 at 12:38:40PM +0200, Jani Nikula wrote:
> 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?
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, >->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
--
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation
