[Intel-gfx] [PATCH 07/58] drm/i915/dp: convert to encoder disable/enable
Daniel Vetter
daniel.vetter at ffwll.ch
Tue Sep 4 21:42:04 CEST 2012
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
More information about the Intel-gfx
mailing list