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