Re: [PATCH 4/8] drm/i915: Use a helper to get the aux power domain

[Imre Deak <imre.deak@intel.com>]

On Tue, Oct 30, 2018 at 02:16:08PM -0700, Lucas De Marchi wrote:
> On Tue, Oct 30, 2018 at 05:40:47PM +0200, Imre Deak wrote:
> > From ICL onwards the AUX power domain may change dynamically based on
> > whether a DDI/TypeC port is in thunderbolt or non-thunderbolt mode, so
> > use a helper function instead of a static field to get the current
> > domain.
> > 
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_ddi.c     |  2 +-
> >  drivers/gpu/drm/i915/intel_display.c | 22 +++++++++++
> >  drivers/gpu/drm/i915/intel_dp.c      | 73 +++++++++++++++---------------------
> >  drivers/gpu/drm/i915/intel_drv.h     |  3 +-
> >  4 files changed, 56 insertions(+), 44 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index 3739ef003819..5bb459011a49 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -2099,7 +2099,7 @@ intel_ddi_main_link_aux_domain(struct intel_dp *intel_dp)
> >  	 * returns the correct domain for other ports too.
> >  	 */
> >  	return dig_port->aux_ch == AUX_CH_A ? POWER_DOMAIN_AUX_IO_A :
> > -					      intel_dp->aux_power_domain;
> > +					      intel_aux_power_domain(dig_port);
> >  }
> >  
> >  static u64 intel_ddi_get_power_domains(struct intel_encoder *encoder,
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index c3cadc09f859..36710a30fb37 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -5949,6 +5949,28 @@ enum intel_display_power_domain intel_port_to_power_domain(enum port port)
> >  	}
> >  }
> >  
> > +enum intel_display_power_domain
> > +intel_aux_power_domain(struct intel_digital_port *dig_port)
> > +{
> 
> couldn't we just move aux_power_domain up like yod did for aux_ch?  This way we initialize the
> power domain bassed on the aux channel, and just use it on the other functions. I think this is
> more future-proof for platforms changing the mapping aux <-> power domain. And avoids passing
> dev_priv around later just for checking that.

Well, the fact that the mapping can change dynamically is the reason I
chose to have a helper. Once Thunderbolt support is added we would check
in this function something like dig_port->is_tbt and return
POWER_DOMAIN_AUX_TBT_[1-4] instead of AUX_[A-D].

> 
> Lucas De Marchi
> 
> > +	switch (dig_port->aux_ch) {
> > +	case AUX_CH_A:
> > +		return POWER_DOMAIN_AUX_A;
> > +	case AUX_CH_B:
> > +		return POWER_DOMAIN_AUX_B;
> > +	case AUX_CH_C:
> > +		return POWER_DOMAIN_AUX_C;
> > +	case AUX_CH_D:
> > +		return POWER_DOMAIN_AUX_D;
> > +	case AUX_CH_E:
> > +		return POWER_DOMAIN_AUX_E;
> > +	case AUX_CH_F:
> > +		return POWER_DOMAIN_AUX_F;
> > +	default:
> > +		MISSING_CASE(dig_port->aux_ch);
> > +		return POWER_DOMAIN_AUX_A;
> > +	}
> > +}
> > +
> >  static u64 get_crtc_power_domains(struct drm_crtc *crtc,
> >  				  struct intel_crtc_state *crtc_state)
> >  {
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 6645c9faca9a..e6f59ef59be6 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -690,7 +690,8 @@ static void pps_lock(struct intel_dp *intel_dp)
> >  	 * See intel_power_sequencer_reset() why we need
> >  	 * a power domain reference here.
> >  	 */
> > -	intel_display_power_get(dev_priv, intel_dp->aux_power_domain);
> > +	intel_display_power_get(dev_priv,
> > +				intel_aux_power_domain(dp_to_dig_port(intel_dp)));
> >  
> >  	mutex_lock(&dev_priv->pps_mutex);
> >  }
> > @@ -701,7 +702,8 @@ static void pps_unlock(struct intel_dp *intel_dp)
> >  
> >  	mutex_unlock(&dev_priv->pps_mutex);
> >  
> > -	intel_display_power_put(dev_priv, intel_dp->aux_power_domain);
> > +	intel_display_power_put(dev_priv,
> > +				intel_aux_power_domain(dp_to_dig_port(intel_dp)));
> >  }
> >  
> >  static void
> > @@ -1505,29 +1507,6 @@ intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
> >  	return ret;
> >  }
> >  
> > -static enum intel_display_power_domain
> > -intel_aux_power_domain(struct intel_dp *intel_dp)
> > -{
> > -	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > -
> > -	switch (dig_port->aux_ch) {
> > -	case AUX_CH_A:
> > -		return POWER_DOMAIN_AUX_A;
> > -	case AUX_CH_B:
> > -		return POWER_DOMAIN_AUX_B;
> > -	case AUX_CH_C:
> > -		return POWER_DOMAIN_AUX_C;
> > -	case AUX_CH_D:
> > -		return POWER_DOMAIN_AUX_D;
> > -	case AUX_CH_E:
> > -		return POWER_DOMAIN_AUX_E;
> > -	case AUX_CH_F:
> > -		return POWER_DOMAIN_AUX_F;
> > -	default:
> > -		MISSING_CASE(dig_port->aux_ch);
> > -		return POWER_DOMAIN_AUX_A;
> > -	}
> > -}
> >  
> >  static i915_reg_t g4x_aux_ctl_reg(struct intel_dp *intel_dp)
> >  {
> > @@ -1654,8 +1633,6 @@ intel_dp_aux_init(struct intel_dp *intel_dp)
> >  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> >  	struct intel_encoder *encoder = &dig_port->base;
> >  
> > -	intel_dp->aux_power_domain = intel_aux_power_domain(intel_dp);
> > -
> >  	if (INTEL_GEN(dev_priv) >= 9) {
> >  		intel_dp->aux_ch_ctl_reg = skl_aux_ctl_reg;
> >  		intel_dp->aux_ch_data_reg = skl_aux_data_reg;
> > @@ -2356,7 +2333,8 @@ static bool edp_panel_vdd_on(struct intel_dp *intel_dp)
> >  	if (edp_have_panel_vdd(intel_dp))
> >  		return need_to_disable;
> >  
> > -	intel_display_power_get(dev_priv, intel_dp->aux_power_domain);
> > +	intel_display_power_get(dev_priv,
> > +				intel_aux_power_domain(intel_dig_port));
> >  
> >  	DRM_DEBUG_KMS("Turning eDP port %c VDD on\n",
> >  		      port_name(intel_dig_port->base.port));
> > @@ -2442,7 +2420,8 @@ static void edp_panel_vdd_off_sync(struct intel_dp *intel_dp)
> >  	if ((pp & PANEL_POWER_ON) == 0)
> >  		intel_dp->panel_power_off_time = ktime_get_boottime();
> >  
> > -	intel_display_power_put(dev_priv, intel_dp->aux_power_domain);
> > +	intel_display_power_put(dev_priv,
> > +				intel_aux_power_domain(intel_dig_port));
> >  }
> >  
> >  static void edp_panel_vdd_work(struct work_struct *__work)
> > @@ -2555,6 +2534,7 @@ void intel_edp_panel_on(struct intel_dp *intel_dp)
> >  static void edp_panel_off(struct intel_dp *intel_dp)
> >  {
> >  	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> >  	u32 pp;
> >  	i915_reg_t pp_ctrl_reg;
> >  
> > @@ -2564,10 +2544,10 @@ static void edp_panel_off(struct intel_dp *intel_dp)
> >  		return;
> >  
> >  	DRM_DEBUG_KMS("Turn eDP port %c panel power off\n",
> > -		      port_name(dp_to_dig_port(intel_dp)->base.port));
> > +		      port_name(dig_port->base.port));
> >  
> >  	WARN(!intel_dp->want_panel_vdd, "Need eDP port %c VDD to turn off panel\n",
> > -	     port_name(dp_to_dig_port(intel_dp)->base.port));
> > +	     port_name(dig_port->base.port));
> >  
> >  	pp = ironlake_get_pp_control(intel_dp);
> >  	/* We need to switch off panel power _and_ force vdd, for otherwise some
> > @@ -2586,7 +2566,7 @@ static void edp_panel_off(struct intel_dp *intel_dp)
> >  	intel_dp->panel_power_off_time = ktime_get_boottime();
> >  
> >  	/* We got a reference when we enabled the VDD. */
> > -	intel_display_power_put(dev_priv, intel_dp->aux_power_domain);
> > +	intel_display_power_put(dev_priv, intel_aux_power_domain(dig_port));
> >  }
> >  
> >  void intel_edp_panel_off(struct intel_dp *intel_dp)
> > @@ -5069,14 +5049,17 @@ intel_dp_detect(struct drm_connector *connector,
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(connector->dev);
> >  	struct intel_dp *intel_dp = intel_attached_dp(connector);
> > -	struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
> > +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > +	struct intel_encoder *encoder = &dig_port->base;
> >  	enum drm_connector_status status;
> > +	enum intel_display_power_domain aux_domain =
> > +		intel_aux_power_domain(dig_port);
> >  
> >  	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> >  		      connector->base.id, connector->name);
> >  	WARN_ON(!drm_modeset_is_locked(&dev_priv->drm.mode_config.connection_mutex));
> >  
> > -	intel_display_power_get(dev_priv, intel_dp->aux_power_domain);
> > +	intel_display_power_get(dev_priv, aux_domain);
> >  
> >  	/* Can't disconnect eDP */
> >  	if (intel_dp_is_edp(intel_dp))
> > @@ -5138,7 +5121,7 @@ intel_dp_detect(struct drm_connector *connector,
> >  		ret = intel_dp_retrain_link(encoder, ctx);
> >  		if (ret) {
> >  			intel_display_power_put(dev_priv,
> > -						intel_dp->aux_power_domain);
> > +						intel_aux_power_domain(dig_port));
> >  			return ret;
> >  		}
> >  	}
> > @@ -5162,7 +5145,7 @@ intel_dp_detect(struct drm_connector *connector,
> >  	if (status != connector_status_connected && !intel_dp->is_mst)
> >  		intel_dp_unset_edid(intel_dp);
> >  
> > -	intel_display_power_put(dev_priv, intel_dp->aux_power_domain);
> > +	intel_display_power_put(dev_priv, aux_domain);
> >  	return status;
> >  }
> >  
> > @@ -5170,8 +5153,11 @@ static void
> >  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 intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > +	struct intel_encoder *intel_encoder = &dig_port->base;
> >  	struct drm_i915_private *dev_priv = to_i915(intel_encoder->base.dev);
> > +	enum intel_display_power_domain aux_domain =
> > +		intel_aux_power_domain(dig_port);
> >  
> >  	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> >  		      connector->base.id, connector->name);
> > @@ -5180,11 +5166,11 @@ intel_dp_force(struct drm_connector *connector)
> >  	if (connector->status != connector_status_connected)
> >  		return;
> >  
> > -	intel_display_power_get(dev_priv, intel_dp->aux_power_domain);
> > +	intel_display_power_get(dev_priv, aux_domain);
> >  
> >  	intel_dp_set_edid(intel_dp);
> >  
> > -	intel_display_power_put(dev_priv, intel_dp->aux_power_domain);
> > +	intel_display_power_put(dev_priv, aux_domain);
> >  }
> >  
> >  static int intel_dp_get_modes(struct drm_connector *connector)
> > @@ -5530,6 +5516,7 @@ static const struct intel_hdcp_shim intel_dp_hdcp_shim = {
> >  static void intel_edp_panel_vdd_sanitize(struct intel_dp *intel_dp)
> >  {
> >  	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> >  
> >  	lockdep_assert_held(&dev_priv->pps_mutex);
> >  
> > @@ -5543,7 +5530,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");
> > -	intel_display_power_get(dev_priv, intel_dp->aux_power_domain);
> > +	intel_display_power_get(dev_priv, intel_aux_power_domain(dig_port));
> >  
> >  	edp_panel_vdd_schedule_off(intel_dp);
> >  }
> > @@ -5641,7 +5628,8 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
> >  		return IRQ_NONE;
> >  	}
> >  
> > -	intel_display_power_get(dev_priv, intel_dp->aux_power_domain);
> > +	intel_display_power_get(dev_priv,
> > +				intel_aux_power_domain(intel_dig_port));
> >  
> >  	if (intel_dp->is_mst) {
> >  		if (intel_dp_check_mst_status(intel_dp) == -EINVAL) {
> > @@ -5670,7 +5658,8 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
> >  	ret = IRQ_HANDLED;
> >  
> >  put_power:
> > -	intel_display_power_put(dev_priv, intel_dp->aux_power_domain);
> > +	intel_display_power_put(dev_priv,
> > +				intel_aux_power_domain(intel_dig_port));
> >  
> >  	return ret;
> >  }
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index a242a118389d..a3d7b93ecddd 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1130,7 +1130,6 @@ struct intel_dp {
> >  	/* sink or branch descriptor */
> >  	struct drm_dp_desc desc;
> >  	struct drm_dp_aux aux;
> > -	enum intel_display_power_domain aux_power_domain;
> >  	uint8_t train_set[4];
> >  	int panel_power_up_delay;
> >  	int panel_power_down_delay;
> > @@ -1709,6 +1708,8 @@ bool hsw_crtc_state_ips_capable(const struct intel_crtc_state *crtc_state);
> >  void hsw_enable_ips(const struct intel_crtc_state *crtc_state);
> >  void hsw_disable_ips(const struct intel_crtc_state *crtc_state);
> >  enum intel_display_power_domain intel_port_to_power_domain(enum port port);
> > +enum intel_display_power_domain
> > +intel_aux_power_domain(struct intel_digital_port *dig_port);
> >  void intel_mode_from_pipe_config(struct drm_display_mode *mode,
> >  				 struct intel_crtc_state *pipe_config);
> >  void intel_crtc_arm_fifo_underrun(struct intel_crtc *crtc,
> > -- 
> > 2.13.2
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx