[Intel-gfx] [PATCH 11/11] drm/i915: Don't modeset with old mode when set_crtc fails

[Ander Conselvan De Oliveira]

On Fri, 2015-04-10 at 12:12 +0200, Daniel Vetter wrote:
> On Fri, Apr 10, 2015 at 11:38:40AM +0300, Ander Conselvan de Oliveira wrote:
> > The modeset code is now properly divided in two phases, so that it only
> > changes hardware state if it succeeds, so there's no ill-effect that
> > needs to be undone on failure anymore.
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 48 ------------------------------------
> >  1 file changed, 48 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 06f3b83..84efd7a 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -12136,33 +12136,9 @@ intel_modeset_stage_output_state(struct drm_device *dev,
> >  	return 0;
> >  }
> >  
> > -static void disable_crtc_nofb(struct intel_crtc *crtc)
> > -{
> > -	struct drm_device *dev = crtc->base.dev;
> > -	struct intel_encoder *encoder;
> > -	struct intel_connector *connector;
> > -
> > -	DRM_DEBUG_KMS("Trying to restore without FB -> disabling pipe %c\n",
> > -		      pipe_name(crtc->pipe));
> > -
> > -	for_each_intel_connector(dev, connector) {
> > -		if (connector->new_encoder &&
> > -		    connector->new_encoder->new_crtc == crtc)
> > -			connector->new_encoder = NULL;
> > -	}
> > -
> > -	for_each_intel_encoder(dev, encoder) {
> > -		if (encoder->new_crtc == crtc)
> > -			encoder->new_crtc = NULL;
> > -	}
> > -
> > -	crtc->new_enabled = false;
> > -}
> > -
> >  static int intel_crtc_set_config(struct drm_mode_set *set)
> >  {
> >  	struct drm_device *dev;
> > -	struct drm_mode_set save_set;
> >  	struct drm_atomic_state *state = NULL;
> >  	struct intel_set_config *config;
> >  	struct intel_crtc_state *pipe_config;
> > @@ -12195,12 +12171,6 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
> >  	if (ret)
> >  		goto out_config;
> >  
> > -	save_set.crtc = set->crtc;
> > -	save_set.mode = &set->crtc->mode;
> > -	save_set.x = set->crtc->x;
> > -	save_set.y = set->crtc->y;
> > -	save_set.fb = set->crtc->primary->fb;
> > -
> >  	state = drm_atomic_state_alloc(dev);
> >  	if (!state) {
> >  		ret = -ENOMEM;
> > @@ -12280,24 +12250,6 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
> >  			      set->crtc->base.id, ret);
> >  fail:
> >  		intel_set_config_restore_state(dev, config);
> > -
> > -		drm_atomic_state_clear(state);
> > -
> > -		/*
> > -		 * HACK: if the pipe was on, but we didn't have a framebuffer,
> > -		 * force the pipe off to avoid oopsing in the modeset code
> > -		 * due to fb==NULL. This should only happen during boot since
> > -		 * we don't yet reconstruct the FB from the hardware state.
> > -		 */
> > -		if (to_intel_crtc(save_set.crtc)->new_enabled && !save_set.fb)
> > -			disable_crtc_nofb(to_intel_crtc(save_set.crtc));
> > -
> > -		/* Try to restore the config */
> > -		if (config->mode_changed &&
> > -		    intel_set_mode(save_set.crtc, save_set.mode,
> > -				   save_set.x, save_set.y, save_set.fb,
> > -				   state))
> > -			DRM_ERROR("failed to restore config after modeset failure\n");
> 
> Hm removing this reminds me that we still have troubles with always
> assuming that there is a primary fb around. Getting rid of all the fb
> pointers in our modeset code looks like a sizeable task too.

I only see the fb argument being used in two places now: the part where
we update the primary plane in __intel_set_mode() and in
compute_baseline_pipe_bpp() called from intel_modeset_pipe_config(). I
think those are easy to solve by adding the plane state. Or are we
talking about something else here?

Ander