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

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

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.
> Ofc we still need the ww-mutex deadlock avoidance wrapped around this, but
> since atomic_commit will first compute all derived state there's no need
> to duplicate all the i915-internal state too. That should all work like
> with any other normal modeset.
No need to wrap, readout touches everything so lock_all is fine.

~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx