Re: [PATCH v2 10/20] drm/i915: Convert suspend/resume to atomic.

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Jul 07, 2015 at 12:33:25PM +0200, Maarten Lankhorst wrote:
> Op 07-07-15 om 11:57 schreef Daniel Vetter:
> > On Tue, Jul 07, 2015 at 09:08:21AM +0200, Maarten Lankhorst wrote:
> >> @@ -10421,10 +10414,6 @@ void intel_release_load_detect_pipe(struct drm_connector *connector,
> >>  		if (IS_ERR(crtc_state))
> >>  			goto fail;
> >>  
> >> -		to_intel_connector(connector)->new_encoder = NULL;
> >> -		intel_encoder->new_crtc = NULL;
> >> -		intel_crtc->new_enabled = false;
> > load_detect changes should be a separate patch. Or the commit message
> > needs to explain why this needs to be one.
> These members no longer exist. ;-) all the new_ stuff was to restore things pre-atomic, the atomic updates are good enough here.
> 
> Making it a separate patch's probably ok.

Yeah I think splitting out the new_* removal would address most of my
concerns here.

[snip]

> >> +	struct intel_connector *conn;
> >> +	struct intel_plane *plane;
> >> +	struct drm_crtc *crtc;
> >> +	int ret;
> >>  
> >> -		/*
> >> -		 * We need to use raw interfaces for restoring state to avoid
> >> -		 * checking (bogus) intermediate states.
> >> -		 */
> >> -		for_each_pipe(dev_priv, pipe) {
> >> -			struct drm_crtc *crtc =
> >> -				dev_priv->pipe_to_crtc_mapping[pipe];
> >> +	if (!state)
> > debug output missing that the state alloc failed. Perhaps just goto fail;
> > since state_free can cope with a NULL state.
> It can only fail because of kmalloc, which prints its own warnings.

Might still be useful just to have unified error reporting - you need to
guess the caller otherwise which would make debug (if this ever happesn)
harder. But really just a bikeshed.

> 
> >> +		return;
> >>  
> >> -			intel_crtc_restore_mode(crtc);
> >> -		}
> >> -	} else {
> >> -		intel_modeset_update_staged_output_state(dev);
> >> +	state->acquire_ctx = dev->mode_config.acquire_ctx;
> >> +
> >> +	/* preserve complete old state, including dpll */
> >> +	intel_atomic_get_shared_dpll_state(state);
> >> +
> >> +	for_each_crtc(dev, crtc) {
> >> +		struct drm_crtc_state *crtc_state =
> >> +			drm_atomic_get_crtc_state(state, crtc);
> >> +
> >> +		ret = PTR_ERR_OR_ZERO(crtc_state);
> >> +		if (ret)
> >> +			goto err;
> >> +
> >> +		/* force a restore */
> >> +		crtc_state->mode_changed = true;
> >> +	}
> >> +
> >> +	for_each_intel_plane(dev, plane) {
> >> +		ret = PTR_ERR_OR_ZERO(drm_atomic_get_plane_state(state, &plane->base));
> >> +		if (ret)
> >> +			goto err;
> >> +	}
> >> +
> >> +	for_each_intel_connector(dev, conn) {
> >> +		ret = PTR_ERR_OR_ZERO(drm_atomic_get_connector_state(state, &conn->base));
> >> +		if (ret)
> >> +			goto err;
> >>  	}
> >>  
> >> -	intel_modeset_check_state(dev);
> >> +	intel_modeset_setup_hw_state(dev);
> >> +
> >> +	i915_redisable_vga(dev);
> > Since we've only badly bruised escape this trap I think this deserves a
> > comment:
> >
> > 	/*
> > 	 * WARNING: We can't do a full atomic modeset including
> > 	 * compute/check phase here since especially encoder
> > 	 * compute_config functions depend upon output detection state.
> > 	 * And that's just not yet available at driver load. Therefore we
> > 	 * must read out the entire relevant hw state (including any
> > 	 * driver internal state) faithfully here and only apply the
> > 	 * commit side.
> > 	 */
> >
> > Hm, makes me think ... should we end up calling just dev->atomic_commit(state) here
> > once atomic modeset is fully working?
> Not for initial hw readout unless you want to call detect in this function for all encoders.. resume's fine probably.

I meant calling dev->mode_config.funcs->atomic_commit(state) directly,
without calling ->atomic_check at all. That should avoid any state
recomputation (otherwise our check/commit split is botched) and hence be
exactly what we need here. I didn't check how close intel_set_mode is
compared our ->atomic_commit implementation after this series (didn't
apply them all). But I think from a semantic pov those two should match.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx