[v2 0/6] Forward Error Correction

[v2 0/6] Forward Error Correction

[Anusha Srivatsa]

With Display Compression, the bit error in the pixel
stream can turn into a significant corruption on
the screen. The DP1.4 adds FEC - Forward Error Correction
scheme which uses Reed-Solomon parity/correction check
generated by the source and used by the sink to detect
and correct small numbers of bit errors in the compressed
stream.

v2: Avoid doing aux channel read eberytime we check
for FEC support. Instead cache the value of the DPCD
registers, similar to the DSC implementaion (Jani)

This is rebased on top of Manasi's End-to-end DSC
Implementation: https://patchwork.freedesktop.org/series/47514/

Tested on Odelia Board after applying the FEC workaround.

Anusha Srivatsa (6):
  i915/dp/fec: Cache the FEC_CAPABLE DPCD register
  i915/dp/fec: Check for FEC Support
  drm/dp/fec: DRM helper for Forward Error Correction
  drm/i915/fec: Set FEC_READY in FEC_CONFIGURATION
  i915/dp/fec: Configure the Forward Error Correction bits.
  drm/i915/fec: Disable FEC state.

 drivers/gpu/drm/i915/i915_reg.h  |  2 +
 drivers/gpu/drm/i915/intel_ddi.c |  5 ++
 drivers/gpu/drm/i915/intel_dp.c  | 82 +++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_drv.h |  9 +++-
 include/drm/drm_dp_helper.h      |  7 +++
 5 files changed, 102 insertions(+), 3 deletions(-)

-- 
2.17.1

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

[v2 1/6] i915/dp/fec: Cache the FEC_CAPABLE DPCD register

[Anusha Srivatsa]

Similar to DSC DPCD registers, let us cache
FEC_CAPABLE register to avoid using stale
values. With this we can avoid aux reads
everytime and instead read the cached values.

Suggested-by: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Manasi Navare <manasi.d.navare@intel.com>
Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c  | 8 ++++++++
 drivers/gpu/drm/i915/intel_drv.h | 1 +
 2 files changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index dcc5a207fcbd..8a0e0a0b26f6 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4163,8 +4163,10 @@ static void intel_dp_get_dsc_sink_cap(struct intel_dp *intel_dp)
 	/*
 	 *Clear the cached register set to avoid using stale values
 	 * for the sinks that do not support DSC.
+	 * Similarly, clear the cached FEC register.
 	 */
 	memset(intel_dp->dsc_dpcd, 0, sizeof(intel_dp->dsc_dpcd));
+	memset(intel_dp->fec_dpcd, 0, sizeof(intel_dp->fec_dpcd));
 
 	/* Cache the DSC DPCD if eDP or DP rev >= 1.4 */
 	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x14 ||
@@ -4179,6 +4181,12 @@ static void intel_dp_get_dsc_sink_cap(struct intel_dp *intel_dp)
 			      (int)sizeof(intel_dp->dsc_dpcd),
 			      intel_dp->dsc_dpcd);
 	}
+	/* FEC is supported only on DP 1.4 */
+	if (!intel_dp_is_edp(intel_dp) && intel_dp->dpcd[DP_DPCD_REV] >= 0x14) {
+		if (drm_dp_dpcd_readb(&intel_dp->aux, DP_FEC_CAPABILITY,
+				      intel_dp->fec_dpcd) < 0)
+			DRM_ERROR("Failed to read FEC DPCD register\n");
+	}
 }
 
 static bool
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 7b4af8cba279..b87ea052c9ca 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1086,6 +1086,7 @@ struct intel_dp {
 	uint8_t downstream_ports[DP_MAX_DOWNSTREAM_PORTS];
 	uint8_t edp_dpcd[EDP_DISPLAY_CTL_CAP_SIZE];
 	u8 dsc_dpcd[DP_DSC_RECEIVER_CAP_SIZE];
+	u8 fec_dpcd[1];
 	/* source rates */
 	int num_source_rates;
 	const int *source_rates;
-- 
2.17.1

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

[v2 2/6] drm/dp/fec: DRM helper for Forward Error Correction

[Anusha Srivatsa]

DP 1.4 has Forward Error Correction Support(FEC).
Add helper function to check if the sink device
supports FEC.

v2: Separate the helper and the code that uses the helper into
two separate patches. (Manasi)

v3:
- Move the code to drm_dp_helper.c (Manasi)
- change the return type, code style changes (Gaurav)
- Use drm_dp_dpcd_readb instead of drm_dp_dpcd_read. (Jani)

v4:
- Avoid aux reads everytime, instead read cached
values of dpcd register (jani)
- Move helper to drm_dp_helper.h like other dsc
helpers.(Anusha)

Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Manasi Navare <manasi.d.navare@intel.com>
Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
---
 include/drm/drm_dp_helper.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 33cb78925094..82bbccc69acf 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -1099,6 +1099,13 @@ drm_dp_dsc_sink_max_slice_width(const u8 dsc_dpcd[DP_DSC_RECEIVER_CAP_SIZE])
 		DP_DSC_SLICE_WIDTH_MULTIPLIER;
 }
 
+/* Forward Error Correction Support on DP 1.4*/
+static inline bool
+drm_dp_sink_supports_fec(const u8 fec_dpcd[])
+{
+	return fec_dpcd[0] & DP_FEC_CAPABLE;
+}
+
 /*
  * DisplayPort AUX channel
  */
-- 
2.17.1

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

[v2 3/6] i915/dp/fec: Check for FEC Support

[Anusha Srivatsa]

For DP 1.4 and above, Display Stream compression can be
enabled only if Forward Error Correctin can be performed.

Check if the sink supports FEC using the helper.

v2: Mention External DP where ever FEC is mentioned
in the code.Check return status of dpcd reads. (Gaurav)
- Do regular mode check even if FEC is not supported. (manasi)

v3: Do not perform any dpcd writes in the atomic
check phase. (DK, Manasi)

v4: Use debug level logging for scenario where sink does
not support a feature. (DK)

v5: Correct commit message. rebase.

Cc: Gaurav K Singh <gaurav.k.singh@intel.com>
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Manasi Navare <manasi.d.navare@intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 8a0e0a0b26f6..318494afd14a 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -650,7 +650,7 @@ intel_dp_mode_valid(struct drm_connector *connector,
 			dsc_slice_count =
 				drm_dp_dsc_sink_max_slice_count(intel_dp->dsc_dpcd,
 								true);
-		} else {
+		} else if (drm_dp_sink_supports_fec(intel_dp->fec_dpcd)) {
 			dsc_max_output_bpp =
 				intel_dp_dsc_get_output_bpp(max_link_clock,
 							    max_lanes,
@@ -660,7 +660,8 @@ intel_dp_mode_valid(struct drm_connector *connector,
 				intel_dp_dsc_get_slice_count(intel_dp,
 							     target_clock,
 							     mode->hdisplay);
-		}
+		} else
+			DRM_DEBUG_KMS("Sink device does not Support FEC\n");
 	}
 
 	if ((mode_rate > max_rate && !(dsc_max_output_bpp && dsc_slice_count)) ||
@@ -2033,6 +2034,13 @@ static bool intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
 	if (pipe == PIPE_A && !intel_dp_is_edp(intel_dp))
 		return false;
 
+	/* DSC not supported if external DP sink does not support FEC */
+	if (!intel_dp_is_edp(intel_dp) &&
+	    !drm_dp_sink_supports_fec(intel_dp->fec_dpcd)) {
+		DRM_DEBUG_KMS("Sink does not support Forward Error Correction, disabling Display Compression\n");
+		return false;
+	}
+
 	/* DSC not supported for DSC sink BPC < 8 */
 	if (limits->max_bpp < 3 * DP_DSC_MIN_SUPPORTED_BPC) {
 		DRM_DEBUG_KMS("No DSC support for less than 8bpc\n");
-- 
2.17.1

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

[v2 4/6] drm/i915/fec: Set FEC_READY in FEC_CONFIGURATION

[Anusha Srivatsa]

If the panel supports FEC, the driver has to
set the FEC_READY bit in the dpcd register:
FEC_CONFIGURATION.

This has to happen before link training.

v2: s/intel_dp_set_fec_ready/intel_dp_sink_set_fec_ready
   - change commit message. (Gaurav)

v3: rebased.

Cc: Gaurav K Singh <gaurav.k.singh@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Manasi Navare <manasi.d.navare@intel.com>
Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c |  1 +
 drivers/gpu/drm/i915/intel_dp.c  | 17 +++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h |  3 +++
 3 files changed, 21 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 2db6284d3a96..f531900165bf 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -2909,6 +2909,7 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
 		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
 	intel_dp_sink_set_decompression_state(intel_dp, crtc_state,
 					      DP_DECOMPRESSION_EN);
+	intel_dp_sink_set_fec_ready(intel_dp, crtc_state, DP_FEC_READY);
 	intel_dp_start_link_train(intel_dp);
 	if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
 		intel_dp_stop_link_train(intel_dp);
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 318494afd14a..b4e8af3142a2 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3000,6 +3000,23 @@ void intel_dp_sink_set_decompression_state(struct intel_dp *intel_dp,
 			      state == DP_DECOMPRESSION_EN ? "enable" : "disable");
 }
 
+void intel_dp_sink_set_fec_ready(struct intel_dp *intel_dp,
+				 const struct intel_crtc_state *crtc_state,
+				 int state)
+{
+	int ret;
+
+	if (!crtc_state->dsc_params.compression_enable)
+		return;
+
+	if (intel_dp_is_edp(intel_dp))
+		return;
+
+	ret = drm_dp_dpcd_writeb(&intel_dp->aux, DP_FEC_CONFIGURATION, state);
+	if (ret < 0)
+		DRM_DEBUG_KMS("Failed to get FEC enabled in sink\n");
+}
+
 /* If the sink supports it, try to set the power state appropriately */
 void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode)
 {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index b87ea052c9ca..fbc9fa06e8be 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1709,6 +1709,9 @@ void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
 void intel_dp_sink_set_decompression_state(struct intel_dp *intel_dp,
 					   const struct intel_crtc_state *crtc_state,
 					   int state);
+void intel_dp_sink_set_fec_ready(struct intel_dp *intel_dp,
+				 const struct intel_crtc_state *crtc_state,
+				 int state);
 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);
-- 
2.17.1

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

[v2 5/6] i915/dp/fec: Configure the Forward Error Correction bits.

[Anusha Srivatsa]

If FEC is supported, the corresponding
DP_TP_CTL register bits have to be configured.

The driver has to program the FEC_ENABLE in DP_TP_CTL[30] register
and wait till FEC_STATUS in DP_TP_CTL[28] is 1.
Also add the warn message to make sure that the control
register is already active while enabling FEC.

v2:
- Change commit message. Configure fec state after
  link training (Manasi, Gaurav)
- Remove redundent checks (Manasi)
- Remove the registers that get added automagically (Anusha)

v3: s/intel_dp_set_fec_state()/intel_dp_enable_fec_state() (Gaurav)

v4: rebased.

Cc: Gaurav K Singh <gaurav.k.singh@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Manasi Navare <manasi.d.navare@intel.com>
Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h  |  2 ++
 drivers/gpu/drm/i915/intel_ddi.c |  2 ++
 drivers/gpu/drm/i915/intel_dp.c  | 27 +++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h |  3 ++-
 4 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index bcde78bc0027..c8d7fdcd7823 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -9093,6 +9093,7 @@ enum skl_power_gate {
 #define _DP_TP_CTL_B			0x64140
 #define DP_TP_CTL(port) _MMIO_PORT(port, _DP_TP_CTL_A, _DP_TP_CTL_B)
 #define  DP_TP_CTL_ENABLE			(1 << 31)
+#define  DP_TP_CTL_FEC_ENABLE			(1 << 30)
 #define  DP_TP_CTL_MODE_SST			(0 << 27)
 #define  DP_TP_CTL_MODE_MST			(1 << 27)
 #define  DP_TP_CTL_FORCE_ACT			(1 << 25)
@@ -9111,6 +9112,7 @@ enum skl_power_gate {
 #define _DP_TP_STATUS_A			0x64044
 #define _DP_TP_STATUS_B			0x64144
 #define DP_TP_STATUS(port) _MMIO_PORT(port, _DP_TP_STATUS_A, _DP_TP_STATUS_B)
+#define  DP_TP_STATUS_FEC_ENABLE_LIVE		(1 << 28)
 #define  DP_TP_STATUS_IDLE_DONE			(1 << 25)
 #define  DP_TP_STATUS_ACT_SENT			(1 << 24)
 #define  DP_TP_STATUS_MODE_STATUS_MST		(1 << 23)
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index f531900165bf..67c013ea4d39 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -2911,6 +2911,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
 					      DP_DECOMPRESSION_EN);
 	intel_dp_sink_set_fec_ready(intel_dp, crtc_state, DP_FEC_READY);
 	intel_dp_start_link_train(intel_dp);
+	intel_dp_enable_fec_state(intel_dp, crtc_state);
+
 	if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
 		intel_dp_stop_link_train(intel_dp);
 
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index b4e8af3142a2..b9f85502d9ff 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3017,6 +3017,33 @@ void intel_dp_sink_set_fec_ready(struct intel_dp *intel_dp,
 		DRM_DEBUG_KMS("Failed to get FEC enabled in sink\n");
 }
 
+void intel_dp_enable_fec_state(struct intel_dp *intel_dp,
+			       const struct intel_crtc_state *crtc_state)
+{
+	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
+	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	enum port port = intel_dig_port->base.port;
+	u32 val;
+
+	/* FEC support exists for DP 1.4 only */
+	if (intel_dp_is_edp(intel_dp))
+		return;
+
+	/* If Display Compression is not enabled, FEC need not be configured */
+	if (!crtc_state->dsc_params.compression_enable)
+		return;
+
+	val = I915_READ(DP_TP_CTL(port));
+	val |= DP_TP_CTL_FEC_ENABLE;
+	I915_WRITE(DP_TP_CTL(port), val);
+
+	if (intel_wait_for_register(dev_priv, DP_TP_STATUS(port),
+				    DP_TP_STATUS_FEC_ENABLE_LIVE,
+				    DP_TP_STATUS_FEC_ENABLE_LIVE,
+				    1))
+		DRM_ERROR("Timed out waiting for FEC Enable Status\n");
+}
+
 /* If the sink supports it, try to set the power state appropriately */
 void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode)
 {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index fbc9fa06e8be..e51d612a9f42 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1532,7 +1532,6 @@ intel_encoder_current_mode(struct intel_encoder *encoder);
 bool intel_port_is_tc(struct drm_i915_private *dev_priv, enum port port);
 enum tc_port intel_port_to_tc(struct drm_i915_private *dev_priv,
 			      enum port port);
-
 enum pipe intel_get_pipe_from_connector(struct intel_connector *connector);
 int intel_get_pipe_from_crtc_id_ioctl(struct drm_device *dev, void *data,
 				      struct drm_file *file_priv);
@@ -1712,6 +1711,8 @@ void intel_dp_sink_set_decompression_state(struct intel_dp *intel_dp,
 void intel_dp_sink_set_fec_ready(struct intel_dp *intel_dp,
 				 const struct intel_crtc_state *crtc_state,
 				 int state);
+void intel_dp_enable_fec_state(struct intel_dp *intel_dp,
+			       const struct intel_crtc_state *crtc_state);
 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);
-- 
2.17.1

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

[v2 6/6] drm/i915/fec: Disable FEC state.

[Anusha Srivatsa]

Set the suitable bits in DP_TP_CTL to stop
bit correction when DSC is disabled.

v2:
- rebased.
- Add additional check for compression state. (Gaurav)

v3: rebased.

Cc: Gaurav K Singh <gaurav.k.singh@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Manasi Navare <manasi.d.navare@intel.com>
Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c |  2 ++
 drivers/gpu/drm/i915/intel_dp.c  | 18 ++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h |  2 ++
 3 files changed, 22 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 67c013ea4d39..fefa92070b2d 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -3245,6 +3245,8 @@ static void intel_disable_ddi_dp(struct intel_encoder *encoder,
 	/* Disable the decompression in DP Sink */
 	intel_dp_sink_set_decompression_state(intel_dp, old_crtc_state,
 					      ~DP_DECOMPRESSION_EN);
+	/* Disable FEC in DP Sink */
+	intel_dp_disable_fec_state(intel_dp, old_crtc_state);
 }
 
 static void intel_disable_ddi_hdmi(struct intel_encoder *encoder,
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index b9f85502d9ff..1db1a738c85f 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3044,6 +3044,24 @@ void intel_dp_enable_fec_state(struct intel_dp *intel_dp,
 		DRM_ERROR("Timed out waiting for FEC Enable Status\n");
 }
 
+void intel_dp_disable_fec_state(struct intel_dp *intel_dp,
+				const struct intel_crtc_state *crtc_state)
+{
+	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
+	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	enum port port = intel_dig_port->base.port;
+	u32 val;
+
+	if (crtc_state->dsc_params.compression_enable)
+		DRM_DEBUG_KMS("Compression still enabled\n");
+		return;
+
+	val = I915_READ(DP_TP_CTL(port));
+	val &= ~DP_TP_CTL_FEC_ENABLE;
+	I915_WRITE(DP_TP_CTL(port), val);
+	POSTING_READ(DP_TP_CTL(port));
+}
+
 /* If the sink supports it, try to set the power state appropriately */
 void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode)
 {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index e51d612a9f42..0c2429f7cc35 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1713,6 +1713,8 @@ void intel_dp_sink_set_fec_ready(struct intel_dp *intel_dp,
 				 int state);
 void intel_dp_enable_fec_state(struct intel_dp *intel_dp,
 			       const struct intel_crtc_state *crtc_state);
+void intel_dp_disable_fec_state(struct intel_dp *intel_dp,
+				const struct intel_crtc_state *crtc_state);
 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);
-- 
2.17.1

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

Re: [v2 1/6] i915/dp/fec: Cache the FEC_CAPABLE DPCD register

[Manasi Navare]

On Mon, Oct 15, 2018 at 02:50:32PM -0700, Anusha Srivatsa wrote:
> Similar to DSC DPCD registers, let us cache
> FEC_CAPABLE register to avoid using stale
> values. With this we can avoid aux reads
> everytime and instead read the cached values.
> 
> Suggested-by: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c  | 8 ++++++++
>  drivers/gpu/drm/i915/intel_drv.h | 1 +
>  2 files changed, 9 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index dcc5a207fcbd..8a0e0a0b26f6 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4163,8 +4163,10 @@ static void intel_dp_get_dsc_sink_cap(struct intel_dp *intel_dp)
>  	/*
>  	 *Clear the cached register set to avoid using stale values
>  	 * for the sinks that do not support DSC.
> +	 * Similarly, clear the cached FEC register.
>  	 */
>  	memset(intel_dp->dsc_dpcd, 0, sizeof(intel_dp->dsc_dpcd));
> +	memset(intel_dp->fec_dpcd, 0, sizeof(intel_dp->fec_dpcd));

Memset is an expensive opertaion for just a single value,
just set that to 0

>  
>  	/* Cache the DSC DPCD if eDP or DP rev >= 1.4 */
>  	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x14 ||
> @@ -4179,6 +4181,12 @@ static void intel_dp_get_dsc_sink_cap(struct intel_dp *intel_dp)
>  			      (int)sizeof(intel_dp->dsc_dpcd),
>  			      intel_dp->dsc_dpcd);
>  	}
> +	/* FEC is supported only on DP 1.4 */
> +	if (!intel_dp_is_edp(intel_dp) && intel_dp->dpcd[DP_DPCD_REV] >= 0x14) {
> +		if (drm_dp_dpcd_readb(&intel_dp->aux, DP_FEC_CAPABILITY,
> +				      intel_dp->fec_dpcd) < 0)
> +			DRM_ERROR("Failed to read FEC DPCD register\n");
> +	}
>  }
>  
>  static bool
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 7b4af8cba279..b87ea052c9ca 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1086,6 +1086,7 @@ struct intel_dp {
>  	uint8_t downstream_ports[DP_MAX_DOWNSTREAM_PORTS];
>  	uint8_t edp_dpcd[EDP_DISPLAY_CTL_CAP_SIZE];
>  	u8 dsc_dpcd[DP_DSC_RECEIVER_CAP_SIZE];
> +	u8 fec_dpcd[1];

Why create an array for just 1 value, I think just a u8 fec_capable field
will suffice

Manasi

>  	/* source rates */
>  	int num_source_rates;
>  	const int *source_rates;
> -- 
> 2.17.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for Forward Error Correction (rev2)

[Patchwork]

== Series Details ==

Series: Forward Error Correction (rev2)
URL   : https://patchwork.freedesktop.org/series/47848/
State : failure

== Summary ==

Applying: i915/dp/fec: Cache the FEC_CAPABLE DPCD register
error: sha1 information is lacking or useless (drivers/gpu/drm/i915/intel_dp.c).
error: could not build fake ancestor
Patch failed at 0001 i915/dp/fec: Cache the FEC_CAPABLE DPCD register
Use 'git am --show-current-patch' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

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

Re: [v2 1/6] i915/dp/fec: Cache the FEC_CAPABLE DPCD register

[Jani Nikula]

On Mon, 15 Oct 2018, Manasi Navare <manasi.d.navare@intel.com> wrote:
> On Mon, Oct 15, 2018 at 02:50:32PM -0700, Anusha Srivatsa wrote:
>> Similar to DSC DPCD registers, let us cache
>> FEC_CAPABLE register to avoid using stale
>> values. With this we can avoid aux reads
>> everytime and instead read the cached values.
>> 
>> Suggested-by: Jani Nikula <jani.nikula@linux.intel.com>
>> Cc: Jani Nikula <jani.nikula@linux.intel.com>
>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>> Cc: Manasi Navare <manasi.d.navare@intel.com>
>> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_dp.c  | 8 ++++++++
>>  drivers/gpu/drm/i915/intel_drv.h | 1 +
>>  2 files changed, 9 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index dcc5a207fcbd..8a0e0a0b26f6 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -4163,8 +4163,10 @@ static void intel_dp_get_dsc_sink_cap(struct intel_dp *intel_dp)
>>  	/*
>>  	 *Clear the cached register set to avoid using stale values
>>  	 * for the sinks that do not support DSC.
>> +	 * Similarly, clear the cached FEC register.
>>  	 */
>>  	memset(intel_dp->dsc_dpcd, 0, sizeof(intel_dp->dsc_dpcd));
>> +	memset(intel_dp->fec_dpcd, 0, sizeof(intel_dp->fec_dpcd));
>
> Memset is an expensive opertaion for just a single value,
> just set that to 0

Agreed on making it a single byte and setting it to 0, but the memset
impact on performance is noise compared to e.g. the native aux
transactions on this path.

BR,
Jani.


>
>>  
>>  	/* Cache the DSC DPCD if eDP or DP rev >= 1.4 */
>>  	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x14 ||
>> @@ -4179,6 +4181,12 @@ static void intel_dp_get_dsc_sink_cap(struct intel_dp *intel_dp)
>>  			      (int)sizeof(intel_dp->dsc_dpcd),
>>  			      intel_dp->dsc_dpcd);
>>  	}
>> +	/* FEC is supported only on DP 1.4 */
>> +	if (!intel_dp_is_edp(intel_dp) && intel_dp->dpcd[DP_DPCD_REV] >= 0x14) {
>> +		if (drm_dp_dpcd_readb(&intel_dp->aux, DP_FEC_CAPABILITY,
>> +				      intel_dp->fec_dpcd) < 0)
>> +			DRM_ERROR("Failed to read FEC DPCD register\n");
>> +	}
>>  }
>>  
>>  static bool
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 7b4af8cba279..b87ea052c9ca 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1086,6 +1086,7 @@ struct intel_dp {
>>  	uint8_t downstream_ports[DP_MAX_DOWNSTREAM_PORTS];
>>  	uint8_t edp_dpcd[EDP_DISPLAY_CTL_CAP_SIZE];
>>  	u8 dsc_dpcd[DP_DSC_RECEIVER_CAP_SIZE];
>> +	u8 fec_dpcd[1];
>
> Why create an array for just 1 value, I think just a u8 fec_capable field
> will suffice
>
> Manasi
>
>>  	/* source rates */
>>  	int num_source_rates;
>>  	const int *source_rates;
>> -- 
>> 2.17.1
>> 

-- 
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [v2 3/6] i915/dp/fec: Check for FEC Support

[Manasi Navare]

On Mon, Oct 15, 2018 at 02:50:34PM -0700, Anusha Srivatsa wrote:
> For DP 1.4 and above, Display Stream compression can be
> enabled only if Forward Error Correctin can be performed.
> 
> Check if the sink supports FEC using the helper.
> 
> v2: Mention External DP where ever FEC is mentioned
> in the code.Check return status of dpcd reads. (Gaurav)
> - Do regular mode check even if FEC is not supported. (manasi)
> 
> v3: Do not perform any dpcd writes in the atomic
> check phase. (DK, Manasi)
> 
> v4: Use debug level logging for scenario where sink does
> not support a feature. (DK)
> 
> v5: Correct commit message. rebase.
> 
> Cc: Gaurav K Singh <gaurav.k.singh@intel.com>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 8a0e0a0b26f6..318494afd14a 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -650,7 +650,7 @@ intel_dp_mode_valid(struct drm_connector *connector,
>  			dsc_slice_count =
>  				drm_dp_dsc_sink_max_slice_count(intel_dp->dsc_dpcd,
>  								true);
> -		} else {
> +		} else if (drm_dp_sink_supports_fec(intel_dp->fec_dpcd)) {

Pass just a single field  as suggested in Patch 1.
With that change,

Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>

Manasi

>  			dsc_max_output_bpp =
>  				intel_dp_dsc_get_output_bpp(max_link_clock,
>  							    max_lanes,
> @@ -660,7 +660,8 @@ intel_dp_mode_valid(struct drm_connector *connector,
>  				intel_dp_dsc_get_slice_count(intel_dp,
>  							     target_clock,
>  							     mode->hdisplay);
> -		}
> +		} else
> +			DRM_DEBUG_KMS("Sink device does not Support FEC\n");
>  	}
>  
>  	if ((mode_rate > max_rate && !(dsc_max_output_bpp && dsc_slice_count)) ||
> @@ -2033,6 +2034,13 @@ static bool intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
>  	if (pipe == PIPE_A && !intel_dp_is_edp(intel_dp))
>  		return false;
>  
> +	/* DSC not supported if external DP sink does not support FEC */
> +	if (!intel_dp_is_edp(intel_dp) &&
> +	    !drm_dp_sink_supports_fec(intel_dp->fec_dpcd)) {
> +		DRM_DEBUG_KMS("Sink does not support Forward Error Correction, disabling Display Compression\n");
> +		return false;
> +	}
> +
>  	/* DSC not supported for DSC sink BPC < 8 */
>  	if (limits->max_bpp < 3 * DP_DSC_MIN_SUPPORTED_BPC) {
>  		DRM_DEBUG_KMS("No DSC support for less than 8bpc\n");
> -- 
> 2.17.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [v2 4/6] drm/i915/fec: Set FEC_READY in FEC_CONFIGURATION

[Manasi Navare]

On Mon, Oct 15, 2018 at 02:50:35PM -0700, Anusha Srivatsa wrote:
> If the panel supports FEC, the driver has to
> set the FEC_READY bit in the dpcd register:
> FEC_CONFIGURATION.
> 
> This has to happen before link training.
> 
> v2: s/intel_dp_set_fec_ready/intel_dp_sink_set_fec_ready
>    - change commit message. (Gaurav)
> 
> v3: rebased.
> 
> Cc: Gaurav K Singh <gaurav.k.singh@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>

This looks good to me.

Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>

Manasi

> ---
>  drivers/gpu/drm/i915/intel_ddi.c |  1 +
>  drivers/gpu/drm/i915/intel_dp.c  | 17 +++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h |  3 +++
>  3 files changed, 21 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 2db6284d3a96..f531900165bf 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -2909,6 +2909,7 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
>  		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
>  	intel_dp_sink_set_decompression_state(intel_dp, crtc_state,
>  					      DP_DECOMPRESSION_EN);
> +	intel_dp_sink_set_fec_ready(intel_dp, crtc_state, DP_FEC_READY);
>  	intel_dp_start_link_train(intel_dp);
>  	if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
>  		intel_dp_stop_link_train(intel_dp);
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 318494afd14a..b4e8af3142a2 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3000,6 +3000,23 @@ void intel_dp_sink_set_decompression_state(struct intel_dp *intel_dp,
>  			      state == DP_DECOMPRESSION_EN ? "enable" : "disable");
>  }
>  
> +void intel_dp_sink_set_fec_ready(struct intel_dp *intel_dp,
> +				 const struct intel_crtc_state *crtc_state,
> +				 int state)
> +{
> +	int ret;
> +
> +	if (!crtc_state->dsc_params.compression_enable)
> +		return;
> +
> +	if (intel_dp_is_edp(intel_dp))
> +		return;
> +
> +	ret = drm_dp_dpcd_writeb(&intel_dp->aux, DP_FEC_CONFIGURATION, state);
> +	if (ret < 0)
> +		DRM_DEBUG_KMS("Failed to get FEC enabled in sink\n");
> +}
> +
>  /* If the sink supports it, try to set the power state appropriately */
>  void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode)
>  {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index b87ea052c9ca..fbc9fa06e8be 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1709,6 +1709,9 @@ void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
>  void intel_dp_sink_set_decompression_state(struct intel_dp *intel_dp,
>  					   const struct intel_crtc_state *crtc_state,
>  					   int state);
> +void intel_dp_sink_set_fec_ready(struct intel_dp *intel_dp,
> +				 const struct intel_crtc_state *crtc_state,
> +				 int state);
>  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);
> -- 
> 2.17.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [v2 5/6] i915/dp/fec: Configure the Forward Error Correction bits.

[Manasi Navare]

On Mon, Oct 15, 2018 at 02:50:36PM -0700, Anusha Srivatsa wrote:
> If FEC is supported, the corresponding
> DP_TP_CTL register bits have to be configured.
> 
> The driver has to program the FEC_ENABLE in DP_TP_CTL[30] register
> and wait till FEC_STATUS in DP_TP_CTL[28] is 1.
> Also add the warn message to make sure that the control
> register is already active while enabling FEC.
> 
> v2:
> - Change commit message. Configure fec state after
>   link training (Manasi, Gaurav)
> - Remove redundent checks (Manasi)
> - Remove the registers that get added automagically (Anusha)
> 
> v3: s/intel_dp_set_fec_state()/intel_dp_enable_fec_state() (Gaurav)
> 
> v4: rebased.
> 
> Cc: Gaurav K Singh <gaurav.k.singh@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h  |  2 ++
>  drivers/gpu/drm/i915/intel_ddi.c |  2 ++
>  drivers/gpu/drm/i915/intel_dp.c  | 27 +++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h |  3 ++-
>  4 files changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index bcde78bc0027..c8d7fdcd7823 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -9093,6 +9093,7 @@ enum skl_power_gate {
>  #define _DP_TP_CTL_B			0x64140
>  #define DP_TP_CTL(port) _MMIO_PORT(port, _DP_TP_CTL_A, _DP_TP_CTL_B)
>  #define  DP_TP_CTL_ENABLE			(1 << 31)
> +#define  DP_TP_CTL_FEC_ENABLE			(1 << 30)
>  #define  DP_TP_CTL_MODE_SST			(0 << 27)
>  #define  DP_TP_CTL_MODE_MST			(1 << 27)
>  #define  DP_TP_CTL_FORCE_ACT			(1 << 25)
> @@ -9111,6 +9112,7 @@ enum skl_power_gate {
>  #define _DP_TP_STATUS_A			0x64044
>  #define _DP_TP_STATUS_B			0x64144
>  #define DP_TP_STATUS(port) _MMIO_PORT(port, _DP_TP_STATUS_A, _DP_TP_STATUS_B)
> +#define  DP_TP_STATUS_FEC_ENABLE_LIVE		(1 << 28)
>  #define  DP_TP_STATUS_IDLE_DONE			(1 << 25)
>  #define  DP_TP_STATUS_ACT_SENT			(1 << 24)
>  #define  DP_TP_STATUS_MODE_STATUS_MST		(1 << 23)
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index f531900165bf..67c013ea4d39 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -2911,6 +2911,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
>  					      DP_DECOMPRESSION_EN);
>  	intel_dp_sink_set_fec_ready(intel_dp, crtc_state, DP_FEC_READY);
>  	intel_dp_start_link_train(intel_dp);
> +	intel_dp_enable_fec_state(intel_dp, crtc_state);
> +
>  	if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
>  		intel_dp_stop_link_train(intel_dp);
>  
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index b4e8af3142a2..b9f85502d9ff 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3017,6 +3017,33 @@ void intel_dp_sink_set_fec_ready(struct intel_dp *intel_dp,
>  		DRM_DEBUG_KMS("Failed to get FEC enabled in sink\n");
>  }
>  
> +void intel_dp_enable_fec_state(struct intel_dp *intel_dp,
> +			       const struct intel_crtc_state *crtc_state)
> +{
> +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +	enum port port = intel_dig_port->base.port;
> +	u32 val;
> +
> +	/* FEC support exists for DP 1.4 only */
> +	if (intel_dp_is_edp(intel_dp))
> +		return;
> +
> +	/* If Display Compression is not enabled, FEC need not be configured */
> +	if (!crtc_state->dsc_params.compression_enable)
> +		return;
> +
> +	val = I915_READ(DP_TP_CTL(port));
> +	val |= DP_TP_CTL_FEC_ENABLE;
> +	I915_WRITE(DP_TP_CTL(port), val);
> +
> +	if (intel_wait_for_register(dev_priv, DP_TP_STATUS(port),
> +				    DP_TP_STATUS_FEC_ENABLE_LIVE,
> +				    DP_TP_STATUS_FEC_ENABLE_LIVE,
> +				    1))
> +		DRM_ERROR("Timed out waiting for FEC Enable Status\n");

I think in case of the error where we could not enable FEC,
we should also set the crtc_state->dsc_params.compression_enable = 0 since we
should not be enabling DSC without FEC enabled.

Jani/Anusha, thought?

Manasi

> +}
> +
>  /* If the sink supports it, try to set the power state appropriately */
>  void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode)
>  {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index fbc9fa06e8be..e51d612a9f42 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1532,7 +1532,6 @@ intel_encoder_current_mode(struct intel_encoder *encoder);
>  bool intel_port_is_tc(struct drm_i915_private *dev_priv, enum port port);
>  enum tc_port intel_port_to_tc(struct drm_i915_private *dev_priv,
>  			      enum port port);
> -
>  enum pipe intel_get_pipe_from_connector(struct intel_connector *connector);
>  int intel_get_pipe_from_crtc_id_ioctl(struct drm_device *dev, void *data,
>  				      struct drm_file *file_priv);
> @@ -1712,6 +1711,8 @@ void intel_dp_sink_set_decompression_state(struct intel_dp *intel_dp,
>  void intel_dp_sink_set_fec_ready(struct intel_dp *intel_dp,
>  				 const struct intel_crtc_state *crtc_state,
>  				 int state);
> +void intel_dp_enable_fec_state(struct intel_dp *intel_dp,
> +			       const struct intel_crtc_state *crtc_state);
>  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);
> -- 
> 2.17.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [v2 6/6] drm/i915/fec: Disable FEC state.

[Manasi Navare]

Hi Anusha,

Find my comments below:

On Mon, Oct 15, 2018 at 02:50:37PM -0700, Anusha Srivatsa wrote:
> Set the suitable bits in DP_TP_CTL to stop
> bit correction when DSC is disabled.
> 

> - rebased.
> - Add additional check for compression state. (Gaurav)
> 
> v3: rebased.
> 
> Cc: Gaurav K Singh <gaurav.k.singh@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c |  2 ++
>  drivers/gpu/drm/i915/intel_dp.c  | 18 ++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h |  2 ++
>  3 files changed, 22 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 67c013ea4d39..fefa92070b2d 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -3245,6 +3245,8 @@ static void intel_disable_ddi_dp(struct intel_encoder *encoder,
>  	/* Disable the decompression in DP Sink */
>  	intel_dp_sink_set_decompression_state(intel_dp, old_crtc_state,
>  					      ~DP_DECOMPRESSION_EN);
> +	/* Disable FEC in DP Sink */
> +	intel_dp_disable_fec_state(intel_dp, old_crtc_state);
>  }
>  
>  static void intel_disable_ddi_hdmi(struct intel_encoder *encoder,
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index b9f85502d9ff..1db1a738c85f 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3044,6 +3044,24 @@ void intel_dp_enable_fec_state(struct intel_dp *intel_dp,
>  		DRM_ERROR("Timed out waiting for FEC Enable Status\n");
>  }
>  
> +void intel_dp_disable_fec_state(struct intel_dp *intel_dp,
> +				const struct intel_crtc_state *crtc_state)
> +{
> +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +	enum port port = intel_dig_port->base.port;
> +	u32 val;
> +
> +	if (crtc_state->dsc_params.compression_enable)
> +		DRM_DEBUG_KMS("Compression still enabled\n");

This check is wrong. The crtc_state->dsc_params.compression_enable only indicates
that compression enable is allowed in atomic check. This does not indicate whether
it is enabled or disabled in source/sink

Infact this function shd return if !crtc_state->dsc_params.compression_enable, see
what intel_dp_sink_set_decompression_state does.

After that to make sure DSC is disabled, you can read DP_DSC_ENABLE DPCD register and check
DP_DECOMPRESSION_EN bit before proceeding to disabling FEC.

Manasi

> +		return;
> +
> +	val = I915_READ(DP_TP_CTL(port));
> +	val &= ~DP_TP_CTL_FEC_ENABLE;
> +	I915_WRITE(DP_TP_CTL(port), val);
> +	POSTING_READ(DP_TP_CTL(port));
> +}
> +
>  /* If the sink supports it, try to set the power state appropriately */
>  void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode)
>  {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index e51d612a9f42..0c2429f7cc35 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1713,6 +1713,8 @@ void intel_dp_sink_set_fec_ready(struct intel_dp *intel_dp,
>  				 int state);
>  void intel_dp_enable_fec_state(struct intel_dp *intel_dp,
>  			       const struct intel_crtc_state *crtc_state);
> +void intel_dp_disable_fec_state(struct intel_dp *intel_dp,
> +				const struct intel_crtc_state *crtc_state);
>  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);
> -- 
> 2.17.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [v2 6/6] drm/i915/fec: Disable FEC state.

[Srivatsa, Anusha]

>-----Original Message-----
>From: Navare, Manasi D
>Sent: Thursday, October 18, 2018 4:16 PM
>To: Srivatsa, Anusha <anusha.srivatsa@intel.com>
>Cc: intel-gfx@lists.freedesktop.org; Singh, Gaurav K <gaurav.k.singh@intel.com>;
>Jani Nikula <jani.nikula@linux.intel.com>; Ville Syrjala
><ville.syrjala@linux.intel.com>
>Subject: Re: [v2 6/6] drm/i915/fec: Disable FEC state.
>
>Hi Anusha,
>
>Find my comments below:
>
>On Mon, Oct 15, 2018 at 02:50:37PM -0700, Anusha Srivatsa wrote:
>> Set the suitable bits in DP_TP_CTL to stop bit correction when DSC is
>> disabled.
>>
>
>> - rebased.
>> - Add additional check for compression state. (Gaurav)
>>
>> v3: rebased.
>>
>> Cc: Gaurav K Singh <gaurav.k.singh@intel.com>
>> Cc: Jani Nikula <jani.nikula@linux.intel.com>
>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>> Cc: Manasi Navare <manasi.d.navare@intel.com>
>> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_ddi.c |  2 ++
>> drivers/gpu/drm/i915/intel_dp.c  | 18 ++++++++++++++++++
>> drivers/gpu/drm/i915/intel_drv.h |  2 ++
>>  3 files changed, 22 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c
>> b/drivers/gpu/drm/i915/intel_ddi.c
>> index 67c013ea4d39..fefa92070b2d 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -3245,6 +3245,8 @@ static void intel_disable_ddi_dp(struct intel_encoder
>*encoder,
>>  	/* Disable the decompression in DP Sink */
>>  	intel_dp_sink_set_decompression_state(intel_dp, old_crtc_state,
>>  					      ~DP_DECOMPRESSION_EN);
>> +	/* Disable FEC in DP Sink */
>> +	intel_dp_disable_fec_state(intel_dp, old_crtc_state);
>>  }
>>
>>  static void intel_disable_ddi_hdmi(struct intel_encoder *encoder,
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>> b/drivers/gpu/drm/i915/intel_dp.c index b9f85502d9ff..1db1a738c85f
>> 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -3044,6 +3044,24 @@ void intel_dp_enable_fec_state(struct intel_dp
>*intel_dp,
>>  		DRM_ERROR("Timed out waiting for FEC Enable Status\n");  }
>>
>> +void intel_dp_disable_fec_state(struct intel_dp *intel_dp,
>> +				const struct intel_crtc_state *crtc_state) {
>> +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
>> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
>> +	enum port port = intel_dig_port->base.port;
>> +	u32 val;
>> +
>> +	if (crtc_state->dsc_params.compression_enable)
>> +		DRM_DEBUG_KMS("Compression still enabled\n");
>
>This check is wrong. The crtc_state->dsc_params.compression_enable only
>indicates that compression enable is allowed in atomic check. This does not
>indicate whether it is enabled or disabled in source/sink
>
>Infact this function shd return if !crtc_state->dsc_params.compression_enable,
>see what intel_dp_sink_set_decompression_state does.
>
>After that to make sure DSC is disabled, you can read DP_DSC_ENABLE DPCD
>register and check DP_DECOMPRESSION_EN bit before proceeding to disabling
>FEC.

That is a good point. I will make the changes. 
Thanks For the review.

Anusha 
>Manasi
>
>> +		return;
>> +
>> +	val = I915_READ(DP_TP_CTL(port));
>> +	val &= ~DP_TP_CTL_FEC_ENABLE;
>> +	I915_WRITE(DP_TP_CTL(port), val);
>> +	POSTING_READ(DP_TP_CTL(port));
>> +}
>> +
>>  /* If the sink supports it, try to set the power state appropriately
>> */  void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode)  {
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>> b/drivers/gpu/drm/i915/intel_drv.h
>> index e51d612a9f42..0c2429f7cc35 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1713,6 +1713,8 @@ void intel_dp_sink_set_fec_ready(struct intel_dp
>*intel_dp,
>>  				 int state);
>>  void intel_dp_enable_fec_state(struct intel_dp *intel_dp,
>>  			       const struct intel_crtc_state *crtc_state);
>> +void intel_dp_disable_fec_state(struct intel_dp *intel_dp,
>> +				const struct intel_crtc_state *crtc_state);
>>  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);
>> --
>> 2.17.1
>>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [v2 5/6] i915/dp/fec: Configure the Forward Error Correction bits.

[Ville Syrjälä]

On Mon, Oct 15, 2018 at 02:50:36PM -0700, Anusha Srivatsa wrote:
> If FEC is supported, the corresponding
> DP_TP_CTL register bits have to be configured.
> 
> The driver has to program the FEC_ENABLE in DP_TP_CTL[30] register
> and wait till FEC_STATUS in DP_TP_CTL[28] is 1.
> Also add the warn message to make sure that the control
> register is already active while enabling FEC.
> 
> v2:
> - Change commit message. Configure fec state after
>   link training (Manasi, Gaurav)
> - Remove redundent checks (Manasi)
> - Remove the registers that get added automagically (Anusha)
> 
> v3: s/intel_dp_set_fec_state()/intel_dp_enable_fec_state() (Gaurav)
> 
> v4: rebased.
> 
> Cc: Gaurav K Singh <gaurav.k.singh@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h  |  2 ++
>  drivers/gpu/drm/i915/intel_ddi.c |  2 ++
>  drivers/gpu/drm/i915/intel_dp.c  | 27 +++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h |  3 ++-
>  4 files changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index bcde78bc0027..c8d7fdcd7823 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -9093,6 +9093,7 @@ enum skl_power_gate {
>  #define _DP_TP_CTL_B			0x64140
>  #define DP_TP_CTL(port) _MMIO_PORT(port, _DP_TP_CTL_A, _DP_TP_CTL_B)
>  #define  DP_TP_CTL_ENABLE			(1 << 31)
> +#define  DP_TP_CTL_FEC_ENABLE			(1 << 30)
>  #define  DP_TP_CTL_MODE_SST			(0 << 27)
>  #define  DP_TP_CTL_MODE_MST			(1 << 27)
>  #define  DP_TP_CTL_FORCE_ACT			(1 << 25)
> @@ -9111,6 +9112,7 @@ enum skl_power_gate {
>  #define _DP_TP_STATUS_A			0x64044
>  #define _DP_TP_STATUS_B			0x64144
>  #define DP_TP_STATUS(port) _MMIO_PORT(port, _DP_TP_STATUS_A, _DP_TP_STATUS_B)
> +#define  DP_TP_STATUS_FEC_ENABLE_LIVE		(1 << 28)
>  #define  DP_TP_STATUS_IDLE_DONE			(1 << 25)
>  #define  DP_TP_STATUS_ACT_SENT			(1 << 24)
>  #define  DP_TP_STATUS_MODE_STATUS_MST		(1 << 23)
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index f531900165bf..67c013ea4d39 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -2911,6 +2911,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
>  					      DP_DECOMPRESSION_EN);
>  	intel_dp_sink_set_fec_ready(intel_dp, crtc_state, DP_FEC_READY);
>  	intel_dp_start_link_train(intel_dp);
> +	intel_dp_enable_fec_state(intel_dp, crtc_state);

This doesn't look like the correct spot. According to the spec it
should be after stop_link_train() (where we set DP_TP_CTL to
to normal).

> +
>  	if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
>  		intel_dp_stop_link_train(intel_dp);
>  
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index b4e8af3142a2..b9f85502d9ff 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3017,6 +3017,33 @@ void intel_dp_sink_set_fec_ready(struct intel_dp *intel_dp,
>  		DRM_DEBUG_KMS("Failed to get FEC enabled in sink\n");
>  }
>  
> +void intel_dp_enable_fec_state(struct intel_dp *intel_dp,
> +			       const struct intel_crtc_state *crtc_state)
> +{
> +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +	enum port port = intel_dig_port->base.port;
> +	u32 val;
> +
> +	/* FEC support exists for DP 1.4 only */
> +	if (intel_dp_is_edp(intel_dp))
> +		return;
> +
> +	/* If Display Compression is not enabled, FEC need not be configured */
> +	if (!crtc_state->dsc_params.compression_enable)
> +		return;
> +
> +	val = I915_READ(DP_TP_CTL(port));
> +	val |= DP_TP_CTL_FEC_ENABLE;
> +	I915_WRITE(DP_TP_CTL(port), val);
> +
> +	if (intel_wait_for_register(dev_priv, DP_TP_STATUS(port),
> +				    DP_TP_STATUS_FEC_ENABLE_LIVE,
> +				    DP_TP_STATUS_FEC_ENABLE_LIVE,
> +				    1))
> +		DRM_ERROR("Timed out waiting for FEC Enable Status\n");
> +}

There is no reason to stick these into intel_dp.c when the
only caller is in intel_ddi.c.

We also seem to be missing platform checks. FEC doesn't appear to
be a thing prior to ICL. I guess the edp+compression_enable takes care
of this, but I think I'd rather stick a new fec_enable bool into
the crtc state. That way we get can add it to the state checker as well.

> +
>  /* If the sink supports it, try to set the power state appropriately */
>  void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode)
>  {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index fbc9fa06e8be..e51d612a9f42 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1532,7 +1532,6 @@ intel_encoder_current_mode(struct intel_encoder *encoder);
>  bool intel_port_is_tc(struct drm_i915_private *dev_priv, enum port port);
>  enum tc_port intel_port_to_tc(struct drm_i915_private *dev_priv,
>  			      enum port port);
> -
>  enum pipe intel_get_pipe_from_connector(struct intel_connector *connector);
>  int intel_get_pipe_from_crtc_id_ioctl(struct drm_device *dev, void *data,
>  				      struct drm_file *file_priv);
> @@ -1712,6 +1711,8 @@ void intel_dp_sink_set_decompression_state(struct intel_dp *intel_dp,
>  void intel_dp_sink_set_fec_ready(struct intel_dp *intel_dp,
>  				 const struct intel_crtc_state *crtc_state,
>  				 int state);
> +void intel_dp_enable_fec_state(struct intel_dp *intel_dp,
> +			       const struct intel_crtc_state *crtc_state);
>  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);
> -- 
> 2.17.1

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

Re: [v2 6/6] drm/i915/fec: Disable FEC state.

[Ville Syrjälä]

On Mon, Oct 15, 2018 at 02:50:37PM -0700, Anusha Srivatsa wrote:
> Set the suitable bits in DP_TP_CTL to stop
> bit correction when DSC is disabled.
> 
> v2:
> - rebased.
> - Add additional check for compression state. (Gaurav)
> 
> v3: rebased.
> 
> Cc: Gaurav K Singh <gaurav.k.singh@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Manasi Navare <manasi.d.navare@intel.com>
> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c |  2 ++
>  drivers/gpu/drm/i915/intel_dp.c  | 18 ++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h |  2 ++
>  3 files changed, 22 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 67c013ea4d39..fefa92070b2d 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -3245,6 +3245,8 @@ static void intel_disable_ddi_dp(struct intel_encoder *encoder,
>  	/* Disable the decompression in DP Sink */
>  	intel_dp_sink_set_decompression_state(intel_dp, old_crtc_state,
>  					      ~DP_DECOMPRESSION_EN);
> +	/* Disable FEC in DP Sink */
> +	intel_dp_disable_fec_state(intel_dp, old_crtc_state);

This looks like the wrong spot as well.

Should be in intel_disable_ddi_buf() if I'm reading the spec correctly.

>  }
>  
>  static void intel_disable_ddi_hdmi(struct intel_encoder *encoder,
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index b9f85502d9ff..1db1a738c85f 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3044,6 +3044,24 @@ void intel_dp_enable_fec_state(struct intel_dp *intel_dp,
>  		DRM_ERROR("Timed out waiting for FEC Enable Status\n");
>  }
>  
> +void intel_dp_disable_fec_state(struct intel_dp *intel_dp,
> +				const struct intel_crtc_state *crtc_state)
> +{
> +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +	enum port port = intel_dig_port->base.port;
> +	u32 val;
> +
> +	if (crtc_state->dsc_params.compression_enable)
> +		DRM_DEBUG_KMS("Compression still enabled\n");
> +		return;
> +
> +	val = I915_READ(DP_TP_CTL(port));
> +	val &= ~DP_TP_CTL_FEC_ENABLE;
> +	I915_WRITE(DP_TP_CTL(port), val);
> +	POSTING_READ(DP_TP_CTL(port));
> +}

Same comments as for the other patch.

> +
>  /* If the sink supports it, try to set the power state appropriately */
>  void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode)
>  {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index e51d612a9f42..0c2429f7cc35 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1713,6 +1713,8 @@ void intel_dp_sink_set_fec_ready(struct intel_dp *intel_dp,
>  				 int state);
>  void intel_dp_enable_fec_state(struct intel_dp *intel_dp,
>  			       const struct intel_crtc_state *crtc_state);
> +void intel_dp_disable_fec_state(struct intel_dp *intel_dp,
> +				const struct intel_crtc_state *crtc_state);
>  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);
> -- 
> 2.17.1

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

Re: [v2 5/6] i915/dp/fec: Configure the Forward Error Correction bits.

[Manasi Navare]

On Fri, Oct 19, 2018 at 09:39:11PM +0300, Ville Syrjälä wrote:
> On Mon, Oct 15, 2018 at 02:50:36PM -0700, Anusha Srivatsa wrote:
> > If FEC is supported, the corresponding
> > DP_TP_CTL register bits have to be configured.
> > 
> > The driver has to program the FEC_ENABLE in DP_TP_CTL[30] register
> > and wait till FEC_STATUS in DP_TP_CTL[28] is 1.
> > Also add the warn message to make sure that the control
> > register is already active while enabling FEC.
> > 
> > v2:
> > - Change commit message. Configure fec state after
> >   link training (Manasi, Gaurav)
> > - Remove redundent checks (Manasi)
> > - Remove the registers that get added automagically (Anusha)
> > 
> > v3: s/intel_dp_set_fec_state()/intel_dp_enable_fec_state() (Gaurav)
> > 
> > v4: rebased.
> > 
> > Cc: Gaurav K Singh <gaurav.k.singh@intel.com>
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h  |  2 ++
> >  drivers/gpu/drm/i915/intel_ddi.c |  2 ++
> >  drivers/gpu/drm/i915/intel_dp.c  | 27 +++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_drv.h |  3 ++-
> >  4 files changed, 33 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index bcde78bc0027..c8d7fdcd7823 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -9093,6 +9093,7 @@ enum skl_power_gate {
> >  #define _DP_TP_CTL_B			0x64140
> >  #define DP_TP_CTL(port) _MMIO_PORT(port, _DP_TP_CTL_A, _DP_TP_CTL_B)
> >  #define  DP_TP_CTL_ENABLE			(1 << 31)
> > +#define  DP_TP_CTL_FEC_ENABLE			(1 << 30)
> >  #define  DP_TP_CTL_MODE_SST			(0 << 27)
> >  #define  DP_TP_CTL_MODE_MST			(1 << 27)
> >  #define  DP_TP_CTL_FORCE_ACT			(1 << 25)
> > @@ -9111,6 +9112,7 @@ enum skl_power_gate {
> >  #define _DP_TP_STATUS_A			0x64044
> >  #define _DP_TP_STATUS_B			0x64144
> >  #define DP_TP_STATUS(port) _MMIO_PORT(port, _DP_TP_STATUS_A, _DP_TP_STATUS_B)
> > +#define  DP_TP_STATUS_FEC_ENABLE_LIVE		(1 << 28)
> >  #define  DP_TP_STATUS_IDLE_DONE			(1 << 25)
> >  #define  DP_TP_STATUS_ACT_SENT			(1 << 24)
> >  #define  DP_TP_STATUS_MODE_STATUS_MST		(1 << 23)
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index f531900165bf..67c013ea4d39 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -2911,6 +2911,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
> >  					      DP_DECOMPRESSION_EN);
> >  	intel_dp_sink_set_fec_ready(intel_dp, crtc_state, DP_FEC_READY);
> >  	intel_dp_start_link_train(intel_dp);
> > +	intel_dp_enable_fec_state(intel_dp, crtc_state);
> 
> This doesn't look like the correct spot. According to the spec it
> should be after stop_link_train() (where we set DP_TP_CTL to
> to normal).
>

Yes I see in the display port enabling sequence page of the spec, that it says enable FEC
after DP_TP_CTL is set to Normal. Also the FEC page says that this should be enabled only after ensuring that link training
completed. So according to that this call should happen after
intel_dp_stop_link_train(). 
So yes move this to after intel_dp_stop_link_training().

> > +
> >  	if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
> >  		intel_dp_stop_link_train(intel_dp);
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index b4e8af3142a2..b9f85502d9ff 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -3017,6 +3017,33 @@ void intel_dp_sink_set_fec_ready(struct intel_dp *intel_dp,
> >  		DRM_DEBUG_KMS("Failed to get FEC enabled in sink\n");
> >  }
> >  
> > +void intel_dp_enable_fec_state(struct intel_dp *intel_dp,
> > +			       const struct intel_crtc_state *crtc_state)
> > +{
> > +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > +	enum port port = intel_dig_port->base.port;
> > +	u32 val;
> > +
> > +	/* FEC support exists for DP 1.4 only */
> > +	if (intel_dp_is_edp(intel_dp))
> > +		return;
> > +
> > +	/* If Display Compression is not enabled, FEC need not be configured */
> > +	if (!crtc_state->dsc_params.compression_enable)
> > +		return;
> > +
> > +	val = I915_READ(DP_TP_CTL(port));
> > +	val |= DP_TP_CTL_FEC_ENABLE;
> > +	I915_WRITE(DP_TP_CTL(port), val);
> > +
> > +	if (intel_wait_for_register(dev_priv, DP_TP_STATUS(port),
> > +				    DP_TP_STATUS_FEC_ENABLE_LIVE,
> > +				    DP_TP_STATUS_FEC_ENABLE_LIVE,
> > +				    1))
> > +		DRM_ERROR("Timed out waiting for FEC Enable Status\n");
> > +}
> 
> There is no reason to stick these into intel_dp.c when the
> only caller is in intel_ddi.c.
>

So may be move the set_dsc_state function also from intel_dp.c to intel_ddi.c
Or does it make sense to move all these DSC related functions including fec to intel_dsc.c?
 
> We also seem to be missing platform checks. FEC doesn't appear to
> be a thing prior to ICL. I guess the edp+compression_enable takes care
> of this, but I think I'd rather stick a new fec_enable bool into
> the crtc state. That way we get can add it to the state checker as well.
>

fec_enable can be added to crtc_state->dsc_params may be?

Manasi
 
> > +
> >  /* If the sink supports it, try to set the power state appropriately */
> >  void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode)
> >  {
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index fbc9fa06e8be..e51d612a9f42 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1532,7 +1532,6 @@ intel_encoder_current_mode(struct intel_encoder *encoder);
> >  bool intel_port_is_tc(struct drm_i915_private *dev_priv, enum port port);
> >  enum tc_port intel_port_to_tc(struct drm_i915_private *dev_priv,
> >  			      enum port port);
> > -
> >  enum pipe intel_get_pipe_from_connector(struct intel_connector *connector);
> >  int intel_get_pipe_from_crtc_id_ioctl(struct drm_device *dev, void *data,
> >  				      struct drm_file *file_priv);
> > @@ -1712,6 +1711,8 @@ void intel_dp_sink_set_decompression_state(struct intel_dp *intel_dp,
> >  void intel_dp_sink_set_fec_ready(struct intel_dp *intel_dp,
> >  				 const struct intel_crtc_state *crtc_state,
> >  				 int state);
> > +void intel_dp_enable_fec_state(struct intel_dp *intel_dp,
> > +			       const struct intel_crtc_state *crtc_state);
> >  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);
> > -- 
> > 2.17.1
> 
> -- 
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [v2 5/6] i915/dp/fec: Configure the Forward Error Correction bits.

[Ville Syrjälä]

On Fri, Oct 19, 2018 at 12:24:43PM -0700, Manasi Navare wrote:
> On Fri, Oct 19, 2018 at 09:39:11PM +0300, Ville Syrjälä wrote:
> > On Mon, Oct 15, 2018 at 02:50:36PM -0700, Anusha Srivatsa wrote:
> > > If FEC is supported, the corresponding
> > > DP_TP_CTL register bits have to be configured.
> > > 
> > > The driver has to program the FEC_ENABLE in DP_TP_CTL[30] register
> > > and wait till FEC_STATUS in DP_TP_CTL[28] is 1.
> > > Also add the warn message to make sure that the control
> > > register is already active while enabling FEC.
> > > 
> > > v2:
> > > - Change commit message. Configure fec state after
> > >   link training (Manasi, Gaurav)
> > > - Remove redundent checks (Manasi)
> > > - Remove the registers that get added automagically (Anusha)
> > > 
> > > v3: s/intel_dp_set_fec_state()/intel_dp_enable_fec_state() (Gaurav)
> > > 
> > > v4: rebased.
> > > 
> > > Cc: Gaurav K Singh <gaurav.k.singh@intel.com>
> > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_reg.h  |  2 ++
> > >  drivers/gpu/drm/i915/intel_ddi.c |  2 ++
> > >  drivers/gpu/drm/i915/intel_dp.c  | 27 +++++++++++++++++++++++++++
> > >  drivers/gpu/drm/i915/intel_drv.h |  3 ++-
> > >  4 files changed, 33 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index bcde78bc0027..c8d7fdcd7823 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -9093,6 +9093,7 @@ enum skl_power_gate {
> > >  #define _DP_TP_CTL_B			0x64140
> > >  #define DP_TP_CTL(port) _MMIO_PORT(port, _DP_TP_CTL_A, _DP_TP_CTL_B)
> > >  #define  DP_TP_CTL_ENABLE			(1 << 31)
> > > +#define  DP_TP_CTL_FEC_ENABLE			(1 << 30)
> > >  #define  DP_TP_CTL_MODE_SST			(0 << 27)
> > >  #define  DP_TP_CTL_MODE_MST			(1 << 27)
> > >  #define  DP_TP_CTL_FORCE_ACT			(1 << 25)
> > > @@ -9111,6 +9112,7 @@ enum skl_power_gate {
> > >  #define _DP_TP_STATUS_A			0x64044
> > >  #define _DP_TP_STATUS_B			0x64144
> > >  #define DP_TP_STATUS(port) _MMIO_PORT(port, _DP_TP_STATUS_A, _DP_TP_STATUS_B)
> > > +#define  DP_TP_STATUS_FEC_ENABLE_LIVE		(1 << 28)
> > >  #define  DP_TP_STATUS_IDLE_DONE			(1 << 25)
> > >  #define  DP_TP_STATUS_ACT_SENT			(1 << 24)
> > >  #define  DP_TP_STATUS_MODE_STATUS_MST		(1 << 23)
> > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > > index f531900165bf..67c013ea4d39 100644
> > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > @@ -2911,6 +2911,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
> > >  					      DP_DECOMPRESSION_EN);
> > >  	intel_dp_sink_set_fec_ready(intel_dp, crtc_state, DP_FEC_READY);
> > >  	intel_dp_start_link_train(intel_dp);
> > > +	intel_dp_enable_fec_state(intel_dp, crtc_state);
> > 
> > This doesn't look like the correct spot. According to the spec it
> > should be after stop_link_train() (where we set DP_TP_CTL to
> > to normal).
> >
> 
> Yes I see in the display port enabling sequence page of the spec, that it says enable FEC
> after DP_TP_CTL is set to Normal. Also the FEC page says that this should be enabled only after ensuring that link training
> completed. So according to that this call should happen after
> intel_dp_stop_link_train(). 
> So yes move this to after intel_dp_stop_link_training().
> 
> > > +
> > >  	if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
> > >  		intel_dp_stop_link_train(intel_dp);
> > >  
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > index b4e8af3142a2..b9f85502d9ff 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -3017,6 +3017,33 @@ void intel_dp_sink_set_fec_ready(struct intel_dp *intel_dp,
> > >  		DRM_DEBUG_KMS("Failed to get FEC enabled in sink\n");
> > >  }
> > >  
> > > +void intel_dp_enable_fec_state(struct intel_dp *intel_dp,
> > > +			       const struct intel_crtc_state *crtc_state)
> > > +{
> > > +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > > +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > > +	enum port port = intel_dig_port->base.port;
> > > +	u32 val;
> > > +
> > > +	/* FEC support exists for DP 1.4 only */
> > > +	if (intel_dp_is_edp(intel_dp))
> > > +		return;
> > > +
> > > +	/* If Display Compression is not enabled, FEC need not be configured */
> > > +	if (!crtc_state->dsc_params.compression_enable)
> > > +		return;
> > > +
> > > +	val = I915_READ(DP_TP_CTL(port));
> > > +	val |= DP_TP_CTL_FEC_ENABLE;
> > > +	I915_WRITE(DP_TP_CTL(port), val);
> > > +
> > > +	if (intel_wait_for_register(dev_priv, DP_TP_STATUS(port),
> > > +				    DP_TP_STATUS_FEC_ENABLE_LIVE,
> > > +				    DP_TP_STATUS_FEC_ENABLE_LIVE,
> > > +				    1))
> > > +		DRM_ERROR("Timed out waiting for FEC Enable Status\n");
> > > +}
> > 
> > There is no reason to stick these into intel_dp.c when the
> > only caller is in intel_ddi.c.
> >
> 
> So may be move the set_dsc_state function also from intel_dp.c to intel_ddi.c
> Or does it make sense to move all these DSC related functions including fec to intel_dsc.c?
>  
> > We also seem to be missing platform checks. FEC doesn't appear to
> > be a thing prior to ICL. I guess the edp+compression_enable takes care
> > of this, but I think I'd rather stick a new fec_enable bool into
> > the crtc state. That way we get can add it to the state checker as well.
> >
> 
> fec_enable can be added to crtc_state->dsc_params may be?

Isn't FEC something that can be used w/o DSC too?

> 
> Manasi
>  
> > > +
> > >  /* If the sink supports it, try to set the power state appropriately */
> > >  void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode)
> > >  {
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index fbc9fa06e8be..e51d612a9f42 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1532,7 +1532,6 @@ intel_encoder_current_mode(struct intel_encoder *encoder);
> > >  bool intel_port_is_tc(struct drm_i915_private *dev_priv, enum port port);
> > >  enum tc_port intel_port_to_tc(struct drm_i915_private *dev_priv,
> > >  			      enum port port);
> > > -
> > >  enum pipe intel_get_pipe_from_connector(struct intel_connector *connector);
> > >  int intel_get_pipe_from_crtc_id_ioctl(struct drm_device *dev, void *data,
> > >  				      struct drm_file *file_priv);
> > > @@ -1712,6 +1711,8 @@ void intel_dp_sink_set_decompression_state(struct intel_dp *intel_dp,
> > >  void intel_dp_sink_set_fec_ready(struct intel_dp *intel_dp,
> > >  				 const struct intel_crtc_state *crtc_state,
> > >  				 int state);
> > > +void intel_dp_enable_fec_state(struct intel_dp *intel_dp,
> > > +			       const struct intel_crtc_state *crtc_state);
> > >  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);
> > > -- 
> > > 2.17.1
> > 
> > -- 
> > Ville Syrjälä
> > Intel

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

Re: [v2 5/6] i915/dp/fec: Configure the Forward Error Correction bits.

[Manasi Navare]

On Fri, Oct 19, 2018 at 10:58:29PM +0300, Ville Syrjälä wrote:
> On Fri, Oct 19, 2018 at 12:24:43PM -0700, Manasi Navare wrote:
> > On Fri, Oct 19, 2018 at 09:39:11PM +0300, Ville Syrjälä wrote:
> > > On Mon, Oct 15, 2018 at 02:50:36PM -0700, Anusha Srivatsa wrote:
> > > > If FEC is supported, the corresponding
> > > > DP_TP_CTL register bits have to be configured.
> > > > 
> > > > The driver has to program the FEC_ENABLE in DP_TP_CTL[30] register
> > > > and wait till FEC_STATUS in DP_TP_CTL[28] is 1.
> > > > Also add the warn message to make sure that the control
> > > > register is already active while enabling FEC.
> > > > 
> > > > v2:
> > > > - Change commit message. Configure fec state after
> > > >   link training (Manasi, Gaurav)
> > > > - Remove redundent checks (Manasi)
> > > > - Remove the registers that get added automagically (Anusha)
> > > > 
> > > > v3: s/intel_dp_set_fec_state()/intel_dp_enable_fec_state() (Gaurav)
> > > > 
> > > > v4: rebased.
> > > > 
> > > > Cc: Gaurav K Singh <gaurav.k.singh@intel.com>
> > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > > > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_reg.h  |  2 ++
> > > >  drivers/gpu/drm/i915/intel_ddi.c |  2 ++
> > > >  drivers/gpu/drm/i915/intel_dp.c  | 27 +++++++++++++++++++++++++++
> > > >  drivers/gpu/drm/i915/intel_drv.h |  3 ++-
> > > >  4 files changed, 33 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > > index bcde78bc0027..c8d7fdcd7823 100644
> > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > @@ -9093,6 +9093,7 @@ enum skl_power_gate {
> > > >  #define _DP_TP_CTL_B			0x64140
> > > >  #define DP_TP_CTL(port) _MMIO_PORT(port, _DP_TP_CTL_A, _DP_TP_CTL_B)
> > > >  #define  DP_TP_CTL_ENABLE			(1 << 31)
> > > > +#define  DP_TP_CTL_FEC_ENABLE			(1 << 30)
> > > >  #define  DP_TP_CTL_MODE_SST			(0 << 27)
> > > >  #define  DP_TP_CTL_MODE_MST			(1 << 27)
> > > >  #define  DP_TP_CTL_FORCE_ACT			(1 << 25)
> > > > @@ -9111,6 +9112,7 @@ enum skl_power_gate {
> > > >  #define _DP_TP_STATUS_A			0x64044
> > > >  #define _DP_TP_STATUS_B			0x64144
> > > >  #define DP_TP_STATUS(port) _MMIO_PORT(port, _DP_TP_STATUS_A, _DP_TP_STATUS_B)
> > > > +#define  DP_TP_STATUS_FEC_ENABLE_LIVE		(1 << 28)
> > > >  #define  DP_TP_STATUS_IDLE_DONE			(1 << 25)
> > > >  #define  DP_TP_STATUS_ACT_SENT			(1 << 24)
> > > >  #define  DP_TP_STATUS_MODE_STATUS_MST		(1 << 23)
> > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > > > index f531900165bf..67c013ea4d39 100644
> > > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > > @@ -2911,6 +2911,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
> > > >  					      DP_DECOMPRESSION_EN);
> > > >  	intel_dp_sink_set_fec_ready(intel_dp, crtc_state, DP_FEC_READY);
> > > >  	intel_dp_start_link_train(intel_dp);
> > > > +	intel_dp_enable_fec_state(intel_dp, crtc_state);
> > > 
> > > This doesn't look like the correct spot. According to the spec it
> > > should be after stop_link_train() (where we set DP_TP_CTL to
> > > to normal).
> > >
> > 
> > Yes I see in the display port enabling sequence page of the spec, that it says enable FEC
> > after DP_TP_CTL is set to Normal. Also the FEC page says that this should be enabled only after ensuring that link training
> > completed. So according to that this call should happen after
> > intel_dp_stop_link_train(). 
> > So yes move this to after intel_dp_stop_link_training().
> > 
> > > > +
> > > >  	if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
> > > >  		intel_dp_stop_link_train(intel_dp);
> > > >  
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > > index b4e8af3142a2..b9f85502d9ff 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -3017,6 +3017,33 @@ void intel_dp_sink_set_fec_ready(struct intel_dp *intel_dp,
> > > >  		DRM_DEBUG_KMS("Failed to get FEC enabled in sink\n");
> > > >  }
> > > >  
> > > > +void intel_dp_enable_fec_state(struct intel_dp *intel_dp,
> > > > +			       const struct intel_crtc_state *crtc_state)
> > > > +{
> > > > +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > > > +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > > > +	enum port port = intel_dig_port->base.port;
> > > > +	u32 val;
> > > > +
> > > > +	/* FEC support exists for DP 1.4 only */
> > > > +	if (intel_dp_is_edp(intel_dp))
> > > > +		return;
> > > > +
> > > > +	/* If Display Compression is not enabled, FEC need not be configured */
> > > > +	if (!crtc_state->dsc_params.compression_enable)
> > > > +		return;
> > > > +
> > > > +	val = I915_READ(DP_TP_CTL(port));
> > > > +	val |= DP_TP_CTL_FEC_ENABLE;
> > > > +	I915_WRITE(DP_TP_CTL(port), val);
> > > > +
> > > > +	if (intel_wait_for_register(dev_priv, DP_TP_STATUS(port),
> > > > +				    DP_TP_STATUS_FEC_ENABLE_LIVE,
> > > > +				    DP_TP_STATUS_FEC_ENABLE_LIVE,
> > > > +				    1))
> > > > +		DRM_ERROR("Timed out waiting for FEC Enable Status\n");
> > > > +}
> > > 
> > > There is no reason to stick these into intel_dp.c when the
> > > only caller is in intel_ddi.c.
> > >
> > 
> > So may be move the set_dsc_state function also from intel_dp.c to intel_ddi.c
> > Or does it make sense to move all these DSC related functions including fec to intel_dsc.c?
> >  
> > > We also seem to be missing platform checks. FEC doesn't appear to
> > > be a thing prior to ICL. I guess the edp+compression_enable takes care
> > > of this, but I think I'd rather stick a new fec_enable bool into
> > > the crtc state. That way we get can add it to the state checker as well.
> > >
> > 
> > fec_enable can be added to crtc_state->dsc_params may be?
> 
> Isn't FEC something that can be used w/o DSC too?
>

Nope, FEC always goes hand in hand with DSC on external DP and that DSC on external DP
cannot be enabled without FEC.

Manasi
 
> > 
> > Manasi
> >  
> > > > +
> > > >  /* If the sink supports it, try to set the power state appropriately */
> > > >  void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode)
> > > >  {
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > > index fbc9fa06e8be..e51d612a9f42 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -1532,7 +1532,6 @@ intel_encoder_current_mode(struct intel_encoder *encoder);
> > > >  bool intel_port_is_tc(struct drm_i915_private *dev_priv, enum port port);
> > > >  enum tc_port intel_port_to_tc(struct drm_i915_private *dev_priv,
> > > >  			      enum port port);
> > > > -
> > > >  enum pipe intel_get_pipe_from_connector(struct intel_connector *connector);
> > > >  int intel_get_pipe_from_crtc_id_ioctl(struct drm_device *dev, void *data,
> > > >  				      struct drm_file *file_priv);
> > > > @@ -1712,6 +1711,8 @@ void intel_dp_sink_set_decompression_state(struct intel_dp *intel_dp,
> > > >  void intel_dp_sink_set_fec_ready(struct intel_dp *intel_dp,
> > > >  				 const struct intel_crtc_state *crtc_state,
> > > >  				 int state);
> > > > +void intel_dp_enable_fec_state(struct intel_dp *intel_dp,
> > > > +			       const struct intel_crtc_state *crtc_state);
> > > >  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);
> > > > -- 
> > > > 2.17.1
> > > 
> > > -- 
> > > Ville Syrjälä
> > > Intel
> 
> -- 
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [v2 5/6] i915/dp/fec: Configure the Forward Error Correction bits.

[Srivatsa, Anusha]

>-----Original Message-----
>From: Navare, Manasi D
>Sent: Friday, October 19, 2018 1:29 PM
>To: Ville Syrjälä <ville.syrjala@linux.intel.com>
>Cc: Srivatsa, Anusha <anusha.srivatsa@intel.com>; intel-
>gfx@lists.freedesktop.org; Singh, Gaurav K <gaurav.k.singh@intel.com>; Jani
>Nikula <jani.nikula@linux.intel.com>
>Subject: Re: [v2 5/6] i915/dp/fec: Configure the Forward Error Correction bits.
>
>On Fri, Oct 19, 2018 at 10:58:29PM +0300, Ville Syrjälä wrote:
>> On Fri, Oct 19, 2018 at 12:24:43PM -0700, Manasi Navare wrote:
>> > On Fri, Oct 19, 2018 at 09:39:11PM +0300, Ville Syrjälä wrote:
>> > > On Mon, Oct 15, 2018 at 02:50:36PM -0700, Anusha Srivatsa wrote:
>> > > > If FEC is supported, the corresponding DP_TP_CTL register bits
>> > > > have to be configured.
>> > > >
>> > > > The driver has to program the FEC_ENABLE in DP_TP_CTL[30]
>> > > > register and wait till FEC_STATUS in DP_TP_CTL[28] is 1.
>> > > > Also add the warn message to make sure that the control register
>> > > > is already active while enabling FEC.
>> > > >
>> > > > v2:
>> > > > - Change commit message. Configure fec state after
>> > > >   link training (Manasi, Gaurav)
>> > > > - Remove redundent checks (Manasi)
>> > > > - Remove the registers that get added automagically (Anusha)
>> > > >
>> > > > v3: s/intel_dp_set_fec_state()/intel_dp_enable_fec_state()
>> > > > (Gaurav)
>> > > >
>> > > > v4: rebased.
>> > > >
>> > > > Cc: Gaurav K Singh <gaurav.k.singh@intel.com>
>> > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
>> > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>> > > > Cc: Manasi Navare <manasi.d.navare@intel.com>
>> > > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
>> > > > ---
>> > > >  drivers/gpu/drm/i915/i915_reg.h  |  2 ++
>> > > > drivers/gpu/drm/i915/intel_ddi.c |  2 ++
>> > > > drivers/gpu/drm/i915/intel_dp.c  | 27
>> > > > +++++++++++++++++++++++++++  drivers/gpu/drm/i915/intel_drv.h |
>> > > > 3 ++-
>> > > >  4 files changed, 33 insertions(+), 1 deletion(-)
>> > > >
>> > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> > > > b/drivers/gpu/drm/i915/i915_reg.h index
>> > > > bcde78bc0027..c8d7fdcd7823 100644
>> > > > --- a/drivers/gpu/drm/i915/i915_reg.h
>> > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
>> > > > @@ -9093,6 +9093,7 @@ enum skl_power_gate {
>> > > >  #define _DP_TP_CTL_B			0x64140
>> > > >  #define DP_TP_CTL(port) _MMIO_PORT(port, _DP_TP_CTL_A,
>_DP_TP_CTL_B)
>> > > >  #define  DP_TP_CTL_ENABLE			(1 << 31)
>> > > > +#define  DP_TP_CTL_FEC_ENABLE			(1 << 30)
>> > > >  #define  DP_TP_CTL_MODE_SST			(0 << 27)
>> > > >  #define  DP_TP_CTL_MODE_MST			(1 << 27)
>> > > >  #define  DP_TP_CTL_FORCE_ACT			(1 << 25)
>> > > > @@ -9111,6 +9112,7 @@ enum skl_power_gate {
>> > > >  #define _DP_TP_STATUS_A			0x64044
>> > > >  #define _DP_TP_STATUS_B			0x64144
>> > > >  #define DP_TP_STATUS(port) _MMIO_PORT(port, _DP_TP_STATUS_A,
>> > > > _DP_TP_STATUS_B)
>> > > > +#define  DP_TP_STATUS_FEC_ENABLE_LIVE		(1 << 28)
>> > > >  #define  DP_TP_STATUS_IDLE_DONE			(1 << 25)
>> > > >  #define  DP_TP_STATUS_ACT_SENT			(1 << 24)
>> > > >  #define  DP_TP_STATUS_MODE_STATUS_MST		(1 << 23)
>> > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
>> > > > b/drivers/gpu/drm/i915/intel_ddi.c
>> > > > index f531900165bf..67c013ea4d39 100644
>> > > > --- a/drivers/gpu/drm/i915/intel_ddi.c
>> > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> > > > @@ -2911,6 +2911,8 @@ static void intel_ddi_pre_enable_dp(struct
>intel_encoder *encoder,
>> > > >  					      DP_DECOMPRESSION_EN);
>> > > >  	intel_dp_sink_set_fec_ready(intel_dp, crtc_state, DP_FEC_READY);
>> > > >  	intel_dp_start_link_train(intel_dp);
>> > > > +	intel_dp_enable_fec_state(intel_dp, crtc_state);
>> > >
>> > > This doesn't look like the correct spot. According to the spec it
>> > > should be after stop_link_train() (where we set DP_TP_CTL to to
>> > > normal).
>> > >
>> >
>> > Yes I see in the display port enabling sequence page of the spec,
>> > that it says enable FEC after DP_TP_CTL is set to Normal. Also the
>> > FEC page says that this should be enabled only after ensuring that
>> > link training completed. So according to that this call should happen after
>intel_dp_stop_link_train().
>> > So yes move this to after intel_dp_stop_link_training().
>> >
>> > > > +
>> > > >  	if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
>> > > >  		intel_dp_stop_link_train(intel_dp);
>> > > >
>> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
>> > > > b/drivers/gpu/drm/i915/intel_dp.c index
>> > > > b4e8af3142a2..b9f85502d9ff 100644
>> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
>> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
>> > > > @@ -3017,6 +3017,33 @@ void intel_dp_sink_set_fec_ready(struct
>intel_dp *intel_dp,
>> > > >  		DRM_DEBUG_KMS("Failed to get FEC enabled in sink\n");  }
>> > > >
>> > > > +void intel_dp_enable_fec_state(struct intel_dp *intel_dp,
>> > > > +			       const struct intel_crtc_state *crtc_state) {
>> > > > +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
>> > > > +	struct intel_digital_port *intel_dig_port =
>dp_to_dig_port(intel_dp);
>> > > > +	enum port port = intel_dig_port->base.port;
>> > > > +	u32 val;
>> > > > +
>> > > > +	/* FEC support exists for DP 1.4 only */
>> > > > +	if (intel_dp_is_edp(intel_dp))
>> > > > +		return;
>> > > > +
>> > > > +	/* If Display Compression is not enabled, FEC need not be
>configured */
>> > > > +	if (!crtc_state->dsc_params.compression_enable)
>> > > > +		return;
>> > > > +
>> > > > +	val = I915_READ(DP_TP_CTL(port));
>> > > > +	val |= DP_TP_CTL_FEC_ENABLE;
>> > > > +	I915_WRITE(DP_TP_CTL(port), val);
>> > > > +
>> > > > +	if (intel_wait_for_register(dev_priv, DP_TP_STATUS(port),
>> > > > +				    DP_TP_STATUS_FEC_ENABLE_LIVE,
>> > > > +				    DP_TP_STATUS_FEC_ENABLE_LIVE,
>> > > > +				    1))
>> > > > +		DRM_ERROR("Timed out waiting for FEC Enable
>Status\n"); }
>> > >
>> > > There is no reason to stick these into intel_dp.c when the only
>> > > caller is in intel_ddi.c.
>> > >
>> >
>> > So may be move the set_dsc_state function also from intel_dp.c to
>> > intel_ddi.c Or does it make sense to move all these DSC related functions
>including fec to intel_dsc.c?
>> >
>> > > We also seem to be missing platform checks. FEC doesn't appear to
>> > > be a thing prior to ICL. I guess the edp+compression_enable takes
>> > > care of this, but I think I'd rather stick a new fec_enable bool
>> > > into the crtc state. That way we get can add it to the state checker as well.
>> > >
>> > fec_enable can be added to crtc_state->dsc_params may be?
>>
>> Isn't FEC something that can be used w/o DSC too?
>>
>
>Nope, FEC always goes hand in hand with DSC on external DP and that DSC on
>external DP cannot be enabled without FEC.

Yes, and we are checking that only when DSC is enabled on external DP, configure FEC. Why add it to crtc state too?

Anusha 
>Manasi
>
>> >
>> > Manasi
>> >
>> > > > +
>> > > >  /* If the sink supports it, try to set the power state
>> > > > appropriately */  void intel_dp_sink_dpms(struct intel_dp
>> > > > *intel_dp, int mode)  { diff --git
>> > > > a/drivers/gpu/drm/i915/intel_drv.h
>> > > > b/drivers/gpu/drm/i915/intel_drv.h
>> > > > index fbc9fa06e8be..e51d612a9f42 100644
>> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
>> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
>> > > > @@ -1532,7 +1532,6 @@ intel_encoder_current_mode(struct
>> > > > intel_encoder *encoder);  bool intel_port_is_tc(struct
>> > > > drm_i915_private *dev_priv, enum port port);  enum tc_port
>intel_port_to_tc(struct drm_i915_private *dev_priv,
>> > > >  			      enum port port);
>> > > > -
>> > > >  enum pipe intel_get_pipe_from_connector(struct intel_connector
>> > > > *connector);  int intel_get_pipe_from_crtc_id_ioctl(struct drm_device
>*dev, void *data,
>> > > >  				      struct drm_file *file_priv); @@ -1712,6
>+1711,8 @@
>> > > > void intel_dp_sink_set_decompression_state(struct intel_dp
>> > > > *intel_dp,  void intel_dp_sink_set_fec_ready(struct intel_dp *intel_dp,
>> > > >  				 const struct intel_crtc_state *crtc_state,
>> > > >  				 int state);
>> > > > +void intel_dp_enable_fec_state(struct intel_dp *intel_dp,
>> > > > +			       const struct intel_crtc_state *crtc_state);
>> > > >  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);
>> > > > --
>> > > > 2.17.1
>> > >
>> > > --
>> > > Ville Syrjälä
>> > > Intel
>>
>> --
>> Ville Syrjälä
>> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [v2 5/6] i915/dp/fec: Configure the Forward Error Correction bits.

[Ville Syrjälä]

On Fri, Oct 19, 2018 at 01:29:05PM -0700, Manasi Navare wrote:
> On Fri, Oct 19, 2018 at 10:58:29PM +0300, Ville Syrjälä wrote:
> > On Fri, Oct 19, 2018 at 12:24:43PM -0700, Manasi Navare wrote:
> > > On Fri, Oct 19, 2018 at 09:39:11PM +0300, Ville Syrjälä wrote:
> > > > On Mon, Oct 15, 2018 at 02:50:36PM -0700, Anusha Srivatsa wrote:
> > > > > If FEC is supported, the corresponding
> > > > > DP_TP_CTL register bits have to be configured.
> > > > > 
> > > > > The driver has to program the FEC_ENABLE in DP_TP_CTL[30] register
> > > > > and wait till FEC_STATUS in DP_TP_CTL[28] is 1.
> > > > > Also add the warn message to make sure that the control
> > > > > register is already active while enabling FEC.
> > > > > 
> > > > > v2:
> > > > > - Change commit message. Configure fec state after
> > > > >   link training (Manasi, Gaurav)
> > > > > - Remove redundent checks (Manasi)
> > > > > - Remove the registers that get added automagically (Anusha)
> > > > > 
> > > > > v3: s/intel_dp_set_fec_state()/intel_dp_enable_fec_state() (Gaurav)
> > > > > 
> > > > > v4: rebased.
> > > > > 
> > > > > Cc: Gaurav K Singh <gaurav.k.singh@intel.com>
> > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > > > > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > > > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/i915_reg.h  |  2 ++
> > > > >  drivers/gpu/drm/i915/intel_ddi.c |  2 ++
> > > > >  drivers/gpu/drm/i915/intel_dp.c  | 27 +++++++++++++++++++++++++++
> > > > >  drivers/gpu/drm/i915/intel_drv.h |  3 ++-
> > > > >  4 files changed, 33 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > > > index bcde78bc0027..c8d7fdcd7823 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > > @@ -9093,6 +9093,7 @@ enum skl_power_gate {
> > > > >  #define _DP_TP_CTL_B			0x64140
> > > > >  #define DP_TP_CTL(port) _MMIO_PORT(port, _DP_TP_CTL_A, _DP_TP_CTL_B)
> > > > >  #define  DP_TP_CTL_ENABLE			(1 << 31)
> > > > > +#define  DP_TP_CTL_FEC_ENABLE			(1 << 30)
> > > > >  #define  DP_TP_CTL_MODE_SST			(0 << 27)
> > > > >  #define  DP_TP_CTL_MODE_MST			(1 << 27)
> > > > >  #define  DP_TP_CTL_FORCE_ACT			(1 << 25)
> > > > > @@ -9111,6 +9112,7 @@ enum skl_power_gate {
> > > > >  #define _DP_TP_STATUS_A			0x64044
> > > > >  #define _DP_TP_STATUS_B			0x64144
> > > > >  #define DP_TP_STATUS(port) _MMIO_PORT(port, _DP_TP_STATUS_A, _DP_TP_STATUS_B)
> > > > > +#define  DP_TP_STATUS_FEC_ENABLE_LIVE		(1 << 28)
> > > > >  #define  DP_TP_STATUS_IDLE_DONE			(1 << 25)
> > > > >  #define  DP_TP_STATUS_ACT_SENT			(1 << 24)
> > > > >  #define  DP_TP_STATUS_MODE_STATUS_MST		(1 << 23)
> > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > > > > index f531900165bf..67c013ea4d39 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > > > @@ -2911,6 +2911,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
> > > > >  					      DP_DECOMPRESSION_EN);
> > > > >  	intel_dp_sink_set_fec_ready(intel_dp, crtc_state, DP_FEC_READY);
> > > > >  	intel_dp_start_link_train(intel_dp);
> > > > > +	intel_dp_enable_fec_state(intel_dp, crtc_state);
> > > > 
> > > > This doesn't look like the correct spot. According to the spec it
> > > > should be after stop_link_train() (where we set DP_TP_CTL to
> > > > to normal).
> > > >
> > > 
> > > Yes I see in the display port enabling sequence page of the spec, that it says enable FEC
> > > after DP_TP_CTL is set to Normal. Also the FEC page says that this should be enabled only after ensuring that link training
> > > completed. So according to that this call should happen after
> > > intel_dp_stop_link_train(). 
> > > So yes move this to after intel_dp_stop_link_training().
> > > 
> > > > > +
> > > > >  	if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
> > > > >  		intel_dp_stop_link_train(intel_dp);
> > > > >  
> > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > > > index b4e8af3142a2..b9f85502d9ff 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > > @@ -3017,6 +3017,33 @@ void intel_dp_sink_set_fec_ready(struct intel_dp *intel_dp,
> > > > >  		DRM_DEBUG_KMS("Failed to get FEC enabled in sink\n");
> > > > >  }
> > > > >  
> > > > > +void intel_dp_enable_fec_state(struct intel_dp *intel_dp,
> > > > > +			       const struct intel_crtc_state *crtc_state)
> > > > > +{
> > > > > +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > > > > +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > > > > +	enum port port = intel_dig_port->base.port;
> > > > > +	u32 val;
> > > > > +
> > > > > +	/* FEC support exists for DP 1.4 only */
> > > > > +	if (intel_dp_is_edp(intel_dp))
> > > > > +		return;
> > > > > +
> > > > > +	/* If Display Compression is not enabled, FEC need not be configured */
> > > > > +	if (!crtc_state->dsc_params.compression_enable)
> > > > > +		return;
> > > > > +
> > > > > +	val = I915_READ(DP_TP_CTL(port));
> > > > > +	val |= DP_TP_CTL_FEC_ENABLE;
> > > > > +	I915_WRITE(DP_TP_CTL(port), val);
> > > > > +
> > > > > +	if (intel_wait_for_register(dev_priv, DP_TP_STATUS(port),
> > > > > +				    DP_TP_STATUS_FEC_ENABLE_LIVE,
> > > > > +				    DP_TP_STATUS_FEC_ENABLE_LIVE,
> > > > > +				    1))
> > > > > +		DRM_ERROR("Timed out waiting for FEC Enable Status\n");
> > > > > +}
> > > > 
> > > > There is no reason to stick these into intel_dp.c when the
> > > > only caller is in intel_ddi.c.
> > > >
> > > 
> > > So may be move the set_dsc_state function also from intel_dp.c to intel_ddi.c
> > > Or does it make sense to move all these DSC related functions including fec to intel_dsc.c?
> > >  
> > > > We also seem to be missing platform checks. FEC doesn't appear to
> > > > be a thing prior to ICL. I guess the edp+compression_enable takes care
> > > > of this, but I think I'd rather stick a new fec_enable bool into
> > > > the crtc state. That way we get can add it to the state checker as well.
> > > >
> > > 
> > > fec_enable can be added to crtc_state->dsc_params may be?
> > 
> > Isn't FEC something that can be used w/o DSC too?
> >
> 
> Nope, FEC always goes hand in hand with DSC on external DP and that DSC on external DP
> cannot be enabled without FEC.

Sure, DSC needs FEC. But FEC doesn't need DSC. At least if I'm reading
the spec correctly.

FEC is enabled for the entire main link, so everything getting
transported over it get FECced. DSC on the other hand operates on
on a stream level. So assuming a MST branch device that can do FEC
but can't do DSC you should be able transport both compressed and
uncompressed streams through it simultaneously. Kinda makes me think
that we want to enable FEC for MST whenever the downstream device
supports it.

And I suppose we could also consider using FEC opportunistically
for uncompressed SST when we have the extra link bandwidth to
handle the extra overhead.

> 
> Manasi
>  
> > > 
> > > Manasi
> > >  
> > > > > +
> > > > >  /* If the sink supports it, try to set the power state appropriately */
> > > > >  void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode)
> > > > >  {
> > > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > > > index fbc9fa06e8be..e51d612a9f42 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > > @@ -1532,7 +1532,6 @@ intel_encoder_current_mode(struct intel_encoder *encoder);
> > > > >  bool intel_port_is_tc(struct drm_i915_private *dev_priv, enum port port);
> > > > >  enum tc_port intel_port_to_tc(struct drm_i915_private *dev_priv,
> > > > >  			      enum port port);
> > > > > -
> > > > >  enum pipe intel_get_pipe_from_connector(struct intel_connector *connector);
> > > > >  int intel_get_pipe_from_crtc_id_ioctl(struct drm_device *dev, void *data,
> > > > >  				      struct drm_file *file_priv);
> > > > > @@ -1712,6 +1711,8 @@ void intel_dp_sink_set_decompression_state(struct intel_dp *intel_dp,
> > > > >  void intel_dp_sink_set_fec_ready(struct intel_dp *intel_dp,
> > > > >  				 const struct intel_crtc_state *crtc_state,
> > > > >  				 int state);
> > > > > +void intel_dp_enable_fec_state(struct intel_dp *intel_dp,
> > > > > +			       const struct intel_crtc_state *crtc_state);
> > > > >  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);
> > > > > -- 
> > > > > 2.17.1
> > > > 
> > > > -- 
> > > > Ville Syrjälä
> > > > Intel
> > 
> > -- 
> > Ville Syrjälä
> > Intel

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

Re: [v2 5/6] i915/dp/fec: Configure the Forward Error Correction bits.

[Srivatsa, Anusha]

>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>Sent: Friday, October 19, 2018 4:12 PM
>To: Navare, Manasi D <manasi.d.navare@intel.com>
>Cc: Srivatsa, Anusha <anusha.srivatsa@intel.com>; intel-
>gfx@lists.freedesktop.org; Singh, Gaurav K <gaurav.k.singh@intel.com>; Jani
>Nikula <jani.nikula@linux.intel.com>
>Subject: Re: [v2 5/6] i915/dp/fec: Configure the Forward Error Correction bits.
>
>On Fri, Oct 19, 2018 at 01:29:05PM -0700, Manasi Navare wrote:
>> On Fri, Oct 19, 2018 at 10:58:29PM +0300, Ville Syrjälä wrote:
>> > On Fri, Oct 19, 2018 at 12:24:43PM -0700, Manasi Navare wrote:
>> > > On Fri, Oct 19, 2018 at 09:39:11PM +0300, Ville Syrjälä wrote:
>> > > > On Mon, Oct 15, 2018 at 02:50:36PM -0700, Anusha Srivatsa wrote:
>> > > > > If FEC is supported, the corresponding DP_TP_CTL register bits
>> > > > > have to be configured.
>> > > > >
>> > > > > The driver has to program the FEC_ENABLE in DP_TP_CTL[30]
>> > > > > register and wait till FEC_STATUS in DP_TP_CTL[28] is 1.
>> > > > > Also add the warn message to make sure that the control
>> > > > > register is already active while enabling FEC.
>> > > > >
>> > > > > v2:
>> > > > > - Change commit message. Configure fec state after
>> > > > >   link training (Manasi, Gaurav)
>> > > > > - Remove redundent checks (Manasi)
>> > > > > - Remove the registers that get added automagically (Anusha)
>> > > > >
>> > > > > v3: s/intel_dp_set_fec_state()/intel_dp_enable_fec_state()
>> > > > > (Gaurav)
>> > > > >
>> > > > > v4: rebased.
>> > > > >
>> > > > > Cc: Gaurav K Singh <gaurav.k.singh@intel.com>
>> > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
>> > > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>> > > > > Cc: Manasi Navare <manasi.d.navare@intel.com>
>> > > > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
>> > > > > ---
>> > > > >  drivers/gpu/drm/i915/i915_reg.h  |  2 ++
>> > > > > drivers/gpu/drm/i915/intel_ddi.c |  2 ++
>> > > > > drivers/gpu/drm/i915/intel_dp.c  | 27
>> > > > > +++++++++++++++++++++++++++  drivers/gpu/drm/i915/intel_drv.h
>> > > > > |  3 ++-
>> > > > >  4 files changed, 33 insertions(+), 1 deletion(-)
>> > > > >
>> > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> > > > > b/drivers/gpu/drm/i915/i915_reg.h index
>> > > > > bcde78bc0027..c8d7fdcd7823 100644
>> > > > > --- a/drivers/gpu/drm/i915/i915_reg.h
>> > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
>> > > > > @@ -9093,6 +9093,7 @@ enum skl_power_gate {
>> > > > >  #define _DP_TP_CTL_B			0x64140
>> > > > >  #define DP_TP_CTL(port) _MMIO_PORT(port, _DP_TP_CTL_A,
>_DP_TP_CTL_B)
>> > > > >  #define  DP_TP_CTL_ENABLE			(1 << 31)
>> > > > > +#define  DP_TP_CTL_FEC_ENABLE			(1 << 30)
>> > > > >  #define  DP_TP_CTL_MODE_SST			(0 << 27)
>> > > > >  #define  DP_TP_CTL_MODE_MST			(1 << 27)
>> > > > >  #define  DP_TP_CTL_FORCE_ACT			(1 << 25)
>> > > > > @@ -9111,6 +9112,7 @@ enum skl_power_gate {
>> > > > >  #define _DP_TP_STATUS_A			0x64044
>> > > > >  #define _DP_TP_STATUS_B			0x64144
>> > > > >  #define DP_TP_STATUS(port) _MMIO_PORT(port, _DP_TP_STATUS_A,
>> > > > > _DP_TP_STATUS_B)
>> > > > > +#define  DP_TP_STATUS_FEC_ENABLE_LIVE		(1 << 28)
>> > > > >  #define  DP_TP_STATUS_IDLE_DONE			(1 << 25)
>> > > > >  #define  DP_TP_STATUS_ACT_SENT			(1 << 24)
>> > > > >  #define  DP_TP_STATUS_MODE_STATUS_MST		(1 << 23)
>> > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
>> > > > > b/drivers/gpu/drm/i915/intel_ddi.c
>> > > > > index f531900165bf..67c013ea4d39 100644
>> > > > > --- a/drivers/gpu/drm/i915/intel_ddi.c
>> > > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> > > > > @@ -2911,6 +2911,8 @@ static void intel_ddi_pre_enable_dp(struct
>intel_encoder *encoder,
>> > > > >  					      DP_DECOMPRESSION_EN);
>> > > > >  	intel_dp_sink_set_fec_ready(intel_dp, crtc_state,
>DP_FEC_READY);
>> > > > >  	intel_dp_start_link_train(intel_dp);
>> > > > > +	intel_dp_enable_fec_state(intel_dp, crtc_state);
>> > > >
>> > > > This doesn't look like the correct spot. According to the spec
>> > > > it should be after stop_link_train() (where we set DP_TP_CTL to
>> > > > to normal).
>> > > >
>> > >
>> > > Yes I see in the display port enabling sequence page of the spec,
>> > > that it says enable FEC after DP_TP_CTL is set to Normal. Also the
>> > > FEC page says that this should be enabled only after ensuring that
>> > > link training completed. So according to that this call should happen after
>intel_dp_stop_link_train().
>> > > So yes move this to after intel_dp_stop_link_training().
>> > >
>> > > > > +
>> > > > >  	if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
>> > > > >  		intel_dp_stop_link_train(intel_dp);
>> > > > >
>> > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
>> > > > > b/drivers/gpu/drm/i915/intel_dp.c index
>> > > > > b4e8af3142a2..b9f85502d9ff 100644
>> > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
>> > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
>> > > > > @@ -3017,6 +3017,33 @@ void intel_dp_sink_set_fec_ready(struct
>intel_dp *intel_dp,
>> > > > >  		DRM_DEBUG_KMS("Failed to get FEC enabled in
>sink\n");  }
>> > > > >
>> > > > > +void intel_dp_enable_fec_state(struct intel_dp *intel_dp,
>> > > > > +			       const struct intel_crtc_state *crtc_state) {
>> > > > > +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
>> > > > > +	struct intel_digital_port *intel_dig_port =
>dp_to_dig_port(intel_dp);
>> > > > > +	enum port port = intel_dig_port->base.port;
>> > > > > +	u32 val;
>> > > > > +
>> > > > > +	/* FEC support exists for DP 1.4 only */
>> > > > > +	if (intel_dp_is_edp(intel_dp))
>> > > > > +		return;
>> > > > > +
>> > > > > +	/* If Display Compression is not enabled, FEC need not be
>configured */
>> > > > > +	if (!crtc_state->dsc_params.compression_enable)
>> > > > > +		return;
>> > > > > +
>> > > > > +	val = I915_READ(DP_TP_CTL(port));
>> > > > > +	val |= DP_TP_CTL_FEC_ENABLE;
>> > > > > +	I915_WRITE(DP_TP_CTL(port), val);
>> > > > > +
>> > > > > +	if (intel_wait_for_register(dev_priv, DP_TP_STATUS(port),
>> > > > > +				    DP_TP_STATUS_FEC_ENABLE_LIVE,
>> > > > > +				    DP_TP_STATUS_FEC_ENABLE_LIVE,
>> > > > > +				    1))
>> > > > > +		DRM_ERROR("Timed out waiting for FEC Enable
>Status\n"); }
>> > > >
>> > > > There is no reason to stick these into intel_dp.c when the only
>> > > > caller is in intel_ddi.c.
>> > > >
>> > >
>> > > So may be move the set_dsc_state function also from intel_dp.c to
>> > > intel_ddi.c Or does it make sense to move all these DSC related functions
>including fec to intel_dsc.c?
>> > >
>> > > > We also seem to be missing platform checks. FEC doesn't appear
>> > > > to be a thing prior to ICL. I guess the edp+compression_enable
>> > > > takes care of this, but I think I'd rather stick a new
>> > > > fec_enable bool into the crtc state. That way we get can add it to the
>state checker as well.
>> > > >
>> > >
>> > > fec_enable can be added to crtc_state->dsc_params may be?
>> >
>> > Isn't FEC something that can be used w/o DSC too?
>> >
>>
>> Nope, FEC always goes hand in hand with DSC on external DP and that
>> DSC on external DP cannot be enabled without FEC.
>
>Sure, DSC needs FEC. But FEC doesn't need DSC. At least if I'm reading the spec
>correctly.
>
>FEC is enabled for the entire main link, so everything getting transported over it
>get FECced. DSC on the other hand operates on on a stream level. So assuming a
>MST branch device that can do FEC but can't do DSC you should be able transport
>both compressed and uncompressed streams through it simultaneously. Kinda
>makes me think that we want to enable FEC for MST whenever the downstream
>device supports it.

FEC for MST modes is not supported for ICL platform. We could extend this for future platforms  for branch devices that support FEC.

>And I suppose we could also consider using FEC opportunistically for
>uncompressed SST when we have the extra link bandwidth to handle the extra
>overhead.

Yes. The requirement though is for compressed streams. I am not sure if we will have to pay any power penalty for SST. Also my understanding is that if a few symbols in a non-compressed stream are in error, then only those symbols will be in error. These errors do not cause other pixels to get corrupted. In compressed streams, if a few symbols are in error, it can cause the entire slice to get corrupted. Will we be getting any much return on non-compressed streams?

Anusha 
>>
>> Manasi
>>
>> > >
>> > > Manasi
>> > >
>> > > > > +
>> > > > >  /* If the sink supports it, try to set the power state
>> > > > > appropriately */  void intel_dp_sink_dpms(struct intel_dp
>> > > > > *intel_dp, int mode)  { diff --git
>> > > > > a/drivers/gpu/drm/i915/intel_drv.h
>> > > > > b/drivers/gpu/drm/i915/intel_drv.h
>> > > > > index fbc9fa06e8be..e51d612a9f42 100644
>> > > > > --- a/drivers/gpu/drm/i915/intel_drv.h
>> > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
>> > > > > @@ -1532,7 +1532,6 @@ intel_encoder_current_mode(struct
>> > > > > intel_encoder *encoder);  bool intel_port_is_tc(struct
>> > > > > drm_i915_private *dev_priv, enum port port);  enum tc_port
>intel_port_to_tc(struct drm_i915_private *dev_priv,
>> > > > >  			      enum port port);
>> > > > > -
>> > > > >  enum pipe intel_get_pipe_from_connector(struct
>> > > > > intel_connector *connector);  int
>intel_get_pipe_from_crtc_id_ioctl(struct drm_device *dev, void *data,
>> > > > >  				      struct drm_file *file_priv); @@ -
>1712,6 +1711,8 @@
>> > > > > void intel_dp_sink_set_decompression_state(struct intel_dp
>> > > > > *intel_dp,  void intel_dp_sink_set_fec_ready(struct intel_dp *intel_dp,
>> > > > >  				 const struct intel_crtc_state
>*crtc_state,
>> > > > >  				 int state);
>> > > > > +void intel_dp_enable_fec_state(struct intel_dp *intel_dp,
>> > > > > +			       const struct intel_crtc_state *crtc_state);
>> > > > >  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);
>> > > > > --
>> > > > > 2.17.1
>> > > >
>> > > > --
>> > > > Ville Syrjälä
>> > > > Intel
>> >
>> > --
>> > Ville Syrjälä
>> > Intel
>
>--
>Ville Syrjälä
>Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [v2 5/6] i915/dp/fec: Configure the Forward Error Correction bits.

[Ville Syrjälä]

On Mon, Oct 22, 2018 at 06:04:28PM +0000, Srivatsa, Anusha wrote:
> 
> 
> >-----Original Message-----
> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> >Sent: Friday, October 19, 2018 4:12 PM
> >To: Navare, Manasi D <manasi.d.navare@intel.com>
> >Cc: Srivatsa, Anusha <anusha.srivatsa@intel.com>; intel-
> >gfx@lists.freedesktop.org; Singh, Gaurav K <gaurav.k.singh@intel.com>; Jani
> >Nikula <jani.nikula@linux.intel.com>
> >Subject: Re: [v2 5/6] i915/dp/fec: Configure the Forward Error Correction bits.
> >
> >On Fri, Oct 19, 2018 at 01:29:05PM -0700, Manasi Navare wrote:
> >> On Fri, Oct 19, 2018 at 10:58:29PM +0300, Ville Syrjälä wrote:
> >> > On Fri, Oct 19, 2018 at 12:24:43PM -0700, Manasi Navare wrote:
> >> > > On Fri, Oct 19, 2018 at 09:39:11PM +0300, Ville Syrjälä wrote:
> >> > > > On Mon, Oct 15, 2018 at 02:50:36PM -0700, Anusha Srivatsa wrote:
> >> > > > > If FEC is supported, the corresponding DP_TP_CTL register bits
> >> > > > > have to be configured.
> >> > > > >
> >> > > > > The driver has to program the FEC_ENABLE in DP_TP_CTL[30]
> >> > > > > register and wait till FEC_STATUS in DP_TP_CTL[28] is 1.
> >> > > > > Also add the warn message to make sure that the control
> >> > > > > register is already active while enabling FEC.
> >> > > > >
> >> > > > > v2:
> >> > > > > - Change commit message. Configure fec state after
> >> > > > >   link training (Manasi, Gaurav)
> >> > > > > - Remove redundent checks (Manasi)
> >> > > > > - Remove the registers that get added automagically (Anusha)
> >> > > > >
> >> > > > > v3: s/intel_dp_set_fec_state()/intel_dp_enable_fec_state()
> >> > > > > (Gaurav)
> >> > > > >
> >> > > > > v4: rebased.
> >> > > > >
> >> > > > > Cc: Gaurav K Singh <gaurav.k.singh@intel.com>
> >> > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> >> > > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> >> > > > > Cc: Manasi Navare <manasi.d.navare@intel.com>
> >> > > > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> >> > > > > ---
> >> > > > >  drivers/gpu/drm/i915/i915_reg.h  |  2 ++
> >> > > > > drivers/gpu/drm/i915/intel_ddi.c |  2 ++
> >> > > > > drivers/gpu/drm/i915/intel_dp.c  | 27
> >> > > > > +++++++++++++++++++++++++++  drivers/gpu/drm/i915/intel_drv.h
> >> > > > > |  3 ++-
> >> > > > >  4 files changed, 33 insertions(+), 1 deletion(-)
> >> > > > >
> >> > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> >> > > > > b/drivers/gpu/drm/i915/i915_reg.h index
> >> > > > > bcde78bc0027..c8d7fdcd7823 100644
> >> > > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> >> > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> >> > > > > @@ -9093,6 +9093,7 @@ enum skl_power_gate {
> >> > > > >  #define _DP_TP_CTL_B			0x64140
> >> > > > >  #define DP_TP_CTL(port) _MMIO_PORT(port, _DP_TP_CTL_A,
> >_DP_TP_CTL_B)
> >> > > > >  #define  DP_TP_CTL_ENABLE			(1 << 31)
> >> > > > > +#define  DP_TP_CTL_FEC_ENABLE			(1 << 30)
> >> > > > >  #define  DP_TP_CTL_MODE_SST			(0 << 27)
> >> > > > >  #define  DP_TP_CTL_MODE_MST			(1 << 27)
> >> > > > >  #define  DP_TP_CTL_FORCE_ACT			(1 << 25)
> >> > > > > @@ -9111,6 +9112,7 @@ enum skl_power_gate {
> >> > > > >  #define _DP_TP_STATUS_A			0x64044
> >> > > > >  #define _DP_TP_STATUS_B			0x64144
> >> > > > >  #define DP_TP_STATUS(port) _MMIO_PORT(port, _DP_TP_STATUS_A,
> >> > > > > _DP_TP_STATUS_B)
> >> > > > > +#define  DP_TP_STATUS_FEC_ENABLE_LIVE		(1 << 28)
> >> > > > >  #define  DP_TP_STATUS_IDLE_DONE			(1 << 25)
> >> > > > >  #define  DP_TP_STATUS_ACT_SENT			(1 << 24)
> >> > > > >  #define  DP_TP_STATUS_MODE_STATUS_MST		(1 << 23)
> >> > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> >> > > > > b/drivers/gpu/drm/i915/intel_ddi.c
> >> > > > > index f531900165bf..67c013ea4d39 100644
> >> > > > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> >> > > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> >> > > > > @@ -2911,6 +2911,8 @@ static void intel_ddi_pre_enable_dp(struct
> >intel_encoder *encoder,
> >> > > > >  					      DP_DECOMPRESSION_EN);
> >> > > > >  	intel_dp_sink_set_fec_ready(intel_dp, crtc_state,
> >DP_FEC_READY);
> >> > > > >  	intel_dp_start_link_train(intel_dp);
> >> > > > > +	intel_dp_enable_fec_state(intel_dp, crtc_state);
> >> > > >
> >> > > > This doesn't look like the correct spot. According to the spec
> >> > > > it should be after stop_link_train() (where we set DP_TP_CTL to
> >> > > > to normal).
> >> > > >
> >> > >
> >> > > Yes I see in the display port enabling sequence page of the spec,
> >> > > that it says enable FEC after DP_TP_CTL is set to Normal. Also the
> >> > > FEC page says that this should be enabled only after ensuring that
> >> > > link training completed. So according to that this call should happen after
> >intel_dp_stop_link_train().
> >> > > So yes move this to after intel_dp_stop_link_training().
> >> > >
> >> > > > > +
> >> > > > >  	if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
> >> > > > >  		intel_dp_stop_link_train(intel_dp);
> >> > > > >
> >> > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> >> > > > > b/drivers/gpu/drm/i915/intel_dp.c index
> >> > > > > b4e8af3142a2..b9f85502d9ff 100644
> >> > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> >> > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> >> > > > > @@ -3017,6 +3017,33 @@ void intel_dp_sink_set_fec_ready(struct
> >intel_dp *intel_dp,
> >> > > > >  		DRM_DEBUG_KMS("Failed to get FEC enabled in
> >sink\n");  }
> >> > > > >
> >> > > > > +void intel_dp_enable_fec_state(struct intel_dp *intel_dp,
> >> > > > > +			       const struct intel_crtc_state *crtc_state) {
> >> > > > > +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> >> > > > > +	struct intel_digital_port *intel_dig_port =
> >dp_to_dig_port(intel_dp);
> >> > > > > +	enum port port = intel_dig_port->base.port;
> >> > > > > +	u32 val;
> >> > > > > +
> >> > > > > +	/* FEC support exists for DP 1.4 only */
> >> > > > > +	if (intel_dp_is_edp(intel_dp))
> >> > > > > +		return;
> >> > > > > +
> >> > > > > +	/* If Display Compression is not enabled, FEC need not be
> >configured */
> >> > > > > +	if (!crtc_state->dsc_params.compression_enable)
> >> > > > > +		return;
> >> > > > > +
> >> > > > > +	val = I915_READ(DP_TP_CTL(port));
> >> > > > > +	val |= DP_TP_CTL_FEC_ENABLE;
> >> > > > > +	I915_WRITE(DP_TP_CTL(port), val);
> >> > > > > +
> >> > > > > +	if (intel_wait_for_register(dev_priv, DP_TP_STATUS(port),
> >> > > > > +				    DP_TP_STATUS_FEC_ENABLE_LIVE,
> >> > > > > +				    DP_TP_STATUS_FEC_ENABLE_LIVE,
> >> > > > > +				    1))
> >> > > > > +		DRM_ERROR("Timed out waiting for FEC Enable
> >Status\n"); }
> >> > > >
> >> > > > There is no reason to stick these into intel_dp.c when the only
> >> > > > caller is in intel_ddi.c.
> >> > > >
> >> > >
> >> > > So may be move the set_dsc_state function also from intel_dp.c to
> >> > > intel_ddi.c Or does it make sense to move all these DSC related functions
> >including fec to intel_dsc.c?
> >> > >
> >> > > > We also seem to be missing platform checks. FEC doesn't appear
> >> > > > to be a thing prior to ICL. I guess the edp+compression_enable
> >> > > > takes care of this, but I think I'd rather stick a new
> >> > > > fec_enable bool into the crtc state. That way we get can add it to the
> >state checker as well.
> >> > > >
> >> > >
> >> > > fec_enable can be added to crtc_state->dsc_params may be?
> >> >
> >> > Isn't FEC something that can be used w/o DSC too?
> >> >
> >>
> >> Nope, FEC always goes hand in hand with DSC on external DP and that
> >> DSC on external DP cannot be enabled without FEC.
> >
> >Sure, DSC needs FEC. But FEC doesn't need DSC. At least if I'm reading the spec
> >correctly.
> >
> >FEC is enabled for the entire main link, so everything getting transported over it
> >get FECced. DSC on the other hand operates on on a stream level. So assuming a
> >MST branch device that can do FEC but can't do DSC you should be able transport
> >both compressed and uncompressed streams through it simultaneously. Kinda
> >makes me think that we want to enable FEC for MST whenever the downstream
> >device supports it.
> 
> FEC for MST modes is not supported for ICL platform. We could extend this for future platforms  for branch devices that support FEC.

Hmm. Well, looks like we'll have to think about that soon enough anyway,
so IMO better do it right from the start. Avoids having to rewrite the
SST path yet again when we add the MST support. And we'll also get the
state checker as a bonus.

> 
> >And I suppose we could also consider using FEC opportunistically for
> >uncompressed SST when we have the extra link bandwidth to handle the extra
> >overhead.
> 
> Yes. The requirement though is for compressed streams. I am not sure if we will have to pay any power penalty for SST. Also my understanding is that if a few symbols in a non-compressed stream are in error, then only those symbols will be in error. These errors do not cause other pixels to get corrupted. In compressed streams, if a few symbols are in error, it can cause the entire slice to get corrupted. Will we be getting any much return on non-compressed streams?

No idea.

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

Re: [v2 5/6] i915/dp/fec: Configure the Forward Error Correction bits.

[Manasi Navare]

On Mon, Oct 22, 2018 at 11:04:28AM -0700, Srivatsa, Anusha wrote:
> 
> 
> >-----Original Message-----
> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> >Sent: Friday, October 19, 2018 4:12 PM
> >To: Navare, Manasi D <manasi.d.navare@intel.com>
> >Cc: Srivatsa, Anusha <anusha.srivatsa@intel.com>; intel-
> >gfx@lists.freedesktop.org; Singh, Gaurav K <gaurav.k.singh@intel.com>; Jani
> >Nikula <jani.nikula@linux.intel.com>
> >Subject: Re: [v2 5/6] i915/dp/fec: Configure the Forward Error Correction bits.
> >
> >On Fri, Oct 19, 2018 at 01:29:05PM -0700, Manasi Navare wrote:
> >> On Fri, Oct 19, 2018 at 10:58:29PM +0300, Ville Syrjälä wrote:
> >> > On Fri, Oct 19, 2018 at 12:24:43PM -0700, Manasi Navare wrote:
> >> > > On Fri, Oct 19, 2018 at 09:39:11PM +0300, Ville Syrjälä wrote:
> >> > > > On Mon, Oct 15, 2018 at 02:50:36PM -0700, Anusha Srivatsa wrote:
> >> > > > > If FEC is supported, the corresponding DP_TP_CTL register bits
> >> > > > > have to be configured.
> >> > > > >
> >> > > > > The driver has to program the FEC_ENABLE in DP_TP_CTL[30]
> >> > > > > register and wait till FEC_STATUS in DP_TP_CTL[28] is 1.
> >> > > > > Also add the warn message to make sure that the control
> >> > > > > register is already active while enabling FEC.
> >> > > > >
> >> > > > > v2:
> >> > > > > - Change commit message. Configure fec state after
> >> > > > >   link training (Manasi, Gaurav)
> >> > > > > - Remove redundent checks (Manasi)
> >> > > > > - Remove the registers that get added automagically (Anusha)
> >> > > > >
> >> > > > > v3: s/intel_dp_set_fec_state()/intel_dp_enable_fec_state()
> >> > > > > (Gaurav)
> >> > > > >
> >> > > > > v4: rebased.
> >> > > > >
> >> > > > > Cc: Gaurav K Singh <gaurav.k.singh@intel.com>
> >> > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> >> > > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> >> > > > > Cc: Manasi Navare <manasi.d.navare@intel.com>
> >> > > > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> >> > > > > ---
> >> > > > >  drivers/gpu/drm/i915/i915_reg.h  |  2 ++
> >> > > > > drivers/gpu/drm/i915/intel_ddi.c |  2 ++
> >> > > > > drivers/gpu/drm/i915/intel_dp.c  | 27
> >> > > > > +++++++++++++++++++++++++++  drivers/gpu/drm/i915/intel_drv.h
> >> > > > > |  3 ++-
> >> > > > >  4 files changed, 33 insertions(+), 1 deletion(-)
> >> > > > >
> >> > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> >> > > > > b/drivers/gpu/drm/i915/i915_reg.h index
> >> > > > > bcde78bc0027..c8d7fdcd7823 100644
> >> > > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> >> > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> >> > > > > @@ -9093,6 +9093,7 @@ enum skl_power_gate {
> >> > > > >  #define _DP_TP_CTL_B			0x64140
> >> > > > >  #define DP_TP_CTL(port) _MMIO_PORT(port, _DP_TP_CTL_A,
> >_DP_TP_CTL_B)
> >> > > > >  #define  DP_TP_CTL_ENABLE			(1 << 31)
> >> > > > > +#define  DP_TP_CTL_FEC_ENABLE			(1 << 30)
> >> > > > >  #define  DP_TP_CTL_MODE_SST			(0 << 27)
> >> > > > >  #define  DP_TP_CTL_MODE_MST			(1 << 27)
> >> > > > >  #define  DP_TP_CTL_FORCE_ACT			(1 << 25)
> >> > > > > @@ -9111,6 +9112,7 @@ enum skl_power_gate {
> >> > > > >  #define _DP_TP_STATUS_A			0x64044
> >> > > > >  #define _DP_TP_STATUS_B			0x64144
> >> > > > >  #define DP_TP_STATUS(port) _MMIO_PORT(port, _DP_TP_STATUS_A,
> >> > > > > _DP_TP_STATUS_B)
> >> > > > > +#define  DP_TP_STATUS_FEC_ENABLE_LIVE		(1 << 28)
> >> > > > >  #define  DP_TP_STATUS_IDLE_DONE			(1 << 25)
> >> > > > >  #define  DP_TP_STATUS_ACT_SENT			(1 << 24)
> >> > > > >  #define  DP_TP_STATUS_MODE_STATUS_MST		(1 << 23)
> >> > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> >> > > > > b/drivers/gpu/drm/i915/intel_ddi.c
> >> > > > > index f531900165bf..67c013ea4d39 100644
> >> > > > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> >> > > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> >> > > > > @@ -2911,6 +2911,8 @@ static void intel_ddi_pre_enable_dp(struct
> >intel_encoder *encoder,
> >> > > > >  					      DP_DECOMPRESSION_EN);
> >> > > > >  	intel_dp_sink_set_fec_ready(intel_dp, crtc_state,
> >DP_FEC_READY);
> >> > > > >  	intel_dp_start_link_train(intel_dp);
> >> > > > > +	intel_dp_enable_fec_state(intel_dp, crtc_state);
> >> > > >
> >> > > > This doesn't look like the correct spot. According to the spec
> >> > > > it should be after stop_link_train() (where we set DP_TP_CTL to
> >> > > > to normal).
> >> > > >
> >> > >
> >> > > Yes I see in the display port enabling sequence page of the spec,
> >> > > that it says enable FEC after DP_TP_CTL is set to Normal. Also the
> >> > > FEC page says that this should be enabled only after ensuring that
> >> > > link training completed. So according to that this call should happen after
> >intel_dp_stop_link_train().
> >> > > So yes move this to after intel_dp_stop_link_training().
> >> > >
> >> > > > > +
> >> > > > >  	if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
> >> > > > >  		intel_dp_stop_link_train(intel_dp);
> >> > > > >
> >> > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> >> > > > > b/drivers/gpu/drm/i915/intel_dp.c index
> >> > > > > b4e8af3142a2..b9f85502d9ff 100644
> >> > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> >> > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> >> > > > > @@ -3017,6 +3017,33 @@ void intel_dp_sink_set_fec_ready(struct
> >intel_dp *intel_dp,
> >> > > > >  		DRM_DEBUG_KMS("Failed to get FEC enabled in
> >sink\n");  }
> >> > > > >
> >> > > > > +void intel_dp_enable_fec_state(struct intel_dp *intel_dp,
> >> > > > > +			       const struct intel_crtc_state *crtc_state) {
> >> > > > > +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> >> > > > > +	struct intel_digital_port *intel_dig_port =
> >dp_to_dig_port(intel_dp);
> >> > > > > +	enum port port = intel_dig_port->base.port;
> >> > > > > +	u32 val;
> >> > > > > +
> >> > > > > +	/* FEC support exists for DP 1.4 only */
> >> > > > > +	if (intel_dp_is_edp(intel_dp))
> >> > > > > +		return;
> >> > > > > +
> >> > > > > +	/* If Display Compression is not enabled, FEC need not be
> >configured */
> >> > > > > +	if (!crtc_state->dsc_params.compression_enable)
> >> > > > > +		return;
> >> > > > > +
> >> > > > > +	val = I915_READ(DP_TP_CTL(port));
> >> > > > > +	val |= DP_TP_CTL_FEC_ENABLE;
> >> > > > > +	I915_WRITE(DP_TP_CTL(port), val);
> >> > > > > +
> >> > > > > +	if (intel_wait_for_register(dev_priv, DP_TP_STATUS(port),
> >> > > > > +				    DP_TP_STATUS_FEC_ENABLE_LIVE,
> >> > > > > +				    DP_TP_STATUS_FEC_ENABLE_LIVE,
> >> > > > > +				    1))
> >> > > > > +		DRM_ERROR("Timed out waiting for FEC Enable
> >Status\n"); }
> >> > > >
> >> > > > There is no reason to stick these into intel_dp.c when the only
> >> > > > caller is in intel_ddi.c.
> >> > > >
> >> > >
> >> > > So may be move the set_dsc_state function also from intel_dp.c to
> >> > > intel_ddi.c Or does it make sense to move all these DSC related functions
> >including fec to intel_dsc.c?
> >> > >
> >> > > > We also seem to be missing platform checks. FEC doesn't appear
> >> > > > to be a thing prior to ICL. I guess the edp+compression_enable
> >> > > > takes care of this, but I think I'd rather stick a new
> >> > > > fec_enable bool into the crtc state. That way we get can add it to the
> >state checker as well.
> >> > > >
> >> > >
> >> > > fec_enable can be added to crtc_state->dsc_params may be?
> >> >
> >> > Isn't FEC something that can be used w/o DSC too?
> >> >
> >>
> >> Nope, FEC always goes hand in hand with DSC on external DP and that
> >> DSC on external DP cannot be enabled without FEC.
> >
> >Sure, DSC needs FEC. But FEC doesn't need DSC. At least if I'm reading the spec
> >correctly.
> >
> >FEC is enabled for the entire main link, so everything getting transported over it
> >get FECced. DSC on the other hand operates on on a stream level. So assuming a
> >MST branch device that can do FEC but can't do DSC you should be able transport
> >both compressed and uncompressed streams through it simultaneously. Kinda
> >makes me think that we want to enable FEC for MST whenever the downstream
> >device supports it.
> 
> FEC for MST modes is not supported for ICL platform. We could extend this for future platforms  for branch devices that support FEC.
> 
> >And I suppose we could also consider using FEC opportunistically for
> >uncompressed SST when we have the extra link bandwidth to handle the extra
> >overhead.

But I still dont understand the use of enabling FEC on uncompressed streams. What errors are we trying to correct when the stream
is sent as is? Will it be used to correct the channel errors in that case?

Manasi

> 
> Yes. The requirement though is for compressed streams. I am not sure if we will have to pay any power penalty for SST. Also my understanding is that if a few symbols in a non-compressed stream are in error, then only those symbols will be in error. These errors do not cause other pixels to get corrupted. In compressed streams, if a few symbols are in error, it can cause the entire slice to get corrupted. Will we be getting any much return on non-compressed streams?
> 
> Anusha 
> >>
> >> Manasi
> >>
> >> > >
> >> > > Manasi
> >> > >
> >> > > > > +
> >> > > > >  /* If the sink supports it, try to set the power state
> >> > > > > appropriately */  void intel_dp_sink_dpms(struct intel_dp
> >> > > > > *intel_dp, int mode)  { diff --git
> >> > > > > a/drivers/gpu/drm/i915/intel_drv.h
> >> > > > > b/drivers/gpu/drm/i915/intel_drv.h
> >> > > > > index fbc9fa06e8be..e51d612a9f42 100644
> >> > > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> >> > > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> > > > > @@ -1532,7 +1532,6 @@ intel_encoder_current_mode(struct
> >> > > > > intel_encoder *encoder);  bool intel_port_is_tc(struct
> >> > > > > drm_i915_private *dev_priv, enum port port);  enum tc_port
> >intel_port_to_tc(struct drm_i915_private *dev_priv,
> >> > > > >  			      enum port port);
> >> > > > > -
> >> > > > >  enum pipe intel_get_pipe_from_connector(struct
> >> > > > > intel_connector *connector);  int
> >intel_get_pipe_from_crtc_id_ioctl(struct drm_device *dev, void *data,
> >> > > > >  				      struct drm_file *file_priv); @@ -
> >1712,6 +1711,8 @@
> >> > > > > void intel_dp_sink_set_decompression_state(struct intel_dp
> >> > > > > *intel_dp,  void intel_dp_sink_set_fec_ready(struct intel_dp *intel_dp,
> >> > > > >  				 const struct intel_crtc_state
> >*crtc_state,
> >> > > > >  				 int state);
> >> > > > > +void intel_dp_enable_fec_state(struct intel_dp *intel_dp,
> >> > > > > +			       const struct intel_crtc_state *crtc_state);
> >> > > > >  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);
> >> > > > > --
> >> > > > > 2.17.1
> >> > > >
> >> > > > --
> >> > > > Ville Syrjälä
> >> > > > Intel
> >> >
> >> > --
> >> > Ville Syrjälä
> >> > Intel
> >
> >--
> >Ville Syrjälä
> >Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [v2 5/6] i915/dp/fec: Configure the Forward Error Correction bits.

[Srivatsa, Anusha]

>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>Sent: Monday, October 22, 2018 11:26 AM
>To: Srivatsa, Anusha <anusha.srivatsa@intel.com>
>Cc: Navare, Manasi D <manasi.d.navare@intel.com>; intel-
>gfx@lists.freedesktop.org; Singh, Gaurav K <gaurav.k.singh@intel.com>; Jani
>Nikula <jani.nikula@linux.intel.com>
>Subject: Re: [v2 5/6] i915/dp/fec: Configure the Forward Error Correction bits.
>
>On Mon, Oct 22, 2018 at 06:04:28PM +0000, Srivatsa, Anusha wrote:
>>
>>
>> >-----Original Message-----
>> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>> >Sent: Friday, October 19, 2018 4:12 PM
>> >To: Navare, Manasi D <manasi.d.navare@intel.com>
>> >Cc: Srivatsa, Anusha <anusha.srivatsa@intel.com>; intel-
>> >gfx@lists.freedesktop.org; Singh, Gaurav K
>> ><gaurav.k.singh@intel.com>; Jani Nikula <jani.nikula@linux.intel.com>
>> >Subject: Re: [v2 5/6] i915/dp/fec: Configure the Forward Error Correction bits.
>> >
>> >On Fri, Oct 19, 2018 at 01:29:05PM -0700, Manasi Navare wrote:
>> >> On Fri, Oct 19, 2018 at 10:58:29PM +0300, Ville Syrjälä wrote:
>> >> > On Fri, Oct 19, 2018 at 12:24:43PM -0700, Manasi Navare wrote:
>> >> > > On Fri, Oct 19, 2018 at 09:39:11PM +0300, Ville Syrjälä wrote:
>> >> > > > On Mon, Oct 15, 2018 at 02:50:36PM -0700, Anusha Srivatsa wrote:
>> >> > > > > If FEC is supported, the corresponding DP_TP_CTL register
>> >> > > > > bits have to be configured.
>> >> > > > >
>> >> > > > > The driver has to program the FEC_ENABLE in DP_TP_CTL[30]
>> >> > > > > register and wait till FEC_STATUS in DP_TP_CTL[28] is 1.
>> >> > > > > Also add the warn message to make sure that the control
>> >> > > > > register is already active while enabling FEC.
>> >> > > > >
>> >> > > > > v2:
>> >> > > > > - Change commit message. Configure fec state after
>> >> > > > >   link training (Manasi, Gaurav)
>> >> > > > > - Remove redundent checks (Manasi)
>> >> > > > > - Remove the registers that get added automagically
>> >> > > > > (Anusha)
>> >> > > > >
>> >> > > > > v3: s/intel_dp_set_fec_state()/intel_dp_enable_fec_state()
>> >> > > > > (Gaurav)
>> >> > > > >
>> >> > > > > v4: rebased.
>> >> > > > >
>> >> > > > > Cc: Gaurav K Singh <gaurav.k.singh@intel.com>
>> >> > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
>> >> > > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>> >> > > > > Cc: Manasi Navare <manasi.d.navare@intel.com>
>> >> > > > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
>> >> > > > > ---
>> >> > > > >  drivers/gpu/drm/i915/i915_reg.h  |  2 ++
>> >> > > > > drivers/gpu/drm/i915/intel_ddi.c |  2 ++
>> >> > > > > drivers/gpu/drm/i915/intel_dp.c  | 27
>> >> > > > > +++++++++++++++++++++++++++
>> >> > > > > +++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_drv.
>> >> > > > > +++++++++++++++++++++++++++ h
>> >> > > > > |  3 ++-
>> >> > > > >  4 files changed, 33 insertions(+), 1 deletion(-)
>> >> > > > >
>> >> > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> >> > > > > b/drivers/gpu/drm/i915/i915_reg.h index
>> >> > > > > bcde78bc0027..c8d7fdcd7823 100644
>> >> > > > > --- a/drivers/gpu/drm/i915/i915_reg.h
>> >> > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
>> >> > > > > @@ -9093,6 +9093,7 @@ enum skl_power_gate {
>> >> > > > >  #define _DP_TP_CTL_B			0x64140
>> >> > > > >  #define DP_TP_CTL(port) _MMIO_PORT(port, _DP_TP_CTL_A,
>> >_DP_TP_CTL_B)
>> >> > > > >  #define  DP_TP_CTL_ENABLE			(1 << 31)
>> >> > > > > +#define  DP_TP_CTL_FEC_ENABLE			(1 << 30)
>> >> > > > >  #define  DP_TP_CTL_MODE_SST			(0 << 27)
>> >> > > > >  #define  DP_TP_CTL_MODE_MST			(1 << 27)
>> >> > > > >  #define  DP_TP_CTL_FORCE_ACT			(1 << 25)
>> >> > > > > @@ -9111,6 +9112,7 @@ enum skl_power_gate {
>> >> > > > >  #define _DP_TP_STATUS_A			0x64044
>> >> > > > >  #define _DP_TP_STATUS_B			0x64144
>> >> > > > >  #define DP_TP_STATUS(port) _MMIO_PORT(port,
>> >> > > > > _DP_TP_STATUS_A,
>> >> > > > > _DP_TP_STATUS_B)
>> >> > > > > +#define  DP_TP_STATUS_FEC_ENABLE_LIVE		(1 << 28)
>> >> > > > >  #define  DP_TP_STATUS_IDLE_DONE			(1 <<
>25)
>> >> > > > >  #define  DP_TP_STATUS_ACT_SENT			(1 << 24)
>> >> > > > >  #define  DP_TP_STATUS_MODE_STATUS_MST		(1 <<
>23)
>> >> > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
>> >> > > > > b/drivers/gpu/drm/i915/intel_ddi.c
>> >> > > > > index f531900165bf..67c013ea4d39 100644
>> >> > > > > --- a/drivers/gpu/drm/i915/intel_ddi.c
>> >> > > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> >> > > > > @@ -2911,6 +2911,8 @@ static void
>> >> > > > > intel_ddi_pre_enable_dp(struct
>> >intel_encoder *encoder,
>> >> > > > >  					      DP_DECOMPRESSION_EN);
>> >> > > > >  	intel_dp_sink_set_fec_ready(intel_dp, crtc_state,
>> >DP_FEC_READY);
>> >> > > > >  	intel_dp_start_link_train(intel_dp);
>> >> > > > > +	intel_dp_enable_fec_state(intel_dp, crtc_state);
>> >> > > >
>> >> > > > This doesn't look like the correct spot. According to the
>> >> > > > spec it should be after stop_link_train() (where we set
>> >> > > > DP_TP_CTL to to normal).
>> >> > > >
>> >> > >
>> >> > > Yes I see in the display port enabling sequence page of the
>> >> > > spec, that it says enable FEC after DP_TP_CTL is set to Normal.
>> >> > > Also the FEC page says that this should be enabled only after
>> >> > > ensuring that link training completed. So according to that
>> >> > > this call should happen after
>> >intel_dp_stop_link_train().
>> >> > > So yes move this to after intel_dp_stop_link_training().
>> >> > >
>> >> > > > > +
>> >> > > > >  	if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
>> >> > > > >  		intel_dp_stop_link_train(intel_dp);
>> >> > > > >
>> >> > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
>> >> > > > > b/drivers/gpu/drm/i915/intel_dp.c index
>> >> > > > > b4e8af3142a2..b9f85502d9ff 100644
>> >> > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
>> >> > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
>> >> > > > > @@ -3017,6 +3017,33 @@ void
>> >> > > > > intel_dp_sink_set_fec_ready(struct
>> >intel_dp *intel_dp,
>> >> > > > >  		DRM_DEBUG_KMS("Failed to get FEC enabled in
>> >sink\n");  }
>> >> > > > >
>> >> > > > > +void intel_dp_enable_fec_state(struct intel_dp *intel_dp,
>> >> > > > > +			       const struct intel_crtc_state *crtc_state) {
>> >> > > > > +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
>> >> > > > > +	struct intel_digital_port *intel_dig_port =
>> >dp_to_dig_port(intel_dp);
>> >> > > > > +	enum port port = intel_dig_port->base.port;
>> >> > > > > +	u32 val;
>> >> > > > > +
>> >> > > > > +	/* FEC support exists for DP 1.4 only */
>> >> > > > > +	if (intel_dp_is_edp(intel_dp))
>> >> > > > > +		return;
>> >> > > > > +
>> >> > > > > +	/* If Display Compression is not enabled, FEC need not be
>> >configured */
>> >> > > > > +	if (!crtc_state->dsc_params.compression_enable)
>> >> > > > > +		return;
>> >> > > > > +
>> >> > > > > +	val = I915_READ(DP_TP_CTL(port));
>> >> > > > > +	val |= DP_TP_CTL_FEC_ENABLE;
>> >> > > > > +	I915_WRITE(DP_TP_CTL(port), val);
>> >> > > > > +
>> >> > > > > +	if (intel_wait_for_register(dev_priv, DP_TP_STATUS(port),
>> >> > > > > +				    DP_TP_STATUS_FEC_ENABLE_LIVE,
>> >> > > > > +				    DP_TP_STATUS_FEC_ENABLE_LIVE,
>> >> > > > > +				    1))
>> >> > > > > +		DRM_ERROR("Timed out waiting for FEC Enable
>> >Status\n"); }
>> >> > > >
>> >> > > > There is no reason to stick these into intel_dp.c when the
>> >> > > > only caller is in intel_ddi.c.
>> >> > > >
>> >> > >
>> >> > > So may be move the set_dsc_state function also from intel_dp.c
>> >> > > to intel_ddi.c Or does it make sense to move all these DSC
>> >> > > related functions
>> >including fec to intel_dsc.c?
>> >> > >
>> >> > > > We also seem to be missing platform checks. FEC doesn't
>> >> > > > appear to be a thing prior to ICL. I guess the
>> >> > > > edp+compression_enable takes care of this, but I think I'd
>> >> > > > rather stick a new fec_enable bool into the crtc state. That
>> >> > > > way we get can add it to the
>> >state checker as well.
>> >> > > >
>> >> > >
>> >> > > fec_enable can be added to crtc_state->dsc_params may be?
>> >> >
>> >> > Isn't FEC something that can be used w/o DSC too?
>> >> >
>> >>
>> >> Nope, FEC always goes hand in hand with DSC on external DP and that
>> >> DSC on external DP cannot be enabled without FEC.
>> >
>> >Sure, DSC needs FEC. But FEC doesn't need DSC. At least if I'm
>> >reading the spec correctly.
>> >
>> >FEC is enabled for the entire main link, so everything getting
>> >transported over it get FECced. DSC on the other hand operates on on
>> >a stream level. So assuming a MST branch device that can do FEC but
>> >can't do DSC you should be able transport both compressed and
>> >uncompressed streams through it simultaneously. Kinda makes me think
>> >that we want to enable FEC for MST whenever the downstream device
>supports it.
>>
>> FEC for MST modes is not supported for ICL platform. We could extend this for
>future platforms  for branch devices that support FEC.
>
>Hmm. Well, looks like we'll have to think about that soon enough anyway, so
>IMO better do it right from the start. Avoids having to rewrite the SST path yet
>again when we add the MST support. And we'll also get the state checker as a
>bonus.

Ok,  So,
1. add fec_enable bool as a state in crtc state.
2. move intel_dp_enable_fec_state() to intel-ddi.c
3. Enable fec state after intel_dp_stop_link_training()

Anusha


>>
>> >And I suppose we could also consider using FEC opportunistically for
>> >uncompressed SST when we have the extra link bandwidth to handle the
>> >extra overhead.
>>
>> Yes. The requirement though is for compressed streams. I am not sure if we will
>have to pay any power penalty for SST. Also my understanding is that if a few
>symbols in a non-compressed stream are in error, then only those symbols will be
>in error. These errors do not cause other pixels to get corrupted. In compressed
>streams, if a few symbols are in error, it can cause the entire slice to get
>corrupted. Will we be getting any much return on non-compressed streams?
>
>No idea.
>
>--
>Ville Syrjälä
>Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [v2 5/6] i915/dp/fec: Configure the Forward Error Correction bits.

[Manasi Navare]

On Mon, Oct 22, 2018 at 09:26:06PM +0300, Ville Syrjälä wrote:
> On Mon, Oct 22, 2018 at 06:04:28PM +0000, Srivatsa, Anusha wrote:
> > 
> > 
> > >-----Original Message-----
> > >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > >Sent: Friday, October 19, 2018 4:12 PM
> > >To: Navare, Manasi D <manasi.d.navare@intel.com>
> > >Cc: Srivatsa, Anusha <anusha.srivatsa@intel.com>; intel-
> > >gfx@lists.freedesktop.org; Singh, Gaurav K <gaurav.k.singh@intel.com>; Jani
> > >Nikula <jani.nikula@linux.intel.com>
> > >Subject: Re: [v2 5/6] i915/dp/fec: Configure the Forward Error Correction bits.
> > >
> > >On Fri, Oct 19, 2018 at 01:29:05PM -0700, Manasi Navare wrote:
> > >> On Fri, Oct 19, 2018 at 10:58:29PM +0300, Ville Syrjälä wrote:
> > >> > On Fri, Oct 19, 2018 at 12:24:43PM -0700, Manasi Navare wrote:
> > >> > > On Fri, Oct 19, 2018 at 09:39:11PM +0300, Ville Syrjälä wrote:
> > >> > > > On Mon, Oct 15, 2018 at 02:50:36PM -0700, Anusha Srivatsa wrote:
> > >> > > > > If FEC is supported, the corresponding DP_TP_CTL register bits
> > >> > > > > have to be configured.
> > >> > > > >
> > >> > > > > The driver has to program the FEC_ENABLE in DP_TP_CTL[30]
> > >> > > > > register and wait till FEC_STATUS in DP_TP_CTL[28] is 1.
> > >> > > > > Also add the warn message to make sure that the control
> > >> > > > > register is already active while enabling FEC.
> > >> > > > >
> > >> > > > > v2:
> > >> > > > > - Change commit message. Configure fec state after
> > >> > > > >   link training (Manasi, Gaurav)
> > >> > > > > - Remove redundent checks (Manasi)
> > >> > > > > - Remove the registers that get added automagically (Anusha)
> > >> > > > >
> > >> > > > > v3: s/intel_dp_set_fec_state()/intel_dp_enable_fec_state()
> > >> > > > > (Gaurav)
> > >> > > > >
> > >> > > > > v4: rebased.
> > >> > > > >
> > >> > > > > Cc: Gaurav K Singh <gaurav.k.singh@intel.com>
> > >> > > > > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > >> > > > > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > >> > > > > Cc: Manasi Navare <manasi.d.navare@intel.com>
> > >> > > > > Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > >> > > > > ---
> > >> > > > >  drivers/gpu/drm/i915/i915_reg.h  |  2 ++
> > >> > > > > drivers/gpu/drm/i915/intel_ddi.c |  2 ++
> > >> > > > > drivers/gpu/drm/i915/intel_dp.c  | 27
> > >> > > > > +++++++++++++++++++++++++++  drivers/gpu/drm/i915/intel_drv.h
> > >> > > > > |  3 ++-
> > >> > > > >  4 files changed, 33 insertions(+), 1 deletion(-)
> > >> > > > >
> > >> > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > >> > > > > b/drivers/gpu/drm/i915/i915_reg.h index
> > >> > > > > bcde78bc0027..c8d7fdcd7823 100644
> > >> > > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > >> > > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > >> > > > > @@ -9093,6 +9093,7 @@ enum skl_power_gate {
> > >> > > > >  #define _DP_TP_CTL_B			0x64140
> > >> > > > >  #define DP_TP_CTL(port) _MMIO_PORT(port, _DP_TP_CTL_A,
> > >_DP_TP_CTL_B)
> > >> > > > >  #define  DP_TP_CTL_ENABLE			(1 << 31)
> > >> > > > > +#define  DP_TP_CTL_FEC_ENABLE			(1 << 30)
> > >> > > > >  #define  DP_TP_CTL_MODE_SST			(0 << 27)
> > >> > > > >  #define  DP_TP_CTL_MODE_MST			(1 << 27)
> > >> > > > >  #define  DP_TP_CTL_FORCE_ACT			(1 << 25)
> > >> > > > > @@ -9111,6 +9112,7 @@ enum skl_power_gate {
> > >> > > > >  #define _DP_TP_STATUS_A			0x64044
> > >> > > > >  #define _DP_TP_STATUS_B			0x64144
> > >> > > > >  #define DP_TP_STATUS(port) _MMIO_PORT(port, _DP_TP_STATUS_A,
> > >> > > > > _DP_TP_STATUS_B)
> > >> > > > > +#define  DP_TP_STATUS_FEC_ENABLE_LIVE		(1 << 28)
> > >> > > > >  #define  DP_TP_STATUS_IDLE_DONE			(1 << 25)
> > >> > > > >  #define  DP_TP_STATUS_ACT_SENT			(1 << 24)
> > >> > > > >  #define  DP_TP_STATUS_MODE_STATUS_MST		(1 << 23)
> > >> > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > >> > > > > b/drivers/gpu/drm/i915/intel_ddi.c
> > >> > > > > index f531900165bf..67c013ea4d39 100644
> > >> > > > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > >> > > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > >> > > > > @@ -2911,6 +2911,8 @@ static void intel_ddi_pre_enable_dp(struct
> > >intel_encoder *encoder,
> > >> > > > >  					      DP_DECOMPRESSION_EN);
> > >> > > > >  	intel_dp_sink_set_fec_ready(intel_dp, crtc_state,
> > >DP_FEC_READY);
> > >> > > > >  	intel_dp_start_link_train(intel_dp);
> > >> > > > > +	intel_dp_enable_fec_state(intel_dp, crtc_state);
> > >> > > >
> > >> > > > This doesn't look like the correct spot. According to the spec
> > >> > > > it should be after stop_link_train() (where we set DP_TP_CTL to
> > >> > > > to normal).
> > >> > > >
> > >> > >
> > >> > > Yes I see in the display port enabling sequence page of the spec,
> > >> > > that it says enable FEC after DP_TP_CTL is set to Normal. Also the
> > >> > > FEC page says that this should be enabled only after ensuring that
> > >> > > link training completed. So according to that this call should happen after
> > >intel_dp_stop_link_train().
> > >> > > So yes move this to after intel_dp_stop_link_training().
> > >> > >
> > >> > > > > +
> > >> > > > >  	if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
> > >> > > > >  		intel_dp_stop_link_train(intel_dp);
> > >> > > > >
> > >> > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > >> > > > > b/drivers/gpu/drm/i915/intel_dp.c index
> > >> > > > > b4e8af3142a2..b9f85502d9ff 100644
> > >> > > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > >> > > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > >> > > > > @@ -3017,6 +3017,33 @@ void intel_dp_sink_set_fec_ready(struct
> > >intel_dp *intel_dp,
> > >> > > > >  		DRM_DEBUG_KMS("Failed to get FEC enabled in
> > >sink\n");  }
> > >> > > > >
> > >> > > > > +void intel_dp_enable_fec_state(struct intel_dp *intel_dp,
> > >> > > > > +			       const struct intel_crtc_state *crtc_state) {
> > >> > > > > +	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> > >> > > > > +	struct intel_digital_port *intel_dig_port =
> > >dp_to_dig_port(intel_dp);
> > >> > > > > +	enum port port = intel_dig_port->base.port;
> > >> > > > > +	u32 val;
> > >> > > > > +
> > >> > > > > +	/* FEC support exists for DP 1.4 only */
> > >> > > > > +	if (intel_dp_is_edp(intel_dp))
> > >> > > > > +		return;
> > >> > > > > +
> > >> > > > > +	/* If Display Compression is not enabled, FEC need not be
> > >configured */
> > >> > > > > +	if (!crtc_state->dsc_params.compression_enable)
> > >> > > > > +		return;
> > >> > > > > +
> > >> > > > > +	val = I915_READ(DP_TP_CTL(port));
> > >> > > > > +	val |= DP_TP_CTL_FEC_ENABLE;
> > >> > > > > +	I915_WRITE(DP_TP_CTL(port), val);
> > >> > > > > +
> > >> > > > > +	if (intel_wait_for_register(dev_priv, DP_TP_STATUS(port),
> > >> > > > > +				    DP_TP_STATUS_FEC_ENABLE_LIVE,
> > >> > > > > +				    DP_TP_STATUS_FEC_ENABLE_LIVE,
> > >> > > > > +				    1))
> > >> > > > > +		DRM_ERROR("Timed out waiting for FEC Enable
> > >Status\n"); }
> > >> > > >
> > >> > > > There is no reason to stick these into intel_dp.c when the only
> > >> > > > caller is in intel_ddi.c.
> > >> > > >
> > >> > >
> > >> > > So may be move the set_dsc_state function also from intel_dp.c to
> > >> > > intel_ddi.c Or does it make sense to move all these DSC related functions
> > >including fec to intel_dsc.c?
> > >> > >
> > >> > > > We also seem to be missing platform checks. FEC doesn't appear
> > >> > > > to be a thing prior to ICL. I guess the edp+compression_enable
> > >> > > > takes care of this, but I think I'd rather stick a new
> > >> > > > fec_enable bool into the crtc state. That way we get can add it to the
> > >state checker as well.
> > >> > > >
> > >> > >
> > >> > > fec_enable can be added to crtc_state->dsc_params may be?
> > >> >
> > >> > Isn't FEC something that can be used w/o DSC too?
> > >> >
> > >>
> > >> Nope, FEC always goes hand in hand with DSC on external DP and that
> > >> DSC on external DP cannot be enabled without FEC.
> > >
> > >Sure, DSC needs FEC. But FEC doesn't need DSC. At least if I'm reading the spec
> > >correctly.
> > >
> > >FEC is enabled for the entire main link, so everything getting transported over it
> > >get FECced. DSC on the other hand operates on on a stream level. So assuming a
> > >MST branch device that can do FEC but can't do DSC you should be able transport
> > >both compressed and uncompressed streams through it simultaneously. Kinda
> > >makes me think that we want to enable FEC for MST whenever the downstream
> > >device supports it.
> > 
> > FEC for MST modes is not supported for ICL platform. We could extend this for future platforms  for branch devices that support FEC.
> 
> Hmm. Well, looks like we'll have to think about that soon enough anyway,
> so IMO better do it right from the start. Avoids having to rewrite the
> SST path yet again when we add the MST support. And we'll also get the
> state checker as a bonus.

I think it makes sense to add crtc_state->fec_enable variable to add it non compressed and MST cases later.
But for ICL, to add the basic FEC support, this patch series can only enable it for DSC SST cases.

Manasi

> 
> > 
> > >And I suppose we could also consider using FEC opportunistically for
> > >uncompressed SST when we have the extra link bandwidth to handle the extra
> > >overhead.
> > 
> > Yes. The requirement though is for compressed streams. I am not sure if we will have to pay any power penalty for SST. Also my understanding is that if a few symbols in a non-compressed stream are in error, then only those symbols will be in error. These errors do not cause other pixels to get corrupted. In compressed streams, if a few symbols are in error, it can cause the entire slice to get corrupted. Will we be getting any much return on non-compressed streams?
> 
> No idea.
> 
> -- 
> Ville Syrjälä
> Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx