On Thu, May 09, 2019 at 06:57:56PM +0300, Ville Syrjälä wrote:
> On Thu, May 09, 2019 at 09:19:50AM +0300, Imre Deak wrote:
> > We don't need the AUX power for the whole duration of the detect, only
> > when we're doing AUX transfers. The AUX transfer function takes its own
> > reference on the AUX power domain already. The two places during detect
> > which access display core registers (not specific to a
> > pipe/port/transcoder) only need the power domain that is required for
> > that access. That power domain is equivalent to the device global power
> > domain on most platforms (enabled whenever we hold a runtime PM
> > reference) except on CHV/VLV where it's equivalent to the display power
> > well.
> >
> > Add a new power domain that reflects the above, and use this at the two
> > spots accessing registers. With that we can avoid taking the AUX
> > reference for the whole duration of the detect function.
> >
> > Put the domains asynchronously to avoid the unneeded on-off-on toggling.
> >
> > Cc: Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx>
> > Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/intel_display.h | 1 +
> > drivers/gpu/drm/i915/intel_dp.c | 32 +++++++++++++++++--------
> > drivers/gpu/drm/i915/intel_runtime_pm.c | 4 ++++
> > 3 files changed, 27 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
> > index 1b6f5a71184d..7f3fafdfbd5f 100644
> > --- a/drivers/gpu/drm/i915/intel_display.h
> > +++ b/drivers/gpu/drm/i915/intel_display.h
> > @@ -220,6 +220,7 @@ enum aux_ch {
> > #define aux_ch_name(a) ((a) + 'A')
> >
> > enum intel_display_power_domain {
> > + POWER_DOMAIN_DISPLAY_CORE,
> > POWER_DOMAIN_PIPE_A,
> > POWER_DOMAIN_PIPE_B,
> > POWER_DOMAIN_PIPE_C,
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 1e011998e9d5..24cca12a9a3e 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -216,15 +216,21 @@ static int intel_dp_get_fia_supported_lane_count(struct intel_dp *intel_dp)
> > struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
> > enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
> > + intel_wakeref_t wakeref;
> > u32 lane_info;
> >
> > if (tc_port == PORT_TC_NONE || dig_port->tc_type != TC_PORT_TYPEC)
> > return 4;
> >
> > + wakeref = intel_display_power_get(dev_priv, POWER_DOMAIN_DISPLAY_CORE);
> > +
> > lane_info = (I915_READ(PORT_TX_DFLEXDPSP) &
> > DP_LANE_ASSIGNMENT_MASK(tc_port)) >>
> > DP_LANE_ASSIGNMENT_SHIFT(tc_port);
> >
> > + intel_display_power_put_async(dev_priv, POWER_DOMAIN_DISPLAY_CORE,
> > + wakeref);
> > +
>
> Future idea: maybe we want a with_something() {} for this sort of thing?
Yep, good idea, can add that in this patch.
>
> > switch (lane_info) {
> > default:
> > MISSING_CASE(lane_info);
>
> Unrelated but this thing here looks like a hand rolled hweight(). And
> for some reason it's using decimal for bitmasks.
Yep, that's strange. I can follow up fixing that if noone is
interested.
>
> > @@ -5294,7 +5300,7 @@ static bool icl_digital_port_connected(struct intel_encoder *encoder)
> > *
> > * Return %true if port is connected, %false otherwise.
> > */
> > -bool intel_digital_port_connected(struct intel_encoder *encoder)
> > +static bool __intel_digital_port_connected(struct intel_encoder *encoder)
> > {
> > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> >
> > @@ -5324,6 +5330,20 @@ bool intel_digital_port_connected(struct intel_encoder *encoder)
> > return false;
> > }
> >
> > +bool intel_digital_port_connected(struct intel_encoder *encoder)
> > +{
> > + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > + intel_wakeref_t wakeref;
> > + bool res;
>
> bool is_connected perhaps?
Ok.
>
> > +
> > + wakeref = intel_display_power_get(dev_priv, POWER_DOMAIN_DISPLAY_CORE);
> > + res = __intel_digital_port_connected(encoder);
> > + intel_display_power_put_async(dev_priv, POWER_DOMAIN_DISPLAY_CORE,
> > + wakeref);
> > +
> > + return res;
> > +}
> > +
> > static struct edid *
> > intel_dp_get_edid(struct intel_dp *intel_dp)
> > {
> > @@ -5377,16 +5397,11 @@ intel_dp_detect(struct drm_connector *connector,
> > 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);
> > - intel_wakeref_t wakeref;
> >
> > 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));
> >
> > - wakeref = intel_display_power_get(dev_priv, aux_domain);
> > -
> > /* Can't disconnect eDP */
> > if (intel_dp_is_edp(intel_dp))
> > status = edp_detect(intel_dp);
> > @@ -5450,10 +5465,8 @@ intel_dp_detect(struct drm_connector *connector,
> > int ret;
> >
> > ret = intel_dp_retrain_link(encoder, ctx);
> > - if (ret) {
> > - intel_display_power_put(dev_priv, aux_domain, wakeref);
> > + if (ret)
> > return ret;
> > - }
> > }
> >
> > /*
> > @@ -5475,7 +5488,6 @@ 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, aux_domain, wakeref);
> > return status;
> > }
> >
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 829d483d27e9..618dd42f1b2d 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -406,6 +406,8 @@ const char *
> > intel_display_power_domain_str(enum intel_display_power_domain domain)
> > {
> > switch (domain) {
> > + case POWER_DOMAIN_DISPLAY_CORE:
> > + return "DISPLAY_CORE";
> > case POWER_DOMAIN_PIPE_A:
> > return "PIPE_A";
> > case POWER_DOMAIN_PIPE_B:
> > @@ -2383,6 +2385,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
> > BIT_ULL(POWER_DOMAIN_INIT))
> >
> > #define VLV_DISPLAY_POWER_DOMAINS ( \
> > + BIT_ULL(POWER_DOMAIN_DISPLAY_CORE) | \
> > BIT_ULL(POWER_DOMAIN_PIPE_A) | \
> > BIT_ULL(POWER_DOMAIN_PIPE_B) | \
> > BIT_ULL(POWER_DOMAIN_PIPE_A_PANEL_FITTER) | \
> > @@ -2429,6 +2432,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
> > BIT_ULL(POWER_DOMAIN_INIT))
> >
> > #define CHV_DISPLAY_POWER_DOMAINS ( \
> > + BIT_ULL(POWER_DOMAIN_DISPLAY_CORE) | \
> > BIT_ULL(POWER_DOMAIN_PIPE_A) | \
> > BIT_ULL(POWER_DOMAIN_PIPE_B) | \
> > BIT_ULL(POWER_DOMAIN_PIPE_C) | \
> > --
> > 2.17.1
>
> --
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
