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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
