On Tue, Sep 4, 2012 at 9:33 PM, Jesse Barnes <jbarnes at virtuousgeek.org> wrote: > On Sun, 19 Aug 2012 21:12:24 +0200 > Daniel Vetter <daniel.vetter at ffwll.ch> wrote: > >> -static void intel_dp_prepare(struct drm_encoder *encoder) >> +static void intel_disable_dp(struct intel_encoder *encoder) >> { >> - struct intel_dp *intel_dp = enc_to_intel_dp(encoder); >> - >> + struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); >> >> /* Make sure the panel is off before trying to change the mode. But also >> * ensure that we have vdd while we switch off the panel. */ >> @@ -1262,62 +1261,64 @@ static void intel_dp_prepare(struct drm_encoder *encoder) >> intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON); >> ironlake_edp_panel_off(intel_dp); >> intel_dp_link_down(intel_dp); >> + >> + intel_dp->dpms_mode = DRM_MODE_DPMS_OFF; > > Is this redundant? At init time we'll have cleared this, and at > prepare time it ought to be off already right? If my understanding is correct, we use intel_dp->dpms_mode to decide whether we need to retrain the link. Yes, it's redundant, and a follow-up patch on top of this series will rip it out. But for the conversion I've decided that I'll painstakingly keep any and all funny piece of code in the low-level hw frobbery, just to reduce the risk. Hence I sometimes add "stupid" bits&pieces, just to keep the existing stupid going for a little longer ;-) After all, a patch should do one thing, and one thing only. >> } >> >> -static void intel_dp_commit(struct drm_encoder *encoder) >> +static void intel_enable_dp(struct intel_encoder *encoder) >> { >> - struct intel_dp *intel_dp = enc_to_intel_dp(encoder); >> - struct drm_device *dev = encoder->dev; >> - struct intel_crtc *intel_crtc = to_intel_crtc(intel_dp->base.base.crtc); >> + struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); >> + struct drm_device *dev = encoder->base.dev; >> + struct drm_i915_private *dev_priv = dev->dev_private; >> + uint32_t dp_reg = I915_READ(intel_dp->output_reg); >> >> ironlake_edp_panel_vdd_on(intel_dp); >> intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON); >> - intel_dp_start_link_train(intel_dp); >> - ironlake_edp_panel_on(intel_dp); >> - ironlake_edp_panel_vdd_off(intel_dp, true); >> - intel_dp_complete_link_train(intel_dp); >> + if (!(dp_reg & DP_PORT_EN)) { >> + intel_dp_start_link_train(intel_dp); >> + ironlake_edp_panel_on(intel_dp); >> + ironlake_edp_panel_vdd_off(intel_dp, true); >> + intel_dp_complete_link_train(intel_dp); >> + } else >> + ironlake_edp_panel_vdd_off(intel_dp, false); > > Hm so if we call enable on an already on DP port, we'll just disable > VDD? But shouldn't it already have been off? Same reason, this bogus check will disappear. I /think/ this was to properly cope with take-over from the bios. Again I just painstakingly replicate what's already there (this time from the dpms function). Cheers, Daniel > >> ironlake_edp_backlight_on(intel_dp); >> >> intel_dp->dpms_mode = DRM_MODE_DPMS_ON; >> - >> - if (HAS_PCH_CPT(dev)) >> - intel_cpt_verify_modeset(dev, intel_crtc->pipe); >> } >> >> static void >> -intel_dp_dpms(struct drm_encoder *encoder, int mode) >> +intel_dp_dpms(struct drm_connector *connector, int mode) >> { >> - struct intel_dp *intel_dp = enc_to_intel_dp(encoder); >> - struct drm_device *dev = encoder->dev; >> - struct drm_i915_private *dev_priv = dev->dev_private; >> - uint32_t dp_reg = I915_READ(intel_dp->output_reg); >> + struct intel_dp *intel_dp = intel_attached_dp(connector); >> + >> + /* DP supports only 2 dpms states. */ >> + if (mode != DRM_MODE_DPMS_ON) >> + mode = DRM_MODE_DPMS_OFF; >> + >> + if (mode == connector->dpms) >> + return; >> + >> + connector->dpms = mode; >> + >> + /* Only need to change hw state when actually enabled */ >> + if (!intel_dp->base.base.crtc) { >> + intel_dp->base.connectors_active = false; >> + return; >> + } >> >> if (mode != DRM_MODE_DPMS_ON) { >> - /* Switching the panel off requires vdd. */ >> - ironlake_edp_panel_vdd_on(intel_dp); >> - ironlake_edp_backlight_off(intel_dp); >> - intel_dp_sink_dpms(intel_dp, mode); >> - ironlake_edp_panel_off(intel_dp); >> - intel_dp_link_down(intel_dp); >> + intel_encoder_dpms(&intel_dp->base, mode); >> + WARN_ON(intel_dp->dpms_mode != DRM_MODE_DPMS_OFF); >> >> if (is_cpu_edp(intel_dp)) >> - ironlake_edp_pll_off(encoder); >> + ironlake_edp_pll_off(&intel_dp->base.base); >> } else { >> if (is_cpu_edp(intel_dp)) >> - ironlake_edp_pll_on(encoder); >> + ironlake_edp_pll_on(&intel_dp->base.base); >> >> - ironlake_edp_panel_vdd_on(intel_dp); >> - intel_dp_sink_dpms(intel_dp, mode); >> - if (!(dp_reg & DP_PORT_EN)) { >> - intel_dp_start_link_train(intel_dp); >> - ironlake_edp_panel_on(intel_dp); >> - ironlake_edp_panel_vdd_off(intel_dp, true); >> - intel_dp_complete_link_train(intel_dp); >> - } else >> - ironlake_edp_panel_vdd_off(intel_dp, false); >> - ironlake_edp_backlight_on(intel_dp); >> + intel_encoder_dpms(&intel_dp->base, mode); >> + WARN_ON(intel_dp->dpms_mode != DRM_MODE_DPMS_ON); >> } >> - intel_dp->dpms_mode = mode; >> } >> >> /* >> @@ -2347,15 +2348,15 @@ static void intel_dp_encoder_destroy(struct drm_encoder *encoder) >> } >> >> static const struct drm_encoder_helper_funcs intel_dp_helper_funcs = { >> - .dpms = intel_dp_dpms, >> .mode_fixup = intel_dp_mode_fixup, >> - .prepare = intel_dp_prepare, >> + .prepare = intel_encoder_noop, >> .mode_set = intel_dp_mode_set, >> - .commit = intel_dp_commit, >> + .commit = intel_encoder_noop, >> + .disable = intel_encoder_disable, >> }; >> >> static const struct drm_connector_funcs intel_dp_connector_funcs = { >> - .dpms = drm_helper_connector_dpms, >> + .dpms = intel_dp_dpms, >> .detect = intel_dp_detect, >> .fill_modes = drm_helper_probe_single_connector_modes, >> .set_property = intel_dp_set_property, >> @@ -2486,6 +2487,9 @@ intel_dp_init(struct drm_device *dev, int output_reg, enum port port) >> intel_connector_attach_encoder(intel_connector, intel_encoder); >> drm_sysfs_connector_add(connector); >> >> + intel_encoder->enable = intel_enable_dp; >> + intel_encoder->disable = intel_disable_dp; >> + >> /* Set up the DDC bus. */ >> switch (port) { >> case PORT_A: > > Assuming the above questions are answered: > Reviewed-by: Jesse Barnes <jbarnes at virtuousgeek.org> > > -- > Jesse Barnes, Intel Open Source Technology Center -- Daniel Vetter daniel.vetter at ffwll.ch - +41 (0) 79 365 57 48 - http://blog.ffwll.ch