[PATCH 1/2] drm/i915/gmbus: Lock AUX CH during GMBUS transfers

[Ville Syrjala <ville.syrjala@linux.intel.com>]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

It looks like that (at last on some platforms) GMBUS can interfere
with AUX transfers using the same pins. The symptom is that the AUX
transfer never gets the timeout interrupt from the hardware, and we
eventually reach the wait_event_timeout() timeout.

Prevent parallel GMBUS and AUX actitivity by locking the AUX mutexes
along with gmbus.mutex. We already use the same nested locking order
during CDCLK changes, so it's fine. Ideally we'd just lock the single
AUX mutex that corresponds to the pins being used by GMBUS, but
figuring out that mapping needs a bit more work.

shard-dg1 seems very good at hitting this but IIRC we've see this
on tgl as well at some point, possibly other machines as well:
<7> [197.185322] i915 0000:03:00.0: [drm:intel_dp_aux_xfer [i915]] AUX USBC2/DDI TC2/PHY D: timeout (status 0x7d40023f)
<7> [197.206471] i915 0000:03:00.0: [drm:intel_dp_aux_xfer [i915]] AUX USBC2/DDI TC2/PHY D: timeout (status 0x7d40023f)
<7> [197.206654] i915 0000:03:00.0: [drm:drm_dp_dpcd_access [drm_display_helper]] AUX USBC2/DDI TC2/PHY D: Too many retries, giving up. First error: -110
<7> [197.206704] i915 0000:03:00.0: [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:623:DP-3] disconnected
<7> [197.206733] i915 0000:03:00.0: [drm:drm_helper_probe_single_connector_modes] [CONNECTOR:631:HDMI-A-4]
<7> [197.206739] i915 0000:03:00.0: [drm:intel_hdmi_detect [i915]] [CONNECTOR:631:HDMI-A-4]
<3> [197.217425] i915 0000:03:00.0: [drm] *ERROR* AUX USBC2/DDI TC2/PHY D: did not complete or timeout within 10ms (status 0xad40023f)
<3> [197.230328] i915 0000:03:00.0: [drm] *ERROR* AUX USBC2/DDI TC2/PHY D: did not complete or timeout within 10ms (status 0xad40023f)
<7> [197.231108] i915 0000:03:00.0: [drm:update_display_info.part.0] [CONNECTOR:631:HDMI-A-4] ELD monitor FH-DP4K
...
<7> [197.243982] i915 0000:03:00.0: [drm:drm_client_dev_restore] fbdev: ret=0
<7> [197.244143] i915 0000:03:00.0: [drm:intel_dp_aux_xfer [i915]] AUX USBC2/DDI TC2/PHY D: timeout (status 0x7d40023f)
<6> [197.260566] Console: switching to colour frame buffer device 240x67
<7> [197.265263] i915 0000:03:00.0: [drm:intel_dp_aux_xfer [i915]] AUX USBC2/DDI TC2/PHY D: timeout (status 0x7d40023f)

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_gmbus.c | 53 ++++++++++++++++++++--
 1 file changed, 50 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_gmbus.c b/drivers/gpu/drm/i915/display/intel_gmbus.c
index ea5cf8f51b31..4d0b8c82f6ff 100644
--- a/drivers/gpu/drm/i915/display/intel_gmbus.c
+++ b/drivers/gpu/drm/i915/display/intel_gmbus.c
@@ -834,7 +834,8 @@ int intel_gmbus_output_aksv(struct i2c_adapter *adapter)
 	int ret;
 
 	wakeref = intel_display_power_get(display, POWER_DOMAIN_GMBUS);
-	mutex_lock(&display->gmbus.mutex);
+
+	adapter->lock_ops->lock_bus(adapter, 0);
 
 	/*
 	 * In order to output Aksv to the receiver, use an indexed write to
@@ -843,7 +844,8 @@ int intel_gmbus_output_aksv(struct i2c_adapter *adapter)
 	 */
 	ret = do_gmbus_xfer(adapter, msgs, ARRAY_SIZE(msgs), GMBUS_AKSV_SELECT);
 
-	mutex_unlock(&display->gmbus.mutex);
+	adapter->lock_ops->unlock_bus(adapter, 0);
+
 	intel_display_power_put(display, POWER_DOMAIN_GMBUS, wakeref);
 
 	return ret;
@@ -863,6 +865,36 @@ static const struct i2c_algorithm gmbus_algorithm = {
 	.functionality	= gmbus_func
 };
 
+static void gmbus_lock_aux(struct intel_display *display)
+{
+	struct intel_encoder *encoder;
+
+	/*
+	 * GMBUS can interfere with AUX CH on the same pins, causing
+	 * the AUX hardware to not raise the timeout interrupt.
+	 *
+	 * TODO: only lock the AUX CH using the same pins as
+	 *       GMBUS is currently using...
+	 */
+	for_each_intel_dp(display->drm, encoder) {
+		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
+
+		mutex_lock_nest_lock(&intel_dp->aux.hw_mutex,
+				     &display->gmbus.mutex);
+	}
+}
+
+static void gmbus_unlock_aux(struct intel_display *display)
+{
+	struct intel_encoder *encoder;
+
+	for_each_intel_dp(display->drm, encoder) {
+		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
+
+		mutex_unlock(&intel_dp->aux.hw_mutex);
+	}
+}
+
 static void gmbus_lock_bus(struct i2c_adapter *adapter,
 			   unsigned int flags)
 {
@@ -870,6 +902,8 @@ static void gmbus_lock_bus(struct i2c_adapter *adapter,
 	struct intel_display *display = bus->display;
 
 	mutex_lock(&display->gmbus.mutex);
+
+	gmbus_lock_aux(display);
 }
 
 static int gmbus_trylock_bus(struct i2c_adapter *adapter,
@@ -877,8 +911,19 @@ static int gmbus_trylock_bus(struct i2c_adapter *adapter,
 {
 	struct intel_gmbus *bus = to_intel_gmbus(adapter);
 	struct intel_display *display = bus->display;
+	int locked;
 
-	return mutex_trylock(&display->gmbus.mutex);
+	locked = mutex_trylock(&display->gmbus.mutex);
+	if (!locked)
+		return 0;
+
+	/*
+	 * TODO use mutex_trylock_next_lock() here? Would need
+	 * to track which mutexes to unlocks on failure...
+	 */
+	gmbus_lock_aux(display);
+
+	return 1;
 }
 
 static void gmbus_unlock_bus(struct i2c_adapter *adapter,
@@ -887,6 +932,8 @@ static void gmbus_unlock_bus(struct i2c_adapter *adapter,
 	struct intel_gmbus *bus = to_intel_gmbus(adapter);
 	struct intel_display *display = bus->display;
 
+	gmbus_unlock_aux(display);
+
 	mutex_unlock(&display->gmbus.mutex);
 }
 
-- 
2.52.0