Re: [PATCH v3 1/2] drm/i915: Fix modeset handling during gpu reset, v3.

[Daniel Vetter <daniel@ffwll.ch>]

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@linux.intel.com>
> > Fixes: e2c8b8701e2d ("drm/i915: Use atomic helpers for suspend, v2.")
> > Cc: drm-intel-fixes@lists.freedesktop.org
> > ---
> >  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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx