Re: [RFC PATCH 2/7] drm/i915: Read hw state into an atomic state struct, try 2.

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Jun 23, 2015 at 12:49:00PM +0200, Maarten Lankhorst wrote:
> Op 22-06-15 om 17:31 schreef Daniel Vetter:
> > On Fri, Jun 19, 2015 at 10:02:25AM +0200, Maarten Lankhorst wrote:
> >>  /* Scan out the current hw modeset state, sanitizes it and maps it into the drm
> >> @@ -15491,37 +15569,61 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
> >>  				  bool force_restore)
> >>  {
> >>  	struct drm_i915_private *dev_priv = dev->dev_private;
> >> -	enum pipe pipe;
> >> -	struct intel_crtc *crtc;
> >> +	struct drm_crtc *crtc;
> >> +	struct drm_crtc_state *crtc_state;
> >>  	struct intel_encoder *encoder;
> >> +	struct drm_atomic_state *state;
> >> +	struct intel_shared_dpll_config shared_dplls[I915_NUM_PLLS];
> >>  	int i;
> >>  
> >> -	intel_modeset_readout_hw_state(dev);
> >> -
> >> -	/*
> >> -	 * Now that we have the config, copy it to each CRTC struct
> >> -	 * Note that this could go away if we move to using crtc_config
> >> -	 * checking everywhere.
> >> -	 */
> >> -	for_each_intel_crtc(dev, crtc) {
> >> -		if (crtc->active && i915.fastboot) {
> >> -			intel_mode_from_pipe_config(&crtc->base.mode,
> >> -						    crtc->config);
> >> -			DRM_DEBUG_KMS("[CRTC:%d] found active mode: ",
> >> -				      crtc->base.base.id);
> >> -			drm_mode_debug_printmodeline(&crtc->base.mode);
> >> -		}
> >> +	state = intel_modeset_readout_hw_state(dev);
> >> +	if (IS_ERR(state)) {
> >> +		DRM_ERROR("Failed to read out hw state\n");
> >> +		return;
> >>  	}
> >>  
> >> +	drm_atomic_helper_swap_state(dev, state);
> >> +
> >> +	/* swap sw/hw dpll state */
> >> +	intel_atomic_duplicate_dpll_state(dev_priv, shared_dplls);
> >> +	intel_shared_dpll_commit(state);
> >> +	memcpy(to_intel_atomic_state(state)->shared_dpll,
> >> +	       shared_dplls, sizeof(*shared_dplls) * dev_priv->num_shared_dpll);
> > This is imo way too much open-coding of stuff that's already there. What I
> > had in mind for force_restore was a generic helper to duplicate all the
> > state objects into a drm_atomic_state, without calling any of the
> > ->atomic_check functions or anything. Then the sequence for restoring
> > state would be.
> >
> > 1. saved_state = drm_atomic_helper_duplicate_state()
> > 2. intel_readout_hw_sate() <- this would just update ->state and sanitize
> > it.
> > 3. drm_atomic_commit(saved_state) wrapped in a WARN_ON to make sure it
> > always works.
> Well, for !force_restore we want to do an initial sanitizing modeset too, but with the full disabled state as reference point.
> I'm not sure dupe state followed by a swap, is different from save old state, and read out to current state.

You also dupe internal state like dplls, that's what a helper-level
duplicat state would avoid. And imo that's cleaner. ->atomic_check should
then grab all the internal states needed, if any are necessary. The goal
really is to avoid another special case with too much knowledge of i915
internals, which atomic really should allow us to do.
-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