Op 15-07-17 om 13:40 schreef Daniel Vetter:
> Taking the modeset locks unconditionally isn't the greatest idea,
> because atm that part is still broken and times out (and then atomic
> keels over). And there's really no reason to do so, the old code
> didn't do that either.
>
> To make the patch a bit simpler let's also nuke 2 cases that are only
> around for the old mmioflip paths. Atomic nonblocking workers will not
> die (minus bugs) when a gpu reset happens.
>
> And of course this doesn't fix any of the gpu reset vs. modeset
> deadlock fun, but it at least stop modern CI machines from keeling
> over all over the place for no reason at all.
>
> And we still have the explicit testcases to run the fake gpu reset, so
> coverage isn't that much worse.
>
> v2: Split out additional changes on top, restrict this to purely reducing
> the critical section of modeset locks.
>
> Fixes: 739748939974 ("drm/i915: Fix modeset handling during gpu reset, v5.")
> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/intel_display.c | 53 +++++++++---------------------------
> 1 file changed, 13 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f69333b8995c..e3c55a996f6b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3413,26 +3413,6 @@ static void intel_complete_page_flips(struct drm_i915_private *dev_priv)
> intel_finish_page_flip_cs(dev_priv, crtc->pipe);
> }
>
> -static void intel_update_primary_planes(struct drm_device *dev)
> -{
> - struct drm_crtc *crtc;
> -
> - for_each_crtc(dev, crtc) {
> - struct intel_plane *plane = to_intel_plane(crtc->primary);
> - struct intel_plane_state *plane_state =
> - to_intel_plane_state(plane->base.state);
> -
> - if (plane_state->base.visible) {
> - trace_intel_update_plane(&plane->base,
> - to_intel_crtc(crtc));
> -
> - plane->update_plane(plane,
> - to_intel_crtc_state(crtc->state),
> - plane_state);
> - }
> - }
> -}
> -
> static int
> __intel_display_resume(struct drm_device *dev,
> struct drm_atomic_state *state,
> @@ -3485,6 +3465,12 @@ void intel_prepare_reset(struct drm_i915_private *dev_priv)
> struct drm_atomic_state *state;
> int ret;
>
> +
> + /* reset doesn't touch the display, but flips might get nuked anyway, */
I think this comment was meant to address the reasoning for taking all locks,
so the part about flips being nuked no longer applies with mmio flips gone.
> + if (!i915.force_reset_modeset_test &&
> + !gpu_reset_clobbers_display(dev_priv))
> + return;
> +
> /*
> * Need mode_config.mutex so that we don't
> * trample ongoing ->detect() and whatnot.
> @@ -3498,12 +3484,6 @@ void intel_prepare_reset(struct drm_i915_private *dev_priv)
>
> drm_modeset_backoff(ctx);
> }
> -
> - /* reset doesn't touch the display, but flips might get nuked anyway, */
> - if (!i915.force_reset_modeset_test &&
> - !gpu_reset_clobbers_display(dev_priv))
> - return;
> -
> /*
> * Disabling the crtcs gracefully seems nicer. Also the
> * g33 docs say we should at least disable all the planes.
> @@ -3533,6 +3513,11 @@ void intel_finish_reset(struct drm_i915_private *dev_priv)
> struct drm_atomic_state *state = dev_priv->modeset_restore_state;
> int ret;
>
> + /* reset doesn't touch the display, but flips might get nuked anyway, */
> + if (!i915.force_reset_modeset_test &&
> + !gpu_reset_clobbers_display(dev_priv))
> + return;
Same thing about comment here. Perhaps change the
if (!i915.force_reset_modeset_test && !gpu_reset_clobbers_display())
to
if (!state && !gpu_reset_clobbers_display())
so we don't run into a kernel panic if we change the parameter during reset?
Hm perhaps also either add if (state) to the __intel_display_resume call for <g4x,
or remove the one for drm_atomic_state_put.
With those changes I'm happy, and you can add
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
