Re: [PATCH 05/11] drm/i915: Extract mode_changed computation out of stage_output_config()

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Apr 10, 2015 at 11:38:34AM +0300, Ander Conselvan de Oliveira wrote:
> This should make the conversion to atomic easier, by splitting the
> initialization of the atomic state from the logic that decides if a
> modeset is needed.
> ---
>  drivers/gpu/drm/i915/intel_display.c | 73 ++++++++++++++++++++----------------
>  1 file changed, 41 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3035a3d..72ceb6d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11958,6 +11958,10 @@ static void
>  intel_set_config_compute_mode_changes(struct drm_mode_set *set,
>  				      struct intel_set_config *config)
>  {
> +	struct drm_device *dev = set->crtc->dev;
> +	struct intel_connector *connector;
> +	struct intel_encoder *encoder;
> +	struct intel_crtc *crtc;
>  
>  	/* We should be able to check here if the fb has the same properties
>  	 * and then just flip_or_move it */
> @@ -12001,6 +12005,36 @@ intel_set_config_compute_mode_changes(struct drm_mode_set *set,
>  		config->mode_changed = true;
>  	}
>  
> +	for_each_intel_connector(dev, connector) {
> +		if (&connector->new_encoder->base == connector->base.encoder)
> +			continue;
> +
> +		config->mode_changed = true;
> +		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] encoder changed, full mode switch\n",
> +			      connector->base.base.id,
> +			      connector->base.name);
> +	}
> +
> +	for_each_intel_encoder(dev, encoder) {
> +		if (&encoder->new_crtc->base == encoder->base.crtc)
> +			continue;
> +
> +		DRM_DEBUG_KMS("[ENCODER:%d:%s] crtc changed, full mode switch\n",
> +			      encoder->base.base.id,
> +			      encoder->base.name);
> +		config->mode_changed = true;
> +	}
> +
> +	for_each_intel_crtc(dev, crtc) {
> +		if (crtc->new_enabled == crtc->base.state->enable)
> +			continue;
> +
> +		DRM_DEBUG_KMS("[CRTC:%d] %sabled, full mode switch\n",
> +			      crtc->base.base.id,
> +			      crtc->new_enabled ? "en" : "dis");
> +		config->mode_changed = true;
> +	}
> +
>  	DRM_DEBUG_KMS("computed changes for [CRTC:%d], mode_changed=%d, fb_changed=%d\n",
>  			set->crtc->base.id, config->mode_changed, config->fb_changed);
>  }

Any chance we could reuse some parts of the logic in drm_atomic_helper.c?
Copypasting that to have something which uses state objects instead of our
new_ pointers feels lame.

> @@ -12008,7 +12042,6 @@ intel_set_config_compute_mode_changes(struct drm_mode_set *set,
>  static int
>  intel_modeset_stage_output_state(struct drm_device *dev,
>  				 struct drm_mode_set *set,
> -				 struct intel_set_config *config,
>  				 struct drm_atomic_state *state)
>  {
>  	struct intel_connector *connector;
> @@ -12044,14 +12077,6 @@ intel_modeset_stage_output_state(struct drm_device *dev,
>  				connector->base.base.id,
>  				connector->base.name);
>  		}
> -
> -
> -		if (&connector->new_encoder->base != connector->base.encoder) {
> -			DRM_DEBUG_KMS("[CONNECTOR:%d:%s] encoder changed, full mode switch\n",
> -				      connector->base.base.id,
> -				      connector->base.name);
> -			config->mode_changed = true;
> -		}
>  	}
>  	/* connector->new_encoder is now updated for all connectors. */
>  
> @@ -12104,15 +12129,6 @@ intel_modeset_stage_output_state(struct drm_device *dev,
>  			encoder->new_crtc = NULL;
>  		else if (num_connectors > 1)
>  			return -EINVAL;
> -
> -		/* Only now check for crtc changes so we don't miss encoders
> -		 * that will be disabled. */
> -		if (&encoder->new_crtc->base != encoder->base.crtc) {
> -			DRM_DEBUG_KMS("[ENCODER:%d:%s] crtc changed, full mode switch\n",
> -				      encoder->base.base.id,
> -				      encoder->base.name);
> -			config->mode_changed = true;
> -		}
>  	}
>  	/* Now we've also updated encoder->new_crtc for all encoders. */
>  	for_each_intel_connector(dev, connector) {
> @@ -12138,13 +12154,6 @@ intel_modeset_stage_output_state(struct drm_device *dev,
>  				break;
>  			}
>  		}
> -
> -		if (crtc->new_enabled != crtc->base.state->enable) {
> -			DRM_DEBUG_KMS("[CRTC:%d] %sabled, full mode switch\n",
> -				      crtc->base.base.id,
> -				      crtc->new_enabled ? "en" : "dis");
> -			config->mode_changed = true;
> -		}
>  	}
>  
>  	return 0;
> @@ -12216,12 +12225,6 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
>  	save_set.y = set->crtc->y;
>  	save_set.fb = set->crtc->primary->fb;
>  
> -	/* Compute whether we need a full modeset, only an fb base update or no
> -	 * change at all. In the future we might also check whether only the
> -	 * mode changed, e.g. for LVDS where we only change the panel fitter in
> -	 * such cases. */
> -	intel_set_config_compute_mode_changes(set, config);
> -
>  	state = drm_atomic_state_alloc(dev);
>  	if (!state) {
>  		ret = -ENOMEM;
> @@ -12230,10 +12233,16 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
>  
>  	state->acquire_ctx = dev->mode_config.acquire_ctx;
>  
> -	ret = intel_modeset_stage_output_state(dev, set, config, state);
> +	ret = intel_modeset_stage_output_state(dev, set, state);
>  	if (ret)
>  		goto fail;
>  
> +	/* Compute whether we need a full modeset, only an fb base update or no
> +	 * change at all. In the future we might also check whether only the
> +	 * mode changed, e.g. for LVDS where we only change the panel fitter in
> +	 * such cases. */
> +	intel_set_config_compute_mode_changes(set, config);

Imo rename this to intel_compute_mode_changes or similar, the set_config
is because of the ->set_config hook.
-Daniel

> +
>  	pipe_config = intel_modeset_compute_config(set->crtc, set->mode,
>  						   set->fb, state,
>  						   &modeset_pipes,
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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