Re: [PATCH 03/12] drm/i915: Clean up AUX power domain handling

Imre Deak
2015-11-11T18:24:15Z

On ma, 2015-11-09 at 16:48 +0100, Patrik Jakobsson wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Introduce intel_display_port_aux_power_domain() which simply returns
> the appropriate AUX power domain for a specific port, and then
> replace
> the intel_display_port_power_domain() with calls to the new function
> in the DP code. As long as we're not actually enabling the port we
> don't
> need the lane power domains, and those are handled now purely from
> modeset_update_crtc_power_domains().
>
> My initial motivation for this was to see if I could keep the DPIO
> power
> wells powered down while doing AUX on CHV, but turns out I can't so
> this
> doesn't change anything for CHV at least. But I think it's still a
> worthwile change.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reviewed-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
> ---
> drivers/gpu/drm/i915/intel_display.c | 40
> ++++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/intel_dp.c | 48 +++++++++++---------------
> ----------
> drivers/gpu/drm/i915/intel_drv.h | 2 ++
> 3 files changed, 56 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index d0fec07..c2578d9 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5143,6 +5143,23 @@ static enum intel_display_power_domain
> port_to_power_domain(enum port port)
> }
> }
>
> +static enum intel_display_power_domain port_to_aux_power_domain(enum
> port port)
> +{
> + switch (port) {
> + case PORT_A:
> + return POWER_DOMAIN_AUX_A;
> + case PORT_B:
> + return POWER_DOMAIN_AUX_B;
> + case PORT_C:
> + return POWER_DOMAIN_AUX_C;
> + case PORT_D:
> + return POWER_DOMAIN_AUX_D;
> + default:
> + WARN_ON_ONCE(1);
> + return POWER_DOMAIN_AUX_A;
> + }
> +}

Looks like port E is missing. I chatted with Ville he has some idea to
fix this.

> +
> #define for_each_power_domain(domain, mask)
> \
> for ((domain) = 0; (domain) < POWER_DOMAIN_NUM; (domain)++)
> \
> if ((1 << (domain)) & (mask))
> @@ -5174,6 +5191,29 @@ intel_display_port_power_domain(struct
> intel_encoder *intel_encoder)
> }
> }
>
> +enum intel_display_power_domain
> +intel_display_port_aux_power_domain(struct intel_encoder
> *intel_encoder)
> +{
> + struct drm_device *dev = intel_encoder->base.dev;
> + struct intel_digital_port *intel_dig_port;
> +
> + switch (intel_encoder->type) {
> + case INTEL_OUTPUT_UNKNOWN:
> + /* Only DDI platforms should ever use this output
> type */
> + WARN_ON_ONCE(!HAS_DDI(dev));
> + case INTEL_OUTPUT_DISPLAYPORT:
> + case INTEL_OUTPUT_EDP:
> + intel_dig_port = enc_to_dig_port(&intel_encoder
> ->base);
> + return port_to_aux_power_domain(intel_dig_port
> ->port);
> + case INTEL_OUTPUT_DP_MST:
> + intel_dig_port = enc_to_mst(&intel_encoder->base)
> ->primary;
> + return port_to_aux_power_domain(intel_dig_port
> ->port);
> + default:
> + WARN_ON_ONCE(1);
> + return POWER_DOMAIN_AUX_A;
> + }
> +}
> +
> static unsigned long get_crtc_power_domains(struct drm_crtc *crtc)
> {
> struct drm_device *dev = crtc->dev;
> diff --git a/drivers/gpu/drm/i915/intel_dp.c
> b/drivers/gpu/drm/i915/intel_dp.c
> index 4655af0..3978540 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -277,7 +277,7 @@ static void pps_lock(struct intel_dp *intel_dp)
> * See vlv_power_sequencer_reset() why we need
> * a power domain reference here.
> */
> - power_domain = intel_display_port_power_domain(encoder);
> + power_domain = intel_display_port_aux_power_domain(encoder);
> intel_display_power_get(dev_priv, power_domain);
>
> mutex_lock(&dev_priv->pps_mutex);
> @@ -293,7 +293,7 @@ static void pps_unlock(struct intel_dp *intel_dp)
>
> mutex_unlock(&dev_priv->pps_mutex);
>
> - power_domain = intel_display_port_power_domain(encoder);
> + power_domain = intel_display_port_aux_power_domain(encoder);
> intel_display_power_put(dev_priv, power_domain);
> }
>
> @@ -816,8 +816,6 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
>
> intel_dp_check_edp(intel_dp);
>
> - intel_aux_display_runtime_get(dev_priv);
> -
> /* Try to wait for any previous AUX channel activity */
> for (try = 0; try < 3; try++) {
> status = I915_READ_NOTRACE(ch_ctl);
> @@ -926,7 +924,6 @@ done:
> ret = recv_bytes;
> out:
> pm_qos_update_request(&dev_priv->pm_qos,
> PM_QOS_DEFAULT_VALUE);
> - intel_aux_display_runtime_put(dev_priv);
>
> if (vdd)
> edp_panel_vdd_off(intel_dp, false);
> @@ -1784,7 +1781,7 @@ static bool edp_panel_vdd_on(struct intel_dp
> *intel_dp)
> if (edp_have_panel_vdd(intel_dp))
> return need_to_disable;
>
> - power_domain =
> intel_display_port_power_domain(intel_encoder);
> + power_domain =
> intel_display_port_aux_power_domain(intel_encoder);
> intel_display_power_get(dev_priv, power_domain);
>
> DRM_DEBUG_KMS("Turning eDP port %c VDD on\n",
> @@ -1874,7 +1871,7 @@ static void edp_panel_vdd_off_sync(struct
> intel_dp *intel_dp)
> if ((pp & POWER_TARGET_ON) == 0)
> intel_dp->last_power_cycle = jiffies;
>
> - power_domain =
> intel_display_port_power_domain(intel_encoder);
> + power_domain =
> intel_display_port_aux_power_domain(intel_encoder);
> intel_display_power_put(dev_priv, power_domain);
> }
>
> @@ -2025,7 +2022,7 @@ static void edp_panel_off(struct intel_dp
> *intel_dp)
> wait_panel_off(intel_dp);
>
> /* We got a reference when we enabled the VDD. */
> - power_domain =
> intel_display_port_power_domain(intel_encoder);
> + power_domain =
> intel_display_port_aux_power_domain(intel_encoder);
> intel_display_power_put(dev_priv, power_domain);
> }
>
> @@ -4765,26 +4762,6 @@ intel_dp_unset_edid(struct intel_dp *intel_dp)
> intel_dp->has_audio = false;
> }
>
> -static enum intel_display_power_domain
> -intel_dp_power_get(struct intel_dp *dp)
> -{
> - struct intel_encoder *encoder = &dp_to_dig_port(dp)->base;
> - enum intel_display_power_domain power_domain;
> -
> - power_domain = intel_display_port_power_domain(encoder);
> - intel_display_power_get(to_i915(encoder->base.dev),
> power_domain);
> -
> - return power_domain;
> -}
> -
> -static void
> -intel_dp_power_put(struct intel_dp *dp,
> - enum intel_display_power_domain power_domain)
> -{
> - struct intel_encoder *encoder = &dp_to_dig_port(dp)->base;
> - intel_display_power_put(to_i915(encoder->base.dev),
> power_domain);
> -}
> -
> static enum drm_connector_status
> intel_dp_detect(struct drm_connector *connector, bool force)
> {
> @@ -4808,7 +4785,8 @@ intel_dp_detect(struct drm_connector
> *connector, bool force)
> return connector_status_disconnected;
> }
>
> - power_domain = intel_dp_power_get(intel_dp);
> + power_domain =
> intel_display_port_aux_power_domain(intel_encoder);
> + intel_display_power_get(to_i915(dev), power_domain);
>
> /* Can't disconnect eDP, but you can close the lid... */
> if (is_edp(intel_dp))
> @@ -4853,7 +4831,7 @@ intel_dp_detect(struct drm_connector
> *connector, bool force)
> }
>
> out:
> - intel_dp_power_put(intel_dp, power_domain);
> + intel_display_power_put(to_i915(dev), power_domain);
> return status;
> }
>
> @@ -4862,6 +4840,7 @@ intel_dp_force(struct drm_connector *connector)
> {
> struct intel_dp *intel_dp = intel_attached_dp(connector);
> struct intel_encoder *intel_encoder =
> &dp_to_dig_port(intel_dp)->base;
> + struct drm_i915_private *dev_priv = to_i915(intel_encoder
> ->base.dev);
> enum intel_display_power_domain power_domain;
>
> DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> @@ -4871,11 +4850,12 @@ intel_dp_force(struct drm_connector
> *connector)
> if (connector->status != connector_status_connected)
> return;
>
> - power_domain = intel_dp_power_get(intel_dp);
> + power_domain =
> intel_display_port_aux_power_domain(intel_encoder);
> + intel_display_power_get(dev_priv, power_domain);
>
> intel_dp_set_edid(intel_dp);
>
> - intel_dp_power_put(intel_dp, power_domain);
> + intel_display_power_put(dev_priv, power_domain);
>
> if (intel_encoder->type != INTEL_OUTPUT_EDP)
> intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
> @@ -5091,7 +5071,7 @@ static void intel_edp_panel_vdd_sanitize(struct
> intel_dp *intel_dp)
> * indefinitely.
> */
> DRM_DEBUG_KMS("VDD left on by BIOS, adjusting state
> tracking\n");
> - power_domain =
> intel_display_port_power_domain(&intel_dig_port->base);
> + power_domain =
> intel_display_port_aux_power_domain(&intel_dig_port->base);
> intel_display_power_get(dev_priv, power_domain);
>
> edp_panel_vdd_schedule_off(intel_dp);
> @@ -5172,7 +5152,7 @@ intel_dp_hpd_pulse(struct intel_digital_port
> *intel_dig_port, bool long_hpd)
> port_name(intel_dig_port->port),
> long_hpd ? "long" : "short");
>
> - power_domain =
> intel_display_port_power_domain(intel_encoder);
> + power_domain =
> intel_display_port_aux_power_domain(intel_encoder);
> intel_display_power_get(dev_priv, power_domain);
>
> if (long_hpd) {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index a68b6cd..7d11aa0 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1188,6 +1188,8 @@ void hsw_enable_ips(struct intel_crtc *crtc);
> void hsw_disable_ips(struct intel_crtc *crtc);
> enum intel_display_power_domain
> intel_display_port_power_domain(struct intel_encoder
> *intel_encoder);
> +enum intel_display_power_domain
> +intel_display_port_aux_power_domain(struct intel_encoder
> *intel_encoder);
> void intel_mode_from_pipe_config(struct drm_display_mode *mode,
> struct intel_crtc_state
> *pipe_config);
> void intel_modeset_preclose(struct drm_device *dev, struct drm_file
> *file);