On Mon, May 09, 2016 at 03:54:15PM +0300, Ville Syrjälä wrote:
> On Mon, May 09, 2016 at 01:04:21PM +0200, Maarten Lankhorst wrote:
> > This function would call drm_modeset_lock_all, while the suspend/resume
> > functions already have their own locking. Fix this by factoring out
> > __intel_display_resume, and calling the atomic helpers for duplicating
> > atomic state and disabling all crtc's during suspend.
> >
> > Changes since v1:
> > - Deal with -EDEADLK right after lock_all and clean up calls
> > to hw readout.
> > - Always take all modeset locks so updates during gpu reset are blocked.
> > Changes since v2:
> > - Fix deadlock in intel_update_primary_planes.
> > - Move WARN_ON(EDEADLK) to __intel_display_resume.
> > - pctx -> ctx
> > - only call __intel_display_resume on success in intel_display_resume.
> >
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> > Fixes: e2c8b8701e2d ("drm/i915: Use atomic helpers for suspend, v2.")
> > Cc: drm-intel-fixes@xxxxxxxxxxxxxxxxxxxxx
> > ---
> > drivers/gpu/drm/i915/intel_display.c | 150 ++++++++++++++++++++++-------------
> > 1 file changed, 93 insertions(+), 57 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 14e05091c397..6faa529f35df 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -3130,41 +3130,96 @@ static void intel_update_primary_planes(struct drm_device *dev)
> >
> > for_each_crtc(dev, crtc) {
> > struct intel_plane *plane = to_intel_plane(crtc->primary);
> > - struct intel_plane_state *plane_state;
> > -
> > - drm_modeset_lock_crtc(crtc, &plane->base);
> > - plane_state = to_intel_plane_state(plane->base.state);
> > + struct intel_plane_state *plane_state =
> > + to_intel_plane_state(plane->base.state);
> >
> > if (plane_state->visible)
> > plane->update_plane(&plane->base,
> > to_intel_crtc_state(crtc->state),
> > plane_state);
> > + }
> > +}
> > +
> > +static int
> > +__intel_display_resume(struct drm_device *dev,
> > + struct drm_atomic_state *state)
> > +{
> > + struct drm_crtc_state *crtc_state;
> > + struct drm_crtc *crtc;
> > + int i, ret;
> > +
> > + intel_modeset_setup_hw_state(dev);
> > + i915_redisable_vga(dev);
> >
> > - drm_modeset_unlock_crtc(crtc);
> > + if (!state)
> > + return 0;
>
> Thank you diff for making things illegible.
>
> I'm still not convinced we should be changing the locking scheme
> here, especially in what's supposed to be just a fix.
>
> In general, I'm not sure we want ->detect() and other irrelevant
> stuff to block the GPU reset. The nice thing about g4x+ reset so
> far has been that it's almost unnoticeable. Of course if it's a
> genuine GPU hang the screen will probably have been frozen for
> quite a while when we initiate the reset, so perhaps that's not
> a huge real world issue.
We don't really need dev->mode_config.mutex for atomic commits, so taking
that out and use drm_modeset_lock_all_ctx would give you that.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
