diff mbox series

[v3,04/11] drm/i915: Shove the PHY test into the hotplug work

Message ID 20200930100412.9313-1-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series None | expand

Commit Message

Ville Syrjälä Sept. 30, 2020, 10:04 a.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Doing nay kind modeset stuff from the short hpd handler is
verboten. The ad-hoc PHY test modeset code violates this. And
by calling various link training related functions it's now
blocking further work to plumb the crtc state down into the
link training code.

Let's hack around that by pushing the PHY test stuff into the
hotplug work where it's less of a problem. Still not great but
at least acceptable. We take a few pages from the link retraining
handbook to handle the locking and whatnot.

v2: Fix the intel_dp_hotplug() return value

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

Comments

Imre Deak Sept. 30, 2020, 3:12 p.m. UTC | #1 
On Wed, Sep 30, 2020 at 01:04:12PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Doing nay kind modeset stuff from the short hpd handler is
> verboten. The ad-hoc PHY test modeset code violates this. And
> by calling various link training related functions it's now
> blocking further work to plumb the crtc state down into the
> link training code.
> 
> Let's hack around that by pushing the PHY test stuff into the
> hotplug work where it's less of a problem. Still not great but
> at least acceptable. We take a few pages from the link retraining
> handbook to handle the locking and whatnot.
> 
> v2: Fix the intel_dp_hotplug() return value
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Looks ok to me:
Reviewed-by: Imre Deak <imre.deak@intel.com>

CC'ing also Animesh and Manasi.

> ---
>  drivers/gpu/drm/i915/display/intel_dp.c | 154 ++++++++++++++++++++----
>  1 file changed, 128 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 5c673080ecb1..132b06a649d8 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -5424,25 +5424,6 @@ static u8 intel_dp_autotest_edid(struct intel_dp *intel_dp)
>  	return test_result;
>  }
>  
> -static u8 intel_dp_prepare_phytest(struct intel_dp *intel_dp)
> -{
> -	struct drm_dp_phy_test_params *data =
> -		&intel_dp->compliance.test_data.phytest;
> -
> -	if (drm_dp_get_phy_test_pattern(&intel_dp->aux, data)) {
> -		DRM_DEBUG_KMS("DP Phy Test pattern AUX read failure\n");
> -		return DP_TEST_NAK;
> -	}
> -
> -	/*
> -	 * link_mst is set to false to avoid executing mst related code
> -	 * during compliance testing.
> -	 */
> -	intel_dp->link_mst = false;
> -
> -	return DP_TEST_ACK;
> -}
> -
>  static void intel_dp_phy_pattern_update(struct intel_dp *intel_dp)
>  {
>  	struct drm_i915_private *dev_priv =
> @@ -5590,15 +5571,18 @@ static void intel_dp_process_phy_request(struct intel_dp *intel_dp)
>  
>  static u8 intel_dp_autotest_phy_pattern(struct intel_dp *intel_dp)
>  {
> -	u8 test_result;
> +	struct drm_dp_phy_test_params *data =
> +		&intel_dp->compliance.test_data.phytest;
>  
> -	test_result = intel_dp_prepare_phytest(intel_dp);
> -	if (test_result != DP_TEST_ACK)
> -		DRM_ERROR("Phy test preparation failed\n");
> +	if (drm_dp_get_phy_test_pattern(&intel_dp->aux, data)) {
> +		DRM_DEBUG_KMS("DP Phy Test pattern AUX read failure\n");
> +		return DP_TEST_NAK;
> +	}
>  
> -	intel_dp_process_phy_request(intel_dp);
> +	/* Set test active flag here so userspace doesn't interrupt things */
> +	intel_dp->compliance.test_active = true;
>  
> -	return test_result;
> +	return DP_TEST_ACK;
>  }
>  
>  static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
> @@ -5887,6 +5871,104 @@ int intel_dp_retrain_link(struct intel_encoder *encoder,
>  	return 0;
>  }
>  
> +static int intel_dp_prep_phy_test(struct intel_dp *intel_dp,
> +				  struct drm_modeset_acquire_ctx *ctx,
> +				  u32 *crtc_mask)
> +{
> +	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> +	struct drm_connector_list_iter conn_iter;
> +	struct intel_connector *connector;
> +	int ret = 0;
> +
> +	*crtc_mask = 0;
> +
> +	drm_connector_list_iter_begin(&i915->drm, &conn_iter);
> +	for_each_intel_connector_iter(connector, &conn_iter) {
> +		struct drm_connector_state *conn_state =
> +			connector->base.state;
> +		struct intel_crtc_state *crtc_state;
> +		struct intel_crtc *crtc;
> +
> +		if (!intel_dp_has_connector(intel_dp, conn_state))
> +			continue;
> +
> +		crtc = to_intel_crtc(conn_state->crtc);
> +		if (!crtc)
> +			continue;
> +
> +		ret = drm_modeset_lock(&crtc->base.mutex, ctx);
> +		if (ret)
> +			break;
> +
> +		crtc_state = to_intel_crtc_state(crtc->base.state);
> +
> +		drm_WARN_ON(&i915->drm, !intel_crtc_has_dp_encoder(crtc_state));
> +
> +		if (!crtc_state->hw.active)
> +			continue;
> +
> +		if (conn_state->commit &&
> +		    !try_wait_for_completion(&conn_state->commit->hw_done))
> +			continue;
> +
> +		*crtc_mask |= drm_crtc_mask(&crtc->base);
> +	}
> +	drm_connector_list_iter_end(&conn_iter);
> +
> +	return ret;
> +}
> +
> +static int intel_dp_do_phy_test(struct intel_encoder *encoder,
> +				struct drm_modeset_acquire_ctx *ctx)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> +	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> +	u32 crtc_mask;
> +	int ret;
> +
> +	ret = drm_modeset_lock(&dev_priv->drm.mode_config.connection_mutex,
> +			       ctx);
> +	if (ret)
> +		return ret;
> +
> +	ret = intel_dp_prep_phy_test(intel_dp, ctx, &crtc_mask);
> +	if (ret)
> +		return ret;
> +
> +	if (crtc_mask == 0)
> +		return 0;
> +
> +	drm_dbg_kms(&dev_priv->drm, "[ENCODER:%d:%s] PHY test\n",
> +		    encoder->base.base.id, encoder->base.name);
> +	intel_dp_process_phy_request(intel_dp);
> +
> +	return 0;
> +}
> +
> +static void intel_dp_phy_test(struct intel_encoder *encoder)
> +{
> +	struct drm_modeset_acquire_ctx ctx;
> +	int ret;
> +
> +	drm_modeset_acquire_init(&ctx, 0);
> +
> +	for (;;) {
> +		ret = intel_dp_do_phy_test(encoder, &ctx);
> +
> +		if (ret == -EDEADLK) {
> +			drm_modeset_backoff(&ctx);
> +			continue;
> +		}
> +
> +		break;
> +	}
> +
> +	drm_modeset_drop_locks(&ctx);
> +	drm_modeset_acquire_fini(&ctx);
> +	drm_WARN(encoder->base.dev, ret,
> +		 "Acquiring modeset locks failed with %i\n", ret);
> +}
> +
>  /*
>   * If display is now connected check links status,
>   * there has been known issues of link loss triggering
> @@ -5903,10 +5985,18 @@ static enum intel_hotplug_state
>  intel_dp_hotplug(struct intel_encoder *encoder,
>  		 struct intel_connector *connector)
>  {
> +	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>  	struct drm_modeset_acquire_ctx ctx;
>  	enum intel_hotplug_state state;
>  	int ret;
>  
> +	if (intel_dp->compliance.test_active &&
> +	    intel_dp->compliance.test_type == DP_TEST_LINK_PHY_TEST_PATTERN) {
> +		intel_dp_phy_test(encoder);
> +		/* just do the PHY test and nothing else */
> +		return INTEL_HOTPLUG_UNCHANGED;
> +	}
> +
>  	state = intel_encoder_hotplug(encoder, connector);
>  
>  	drm_modeset_acquire_init(&ctx, 0);
> @@ -6011,11 +6101,23 @@ intel_dp_short_pulse(struct intel_dp *intel_dp)
>  
>  	intel_psr_short_pulse(intel_dp);
>  
> -	if (intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) {
> +	switch (intel_dp->compliance.test_type) {
> +	case DP_TEST_LINK_TRAINING:
>  		drm_dbg_kms(&dev_priv->drm,
>  			    "Link Training Compliance Test requested\n");
>  		/* Send a Hotplug Uevent to userspace to start modeset */
>  		drm_kms_helper_hotplug_event(&dev_priv->drm);
> +		break;
> +	case DP_TEST_LINK_PHY_TEST_PATTERN:
> +		drm_dbg_kms(&dev_priv->drm,
> +			    "PHY test pattern Compliance Test requested\n");
> +		/*
> +		 * Schedule long hpd to do the test
> +		 *
> +		 * FIXME get rid of the ad-hoc phy test modeset code
> +		 * and properly incorporate it into the normal modeset.
> +		 */
> +		return false;
>  	}
>  
>  	return true;
> -- 
> 2.26.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
index 5c673080ecb1..132b06a649d8 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -5424,25 +5424,6 @@  static u8 intel_dp_autotest_edid(struct intel_dp *intel_dp)
 	return test_result;
 }
 
-static u8 intel_dp_prepare_phytest(struct intel_dp *intel_dp)
-{
-	struct drm_dp_phy_test_params *data =
-		&intel_dp->compliance.test_data.phytest;
-
-	if (drm_dp_get_phy_test_pattern(&intel_dp->aux, data)) {
-		DRM_DEBUG_KMS("DP Phy Test pattern AUX read failure\n");
-		return DP_TEST_NAK;
-	}
-
-	/*
-	 * link_mst is set to false to avoid executing mst related code
-	 * during compliance testing.
-	 */
-	intel_dp->link_mst = false;
-
-	return DP_TEST_ACK;
-}
-
 static void intel_dp_phy_pattern_update(struct intel_dp *intel_dp)
 {
 	struct drm_i915_private *dev_priv =
@@ -5590,15 +5571,18 @@  static void intel_dp_process_phy_request(struct intel_dp *intel_dp)
 
 static u8 intel_dp_autotest_phy_pattern(struct intel_dp *intel_dp)
 {
-	u8 test_result;
+	struct drm_dp_phy_test_params *data =
+		&intel_dp->compliance.test_data.phytest;
 
-	test_result = intel_dp_prepare_phytest(intel_dp);
-	if (test_result != DP_TEST_ACK)
-		DRM_ERROR("Phy test preparation failed\n");
+	if (drm_dp_get_phy_test_pattern(&intel_dp->aux, data)) {
+		DRM_DEBUG_KMS("DP Phy Test pattern AUX read failure\n");
+		return DP_TEST_NAK;
+	}
 
-	intel_dp_process_phy_request(intel_dp);
+	/* Set test active flag here so userspace doesn't interrupt things */
+	intel_dp->compliance.test_active = true;
 
-	return test_result;
+	return DP_TEST_ACK;
 }
 
 static void intel_dp_handle_test_request(struct intel_dp *intel_dp)
@@ -5887,6 +5871,104 @@  int intel_dp_retrain_link(struct intel_encoder *encoder,
 	return 0;
 }
 
+static int intel_dp_prep_phy_test(struct intel_dp *intel_dp,
+				  struct drm_modeset_acquire_ctx *ctx,
+				  u32 *crtc_mask)
+{
+	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
+	struct drm_connector_list_iter conn_iter;
+	struct intel_connector *connector;
+	int ret = 0;
+
+	*crtc_mask = 0;
+
+	drm_connector_list_iter_begin(&i915->drm, &conn_iter);
+	for_each_intel_connector_iter(connector, &conn_iter) {
+		struct drm_connector_state *conn_state =
+			connector->base.state;
+		struct intel_crtc_state *crtc_state;
+		struct intel_crtc *crtc;
+
+		if (!intel_dp_has_connector(intel_dp, conn_state))
+			continue;
+
+		crtc = to_intel_crtc(conn_state->crtc);
+		if (!crtc)
+			continue;
+
+		ret = drm_modeset_lock(&crtc->base.mutex, ctx);
+		if (ret)
+			break;
+
+		crtc_state = to_intel_crtc_state(crtc->base.state);
+
+		drm_WARN_ON(&i915->drm, !intel_crtc_has_dp_encoder(crtc_state));
+
+		if (!crtc_state->hw.active)
+			continue;
+
+		if (conn_state->commit &&
+		    !try_wait_for_completion(&conn_state->commit->hw_done))
+			continue;
+
+		*crtc_mask |= drm_crtc_mask(&crtc->base);
+	}
+	drm_connector_list_iter_end(&conn_iter);
+
+	return ret;
+}
+
+static int intel_dp_do_phy_test(struct intel_encoder *encoder,
+				struct drm_modeset_acquire_ctx *ctx)
+{
+	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
+	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
+	u32 crtc_mask;
+	int ret;
+
+	ret = drm_modeset_lock(&dev_priv->drm.mode_config.connection_mutex,
+			       ctx);
+	if (ret)
+		return ret;
+
+	ret = intel_dp_prep_phy_test(intel_dp, ctx, &crtc_mask);
+	if (ret)
+		return ret;
+
+	if (crtc_mask == 0)
+		return 0;
+
+	drm_dbg_kms(&dev_priv->drm, "[ENCODER:%d:%s] PHY test\n",
+		    encoder->base.base.id, encoder->base.name);
+	intel_dp_process_phy_request(intel_dp);
+
+	return 0;
+}
+
+static void intel_dp_phy_test(struct intel_encoder *encoder)
+{
+	struct drm_modeset_acquire_ctx ctx;
+	int ret;
+
+	drm_modeset_acquire_init(&ctx, 0);
+
+	for (;;) {
+		ret = intel_dp_do_phy_test(encoder, &ctx);
+
+		if (ret == -EDEADLK) {
+			drm_modeset_backoff(&ctx);
+			continue;
+		}
+
+		break;
+	}
+
+	drm_modeset_drop_locks(&ctx);
+	drm_modeset_acquire_fini(&ctx);
+	drm_WARN(encoder->base.dev, ret,
+		 "Acquiring modeset locks failed with %i\n", ret);
+}
+
 /*
  * If display is now connected check links status,
  * there has been known issues of link loss triggering
@@ -5903,10 +5985,18 @@  static enum intel_hotplug_state
 intel_dp_hotplug(struct intel_encoder *encoder,
 		 struct intel_connector *connector)
 {
+	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
 	struct drm_modeset_acquire_ctx ctx;
 	enum intel_hotplug_state state;
 	int ret;
 
+	if (intel_dp->compliance.test_active &&
+	    intel_dp->compliance.test_type == DP_TEST_LINK_PHY_TEST_PATTERN) {
+		intel_dp_phy_test(encoder);
+		/* just do the PHY test and nothing else */
+		return INTEL_HOTPLUG_UNCHANGED;
+	}
+
 	state = intel_encoder_hotplug(encoder, connector);
 
 	drm_modeset_acquire_init(&ctx, 0);
@@ -6011,11 +6101,23 @@  intel_dp_short_pulse(struct intel_dp *intel_dp)
 
 	intel_psr_short_pulse(intel_dp);
 
-	if (intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) {
+	switch (intel_dp->compliance.test_type) {
+	case DP_TEST_LINK_TRAINING:
 		drm_dbg_kms(&dev_priv->drm,
 			    "Link Training Compliance Test requested\n");
 		/* Send a Hotplug Uevent to userspace to start modeset */
 		drm_kms_helper_hotplug_event(&dev_priv->drm);
+		break;
+	case DP_TEST_LINK_PHY_TEST_PATTERN:
+		drm_dbg_kms(&dev_priv->drm,
+			    "PHY test pattern Compliance Test requested\n");
+		/*
+		 * Schedule long hpd to do the test
+		 *
+		 * FIXME get rid of the ad-hoc phy test modeset code
+		 * and properly incorporate it into the normal modeset.
+		 */
+		return false;
 	}
 
 	return true;