[Intel-gfx] [PATCH 07/58] drm/i915/dp: convert to encoder disable/enable

[Jesse Barnes]

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?

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

>  	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