| Message ID | 20170715114006.6380-3-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@linux.intel.com> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > 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@linux.intel.com>
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, */ + 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; + /* * Flips in the rings will be nuked by the reset, * so complete all pending flips so that user space @@ -3544,22 +3529,10 @@ void intel_finish_reset(struct drm_i915_private *dev_priv) /* reset doesn't touch the display */ if (!gpu_reset_clobbers_display(dev_priv)) { - if (!state) { - /* - * Flips in the rings have been nuked by the reset, - * so update the base address of all primary - * planes to the the last fb to make sure we're - * showing the correct fb after a reset. - * - * FIXME: Atomic will make this obsolete since we won't schedule - * CS-based flips (which might get lost in gpu resets) any more. - */ - intel_update_primary_planes(dev); - } else { - ret = __intel_display_resume(dev, state, ctx); + /* for testing only restore the display */ + ret = __intel_display_resume(dev, state, ctx); if (ret) DRM_ERROR("Restoring old state failed with %i\n", ret); - } } else { /* * The display has been reset as well,
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@linux.intel.com> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 53 +++++++++--------------------------- 1 file changed, 13 insertions(+), 40 deletions(-)