| Message ID | 20180621155830.11311-1-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jun 21, 2018 at 06:58:29PM +0300, Imre Deak wrote: > So far we got an AUX power domain reference only for the duration of DP > AUX transfers. However, the followings suggest that we also need these > for main link functionality: > - The specification doesn't state whether it's needed or not for main > link functionality, but suggests that these power wells need to be > enabled already during display core initialization (Sequences to > Initialize Display). > - For PSR we need to keep the AUX power well enabled. > - On ICL combo PHY ports (non-TC) the AUX power well is needed for > link training too: while the port is enabled with a DP link training > test pattern trying to toggle the AUX power well will time out. > - On ICL MG PHY ports (TC) the AUX power well is needed also for main > link functionality (both in DP and HDMI modes). > - Windows enables these power wells both for main and AUX lane > functionality. > > Based on the above take an AUX power reference for main link > functionality too. This makes a difference only on GEN10+ (GLK+) > platforms, where we have separate port specific AUX power wells. > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/intel_ddi.c | 33 +++++++++++++++++++++++++++++---- > drivers/gpu/drm/i915/intel_display.c | 12 +++++++++++- > drivers/gpu/drm/i915/intel_drv.h | 3 ++- > 3 files changed, 42 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index 044fe1fb9872..b09cb6969bbb 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -1983,15 +1983,36 @@ bool intel_ddi_get_hw_state(struct intel_encoder *encoder, > return ret; > } > > -static u64 intel_ddi_get_power_domains(struct intel_encoder *encoder) > +static u64 intel_ddi_get_power_domains(struct intel_encoder *encoder, > + struct intel_crtc_state *crtc_state) > { > struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base); > enum pipe pipe; > + u64 domains; > > - if (intel_ddi_get_hw_state(encoder, &pipe)) > - return BIT_ULL(dig_port->ddi_io_power_domain); > + if (!intel_ddi_get_hw_state(encoder, &pipe)) > + return 0; > > - return 0; > + domains = BIT_ULL(dig_port->ddi_io_power_domain); > + if (!crtc_state) > + return domains; > + > + /* > + * TODO: Add support for MST encoders. Atm, the following should never > + * happen since this function will be called only for the primary > + * encoder which doesn't have its own pipe/crtc_state. > + */ > + if (WARN_ON(intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST))) > + return domains; > + > + /* AUX power is only needed for (e)DP mode, not for HDMI. */ > + if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) { > + struct intel_dp *intel_dp = &dig_port->dp; > + > + domains |= BIT_ULL(intel_dp->aux_power_domain); > + } > + > + return domains; > } > > void intel_ddi_enable_pipe_clock(const struct intel_crtc_state *crtc_state) > @@ -2631,6 +2652,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder, > > WARN_ON(is_mst && (port == PORT_A || port == PORT_E)); > > + intel_display_power_get(dev_priv, intel_dp->aux_power_domain); > + > intel_dp_set_link_params(intel_dp, crtc_state->port_clock, > crtc_state->lane_count, is_mst); > > @@ -2775,6 +2798,8 @@ static void intel_ddi_post_disable_dp(struct intel_encoder *encoder, > intel_display_power_put(dev_priv, dig_port->ddi_io_power_domain); > > intel_ddi_clk_disable(encoder); > + > + intel_display_power_put(dev_priv, intel_dp->aux_power_domain); > } > > static void intel_ddi_post_disable_hdmi(struct intel_encoder *encoder, > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 2c8fef3ede54..d9fefcec4b1a 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -15676,11 +15676,21 @@ get_encoder_power_domains(struct drm_i915_private *dev_priv) > for_each_intel_encoder(&dev_priv->drm, encoder) { > u64 get_domains; > enum intel_display_power_domain domain; > + struct intel_crtc_state *crtc_state; > > if (!encoder->get_power_domains) > continue; > > - get_domains = encoder->get_power_domains(encoder); > + /* > + * In case of a dangling encoder without a crtc we still need > + * to get power domain refs. Such encoders will be disabled > + * dropping these references. > + */ > + crtc_state = encoder->base.crtc ? > + to_intel_crtc_state(encoder->base.crtc->state) : > + NULL; Actually I think we can just drop this NULL crtc state case. If the encoder is active it will have a crtc assigned, and only then do we actually shut it down if the connector/crtc is not active. So if we we ever did get into a situation where we didn't have the crtc we'd leak the power references. Also we need the crtc state to shut down the encoder. The entire sanitize_encoder() thing is a bit wonky. In case we have an active encoder without a connector it won't actually do anything. I suppose that could at least happen with sdvo. With ddi it would required the port to be enabled with the wrong mode which I suppose is a slightly less realistic option. Another case when we wouldn't do anything is if the port is enabled without any transcoder having selected it. That could happen with CPT/PPT DP or DDI. Other port types have the pipe select bits on the port so it's not possible to get into that situation. Since we don't seem to have any mysterious bug reports about ports being wonky maybe these things just don't happen in practice. Or everything just happens to work out by accient even if don't shut things down cleanly. > + > + get_domains = encoder->get_power_domains(encoder, crtc_state); > for_each_power_domain(domain, get_domains) > intel_display_power_get(dev_priv, domain); > } > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 0c3ac0eafde0..7174429d930a 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -254,7 +254,8 @@ struct intel_encoder { > struct intel_crtc_state *pipe_config); > /* Returns a mask of power domains that need to be referenced as part > * of the hardware state readout code. */ > - u64 (*get_power_domains)(struct intel_encoder *encoder); > + u64 (*get_power_domains)(struct intel_encoder *encoder, > + struct intel_crtc_state *crtc_state); > /* > * Called during system suspend after all pending requests for the > * encoder are flushed (for example for DP AUX transactions) and > -- > 2.13.2
On Thu, Jun 21, 2018 at 06:58:29PM +0300, Imre Deak wrote: > So far we got an AUX power domain reference only for the duration of DP > AUX transfers. However, the followings suggest that we also need these > for main link functionality: > - The specification doesn't state whether it's needed or not for main > link functionality, but suggests that these power wells need to be > enabled already during display core initialization (Sequences to > Initialize Display). > - For PSR we need to keep the AUX power well enabled. > - On ICL combo PHY ports (non-TC) the AUX power well is needed for > link training too: while the port is enabled with a DP link training > test pattern trying to toggle the AUX power well will time out. > - On ICL MG PHY ports (TC) the AUX power well is needed also for main > link functionality (both in DP and HDMI modes). > - Windows enables these power wells both for main and AUX lane > functionality. > > Based on the above take an AUX power reference for main link > functionality too. This makes a difference only on GEN10+ (GLK+) > platforms, where we have separate port specific AUX power wells. > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > Signed-off-by: Imre Deak <imre.deak@intel.com> > --- > drivers/gpu/drm/i915/intel_ddi.c | 33 +++++++++++++++++++++++++++++---- > drivers/gpu/drm/i915/intel_display.c | 12 +++++++++++- > drivers/gpu/drm/i915/intel_drv.h | 3 ++- > 3 files changed, 42 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index 044fe1fb9872..b09cb6969bbb 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -1983,15 +1983,36 @@ bool intel_ddi_get_hw_state(struct intel_encoder *encoder, > return ret; > } > > -static u64 intel_ddi_get_power_domains(struct intel_encoder *encoder) > +static u64 intel_ddi_get_power_domains(struct intel_encoder *encoder, > + struct intel_crtc_state *crtc_state) > { > struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base); > enum pipe pipe; > + u64 domains; > > - if (intel_ddi_get_hw_state(encoder, &pipe)) > - return BIT_ULL(dig_port->ddi_io_power_domain); > + if (!intel_ddi_get_hw_state(encoder, &pipe)) > + return 0; > > - return 0; > + domains = BIT_ULL(dig_port->ddi_io_power_domain); > + if (!crtc_state) > + return domains; > + > + /* > + * TODO: Add support for MST encoders. Atm, the following should never > + * happen since this function will be called only for the primary > + * encoder which doesn't have its own pipe/crtc_state. > + */ > + if (WARN_ON(intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST))) > + return domains; > + > + /* AUX power is only needed for (e)DP mode, not for HDMI. */ > + if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) { I would use intel_crtc_has_dp_encoder() here so that one doesn't have to think "what is not HDMI?". > + struct intel_dp *intel_dp = &dig_port->dp; > + > + domains |= BIT_ULL(intel_dp->aux_power_domain); > + } > + > + return domains; > } > > void intel_ddi_enable_pipe_clock(const struct intel_crtc_state *crtc_state) > @@ -2631,6 +2652,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder, > > WARN_ON(is_mst && (port == PORT_A || port == PORT_E)); > > + intel_display_power_get(dev_priv, intel_dp->aux_power_domain); > + > intel_dp_set_link_params(intel_dp, crtc_state->port_clock, > crtc_state->lane_count, is_mst); > > @@ -2775,6 +2798,8 @@ static void intel_ddi_post_disable_dp(struct intel_encoder *encoder, > intel_display_power_put(dev_priv, dig_port->ddi_io_power_domain); > > intel_ddi_clk_disable(encoder); > + > + intel_display_power_put(dev_priv, intel_dp->aux_power_domain); > } > > static void intel_ddi_post_disable_hdmi(struct intel_encoder *encoder, > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 2c8fef3ede54..d9fefcec4b1a 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -15676,11 +15676,21 @@ get_encoder_power_domains(struct drm_i915_private *dev_priv) > for_each_intel_encoder(&dev_priv->drm, encoder) { > u64 get_domains; > enum intel_display_power_domain domain; > + struct intel_crtc_state *crtc_state; > > if (!encoder->get_power_domains) > continue; > > - get_domains = encoder->get_power_domains(encoder); > + /* > + * In case of a dangling encoder without a crtc we still need > + * to get power domain refs. Such encoders will be disabled > + * dropping these references. > + */ > + crtc_state = encoder->base.crtc ? > + to_intel_crtc_state(encoder->base.crtc->state) : > + NULL; > + > + get_domains = encoder->get_power_domains(encoder, crtc_state); > for_each_power_domain(domain, get_domains) > intel_display_power_get(dev_priv, domain); > } > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 0c3ac0eafde0..7174429d930a 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -254,7 +254,8 @@ struct intel_encoder { > struct intel_crtc_state *pipe_config); > /* Returns a mask of power domains that need to be referenced as part > * of the hardware state readout code. */ > - u64 (*get_power_domains)(struct intel_encoder *encoder); > + u64 (*get_power_domains)(struct intel_encoder *encoder, > + struct intel_crtc_state *crtc_state); > /* > * Called during system suspend after all pending requests for the > * encoder are flushed (for example for DP AUX transactions) and > -- > 2.13.2
On Thu, Jun 21, 2018 at 08:37:15PM +0300, Ville Syrjälä wrote: > On Thu, Jun 21, 2018 at 06:58:29PM +0300, Imre Deak wrote: > > So far we got an AUX power domain reference only for the duration of DP > > AUX transfers. However, the followings suggest that we also need these > > for main link functionality: > > - The specification doesn't state whether it's needed or not for main > > link functionality, but suggests that these power wells need to be > > enabled already during display core initialization (Sequences to > > Initialize Display). > > - For PSR we need to keep the AUX power well enabled. > > - On ICL combo PHY ports (non-TC) the AUX power well is needed for > > link training too: while the port is enabled with a DP link training > > test pattern trying to toggle the AUX power well will time out. > > - On ICL MG PHY ports (TC) the AUX power well is needed also for main > > link functionality (both in DP and HDMI modes). > > - Windows enables these power wells both for main and AUX lane > > functionality. > > > > Based on the above take an AUX power reference for main link > > functionality too. This makes a difference only on GEN10+ (GLK+) > > platforms, where we have separate port specific AUX power wells. > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > drivers/gpu/drm/i915/intel_ddi.c | 33 +++++++++++++++++++++++++++++---- > > drivers/gpu/drm/i915/intel_display.c | 12 +++++++++++- > > drivers/gpu/drm/i915/intel_drv.h | 3 ++- > > 3 files changed, 42 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > > index 044fe1fb9872..b09cb6969bbb 100644 > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > @@ -1983,15 +1983,36 @@ bool intel_ddi_get_hw_state(struct intel_encoder *encoder, > > return ret; > > } > > > > -static u64 intel_ddi_get_power_domains(struct intel_encoder *encoder) > > +static u64 intel_ddi_get_power_domains(struct intel_encoder *encoder, > > + struct intel_crtc_state *crtc_state) > > { > > struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base); > > enum pipe pipe; > > + u64 domains; > > > > - if (intel_ddi_get_hw_state(encoder, &pipe)) > > - return BIT_ULL(dig_port->ddi_io_power_domain); > > + if (!intel_ddi_get_hw_state(encoder, &pipe)) > > + return 0; > > > > - return 0; > > + domains = BIT_ULL(dig_port->ddi_io_power_domain); > > + if (!crtc_state) > > + return domains; > > + > > + /* > > + * TODO: Add support for MST encoders. Atm, the following should never > > + * happen since this function will be called only for the primary > > + * encoder which doesn't have its own pipe/crtc_state. > > + */ > > + if (WARN_ON(intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST))) > > + return domains; > > + > > + /* AUX power is only needed for (e)DP mode, not for HDMI. */ > > + if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) { > > + struct intel_dp *intel_dp = &dig_port->dp; > > + > > + domains |= BIT_ULL(intel_dp->aux_power_domain); > > + } > > + > > + return domains; > > } > > > > void intel_ddi_enable_pipe_clock(const struct intel_crtc_state *crtc_state) > > @@ -2631,6 +2652,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder, > > > > WARN_ON(is_mst && (port == PORT_A || port == PORT_E)); > > > > + intel_display_power_get(dev_priv, intel_dp->aux_power_domain); > > + > > intel_dp_set_link_params(intel_dp, crtc_state->port_clock, > > crtc_state->lane_count, is_mst); > > > > @@ -2775,6 +2798,8 @@ static void intel_ddi_post_disable_dp(struct intel_encoder *encoder, > > intel_display_power_put(dev_priv, dig_port->ddi_io_power_domain); > > > > intel_ddi_clk_disable(encoder); > > + > > + intel_display_power_put(dev_priv, intel_dp->aux_power_domain); > > } > > > > static void intel_ddi_post_disable_hdmi(struct intel_encoder *encoder, > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 2c8fef3ede54..d9fefcec4b1a 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -15676,11 +15676,21 @@ get_encoder_power_domains(struct drm_i915_private *dev_priv) > > for_each_intel_encoder(&dev_priv->drm, encoder) { > > u64 get_domains; > > enum intel_display_power_domain domain; > > + struct intel_crtc_state *crtc_state; > > > > if (!encoder->get_power_domains) > > continue; > > > > - get_domains = encoder->get_power_domains(encoder); > > + /* > > + * In case of a dangling encoder without a crtc we still need > > + * to get power domain refs. Such encoders will be disabled > > + * dropping these references. > > + */ > > + crtc_state = encoder->base.crtc ? > > + to_intel_crtc_state(encoder->base.crtc->state) : > > + NULL; > > Actually I think we can just drop this NULL crtc state case. If the > encoder is active it will have a crtc assigned, and only then do we > actually shut it down if the connector/crtc is not active. So if we we > ever did get into a situation where we didn't have the crtc we'd > leak the power references. Also we need the crtc state to shut down > the encoder. Err, thanks for catching that. Confused myself reading intel_sanitize_encoder(), thinking that we'll disable there encoders without a crtc, but that's not the case as you pointed out. Since encoder->base.crtc can be NULL for MST I will still keep the above and rely on intel_ddi_get_hw_state() to skip taking power refs for that case. Will update the comment accrodingly. > The entire sanitize_encoder() thing is a bit wonky. In case we have an > active encoder without a connector it won't actually do anything. I > suppose that could at least happen with sdvo. With ddi it would required > the port to be enabled with the wrong mode which I suppose is a > slightly less realistic option. > > Another case when we wouldn't do anything is if the port is enabled > without any transcoder having selected it. That could happen with > CPT/PPT DP or DDI. Other port types have the pipe select bits on the > port so it's not possible to get into that situation. > > Since we don't seem to have any mysterious bug reports about ports being > wonky maybe these things just don't happen in practice. Or everything > just happens to work out by accient even if don't shut things down > cleanly. > > > + > > + get_domains = encoder->get_power_domains(encoder, crtc_state); > > for_each_power_domain(domain, get_domains) > > intel_display_power_get(dev_priv, domain); > > } > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > > index 0c3ac0eafde0..7174429d930a 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -254,7 +254,8 @@ struct intel_encoder { > > struct intel_crtc_state *pipe_config); > > /* Returns a mask of power domains that need to be referenced as part > > * of the hardware state readout code. */ > > - u64 (*get_power_domains)(struct intel_encoder *encoder); > > + u64 (*get_power_domains)(struct intel_encoder *encoder, > > + struct intel_crtc_state *crtc_state); > > /* > > * Called during system suspend after all pending requests for the > > * encoder are flushed (for example for DP AUX transactions) and > > -- > > 2.13.2 > > -- > Ville Syrjälä > Intel
On Thu, Jun 21, 2018 at 08:40:45PM +0300, Ville Syrjälä wrote: > On Thu, Jun 21, 2018 at 06:58:29PM +0300, Imre Deak wrote: > > So far we got an AUX power domain reference only for the duration of DP > > AUX transfers. However, the followings suggest that we also need these > > for main link functionality: > > - The specification doesn't state whether it's needed or not for main > > link functionality, but suggests that these power wells need to be > > enabled already during display core initialization (Sequences to > > Initialize Display). > > - For PSR we need to keep the AUX power well enabled. > > - On ICL combo PHY ports (non-TC) the AUX power well is needed for > > link training too: while the port is enabled with a DP link training > > test pattern trying to toggle the AUX power well will time out. > > - On ICL MG PHY ports (TC) the AUX power well is needed also for main > > link functionality (both in DP and HDMI modes). > > - Windows enables these power wells both for main and AUX lane > > functionality. > > > > Based on the above take an AUX power reference for main link > > functionality too. This makes a difference only on GEN10+ (GLK+) > > platforms, where we have separate port specific AUX power wells. > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > --- > > drivers/gpu/drm/i915/intel_ddi.c | 33 +++++++++++++++++++++++++++++---- > > drivers/gpu/drm/i915/intel_display.c | 12 +++++++++++- > > drivers/gpu/drm/i915/intel_drv.h | 3 ++- > > 3 files changed, 42 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > > index 044fe1fb9872..b09cb6969bbb 100644 > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > @@ -1983,15 +1983,36 @@ bool intel_ddi_get_hw_state(struct intel_encoder *encoder, > > return ret; > > } > > > > -static u64 intel_ddi_get_power_domains(struct intel_encoder *encoder) > > +static u64 intel_ddi_get_power_domains(struct intel_encoder *encoder, > > + struct intel_crtc_state *crtc_state) > > { > > struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base); > > enum pipe pipe; > > + u64 domains; > > > > - if (intel_ddi_get_hw_state(encoder, &pipe)) > > - return BIT_ULL(dig_port->ddi_io_power_domain); > > + if (!intel_ddi_get_hw_state(encoder, &pipe)) > > + return 0; > > > > - return 0; > > + domains = BIT_ULL(dig_port->ddi_io_power_domain); > > + if (!crtc_state) > > + return domains; > > + > > + /* > > + * TODO: Add support for MST encoders. Atm, the following should never > > + * happen since this function will be called only for the primary > > + * encoder which doesn't have its own pipe/crtc_state. > > + */ > > + if (WARN_ON(intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST))) > > + return domains; > > + > > + /* AUX power is only needed for (e)DP mode, not for HDMI. */ > > + if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) { > > I would use intel_crtc_has_dp_encoder() here so that one doesn't > have to think "what is not HDMI?". Ok, makes sense. > > > + struct intel_dp *intel_dp = &dig_port->dp; > > + > > + domains |= BIT_ULL(intel_dp->aux_power_domain); > > + } > > + > > + return domains; > > } > > > > void intel_ddi_enable_pipe_clock(const struct intel_crtc_state *crtc_state) > > @@ -2631,6 +2652,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder, > > > > WARN_ON(is_mst && (port == PORT_A || port == PORT_E)); > > > > + intel_display_power_get(dev_priv, intel_dp->aux_power_domain); > > + > > intel_dp_set_link_params(intel_dp, crtc_state->port_clock, > > crtc_state->lane_count, is_mst); > > > > @@ -2775,6 +2798,8 @@ static void intel_ddi_post_disable_dp(struct intel_encoder *encoder, > > intel_display_power_put(dev_priv, dig_port->ddi_io_power_domain); > > > > intel_ddi_clk_disable(encoder); > > + > > + intel_display_power_put(dev_priv, intel_dp->aux_power_domain); > > } > > > > static void intel_ddi_post_disable_hdmi(struct intel_encoder *encoder, > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 2c8fef3ede54..d9fefcec4b1a 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -15676,11 +15676,21 @@ get_encoder_power_domains(struct drm_i915_private *dev_priv) > > for_each_intel_encoder(&dev_priv->drm, encoder) { > > u64 get_domains; > > enum intel_display_power_domain domain; > > + struct intel_crtc_state *crtc_state; > > > > if (!encoder->get_power_domains) > > continue; > > > > - get_domains = encoder->get_power_domains(encoder); > > + /* > > + * In case of a dangling encoder without a crtc we still need > > + * to get power domain refs. Such encoders will be disabled > > + * dropping these references. > > + */ > > + crtc_state = encoder->base.crtc ? > > + to_intel_crtc_state(encoder->base.crtc->state) : > > + NULL; > > + > > + get_domains = encoder->get_power_domains(encoder, crtc_state); > > for_each_power_domain(domain, get_domains) > > intel_display_power_get(dev_priv, domain); > > } > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > > index 0c3ac0eafde0..7174429d930a 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -254,7 +254,8 @@ struct intel_encoder { > > struct intel_crtc_state *pipe_config); > > /* Returns a mask of power domains that need to be referenced as part > > * of the hardware state readout code. */ > > - u64 (*get_power_domains)(struct intel_encoder *encoder); > > + u64 (*get_power_domains)(struct intel_encoder *encoder, > > + struct intel_crtc_state *crtc_state); > > /* > > * Called during system suspend after all pending requests for the > > * encoder are flushed (for example for DP AUX transactions) and > > -- > > 2.13.2 > > -- > Ville Syrjälä > Intel
On Thu, Jun 21, 2018 at 09:18:55PM +0300, Imre Deak wrote: > On Thu, Jun 21, 2018 at 08:37:15PM +0300, Ville Syrjälä wrote: > > On Thu, Jun 21, 2018 at 06:58:29PM +0300, Imre Deak wrote: > > > So far we got an AUX power domain reference only for the duration of DP > > > AUX transfers. However, the followings suggest that we also need these > > > for main link functionality: > > > - The specification doesn't state whether it's needed or not for main > > > link functionality, but suggests that these power wells need to be > > > enabled already during display core initialization (Sequences to > > > Initialize Display). > > > - For PSR we need to keep the AUX power well enabled. > > > - On ICL combo PHY ports (non-TC) the AUX power well is needed for > > > link training too: while the port is enabled with a DP link training > > > test pattern trying to toggle the AUX power well will time out. > > > - On ICL MG PHY ports (TC) the AUX power well is needed also for main > > > link functionality (both in DP and HDMI modes). > > > - Windows enables these power wells both for main and AUX lane > > > functionality. > > > > > > Based on the above take an AUX power reference for main link > > > functionality too. This makes a difference only on GEN10+ (GLK+) > > > platforms, where we have separate port specific AUX power well Currently I get AUX timeouts and unable to start link training on DP without disable_power_well=0 option set. I think the reason was that we were not getting the power domain for the AUX. So hopefully this patch should fix it. Manasi > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_ddi.c | 33 +++++++++++++++++++++++++++++---- > > > drivers/gpu/drm/i915/intel_display.c | 12 +++++++++++- > > > drivers/gpu/drm/i915/intel_drv.h | 3 ++- > > > 3 files changed, 42 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > > > index 044fe1fb9872..b09cb6969bbb 100644 > > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > > @@ -1983,15 +1983,36 @@ bool intel_ddi_get_hw_state(struct intel_encoder *encoder, > > > return ret; > > > } > > > > > > -static u64 intel_ddi_get_power_domains(struct intel_encoder *encoder) > > > +static u64 intel_ddi_get_power_domains(struct intel_encoder *encoder, > > > + struct intel_crtc_state *crtc_state) > > > { > > > struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base); > > > enum pipe pipe; > > > + u64 domains; > > > > > > - if (intel_ddi_get_hw_state(encoder, &pipe)) > > > - return BIT_ULL(dig_port->ddi_io_power_domain); > > > + if (!intel_ddi_get_hw_state(encoder, &pipe)) > > > + return 0; > > > > > > - return 0; > > > + domains = BIT_ULL(dig_port->ddi_io_power_domain); > > > + if (!crtc_state) > > > + return domains; > > > + > > > + /* > > > + * TODO: Add support for MST encoders. Atm, the following should never > > > + * happen since this function will be called only for the primary > > > + * encoder which doesn't have its own pipe/crtc_state. > > > + */ > > > + if (WARN_ON(intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST))) > > > + return domains; > > > + > > > + /* AUX power is only needed for (e)DP mode, not for HDMI. */ > > > + if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) { > > > + struct intel_dp *intel_dp = &dig_port->dp; > > > + > > > + domains |= BIT_ULL(intel_dp->aux_power_domain); > > > + } > > > + > > > + return domains; > > > } > > > > > > void intel_ddi_enable_pipe_clock(const struct intel_crtc_state *crtc_state) > > > @@ -2631,6 +2652,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder, > > > > > > WARN_ON(is_mst && (port == PORT_A || port == PORT_E)); > > > > > > + intel_display_power_get(dev_priv, intel_dp->aux_power_domain); > > > + > > > intel_dp_set_link_params(intel_dp, crtc_state->port_clock, > > > crtc_state->lane_count, is_mst); > > > > > > @@ -2775,6 +2798,8 @@ static void intel_ddi_post_disable_dp(struct intel_encoder *encoder, > > > intel_display_power_put(dev_priv, dig_port->ddi_io_power_domain); > > > > > > intel_ddi_clk_disable(encoder); > > > + > > > + intel_display_power_put(dev_priv, intel_dp->aux_power_domain); > > > } > > > > > > static void intel_ddi_post_disable_hdmi(struct intel_encoder *encoder, > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > index 2c8fef3ede54..d9fefcec4b1a 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -15676,11 +15676,21 @@ get_encoder_power_domains(struct drm_i915_private *dev_priv) > > > for_each_intel_encoder(&dev_priv->drm, encoder) { > > > u64 get_domains; > > > enum intel_display_power_domain domain; > > > + struct intel_crtc_state *crtc_state; > > > > > > if (!encoder->get_power_domains) > > > continue; > > > > > > - get_domains = encoder->get_power_domains(encoder); > > > + /* > > > + * In case of a dangling encoder without a crtc we still need > > > + * to get power domain refs. Such encoders will be disabled > > > + * dropping these references. > > > + */ > > > + crtc_state = encoder->base.crtc ? > > > + to_intel_crtc_state(encoder->base.crtc->state) : > > > + NULL; > > > > Actually I think we can just drop this NULL crtc state case. If the > > encoder is active it will have a crtc assigned, and only then do we > > actually shut it down if the connector/crtc is not active. So if we we > > ever did get into a situation where we didn't have the crtc we'd > > leak the power references. Also we need the crtc state to shut down > > the encoder. > > Err, thanks for catching that. Confused myself reading > intel_sanitize_encoder(), thinking that we'll disable there encoders > without a crtc, but that's not the case as you pointed out. Since > encoder->base.crtc can be NULL for MST I will still keep the above and > rely on intel_ddi_get_hw_state() to skip taking power refs for that > case. Will update the comment accrodingly. > > > The entire sanitize_encoder() thing is a bit wonky. In case we have an > > active encoder without a connector it won't actually do anything. I > > suppose that could at least happen with sdvo. With ddi it would required > > the port to be enabled with the wrong mode which I suppose is a > > slightly less realistic option. > > > > Another case when we wouldn't do anything is if the port is enabled > > without any transcoder having selected it. That could happen with > > CPT/PPT DP or DDI. Other port types have the pipe select bits on the > > port so it's not possible to get into that situation. > > > > Since we don't seem to have any mysterious bug reports about ports being > > wonky maybe these things just don't happen in practice. Or everything > > just happens to work out by accient even if don't shut things down > > cleanly. > > > > > + > > > + get_domains = encoder->get_power_domains(encoder, crtc_state); > > > for_each_power_domain(domain, get_domains) > > > intel_display_power_get(dev_priv, domain); > > > } > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > > > index 0c3ac0eafde0..7174429d930a 100644 > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > @@ -254,7 +254,8 @@ struct intel_encoder { > > > struct intel_crtc_state *pipe_config); > > > /* Returns a mask of power domains that need to be referenced as part > > > * of the hardware state readout code. */ > > > - u64 (*get_power_domains)(struct intel_encoder *encoder); > > > + u64 (*get_power_domains)(struct intel_encoder *encoder, > > > + struct intel_crtc_state *crtc_state); > > > /* > > > * Called during system suspend after all pending requests for the > > > * encoder are flushed (for example for DP AUX transactions) and > > > -- > > > 2.13.2 > > > > -- > > Ville Syrjälä > > Intel > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Jun 21, 2018 at 11:54:55AM -0700, Manasi Navare wrote: > On Thu, Jun 21, 2018 at 09:18:55PM +0300, Imre Deak wrote: > > On Thu, Jun 21, 2018 at 08:37:15PM +0300, Ville Syrjälä wrote: > > > On Thu, Jun 21, 2018 at 06:58:29PM +0300, Imre Deak wrote: > > > > So far we got an AUX power domain reference only for the duration of DP > > > > AUX transfers. However, the followings suggest that we also need these > > > > for main link functionality: > > > > - The specification doesn't state whether it's needed or not for main > > > > link functionality, but suggests that these power wells need to be > > > > enabled already during display core initialization (Sequences to > > > > Initialize Display). > > > > - For PSR we need to keep the AUX power well enabled. > > > > - On ICL combo PHY ports (non-TC) the AUX power well is needed for > > > > link training too: while the port is enabled with a DP link training > > > > test pattern trying to toggle the AUX power well will time out. > > > > - On ICL MG PHY ports (TC) the AUX power well is needed also for main > > > > link functionality (both in DP and HDMI modes). > > > > - Windows enables these power wells both for main and AUX lane > > > > functionality. > > > > > > > > Based on the above take an AUX power reference for main link > > > > functionality too. This makes a difference only on GEN10+ (GLK+) > > > > platforms, where we have separate port specific AUX power well > > Currently I get AUX timeouts and unable to start link training on DP > without disable_power_well=0 option set. I think the reason was that > we were not getting the power domain for the AUX. So hopefully this > patch should fix it. Yes, that's because during link training - where the port is enabled with a training pattern set - we can't enable the AUX power well, it'll time out. B/c of that AUX transfers will time out too. After disabling the training pattern and enabling the port for real video signals, AUX enabling starts to work again.. --Imre > > Manasi > > > > > > > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> > > > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > Signed-off-by: Imre Deak <imre.deak@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_ddi.c | 33 +++++++++++++++++++++++++++++---- > > > > drivers/gpu/drm/i915/intel_display.c | 12 +++++++++++- > > > > drivers/gpu/drm/i915/intel_drv.h | 3 ++- > > > > 3 files changed, 42 insertions(+), 6 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > > > > index 044fe1fb9872..b09cb6969bbb 100644 > > > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > > > @@ -1983,15 +1983,36 @@ bool intel_ddi_get_hw_state(struct intel_encoder *encoder, > > > > return ret; > > > > } > > > > > > > > -static u64 intel_ddi_get_power_domains(struct intel_encoder *encoder) > > > > +static u64 intel_ddi_get_power_domains(struct intel_encoder *encoder, > > > > + struct intel_crtc_state *crtc_state) > > > > { > > > > struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base); > > > > enum pipe pipe; > > > > + u64 domains; > > > > > > > > - if (intel_ddi_get_hw_state(encoder, &pipe)) > > > > - return BIT_ULL(dig_port->ddi_io_power_domain); > > > > + if (!intel_ddi_get_hw_state(encoder, &pipe)) > > > > + return 0; > > > > > > > > - return 0; > > > > + domains = BIT_ULL(dig_port->ddi_io_power_domain); > > > > + if (!crtc_state) > > > > + return domains; > > > > + > > > > + /* > > > > + * TODO: Add support for MST encoders. Atm, the following should never > > > > + * happen since this function will be called only for the primary > > > > + * encoder which doesn't have its own pipe/crtc_state. > > > > + */ > > > > + if (WARN_ON(intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST))) > > > > + return domains; > > > > + > > > > + /* AUX power is only needed for (e)DP mode, not for HDMI. */ > > > > + if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) { > > > > + struct intel_dp *intel_dp = &dig_port->dp; > > > > + > > > > + domains |= BIT_ULL(intel_dp->aux_power_domain); > > > > + } > > > > + > > > > + return domains; > > > > } > > > > > > > > void intel_ddi_enable_pipe_clock(const struct intel_crtc_state *crtc_state) > > > > @@ -2631,6 +2652,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder, > > > > > > > > WARN_ON(is_mst && (port == PORT_A || port == PORT_E)); > > > > > > > > + intel_display_power_get(dev_priv, intel_dp->aux_power_domain); > > > > + > > > > intel_dp_set_link_params(intel_dp, crtc_state->port_clock, > > > > crtc_state->lane_count, is_mst); > > > > > > > > @@ -2775,6 +2798,8 @@ static void intel_ddi_post_disable_dp(struct intel_encoder *encoder, > > > > intel_display_power_put(dev_priv, dig_port->ddi_io_power_domain); > > > > > > > > intel_ddi_clk_disable(encoder); > > > > + > > > > + intel_display_power_put(dev_priv, intel_dp->aux_power_domain); > > > > } > > > > > > > > static void intel_ddi_post_disable_hdmi(struct intel_encoder *encoder, > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > > index 2c8fef3ede54..d9fefcec4b1a 100644 > > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > > @@ -15676,11 +15676,21 @@ get_encoder_power_domains(struct drm_i915_private *dev_priv) > > > > for_each_intel_encoder(&dev_priv->drm, encoder) { > > > > u64 get_domains; > > > > enum intel_display_power_domain domain; > > > > + struct intel_crtc_state *crtc_state; > > > > > > > > if (!encoder->get_power_domains) > > > > continue; > > > > > > > > - get_domains = encoder->get_power_domains(encoder); > > > > + /* > > > > + * In case of a dangling encoder without a crtc we still need > > > > + * to get power domain refs. Such encoders will be disabled > > > > + * dropping these references. > > > > + */ > > > > + crtc_state = encoder->base.crtc ? > > > > + to_intel_crtc_state(encoder->base.crtc->state) : > > > > + NULL; > > > > > > Actually I think we can just drop this NULL crtc state case. If the > > > encoder is active it will have a crtc assigned, and only then do we > > > actually shut it down if the connector/crtc is not active. So if we we > > > ever did get into a situation where we didn't have the crtc we'd > > > leak the power references. Also we need the crtc state to shut down > > > the encoder. > > > > Err, thanks for catching that. Confused myself reading > > intel_sanitize_encoder(), thinking that we'll disable there encoders > > without a crtc, but that's not the case as you pointed out. Since > > encoder->base.crtc can be NULL for MST I will still keep the above and > > rely on intel_ddi_get_hw_state() to skip taking power refs for that > > case. Will update the comment accrodingly. > > > > > The entire sanitize_encoder() thing is a bit wonky. In case we have an > > > active encoder without a connector it won't actually do anything. I > > > suppose that could at least happen with sdvo. With ddi it would required > > > the port to be enabled with the wrong mode which I suppose is a > > > slightly less realistic option. > > > > > > Another case when we wouldn't do anything is if the port is enabled > > > without any transcoder having selected it. That could happen with > > > CPT/PPT DP or DDI. Other port types have the pipe select bits on the > > > port so it's not possible to get into that situation. > > > > > > Since we don't seem to have any mysterious bug reports about ports being > > > wonky maybe these things just don't happen in practice. Or everything > > > just happens to work out by accient even if don't shut things down > > > cleanly. > > > > > > > + > > > > + get_domains = encoder->get_power_domains(encoder, crtc_state); > > > > for_each_power_domain(domain, get_domains) > > > > intel_display_power_get(dev_priv, domain); > > > > } > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > > > > index 0c3ac0eafde0..7174429d930a 100644 > > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > > @@ -254,7 +254,8 @@ struct intel_encoder { > > > > struct intel_crtc_state *pipe_config); > > > > /* Returns a mask of power domains that need to be referenced as part > > > > * of the hardware state readout code. */ > > > > - u64 (*get_power_domains)(struct intel_encoder *encoder); > > > > + u64 (*get_power_domains)(struct intel_encoder *encoder, > > > > + struct intel_crtc_state *crtc_state); > > > > /* > > > > * Called during system suspend after all pending requests for the > > > > * encoder are flushed (for example for DP AUX transactions) and > > > > -- > > > > 2.13.2 > > > > > > -- > > > Ville Syrjälä > > > Intel > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index 044fe1fb9872..b09cb6969bbb 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -1983,15 +1983,36 @@ bool intel_ddi_get_hw_state(struct intel_encoder *encoder, return ret; } -static u64 intel_ddi_get_power_domains(struct intel_encoder *encoder) +static u64 intel_ddi_get_power_domains(struct intel_encoder *encoder, + struct intel_crtc_state *crtc_state) { struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base); enum pipe pipe; + u64 domains; - if (intel_ddi_get_hw_state(encoder, &pipe)) - return BIT_ULL(dig_port->ddi_io_power_domain); + if (!intel_ddi_get_hw_state(encoder, &pipe)) + return 0; - return 0; + domains = BIT_ULL(dig_port->ddi_io_power_domain); + if (!crtc_state) + return domains; + + /* + * TODO: Add support for MST encoders. Atm, the following should never + * happen since this function will be called only for the primary + * encoder which doesn't have its own pipe/crtc_state. + */ + if (WARN_ON(intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST))) + return domains; + + /* AUX power is only needed for (e)DP mode, not for HDMI. */ + if (!intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI)) { + struct intel_dp *intel_dp = &dig_port->dp; + + domains |= BIT_ULL(intel_dp->aux_power_domain); + } + + return domains; } void intel_ddi_enable_pipe_clock(const struct intel_crtc_state *crtc_state) @@ -2631,6 +2652,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder, WARN_ON(is_mst && (port == PORT_A || port == PORT_E)); + intel_display_power_get(dev_priv, intel_dp->aux_power_domain); + intel_dp_set_link_params(intel_dp, crtc_state->port_clock, crtc_state->lane_count, is_mst); @@ -2775,6 +2798,8 @@ static void intel_ddi_post_disable_dp(struct intel_encoder *encoder, intel_display_power_put(dev_priv, dig_port->ddi_io_power_domain); intel_ddi_clk_disable(encoder); + + intel_display_power_put(dev_priv, intel_dp->aux_power_domain); } static void intel_ddi_post_disable_hdmi(struct intel_encoder *encoder, diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 2c8fef3ede54..d9fefcec4b1a 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -15676,11 +15676,21 @@ get_encoder_power_domains(struct drm_i915_private *dev_priv) for_each_intel_encoder(&dev_priv->drm, encoder) { u64 get_domains; enum intel_display_power_domain domain; + struct intel_crtc_state *crtc_state; if (!encoder->get_power_domains) continue; - get_domains = encoder->get_power_domains(encoder); + /* + * In case of a dangling encoder without a crtc we still need + * to get power domain refs. Such encoders will be disabled + * dropping these references. + */ + crtc_state = encoder->base.crtc ? + to_intel_crtc_state(encoder->base.crtc->state) : + NULL; + + get_domains = encoder->get_power_domains(encoder, crtc_state); for_each_power_domain(domain, get_domains) intel_display_power_get(dev_priv, domain); } diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 0c3ac0eafde0..7174429d930a 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -254,7 +254,8 @@ struct intel_encoder { struct intel_crtc_state *pipe_config); /* Returns a mask of power domains that need to be referenced as part * of the hardware state readout code. */ - u64 (*get_power_domains)(struct intel_encoder *encoder); + u64 (*get_power_domains)(struct intel_encoder *encoder, + struct intel_crtc_state *crtc_state); /* * Called during system suspend after all pending requests for the * encoder are flushed (for example for DP AUX transactions) and
So far we got an AUX power domain reference only for the duration of DP AUX transfers. However, the followings suggest that we also need these for main link functionality: - The specification doesn't state whether it's needed or not for main link functionality, but suggests that these power wells need to be enabled already during display core initialization (Sequences to Initialize Display). - For PSR we need to keep the AUX power well enabled. - On ICL combo PHY ports (non-TC) the AUX power well is needed for link training too: while the port is enabled with a DP link training test pattern trying to toggle the AUX power well will time out. - On ICL MG PHY ports (TC) the AUX power well is needed also for main link functionality (both in DP and HDMI modes). - Windows enables these power wells both for main and AUX lane functionality. Based on the above take an AUX power reference for main link functionality too. This makes a difference only on GEN10+ (GLK+) platforms, where we have separate port specific AUX power wells. Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com> Signed-off-by: Imre Deak <imre.deak@intel.com> --- drivers/gpu/drm/i915/intel_ddi.c | 33 +++++++++++++++++++++++++++++---- drivers/gpu/drm/i915/intel_display.c | 12 +++++++++++- drivers/gpu/drm/i915/intel_drv.h | 3 ++- 3 files changed, 42 insertions(+), 6 deletions(-)