[PATCH] drm/i915: Kill sink_crc for good

[PATCH] drm/i915: Kill sink_crc for good

[Rodrigo Vivi]

It was originally introduced following the VESA spec in order to validate PSR.

However we found so many issues around sink_crc that instead of helping PSR
development it only brought another layer of trouble to the table.

So, sink_crc has been a black whole for us in question of time, effort and hope.

First of the problems is that HW statement is clear: "Do not attempt to use
aux communication with PSR enabled". So the main reason behind sink_crc is
already compromised.

For a while we had hope on the aux-mutex could workaround this problem on SKL+
platforms, but that mutex was not reliable, not tested,
and we shouldn't use according to HW engineers.

Also, nor source, nor sink designed and implemented the sink_crc to be used like
we are trying to use here.

Well, the sink side of things is also apparently not prepared for this
case. Each panel that we tried seemed to have a different behavior with same
code and same source.

So, for all the time we lost on trying to ducktape all these different issues
I believe it is now time to move PSR to a more reliable validation.
Maybe not a perfect one as we dreamed for this sink_crc, but at least more
reliable.

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c |  81 ------------------
 drivers/gpu/drm/i915/intel_dp.c     | 123 ----------------------------
 drivers/gpu/drm/i915/intel_drv.h    |   2 -
 3 files changed, 206 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index f6142d78ede4..e8fa106a3655 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2761,86 +2761,6 @@ DEFINE_SIMPLE_ATTRIBUTE(i915_edp_psr_debug_fops,
 			i915_edp_psr_debug_get, i915_edp_psr_debug_set,
 			"%llu\n");
 
-static int i915_sink_crc(struct seq_file *m, void *data)
-{
-	struct drm_i915_private *dev_priv = node_to_i915(m->private);
-	struct drm_device *dev = &dev_priv->drm;
-	struct intel_connector *connector;
-	struct drm_connector_list_iter conn_iter;
-	struct intel_dp *intel_dp = NULL;
-	struct drm_modeset_acquire_ctx ctx;
-	int ret;
-	u8 crc[6];
-
-	drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
-
-	drm_connector_list_iter_begin(dev, &conn_iter);
-
-	for_each_intel_connector_iter(connector, &conn_iter) {
-		struct drm_crtc *crtc;
-		struct drm_connector_state *state;
-		struct intel_crtc_state *crtc_state;
-
-		if (connector->base.connector_type != DRM_MODE_CONNECTOR_eDP)
-			continue;
-
-retry:
-		ret = drm_modeset_lock(&dev->mode_config.connection_mutex, &ctx);
-		if (ret)
-			goto err;
-
-		state = connector->base.state;
-		if (!state->best_encoder)
-			continue;
-
-		crtc = state->crtc;
-		ret = drm_modeset_lock(&crtc->mutex, &ctx);
-		if (ret)
-			goto err;
-
-		crtc_state = to_intel_crtc_state(crtc->state);
-		if (!crtc_state->base.active)
-			continue;
-
-		/*
-		 * We need to wait for all crtc updates to complete, to make
-		 * sure any pending modesets and plane updates are completed.
-		 */
-		if (crtc_state->base.commit) {
-			ret = wait_for_completion_interruptible(&crtc_state->base.commit->hw_done);
-
-			if (ret)
-				goto err;
-		}
-
-		intel_dp = enc_to_intel_dp(state->best_encoder);
-
-		ret = intel_dp_sink_crc(intel_dp, crtc_state, crc);
-		if (ret)
-			goto err;
-
-		seq_printf(m, "%02x%02x%02x%02x%02x%02x\n",
-			   crc[0], crc[1], crc[2],
-			   crc[3], crc[4], crc[5]);
-		goto out;
-
-err:
-		if (ret == -EDEADLK) {
-			ret = drm_modeset_backoff(&ctx);
-			if (!ret)
-				goto retry;
-		}
-		goto out;
-	}
-	ret = -ENODEV;
-out:
-	drm_connector_list_iter_end(&conn_iter);
-	drm_modeset_drop_locks(&ctx);
-	drm_modeset_acquire_fini(&ctx);
-
-	return ret;
-}
-
 static int i915_energy_uJ(struct seq_file *m, void *data)
 {
 	struct drm_i915_private *dev_priv = node_to_i915(m->private);
@@ -4784,7 +4704,6 @@ static const struct drm_info_list i915_debugfs_list[] = {
 	{"i915_ppgtt_info", i915_ppgtt_info, 0},
 	{"i915_llc", i915_llc, 0},
 	{"i915_edp_psr_status", i915_edp_psr_status, 0},
-	{"i915_sink_crc_eDP1", i915_sink_crc, 0},
 	{"i915_energy_uJ", i915_energy_uJ, 0},
 	{"i915_runtime_pm_status", i915_runtime_pm_status, 0},
 	{"i915_power_domain_info", i915_power_domain_info, 0},
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 5be07e1d816d..efdd4b408d51 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3905,129 +3905,6 @@ intel_dp_configure_mst(struct intel_dp *intel_dp)
 					intel_dp->is_mst);
 }
 
-static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp,
-				  struct intel_crtc_state *crtc_state, bool disable_wa)
-{
-	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);
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state->base.crtc);
-	u8 buf;
-	int ret = 0;
-	int count = 0;
-	int attempts = 10;
-
-	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) {
-		DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
-		ret = -EIO;
-		goto out;
-	}
-
-	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
-			       buf & ~DP_TEST_SINK_START) < 0) {
-		DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
-		ret = -EIO;
-		goto out;
-	}
-
-	do {
-		intel_wait_for_vblank(dev_priv, intel_crtc->pipe);
-
-		if (drm_dp_dpcd_readb(&intel_dp->aux,
-				      DP_TEST_SINK_MISC, &buf) < 0) {
-			ret = -EIO;
-			goto out;
-		}
-		count = buf & DP_TEST_COUNT_MASK;
-	} while (--attempts && count);
-
-	if (attempts == 0) {
-		DRM_DEBUG_KMS("TIMEOUT: Sink CRC counter is not zeroed after calculation is stopped\n");
-		ret = -ETIMEDOUT;
-	}
-
- out:
-	if (disable_wa)
-		hsw_enable_ips(crtc_state);
-	return ret;
-}
-
-static int intel_dp_sink_crc_start(struct intel_dp *intel_dp,
-				   struct intel_crtc_state *crtc_state)
-{
-	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);
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state->base.crtc);
-	u8 buf;
-	int ret;
-
-	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf) < 0)
-		return -EIO;
-
-	if (!(buf & DP_TEST_CRC_SUPPORTED))
-		return -ENOTTY;
-
-	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0)
-		return -EIO;
-
-	if (buf & DP_TEST_SINK_START) {
-		ret = intel_dp_sink_crc_stop(intel_dp, crtc_state, false);
-		if (ret)
-			return ret;
-	}
-
-	hsw_disable_ips(crtc_state);
-
-	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
-			       buf | DP_TEST_SINK_START) < 0) {
-		hsw_enable_ips(crtc_state);
-		return -EIO;
-	}
-
-	intel_wait_for_vblank(dev_priv, intel_crtc->pipe);
-	return 0;
-}
-
-int intel_dp_sink_crc(struct intel_dp *intel_dp, struct intel_crtc_state *crtc_state, u8 *crc)
-{
-	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);
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state->base.crtc);
-	u8 buf;
-	int count, ret;
-	int attempts = 6;
-
-	ret = intel_dp_sink_crc_start(intel_dp, crtc_state);
-	if (ret)
-		return ret;
-
-	do {
-		intel_wait_for_vblank(dev_priv, intel_crtc->pipe);
-
-		if (drm_dp_dpcd_readb(&intel_dp->aux,
-				      DP_TEST_SINK_MISC, &buf) < 0) {
-			ret = -EIO;
-			goto stop;
-		}
-		count = buf & DP_TEST_COUNT_MASK;
-
-	} while (--attempts && count == 0);
-
-	if (attempts == 0) {
-		DRM_ERROR("Panel is unable to calculate any CRC after 6 vblanks\n");
-		ret = -ETIMEDOUT;
-		goto stop;
-	}
-
-	if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0) {
-		ret = -EIO;
-		goto stop;
-	}
-
-stop:
-	intel_dp_sink_crc_stop(intel_dp, crtc_state, true);
-	return ret;
-}
-
 static bool
 intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector)
 {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index e09b42d91d1e..8e6ee2fe66e4 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1665,8 +1665,6 @@ void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
 void intel_dp_encoder_reset(struct drm_encoder *encoder);
 void intel_dp_encoder_suspend(struct intel_encoder *intel_encoder);
 void intel_dp_encoder_destroy(struct drm_encoder *encoder);
-int intel_dp_sink_crc(struct intel_dp *intel_dp,
-		      struct intel_crtc_state *crtc_state, u8 *crc);
 bool intel_dp_compute_config(struct intel_encoder *encoder,
 			     struct intel_crtc_state *pipe_config,
 			     struct drm_connector_state *conn_state);
-- 
2.17.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Kill sink_crc for good

[Dhinakaran Pandiyan]

On Thursday, July 5, 2018 12:25:28 PM PDT Rodrigo Vivi wrote:
> It was originally introduced following the VESA spec in order to validate
> PSR.
> 
> However we found so many issues around sink_crc that instead of helping PSR
> development it only brought another layer of trouble to the table.
> 
> So, sink_crc has been a black whole for us in question of time, effort and
> hope.
> 
> First of the problems is that HW statement is clear: "Do not attempt to use
> aux communication with PSR enabled". So the main reason behind sink_crc is
> already compromised.
> 
> For a while we had hope on the aux-mutex could workaround this problem on
> SKL+ platforms, but that mutex was not reliable, not tested,
> and we shouldn't use according to HW engineers.
> 
> Also, nor source, nor sink designed and implemented the sink_crc to be used
> like we are trying to use here.
> 
> Well, the sink side of things is also apparently not prepared for this
> case. Each panel that we tried seemed to have a different behavior with same
> code and same source.
> 
> So, for all the time we lost on trying to ducktape all these different
> issues I believe it is now time to move PSR to a more reliable validation.
> Maybe not a perfect one as we dreamed for this sink_crc, but at least more
> reliable.
> 
Good riddance.
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c |  81 ------------------
>  drivers/gpu/drm/i915/intel_dp.c     | 123 ----------------------------
>  drivers/gpu/drm/i915/intel_drv.h    |   2 -
>  3 files changed, 206 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c index f6142d78ede4..e8fa106a3655
> 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2761,86 +2761,6 @@ DEFINE_SIMPLE_ATTRIBUTE(i915_edp_psr_debug_fops,
>  			i915_edp_psr_debug_get, i915_edp_psr_debug_set,
>  			"%llu\n");
> 
> -static int i915_sink_crc(struct seq_file *m, void *data)
> -{
> -	struct drm_i915_private *dev_priv = node_to_i915(m->private);
> -	struct drm_device *dev = &dev_priv->drm;
> -	struct intel_connector *connector;
> -	struct drm_connector_list_iter conn_iter;
> -	struct intel_dp *intel_dp = NULL;
> -	struct drm_modeset_acquire_ctx ctx;
> -	int ret;
> -	u8 crc[6];
> -
> -	drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
> -
> -	drm_connector_list_iter_begin(dev, &conn_iter);
> -
> -	for_each_intel_connector_iter(connector, &conn_iter) {
> -		struct drm_crtc *crtc;
> -		struct drm_connector_state *state;
> -		struct intel_crtc_state *crtc_state;
> -
> -		if (connector->base.connector_type != DRM_MODE_CONNECTOR_eDP)
> -			continue;
> -
> -retry:
> -		ret = drm_modeset_lock(&dev->mode_config.connection_mutex, &ctx);
> -		if (ret)
> -			goto err;
> -
> -		state = connector->base.state;
> -		if (!state->best_encoder)
> -			continue;
> -
> -		crtc = state->crtc;
> -		ret = drm_modeset_lock(&crtc->mutex, &ctx);
> -		if (ret)
> -			goto err;
> -
> -		crtc_state = to_intel_crtc_state(crtc->state);
> -		if (!crtc_state->base.active)
> -			continue;
> -
> -		/*
> -		 * We need to wait for all crtc updates to complete, to make
> -		 * sure any pending modesets and plane updates are completed.
> -		 */
> -		if (crtc_state->base.commit) {
> -			ret =
> wait_for_completion_interruptible(&crtc_state->base.commit->hw_done); -
> -			if (ret)
> -				goto err;
> -		}
> -
> -		intel_dp = enc_to_intel_dp(state->best_encoder);
> -
> -		ret = intel_dp_sink_crc(intel_dp, crtc_state, crc);
> -		if (ret)
> -			goto err;
> -
> -		seq_printf(m, "%02x%02x%02x%02x%02x%02x\n",
> -			   crc[0], crc[1], crc[2],
> -			   crc[3], crc[4], crc[5]);
> -		goto out;
> -
> -err:
> -		if (ret == -EDEADLK) {
> -			ret = drm_modeset_backoff(&ctx);
> -			if (!ret)
> -				goto retry;
> -		}
> -		goto out;
> -	}
> -	ret = -ENODEV;
> -out:
> -	drm_connector_list_iter_end(&conn_iter);
> -	drm_modeset_drop_locks(&ctx);
> -	drm_modeset_acquire_fini(&ctx);
> -
> -	return ret;
> -}
> -
>  static int i915_energy_uJ(struct seq_file *m, void *data)
>  {
>  	struct drm_i915_private *dev_priv = node_to_i915(m->private);
> @@ -4784,7 +4704,6 @@ static const struct drm_info_list i915_debugfs_list[]
> = { {"i915_ppgtt_info", i915_ppgtt_info, 0},
>  	{"i915_llc", i915_llc, 0},
>  	{"i915_edp_psr_status", i915_edp_psr_status, 0},
> -	{"i915_sink_crc_eDP1", i915_sink_crc, 0},
>  	{"i915_energy_uJ", i915_energy_uJ, 0},
>  	{"i915_runtime_pm_status", i915_runtime_pm_status, 0},
>  	{"i915_power_domain_info", i915_power_domain_info, 0},
> diff --git a/drivers/gpu/drm/i915/intel_dp.c
> b/drivers/gpu/drm/i915/intel_dp.c index 5be07e1d816d..efdd4b408d51 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3905,129 +3905,6 @@ intel_dp_configure_mst(struct intel_dp *intel_dp)
>  					intel_dp->is_mst);
>  }
> 
> -static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp,
> -				  struct intel_crtc_state *crtc_state, bool disable_wa)
> -{
> -	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);
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state->base.crtc);
> -	u8 buf;
> -	int ret = 0;
> -	int count = 0;
> -	int attempts = 10;
> -
> -	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) {
> -		DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
> -		ret = -EIO;
> -		goto out;
> -	}
> -
> -	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
> -			       buf & ~DP_TEST_SINK_START) < 0) {
> -		DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
> -		ret = -EIO;
> -		goto out;
> -	}
> -
> -	do {
> -		intel_wait_for_vblank(dev_priv, intel_crtc->pipe);
> -
> -		if (drm_dp_dpcd_readb(&intel_dp->aux,
> -				      DP_TEST_SINK_MISC, &buf) < 0) {
> -			ret = -EIO;
> -			goto out;
> -		}
> -		count = buf & DP_TEST_COUNT_MASK;
> -	} while (--attempts && count);
> -
> -	if (attempts == 0) {
> -		DRM_DEBUG_KMS("TIMEOUT: Sink CRC counter is not zeroed after 
calculation
> is stopped\n"); -		ret = -ETIMEDOUT;
> -	}
> -
> - out:
> -	if (disable_wa)
> -		hsw_enable_ips(crtc_state);
> -	return ret;
> -}
> -
> -static int intel_dp_sink_crc_start(struct intel_dp *intel_dp,
> -				   struct intel_crtc_state *crtc_state)
> -{
> -	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);
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state->base.crtc);
> -	u8 buf;
> -	int ret;
> -
> -	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf) < 0)
> -		return -EIO;
> -
> -	if (!(buf & DP_TEST_CRC_SUPPORTED))
> -		return -ENOTTY;
> -
> -	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0)
> -		return -EIO;
> -
> -	if (buf & DP_TEST_SINK_START) {
> -		ret = intel_dp_sink_crc_stop(intel_dp, crtc_state, false);
> -		if (ret)
> -			return ret;
> -	}
> -
> -	hsw_disable_ips(crtc_state);
> -
> -	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
> -			       buf | DP_TEST_SINK_START) < 0) {
> -		hsw_enable_ips(crtc_state);
> -		return -EIO;
> -	}
> -
> -	intel_wait_for_vblank(dev_priv, intel_crtc->pipe);
> -	return 0;
> -}
> -
> -int intel_dp_sink_crc(struct intel_dp *intel_dp, struct intel_crtc_state
> *crtc_state, u8 *crc) -{
> -	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);
> -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state->base.crtc);
> -	u8 buf;
> -	int count, ret;
> -	int attempts = 6;
> -
> -	ret = intel_dp_sink_crc_start(intel_dp, crtc_state);
> -	if (ret)
> -		return ret;
> -
> -	do {
> -		intel_wait_for_vblank(dev_priv, intel_crtc->pipe);
> -
> -		if (drm_dp_dpcd_readb(&intel_dp->aux,
> -				      DP_TEST_SINK_MISC, &buf) < 0) {
> -			ret = -EIO;
> -			goto stop;
> -		}
> -		count = buf & DP_TEST_COUNT_MASK;
> -
> -	} while (--attempts && count == 0);
> -
> -	if (attempts == 0) {
> -		DRM_ERROR("Panel is unable to calculate any CRC after 6 vblanks\n");
> -		ret = -ETIMEDOUT;
> -		goto stop;
> -	}
> -
> -	if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0) {
> -		ret = -EIO;
> -		goto stop;
> -	}
> -
> -stop:
> -	intel_dp_sink_crc_stop(intel_dp, crtc_state, true);
> -	return ret;
> -}
> -
>  static bool
>  intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector)
>  {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h index e09b42d91d1e..8e6ee2fe66e4 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1665,8 +1665,6 @@ void intel_dp_sink_dpms(struct intel_dp *intel_dp, int
> mode); void intel_dp_encoder_reset(struct drm_encoder *encoder);
>  void intel_dp_encoder_suspend(struct intel_encoder *intel_encoder);
>  void intel_dp_encoder_destroy(struct drm_encoder *encoder);
> -int intel_dp_sink_crc(struct intel_dp *intel_dp,
> -		      struct intel_crtc_state *crtc_state, u8 *crc);
>  bool intel_dp_compute_config(struct intel_encoder *encoder,
>  			     struct intel_crtc_state *pipe_config,
>  			     struct drm_connector_state *conn_state);


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Kill sink_crc for good

[Rodrigo Vivi]

On Thu, Jul 05, 2018 at 02:11:45PM -0700, Dhinakaran Pandiyan wrote:
> On Thursday, July 5, 2018 12:25:28 PM PDT Rodrigo Vivi wrote:
> > It was originally introduced following the VESA spec in order to validate
> > PSR.
> > 
> > However we found so many issues around sink_crc that instead of helping PSR
> > development it only brought another layer of trouble to the table.
> > 
> > So, sink_crc has been a black whole for us in question of time, effort and
> > hope.
> > 
> > First of the problems is that HW statement is clear: "Do not attempt to use
> > aux communication with PSR enabled". So the main reason behind sink_crc is
> > already compromised.
> > 
> > For a while we had hope on the aux-mutex could workaround this problem on
> > SKL+ platforms, but that mutex was not reliable, not tested,
> > and we shouldn't use according to HW engineers.
> > 
> > Also, nor source, nor sink designed and implemented the sink_crc to be used
> > like we are trying to use here.
> > 
> > Well, the sink side of things is also apparently not prepared for this
> > case. Each panel that we tried seemed to have a different behavior with same
> > code and same source.
> > 
> > So, for all the time we lost on trying to ducktape all these different
> > issues I believe it is now time to move PSR to a more reliable validation.
> > Maybe not a perfect one as we dreamed for this sink_crc, but at least more
> > reliable.
> > 
> Good riddance.

Bye bye sink_crc, we are not going to miss you! ;)

> Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

Thanks for granting me this honor!

now let's wait for CI to blow-up...
than when we change psr test cases we come back here, trigger a retest and
once CI is happy with push.

> 
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c |  81 ------------------
> >  drivers/gpu/drm/i915/intel_dp.c     | 123 ----------------------------
> >  drivers/gpu/drm/i915/intel_drv.h    |   2 -
> >  3 files changed, 206 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> > b/drivers/gpu/drm/i915/i915_debugfs.c index f6142d78ede4..e8fa106a3655
> > 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -2761,86 +2761,6 @@ DEFINE_SIMPLE_ATTRIBUTE(i915_edp_psr_debug_fops,
> >  			i915_edp_psr_debug_get, i915_edp_psr_debug_set,
> >  			"%llu\n");
> > 
> > -static int i915_sink_crc(struct seq_file *m, void *data)
> > -{
> > -	struct drm_i915_private *dev_priv = node_to_i915(m->private);
> > -	struct drm_device *dev = &dev_priv->drm;
> > -	struct intel_connector *connector;
> > -	struct drm_connector_list_iter conn_iter;
> > -	struct intel_dp *intel_dp = NULL;
> > -	struct drm_modeset_acquire_ctx ctx;
> > -	int ret;
> > -	u8 crc[6];
> > -
> > -	drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
> > -
> > -	drm_connector_list_iter_begin(dev, &conn_iter);
> > -
> > -	for_each_intel_connector_iter(connector, &conn_iter) {
> > -		struct drm_crtc *crtc;
> > -		struct drm_connector_state *state;
> > -		struct intel_crtc_state *crtc_state;
> > -
> > -		if (connector->base.connector_type != DRM_MODE_CONNECTOR_eDP)
> > -			continue;
> > -
> > -retry:
> > -		ret = drm_modeset_lock(&dev->mode_config.connection_mutex, &ctx);
> > -		if (ret)
> > -			goto err;
> > -
> > -		state = connector->base.state;
> > -		if (!state->best_encoder)
> > -			continue;
> > -
> > -		crtc = state->crtc;
> > -		ret = drm_modeset_lock(&crtc->mutex, &ctx);
> > -		if (ret)
> > -			goto err;
> > -
> > -		crtc_state = to_intel_crtc_state(crtc->state);
> > -		if (!crtc_state->base.active)
> > -			continue;
> > -
> > -		/*
> > -		 * We need to wait for all crtc updates to complete, to make
> > -		 * sure any pending modesets and plane updates are completed.
> > -		 */
> > -		if (crtc_state->base.commit) {
> > -			ret =
> > wait_for_completion_interruptible(&crtc_state->base.commit->hw_done); -
> > -			if (ret)
> > -				goto err;
> > -		}
> > -
> > -		intel_dp = enc_to_intel_dp(state->best_encoder);
> > -
> > -		ret = intel_dp_sink_crc(intel_dp, crtc_state, crc);
> > -		if (ret)
> > -			goto err;
> > -
> > -		seq_printf(m, "%02x%02x%02x%02x%02x%02x\n",
> > -			   crc[0], crc[1], crc[2],
> > -			   crc[3], crc[4], crc[5]);
> > -		goto out;
> > -
> > -err:
> > -		if (ret == -EDEADLK) {
> > -			ret = drm_modeset_backoff(&ctx);
> > -			if (!ret)
> > -				goto retry;
> > -		}
> > -		goto out;
> > -	}
> > -	ret = -ENODEV;
> > -out:
> > -	drm_connector_list_iter_end(&conn_iter);
> > -	drm_modeset_drop_locks(&ctx);
> > -	drm_modeset_acquire_fini(&ctx);
> > -
> > -	return ret;
> > -}
> > -
> >  static int i915_energy_uJ(struct seq_file *m, void *data)
> >  {
> >  	struct drm_i915_private *dev_priv = node_to_i915(m->private);
> > @@ -4784,7 +4704,6 @@ static const struct drm_info_list i915_debugfs_list[]
> > = { {"i915_ppgtt_info", i915_ppgtt_info, 0},
> >  	{"i915_llc", i915_llc, 0},
> >  	{"i915_edp_psr_status", i915_edp_psr_status, 0},
> > -	{"i915_sink_crc_eDP1", i915_sink_crc, 0},
> >  	{"i915_energy_uJ", i915_energy_uJ, 0},
> >  	{"i915_runtime_pm_status", i915_runtime_pm_status, 0},
> >  	{"i915_power_domain_info", i915_power_domain_info, 0},
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c index 5be07e1d816d..efdd4b408d51 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -3905,129 +3905,6 @@ intel_dp_configure_mst(struct intel_dp *intel_dp)
> >  					intel_dp->is_mst);
> >  }
> > 
> > -static int intel_dp_sink_crc_stop(struct intel_dp *intel_dp,
> > -				  struct intel_crtc_state *crtc_state, bool disable_wa)
> > -{
> > -	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);
> > -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state->base.crtc);
> > -	u8 buf;
> > -	int ret = 0;
> > -	int count = 0;
> > -	int attempts = 10;
> > -
> > -	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0) {
> > -		DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
> > -		ret = -EIO;
> > -		goto out;
> > -	}
> > -
> > -	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
> > -			       buf & ~DP_TEST_SINK_START) < 0) {
> > -		DRM_DEBUG_KMS("Sink CRC couldn't be stopped properly\n");
> > -		ret = -EIO;
> > -		goto out;
> > -	}
> > -
> > -	do {
> > -		intel_wait_for_vblank(dev_priv, intel_crtc->pipe);
> > -
> > -		if (drm_dp_dpcd_readb(&intel_dp->aux,
> > -				      DP_TEST_SINK_MISC, &buf) < 0) {
> > -			ret = -EIO;
> > -			goto out;
> > -		}
> > -		count = buf & DP_TEST_COUNT_MASK;
> > -	} while (--attempts && count);
> > -
> > -	if (attempts == 0) {
> > -		DRM_DEBUG_KMS("TIMEOUT: Sink CRC counter is not zeroed after 
> calculation
> > is stopped\n"); -		ret = -ETIMEDOUT;
> > -	}
> > -
> > - out:
> > -	if (disable_wa)
> > -		hsw_enable_ips(crtc_state);
> > -	return ret;
> > -}
> > -
> > -static int intel_dp_sink_crc_start(struct intel_dp *intel_dp,
> > -				   struct intel_crtc_state *crtc_state)
> > -{
> > -	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);
> > -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state->base.crtc);
> > -	u8 buf;
> > -	int ret;
> > -
> > -	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK_MISC, &buf) < 0)
> > -		return -EIO;
> > -
> > -	if (!(buf & DP_TEST_CRC_SUPPORTED))
> > -		return -ENOTTY;
> > -
> > -	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_SINK, &buf) < 0)
> > -		return -EIO;
> > -
> > -	if (buf & DP_TEST_SINK_START) {
> > -		ret = intel_dp_sink_crc_stop(intel_dp, crtc_state, false);
> > -		if (ret)
> > -			return ret;
> > -	}
> > -
> > -	hsw_disable_ips(crtc_state);
> > -
> > -	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_SINK,
> > -			       buf | DP_TEST_SINK_START) < 0) {
> > -		hsw_enable_ips(crtc_state);
> > -		return -EIO;
> > -	}
> > -
> > -	intel_wait_for_vblank(dev_priv, intel_crtc->pipe);
> > -	return 0;
> > -}
> > -
> > -int intel_dp_sink_crc(struct intel_dp *intel_dp, struct intel_crtc_state
> > *crtc_state, u8 *crc) -{
> > -	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);
> > -	struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state->base.crtc);
> > -	u8 buf;
> > -	int count, ret;
> > -	int attempts = 6;
> > -
> > -	ret = intel_dp_sink_crc_start(intel_dp, crtc_state);
> > -	if (ret)
> > -		return ret;
> > -
> > -	do {
> > -		intel_wait_for_vblank(dev_priv, intel_crtc->pipe);
> > -
> > -		if (drm_dp_dpcd_readb(&intel_dp->aux,
> > -				      DP_TEST_SINK_MISC, &buf) < 0) {
> > -			ret = -EIO;
> > -			goto stop;
> > -		}
> > -		count = buf & DP_TEST_COUNT_MASK;
> > -
> > -	} while (--attempts && count == 0);
> > -
> > -	if (attempts == 0) {
> > -		DRM_ERROR("Panel is unable to calculate any CRC after 6 vblanks\n");
> > -		ret = -ETIMEDOUT;
> > -		goto stop;
> > -	}
> > -
> > -	if (drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_CRC_R_CR, crc, 6) < 0) {
> > -		ret = -EIO;
> > -		goto stop;
> > -	}
> > -
> > -stop:
> > -	intel_dp_sink_crc_stop(intel_dp, crtc_state, true);
> > -	return ret;
> > -}
> > -
> >  static bool
> >  intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector)
> >  {
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h index e09b42d91d1e..8e6ee2fe66e4 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1665,8 +1665,6 @@ void intel_dp_sink_dpms(struct intel_dp *intel_dp, int
> > mode); void intel_dp_encoder_reset(struct drm_encoder *encoder);
> >  void intel_dp_encoder_suspend(struct intel_encoder *intel_encoder);
> >  void intel_dp_encoder_destroy(struct drm_encoder *encoder);
> > -int intel_dp_sink_crc(struct intel_dp *intel_dp,
> > -		      struct intel_crtc_state *crtc_state, u8 *crc);
> >  bool intel_dp_compute_config(struct intel_encoder *encoder,
> >  			     struct intel_crtc_state *pipe_config,
> >  			     struct drm_connector_state *conn_state);
> 
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.CHECKPATCH: warning for drm/i915: Kill sink_crc for good

[Patchwork]

== Series Details ==

Series: drm/i915: Kill sink_crc for good
URL   : https://patchwork.freedesktop.org/series/46039/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
d2a9904c7bc2 drm/i915: Kill sink_crc for good
-:6: WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#6: 
It was originally introduced following the VESA spec in order to validate PSR.

total: 0 errors, 1 warnings, 0 checks, 230 lines checked

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for drm/i915: Kill sink_crc for good

[Patchwork]

== Series Details ==

Series: drm/i915: Kill sink_crc for good
URL   : https://patchwork.freedesktop.org/series/46039/
State : failure

== Summary ==

= CI Bug Log - changes from CI_DRM_4438 -> Patchwork_9558 =

== Summary - FAILURE ==

  Serious unknown changes coming with Patchwork_9558 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_9558, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/46039/revisions/1/mbox/

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_9558:

  === IGT changes ===

    ==== Possible regressions ====

    igt@kms_frontbuffer_tracking@basic:
      fi-skl-6700hq:      PASS -> FAIL
      fi-kbl-7560u:       PASS -> FAIL
      fi-skl-6600u:       PASS -> FAIL
      fi-kbl-r:           PASS -> FAIL
      fi-cfl-s3:          PASS -> FAIL
      fi-whl-u:           PASS -> FAIL

    
    ==== Warnings ====

    igt@kms_frontbuffer_tracking@basic:
      fi-hsw-peppy:       DMESG-FAIL (fdo#102614, fdo#106103) -> FAIL

    igt@kms_sink_crc_basic:
      fi-whl-u:           PASS -> SKIP
      fi-skl-6700hq:      PASS -> SKIP
      fi-hsw-peppy:       PASS -> SKIP

    
  fdo#102614 https://bugs.freedesktop.org/show_bug.cgi?id=102614
  fdo#106103 https://bugs.freedesktop.org/show_bug.cgi?id=106103


== Participating hosts (47 -> 42) ==

  Missing    (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4438 -> Patchwork_9558

  CI_DRM_4438: b689733af687b4b8072fb62a6bfe267c4e888f5f @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4539: 8b3cc74c6911e9b2835fe6e160f84bae463a70ef @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9558: d2a9904c7bc2864730d77f3d3f282d96359f7f70 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

d2a9904c7bc2 drm/i915: Kill sink_crc for good

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9558/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915: Kill sink_crc for good

[Patchwork]

== Series Details ==

Series: drm/i915: Kill sink_crc for good
URL   : https://patchwork.freedesktop.org/series/46039/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4504 -> Patchwork_9708 =

== Summary - SUCCESS ==

  No regressions found.

  External URL: https://patchwork.freedesktop.org/api/1.0/series/46039/revisions/1/mbox/

== Known issues ==

  Here are the changes found in Patchwork_9708 that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@drv_module_reload@basic-reload:
      fi-glk-j4005:       PASS -> DMESG-WARN (fdo#106725, fdo#106248)

    igt@kms_flip@basic-flip-vs-modeset:
      fi-glk-j4005:       PASS -> DMESG-WARN (fdo#106097, fdo#106000)

    
    ==== Possible fixes ====

    igt@drv_selftest@live_workarounds:
      fi-kbl-7560u:       DMESG-FAIL -> PASS

    igt@prime_vgem@basic-fence-flip:
      fi-ilk-650:         FAIL (fdo#104008) -> PASS

    
  fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008
  fdo#106000 https://bugs.freedesktop.org/show_bug.cgi?id=106000
  fdo#106097 https://bugs.freedesktop.org/show_bug.cgi?id=106097
  fdo#106248 https://bugs.freedesktop.org/show_bug.cgi?id=106248
  fdo#106725 https://bugs.freedesktop.org/show_bug.cgi?id=106725


== Participating hosts (46 -> 41) ==

  Missing    (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-hsw-4200u 


== Build changes ==

    * Linux: CI_DRM_4504 -> Patchwork_9708

  CI_DRM_4504: 0ba31c45beee4f7cfc17c61205294c322a9891bf @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4566: 7270e39a0e6238804827ea7f8db433ad743ed745 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9708: 50c9464e8c451c126045e465354052287cecdbe1 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

50c9464e8c45 drm/i915: Kill sink_crc for good

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9708/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for drm/i915: Kill sink_crc for good

[Patchwork]

== Series Details ==

Series: drm/i915: Kill sink_crc for good
URL   : https://patchwork.freedesktop.org/series/46039/
State : success

== Summary ==

= CI Bug Log - changes from CI_DRM_4504_full -> Patchwork_9708_full =

== Summary - WARNING ==

  Minor unknown changes coming with Patchwork_9708_full need to be verified
  manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_9708_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

== Possible new issues ==

  Here are the unknown changes that may have been introduced in Patchwork_9708_full:

  === IGT changes ===

    ==== Warnings ====

    igt@gem_exec_schedule@deep-bsd2:
      shard-kbl:          PASS -> SKIP +1

    igt@gem_mocs_settings@mocs-rc6-blt:
      shard-kbl:          SKIP -> PASS +1

    
== Known issues ==

  Here are the changes found in Patchwork_9708_full that come from known issues:

  === IGT changes ===

    ==== Issues hit ====

    igt@gem_wait@write-wait-bsd2:
      shard-snb:          SKIP -> INCOMPLETE (fdo#105411)

    igt@kms_color@pipe-a-ctm-max:
      shard-kbl:          PASS -> DMESG-WARN (fdo#106247)

    igt@kms_flip@2x-plain-flip-fb-recreate-interruptible:
      shard-glk:          PASS -> FAIL (fdo#100368) +2

    igt@kms_flip@modeset-vs-vblank-race-interruptible:
      shard-glk:          PASS -> FAIL (fdo#103060)

    
    ==== Possible fixes ====

    igt@kms_cursor_legacy@cursor-vs-flip-toggle:
      shard-kbl:          FAIL (fdo#103355) -> PASS

    igt@kms_flip@2x-plain-flip-fb-recreate:
      shard-glk:          FAIL (fdo#100368) -> PASS

    
  fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
  fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
  fdo#103355 https://bugs.freedesktop.org/show_bug.cgi?id=103355
  fdo#105411 https://bugs.freedesktop.org/show_bug.cgi?id=105411
  fdo#106247 https://bugs.freedesktop.org/show_bug.cgi?id=106247


== Participating hosts (5 -> 5) ==

  No changes in participating hosts


== Build changes ==

    * Linux: CI_DRM_4504 -> Patchwork_9708

  CI_DRM_4504: 0ba31c45beee4f7cfc17c61205294c322a9891bf @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_4566: 7270e39a0e6238804827ea7f8db433ad743ed745 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_9708: 50c9464e8c451c126045e465354052287cecdbe1 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_9708/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Kill sink_crc for good

[Dhinakaran Pandiyan]

On Thu, 2018-07-05 at 14:25 -0700, Rodrigo Vivi wrote:
> On Thu, Jul 05, 2018 at 02:11:45PM -0700, Dhinakaran Pandiyan wrote:
> > 
> > On Thursday, July 5, 2018 12:25:28 PM PDT Rodrigo Vivi wrote:
> > > 
> > > It was originally introduced following the VESA spec in order to
> > > validate
> > > PSR.
> > > 
> > > However we found so many issues around sink_crc that instead of
> > > helping PSR
> > > development it only brought another layer of trouble to the
> > > table.
> > > 
> > > So, sink_crc has been a black whole for us in question of time,
> > > effort and
> > > hope.
> > > 
> > > First of the problems is that HW statement is clear: "Do not
> > > attempt to use
> > > aux communication with PSR enabled". So the main reason behind
> > > sink_crc is
> > > already compromised.
> > > 
> > > For a while we had hope on the aux-mutex could workaround this
> > > problem on
> > > SKL+ platforms, but that mutex was not reliable, not tested,
> > > and we shouldn't use according to HW engineers.
> > > 
> > > Also, nor source, nor sink designed and implemented the sink_crc
> > > to be used
> > > like we are trying to use here.
> > > 
> > > Well, the sink side of things is also apparently not prepared for
> > > this
> > > case. Each panel that we tried seemed to have a different
> > > behavior with same
> > > code and same source.
> > > 
> > > So, for all the time we lost on trying to ducktape all these
> > > different
> > > issues I believe it is now time to move PSR to a more reliable
> > > validation.
> > > Maybe not a perfect one as we dreamed for this sink_crc, but at
> > > least more
> > > reliable.
> > > 
> > Good riddance.
> Bye bye sink_crc, we are not going to miss you! ;)
> 
> > 
> > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Thanks for granting me this honor!
> 
> now let's wait for CI to blow-up...
> than when we change psr test cases we come back here, trigger a
> retest and
> once CI is happy with push.

CI is indeed happy now.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Kill sink_crc for good

[Rodrigo Vivi]

On Wed, Jul 18, 2018 at 12:21:13PM -0700, Dhinakaran Pandiyan wrote:
> On Thu, 2018-07-05 at 14:25 -0700, Rodrigo Vivi wrote:
> > On Thu, Jul 05, 2018 at 02:11:45PM -0700, Dhinakaran Pandiyan wrote:
> > > 
> > > On Thursday, July 5, 2018 12:25:28 PM PDT Rodrigo Vivi wrote:
> > > > 
> > > > It was originally introduced following the VESA spec in order to
> > > > validate
> > > > PSR.
> > > > 
> > > > However we found so many issues around sink_crc that instead of
> > > > helping PSR
> > > > development it only brought another layer of trouble to the
> > > > table.
> > > > 
> > > > So, sink_crc has been a black whole for us in question of time,
> > > > effort and
> > > > hope.
> > > > 
> > > > First of the problems is that HW statement is clear: "Do not
> > > > attempt to use
> > > > aux communication with PSR enabled". So the main reason behind
> > > > sink_crc is
> > > > already compromised.
> > > > 
> > > > For a while we had hope on the aux-mutex could workaround this
> > > > problem on
> > > > SKL+ platforms, but that mutex was not reliable, not tested,
> > > > and we shouldn't use according to HW engineers.
> > > > 
> > > > Also, nor source, nor sink designed and implemented the sink_crc
> > > > to be used
> > > > like we are trying to use here.
> > > > 
> > > > Well, the sink side of things is also apparently not prepared for
> > > > this
> > > > case. Each panel that we tried seemed to have a different
> > > > behavior with same
> > > > code and same source.
> > > > 
> > > > So, for all the time we lost on trying to ducktape all these
> > > > different
> > > > issues I believe it is now time to move PSR to a more reliable
> > > > validation.
> > > > Maybe not a perfect one as we dreamed for this sink_crc, but at
> > > > least more
> > > > reliable.
> > > > 
> > > Good riddance.
> > Bye bye sink_crc, we are not going to miss you! ;)
> > 
> > > 
> > > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Thanks for granting me this honor!
> > 
> > now let's wait for CI to blow-up...
> > than when we change psr test cases we come back here, trigger a
> > retest and
> > once CI is happy with push.
> 
> CI is indeed happy now.

\o/

pushed. Thanks for granting me this honor!

> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx