| Message ID | 20190620140600.11357-15-imre.deak@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Fix TypeC port mode switching | expand |
On Thu, Jun 20, 2019 at 05:05:51PM +0300, Imre Deak wrote: > We must keep the TypeC port mode fixed for the duration of the connector > detection and each AUX transfers. Add a new TypeC lock holding it around > these two sequences. For consistency also hold the lock during the port > mode sanitization. > > Whenever resetting the port mode (only during the detection for now) the > port's AUX power domain must be disabled already. Flush the async power > domain disabling work to ensure this. > > A follow-up patch will make the port mode changing more robust by > postponing the change for active ports. > > v2: > - Fix checkpatch issue: missing annotation for tc_lock. > > Cc: José Roberto de Souza <jose.souza@intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > 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> lgtm. Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/display/intel_dp.c | 7 ++++++ > drivers/gpu/drm/i915/display/intel_tc.c | 30 ++++++++++++++++++++++++- > drivers/gpu/drm/i915/display/intel_tc.h | 2 ++ > drivers/gpu/drm/i915/intel_drv.h | 1 + > 4 files changed, 39 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c > index 0c6afec78f93..8f7188d71d08 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > @@ -1192,6 +1192,7 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp, > struct drm_i915_private *i915 = > to_i915(intel_dig_port->base.base.dev); > struct intel_uncore *uncore = &i915->uncore; > + bool is_tc_port = intel_port_is_tc(i915, intel_dig_port->base.port); > i915_reg_t ch_ctl, ch_data[5]; > u32 aux_clock_divider; > enum intel_display_power_domain aux_domain = > @@ -1207,6 +1208,9 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp, > for (i = 0; i < ARRAY_SIZE(ch_data); i++) > ch_data[i] = intel_dp->aux_ch_data_reg(intel_dp, i); > > + if (is_tc_port) > + intel_tc_port_lock(intel_dig_port); > + > aux_wakeref = intel_display_power_get(i915, aux_domain); > pps_wakeref = pps_lock(intel_dp); > > @@ -1359,6 +1363,9 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp, > pps_unlock(intel_dp, pps_wakeref); > intel_display_power_put_async(i915, aux_domain, aux_wakeref); > > + if (is_tc_port) > + intel_tc_port_unlock(intel_dig_port); > + > return ret; > } > > diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c > index 81b0d5676108..019bc53af28b 100644 > --- a/drivers/gpu/drm/i915/display/intel_tc.c > +++ b/drivers/gpu/drm/i915/display/intel_tc.c > @@ -296,8 +296,11 @@ intel_tc_port_get_target_mode(struct intel_digital_port *dig_port) > > static void intel_tc_port_reset_mode(struct intel_digital_port *dig_port) > { > + struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); > enum tc_port_mode old_tc_mode = dig_port->tc_mode; > > + intel_display_power_flush_work(dev_priv); > + > icl_tc_phy_disconnect(dig_port); > icl_tc_phy_connect(dig_port); > > @@ -312,6 +315,8 @@ void intel_tc_port_sanitize(struct intel_digital_port *dig_port) > struct intel_encoder *encoder = &dig_port->base; > int active_links = 0; > > + mutex_lock(&dig_port->tc_lock); > + > dig_port->tc_mode = intel_tc_port_get_current_mode(dig_port); > if (dig_port->dp.is_mst) > active_links = intel_dp_mst_encoder_active_links(dig_port); > @@ -332,6 +337,8 @@ void intel_tc_port_sanitize(struct intel_digital_port *dig_port) > DRM_DEBUG_KMS("Port %s: sanitize mode (%s)\n", > dig_port->tc_port_name, > tc_port_mode_name(dig_port->tc_mode)); > + > + mutex_unlock(&dig_port->tc_lock); > } > > static bool intel_tc_port_needs_reset(struct intel_digital_port *dig_port) > @@ -351,10 +358,30 @@ static bool intel_tc_port_needs_reset(struct intel_digital_port *dig_port) > */ > bool intel_tc_port_connected(struct intel_digital_port *dig_port) > { > + bool is_connected; > + > + mutex_lock(&dig_port->tc_lock); > + > if (intel_tc_port_needs_reset(dig_port)) > intel_tc_port_reset_mode(dig_port); > > - return tc_port_live_status_mask(dig_port) & BIT(dig_port->tc_mode); > + is_connected = tc_port_live_status_mask(dig_port) & > + BIT(dig_port->tc_mode); > + > + mutex_unlock(&dig_port->tc_lock); > + > + return is_connected; > +} > + > +void intel_tc_port_lock(struct intel_digital_port *dig_port) > +{ > + mutex_lock(&dig_port->tc_lock); > + /* TODO: reset the TypeC port mode if needed */ > +} > + > +void intel_tc_port_unlock(struct intel_digital_port *dig_port) > +{ > + mutex_unlock(&dig_port->tc_lock); > } > > void intel_tc_port_init(struct intel_digital_port *dig_port, bool is_legacy) > @@ -369,5 +396,6 @@ void intel_tc_port_init(struct intel_digital_port *dig_port, bool is_legacy) > snprintf(dig_port->tc_port_name, sizeof(dig_port->tc_port_name), > "%c/TC#%d", port_name(port), tc_port + 1); > > + mutex_init(&dig_port->tc_lock); > dig_port->tc_legacy_port = is_legacy; > } > diff --git a/drivers/gpu/drm/i915/display/intel_tc.h b/drivers/gpu/drm/i915/display/intel_tc.h > index 5a7876a74522..b5af2fe60b22 100644 > --- a/drivers/gpu/drm/i915/display/intel_tc.h > +++ b/drivers/gpu/drm/i915/display/intel_tc.h > @@ -17,6 +17,8 @@ u32 intel_tc_port_get_lane_mask(struct intel_digital_port *dig_port); > int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port); > > void intel_tc_port_sanitize(struct intel_digital_port *dig_port); > +void intel_tc_port_lock(struct intel_digital_port *dig_port); > +void intel_tc_port_unlock(struct intel_digital_port *dig_port); > > void intel_tc_port_init(struct intel_digital_port *dig_port, bool is_legacy); > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 19f6a360acde..d9e7d011ed4a 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1224,6 +1224,7 @@ struct intel_digital_port { > /* Used for DP and ICL+ TypeC/DP and TypeC/HDMI ports. */ > enum aux_ch aux_ch; > enum intel_display_power_domain ddi_io_power_domain; > + struct mutex tc_lock; /* protects the TypeC port mode */ > bool tc_legacy_port:1; > char tc_port_name[8]; > enum tc_port_mode tc_mode; > -- > 2.17.1
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c index 0c6afec78f93..8f7188d71d08 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -1192,6 +1192,7 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp, struct drm_i915_private *i915 = to_i915(intel_dig_port->base.base.dev); struct intel_uncore *uncore = &i915->uncore; + bool is_tc_port = intel_port_is_tc(i915, intel_dig_port->base.port); i915_reg_t ch_ctl, ch_data[5]; u32 aux_clock_divider; enum intel_display_power_domain aux_domain = @@ -1207,6 +1208,9 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp, for (i = 0; i < ARRAY_SIZE(ch_data); i++) ch_data[i] = intel_dp->aux_ch_data_reg(intel_dp, i); + if (is_tc_port) + intel_tc_port_lock(intel_dig_port); + aux_wakeref = intel_display_power_get(i915, aux_domain); pps_wakeref = pps_lock(intel_dp); @@ -1359,6 +1363,9 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp, pps_unlock(intel_dp, pps_wakeref); intel_display_power_put_async(i915, aux_domain, aux_wakeref); + if (is_tc_port) + intel_tc_port_unlock(intel_dig_port); + return ret; } diff --git a/drivers/gpu/drm/i915/display/intel_tc.c b/drivers/gpu/drm/i915/display/intel_tc.c index 81b0d5676108..019bc53af28b 100644 --- a/drivers/gpu/drm/i915/display/intel_tc.c +++ b/drivers/gpu/drm/i915/display/intel_tc.c @@ -296,8 +296,11 @@ intel_tc_port_get_target_mode(struct intel_digital_port *dig_port) static void intel_tc_port_reset_mode(struct intel_digital_port *dig_port) { + struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev); enum tc_port_mode old_tc_mode = dig_port->tc_mode; + intel_display_power_flush_work(dev_priv); + icl_tc_phy_disconnect(dig_port); icl_tc_phy_connect(dig_port); @@ -312,6 +315,8 @@ void intel_tc_port_sanitize(struct intel_digital_port *dig_port) struct intel_encoder *encoder = &dig_port->base; int active_links = 0; + mutex_lock(&dig_port->tc_lock); + dig_port->tc_mode = intel_tc_port_get_current_mode(dig_port); if (dig_port->dp.is_mst) active_links = intel_dp_mst_encoder_active_links(dig_port); @@ -332,6 +337,8 @@ void intel_tc_port_sanitize(struct intel_digital_port *dig_port) DRM_DEBUG_KMS("Port %s: sanitize mode (%s)\n", dig_port->tc_port_name, tc_port_mode_name(dig_port->tc_mode)); + + mutex_unlock(&dig_port->tc_lock); } static bool intel_tc_port_needs_reset(struct intel_digital_port *dig_port) @@ -351,10 +358,30 @@ static bool intel_tc_port_needs_reset(struct intel_digital_port *dig_port) */ bool intel_tc_port_connected(struct intel_digital_port *dig_port) { + bool is_connected; + + mutex_lock(&dig_port->tc_lock); + if (intel_tc_port_needs_reset(dig_port)) intel_tc_port_reset_mode(dig_port); - return tc_port_live_status_mask(dig_port) & BIT(dig_port->tc_mode); + is_connected = tc_port_live_status_mask(dig_port) & + BIT(dig_port->tc_mode); + + mutex_unlock(&dig_port->tc_lock); + + return is_connected; +} + +void intel_tc_port_lock(struct intel_digital_port *dig_port) +{ + mutex_lock(&dig_port->tc_lock); + /* TODO: reset the TypeC port mode if needed */ +} + +void intel_tc_port_unlock(struct intel_digital_port *dig_port) +{ + mutex_unlock(&dig_port->tc_lock); } void intel_tc_port_init(struct intel_digital_port *dig_port, bool is_legacy) @@ -369,5 +396,6 @@ void intel_tc_port_init(struct intel_digital_port *dig_port, bool is_legacy) snprintf(dig_port->tc_port_name, sizeof(dig_port->tc_port_name), "%c/TC#%d", port_name(port), tc_port + 1); + mutex_init(&dig_port->tc_lock); dig_port->tc_legacy_port = is_legacy; } diff --git a/drivers/gpu/drm/i915/display/intel_tc.h b/drivers/gpu/drm/i915/display/intel_tc.h index 5a7876a74522..b5af2fe60b22 100644 --- a/drivers/gpu/drm/i915/display/intel_tc.h +++ b/drivers/gpu/drm/i915/display/intel_tc.h @@ -17,6 +17,8 @@ u32 intel_tc_port_get_lane_mask(struct intel_digital_port *dig_port); int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port); void intel_tc_port_sanitize(struct intel_digital_port *dig_port); +void intel_tc_port_lock(struct intel_digital_port *dig_port); +void intel_tc_port_unlock(struct intel_digital_port *dig_port); void intel_tc_port_init(struct intel_digital_port *dig_port, bool is_legacy); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 19f6a360acde..d9e7d011ed4a 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1224,6 +1224,7 @@ struct intel_digital_port { /* Used for DP and ICL+ TypeC/DP and TypeC/HDMI ports. */ enum aux_ch aux_ch; enum intel_display_power_domain ddi_io_power_domain; + struct mutex tc_lock; /* protects the TypeC port mode */ bool tc_legacy_port:1; char tc_port_name[8]; enum tc_port_mode tc_mode;
We must keep the TypeC port mode fixed for the duration of the connector detection and each AUX transfers. Add a new TypeC lock holding it around these two sequences. For consistency also hold the lock during the port mode sanitization. Whenever resetting the port mode (only during the detection for now) the port's AUX power domain must be disabled already. Flush the async power domain disabling work to ensure this. A follow-up patch will make the port mode changing more robust by postponing the change for active ports. v2: - Fix checkpatch issue: missing annotation for tc_lock. Cc: José Roberto de Souza <jose.souza@intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> 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/display/intel_dp.c | 7 ++++++ drivers/gpu/drm/i915/display/intel_tc.c | 30 ++++++++++++++++++++++++- drivers/gpu/drm/i915/display/intel_tc.h | 2 ++ drivers/gpu/drm/i915/intel_drv.h | 1 + 4 files changed, 39 insertions(+), 1 deletion(-)