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@xxxxxxxxx>
> > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
