[v3 0/7] Forward Error Correction

[v3 0/7] 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)

v3: Add fec as a state to crtc. Move around the code. (Ville)

v4: s/can_fec/fec_enable; s/intel_dp_can_fec/intel_de_supports_fec;
Add intel_dp_source supports_fec() (Ville)

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

Anusha Srivatsa (7):
  i915/dp/fec: Cache the FEC_CAPABLE DPCD register
  drm/dp/fec: DRM helper for Forward Error Correction
  i915/dp/fec: Check for FEC Support
  i915/dp/fec: Add can_fec to the crtc state.
  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 | 60 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_dp.c  | 58 ++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_drv.h | 11 ++++++
 include/drm/drm_dp_helper.h      |  7 ++++
 5 files changed, 136 insertions(+), 2 deletions(-)

-- 
2.17.1

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

[v4 1/7] 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.

v2: Avoid using memset and array for a single
field. (Manasi,Jani)

v3:

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  | 9 +++++++++
 drivers/gpu/drm/i915/intel_drv.h | 1 +
 2 files changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 5a638503e36a..8ae7cf3d4ee1 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4201,6 +4201,9 @@ static void intel_dp_get_dsc_sink_cap(struct intel_dp *intel_dp)
 	 */
 	memset(intel_dp->dsc_dpcd, 0, sizeof(intel_dp->dsc_dpcd));
 
+	/* Clear fec_capable to avoid using stale values */
+	intel_dp->fec_capable = 0;
+
 	/* Cache the DSC DPCD if eDP or DP rev >= 1.4 */
 	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x14 ||
 	    intel_dp->edp_dpcd[0] >= DP_EDP_14) {
@@ -4213,6 +4216,12 @@ static void intel_dp_get_dsc_sink_cap(struct intel_dp *intel_dp)
 		DRM_DEBUG_KMS("DSC DPCD: %*ph\n",
 			      (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)) {
+			if (drm_dp_dpcd_readb(&intel_dp->aux, DP_FEC_CAPABILITY,
+					      &intel_dp->fec_capable) < 0)
+				DRM_ERROR("Failed to read FEC DPCD register\n");
+		}
 	}
 }
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 16bbc3768e02..9a94c6544bf5 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1119,6 +1119,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_capable;
 	/* 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

[v4 2/7] 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)

v5: rebased. Change the helper parameter suitably.

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>
Reviewed-by: Manasi Navare <manasi.d.navare@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 2649529d0d8f..b08f50b852f5 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -1101,6 +1101,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_capable)
+{
+	return fec_capable & 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

[v4 3/7] 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.

v6: pass single field instead of an array for helper
function. (manasi)

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>
Reviewed-by: Manasi Navare <manasi.d.navare@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 8ae7cf3d4ee1..a344be555dd6 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -680,7 +680,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_capable)) {
 			dsc_max_output_bpp =
 				intel_dp_dsc_get_output_bpp(max_link_clock,
 							    max_lanes,
@@ -690,7 +690,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)) ||
@@ -2063,6 +2064,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_capable)) {
+		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

[v4 4/7] i915/dp/fec: Add fec_enable to the crtc state.

[Anusha Srivatsa]

Add a crtc state for FEC. Currently, the state
is determined by platform, DP and DSC being
enabled. Moving forward we can use the state
to have error correction on other scenarios too
if needed.

v2:
- Control compression_enable with the fec_enable
parameter in crtc state and with intel_dp_supports_fec()
(Ville)

- intel_dp_can_fec()/intel_dp_supports_fec()(manasi)

Suggested-by: Ville Syrjala <ville.syrjala@linux.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>
Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c  | 25 ++++++++++++++++++++++++-
 drivers/gpu/drm/i915/intel_drv.h |  3 +++
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index a344be555dd6..5ae3855925f3 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2045,6 +2045,21 @@ intel_dp_compute_link_config_fast(struct intel_dp *intel_dp,
 	return false;
 }
 
+static bool intel_dp_source_supports_fec(struct intel_dp *intel_dp)
+{
+	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
+	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
+	enum port port = dig_port->base.port;
+
+	return INTEL_GEN(dev_priv) >= 11 && port != PORT_A;
+}
+
+static bool intel_dp_supports_fec(struct intel_dp *intel_dp,
+				  struct intel_crtc_state *pipe_config)
+{
+	return intel_dp_source_supports_fec(intel_dp) &&
+		drm_dp_sink_supports_fec(intel_dp->fec_capable);
+}
 static bool intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
 					struct intel_crtc_state *pipe_config,
 					struct link_config_limits *limits)
@@ -2056,6 +2071,8 @@ static bool intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
 	u16 dsc_max_output_bpp = 0;
 	u8 dsc_dp_slice_count = 0;
 
+	pipe_config->fec_enable = !intel_dp_is_edp(intel_dp);
+
 	if (INTEL_GEN(dev_priv) < 10 ||
 	    !drm_dp_sink_supports_dsc(intel_dp->dsc_dpcd))
 		return false;
@@ -2122,7 +2139,13 @@ static bool intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
 			  pipe_config->dsc_params.compressed_bpp);
 		return false;
 	}
-	pipe_config->dsc_params.compression_enable = true;
+
+	if (pipe_config->fec_enable && !intel_dp_supports_fec(intel_dp, pipe_config)) {
+		DRM_DEBUG_KMS("No FEC Support, disabling Compression");
+		pipe_config->dsc_params.compression_enable = false;
+	} else {
+		pipe_config->dsc_params.compression_enable = true;
+	}
 	DRM_DEBUG_KMS("DP DSC computed with Input Bpp = %d "
 		      "Compressed Bpp = %d Slice Count = %d\n",
 		      pipe_config->pipe_bpp,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 9a94c6544bf5..9f701463219b 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -940,6 +940,9 @@ struct intel_crtc_state {
 		u8 slice_count;
 	} dsc_params;
 	struct drm_dsc_config dp_dsc_cfg;
+
+	/* Forward Error correction State */
+	bool fec_enable;
 };
 
 struct intel_crtc {
-- 
2.17.1

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

[v4 5/7] 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. (r-b Manasi)

v4: Use fec crtc state, before setting FEC_READY
bit. (Anusha)

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  | 18 ++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h |  3 +++
 3 files changed, 22 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 1de0a3917d7f..efbada95dc4e 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -2932,6 +2932,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 5ae3855925f3..a750415d1ec0 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3058,6 +3058,24 @@ 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->fec_enable)
+		return;
+
+	/* If compression is not enabled, do not set FEC bits */
+	if (!crtc_state->dsc_params.compression_enable)
+		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 9f701463219b..2a080205968d 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1796,6 +1796,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

[v4 6/7] 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.

v5:
- Move the code to the proper spot, according to spec.(Ville)
- Use fec state as a check too.

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 | 30 ++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h |  2 ++
 3 files changed, 34 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index e85f53cb9cdd..8b1753939299 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -9134,6 +9134,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)
@@ -9152,6 +9153,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 efbada95dc4e..f03f44f332c7 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -2889,6 +2889,32 @@ static void intel_ddi_clk_disable(struct intel_encoder *encoder)
 	}
 }
 
+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 (!crtc_state->fec_enable)
+		return;
+
+	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");
+}
+
 static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
 				    const struct intel_crtc_state *crtc_state,
 				    const struct drm_connector_state *conn_state)
@@ -2934,9 +2960,13 @@ 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);
+
 	if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
 		intel_dp_stop_link_train(intel_dp);
 
+	/* Set FEC state after link training */
+	intel_dp_enable_fec_state(intel_dp, crtc_state);
+
 	icl_enable_phy_clock_gating(dig_port);
 
 	if (!is_mst)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 2a080205968d..1cdfa9c5da43 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1799,6 +1799,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

[v4 7/7] 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.

v4:
- Move the code to the proper spot according to spec (Ville)
- Use proper checks (manasi)

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 | 32 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h |  2 ++
 2 files changed, 34 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index f03f44f332c7..d28280c3b299 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -2915,6 +2915,31 @@ 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;
+	u8 dsc_en_state;
+
+	if (!crtc_state->fec_enable)
+		return;
+
+	if (!crtc_state->dsc_params.compression_enable)
+		return;
+
+	drm_dp_dpcd_readb(&intel_dp->aux, DP_DSC_ENABLE, &dsc_en_state);
+
+	if (!dsc_en_state) {
+		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));
+	}
+}
+
 static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
 				    const struct intel_crtc_state *crtc_state,
 				    const struct drm_connector_state *conn_state)
@@ -3055,7 +3080,11 @@ static void intel_ddi_pre_enable(struct intel_encoder *encoder,
 static void intel_disable_ddi_buf(struct intel_encoder *encoder)
 {
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
+	struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
+	struct intel_dp *intel_dp = &dig_port->dp;
+	struct intel_crtc_state *crtc_state;
 	enum port port = encoder->port;
+
 	bool wait = false;
 	u32 val;
 
@@ -3071,6 +3100,9 @@ static void intel_disable_ddi_buf(struct intel_encoder *encoder)
 	val |= DP_TP_CTL_LINK_TRAIN_PAT1;
 	I915_WRITE(DP_TP_CTL(port), val);
 
+	/* Disable FEC in DP Sink */
+	intel_dp_disable_fec_state(intel_dp, crtc_state);
+
 	if (wait)
 		intel_wait_ddi_buf_idle(dev_priv, port);
 }
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 1cdfa9c5da43..4ed04fad83b7 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1801,6 +1801,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

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

[Patchwork]

== Series Details ==

Series: Forward Error Correction (rev4)
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: [v4 3/7] i915/dp/fec: Check for FEC Support

[Ville Syrjälä]

On Tue, Oct 30, 2018 at 05:45:13PM -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.
> 
> v6: pass single field instead of an array for helper
> function. (manasi)
> 
> 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>
> Reviewed-by: Manasi Navare <manasi.d.navare@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 8ae7cf3d4ee1..a344be555dd6 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -680,7 +680,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_capable)) {
>  			dsc_max_output_bpp =
>  				intel_dp_dsc_get_output_bpp(max_link_clock,
>  							    max_lanes,
> @@ -690,7 +690,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");

Please no debug prints in the mode_valid() that would spam a lot.

Not enough context in the patch for me to see what happens if FEC isn't
supported. Do we just check against the uncompressed limits?

>  	}
>  
>  	if ((mode_rate > max_rate && !(dsc_max_output_bpp && dsc_slice_count)) ||
> @@ -2063,6 +2064,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_capable)) {

Why aren't we checking the source capabilities here as well?

> +		DRM_DEBUG_KMS("Sink does not support Forward Error Correction, disabling Display Compression\n");

"disabling Display Compression" is not true. We're rejecting the entire
thing.

> +		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

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

Re: [v4 4/7] i915/dp/fec: Add fec_enable to the crtc state.

[Ville Syrjälä]

On Tue, Oct 30, 2018 at 05:45:14PM -0700, Anusha Srivatsa wrote:
> Add a crtc state for FEC. Currently, the state
> is determined by platform, DP and DSC being
> enabled. Moving forward we can use the state
> to have error correction on other scenarios too
> if needed.
> 
> v2:
> - Control compression_enable with the fec_enable
> parameter in crtc state and with intel_dp_supports_fec()
> (Ville)
> 
> - intel_dp_can_fec()/intel_dp_supports_fec()(manasi)
> 
> Suggested-by: Ville Syrjala <ville.syrjala@linux.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>
> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c  | 25 ++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/intel_drv.h |  3 +++
>  2 files changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index a344be555dd6..5ae3855925f3 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2045,6 +2045,21 @@ intel_dp_compute_link_config_fast(struct intel_dp *intel_dp,
>  	return false;
>  }
>  
> +static bool intel_dp_source_supports_fec(struct intel_dp *intel_dp)
> +{
> +	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> +	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
> +	enum port port = dig_port->base.port;
> +
> +	return INTEL_GEN(dev_priv) >= 11 && port != PORT_A;
> +}
> +
> +static bool intel_dp_supports_fec(struct intel_dp *intel_dp,
> +				  struct intel_crtc_state *pipe_config)
> +{
> +	return intel_dp_source_supports_fec(intel_dp) &&
> +		drm_dp_sink_supports_fec(intel_dp->fec_capable);
> +}
>  static bool intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
>  					struct intel_crtc_state *pipe_config,
>  					struct link_config_limits *limits)
> @@ -2056,6 +2071,8 @@ static bool intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
>  	u16 dsc_max_output_bpp = 0;
>  	u8 dsc_dp_slice_count = 0;
>  
> +	pipe_config->fec_enable = !intel_dp_is_edp(intel_dp);
> +
>  	if (INTEL_GEN(dev_priv) < 10 ||
>  	    !drm_dp_sink_supports_dsc(intel_dp->dsc_dpcd))
>  		return false;
> @@ -2122,7 +2139,13 @@ static bool intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
>  			  pipe_config->dsc_params.compressed_bpp);
>  		return false;
>  	}
> -	pipe_config->dsc_params.compression_enable = true;
> +
> +	if (pipe_config->fec_enable && !intel_dp_supports_fec(intel_dp, pipe_config)) {
> +		DRM_DEBUG_KMS("No FEC Support, disabling Compression");
> +		pipe_config->dsc_params.compression_enable = false;
> +	} else {
> +		pipe_config->dsc_params.compression_enable = true;
> +	}

Didn't the previous patch already reject the entire operation?

>  	DRM_DEBUG_KMS("DP DSC computed with Input Bpp = %d "
>  		      "Compressed Bpp = %d Slice Count = %d\n",
>  		      pipe_config->pipe_bpp,
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 9a94c6544bf5..9f701463219b 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -940,6 +940,9 @@ struct intel_crtc_state {
>  		u8 slice_count;
>  	} dsc_params;
>  	struct drm_dsc_config dp_dsc_cfg;
> +
> +	/* Forward Error correction State */
> +	bool fec_enable;
>  };
>  
>  struct intel_crtc {
> -- 
> 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: [v4 6/7] i915/dp/fec: Configure the Forward Error Correction bits.

[Ville Syrjälä]

On Tue, Oct 30, 2018 at 05:45:16PM -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.
> 
> v5:
> - Move the code to the proper spot, according to spec.(Ville)
> - Use fec state as a check too.
> 
> 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 | 30 ++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h |  2 ++
>  3 files changed, 34 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index e85f53cb9cdd..8b1753939299 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -9134,6 +9134,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)
> @@ -9152,6 +9153,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 efbada95dc4e..f03f44f332c7 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -2889,6 +2889,32 @@ static void intel_ddi_clk_disable(struct intel_encoder *encoder)
>  	}
>  }
>  
> +void intel_dp_enable_fec_state(struct intel_dp *intel_dp,
> +			       const struct intel_crtc_state *crtc_state)

Can be static. intel_ddi_enable_fec() seems like a better name,
and we can just pass the intel_encoder to this I think. No need to 
do all the "intel_dp to encoder" gynmastics then.

> +{
> +	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 */

This comment is misleading. There are other reasons besides < DP 1.4 for
not using FEC. IMO just drop the comment.

> +	if (!crtc_state->fec_enable)
> +		return;
> +
> +	if (!crtc_state->dsc_params.compression_enable)
> +		return;

That check can be removed now.

> +
> +	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");
> +}
> +
>  static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
>  				    const struct intel_crtc_state *crtc_state,
>  				    const struct drm_connector_state *conn_state)
> @@ -2934,9 +2960,13 @@ 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);
> +

Unrelated whitespace change.

>  	if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
>  		intel_dp_stop_link_train(intel_dp);
>  
> +	/* Set FEC state after link training */

Redundant comment.

> +	intel_dp_enable_fec_state(intel_dp, crtc_state);
> +
>  	icl_enable_phy_clock_gating(dig_port);
>  
>  	if (!is_mst)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 2a080205968d..1cdfa9c5da43 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1799,6 +1799,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: [v4 7/7] drm/i915/fec: Disable FEC state.

[Ville Syrjälä]

On Tue, Oct 30, 2018 at 05:45:17PM -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.
> 
> v4:
> - Move the code to the proper spot according to spec (Ville)
> - Use proper checks (manasi)
> 
> 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 | 32 ++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h |  2 ++
>  2 files changed, 34 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index f03f44f332c7..d28280c3b299 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -2915,6 +2915,31 @@ 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)

Same comments as for the enable.

> +{
> +	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;
> +	u8 dsc_en_state;
> +
> +	if (!crtc_state->fec_enable)
> +		return;
> +
> +	if (!crtc_state->dsc_params.compression_enable)
> +		return;

Not needed.

> +
> +	drm_dp_dpcd_readb(&intel_dp->aux, DP_DSC_ENABLE, &dsc_en_state);

Why? Just disable FEC. fec_enable has already told us that it was
enabled.

> +
> +	if (!dsc_en_state) {
> +		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));
> +	}
> +}
> +
>  static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
>  				    const struct intel_crtc_state *crtc_state,
>  				    const struct drm_connector_state *conn_state)
> @@ -3055,7 +3080,11 @@ static void intel_ddi_pre_enable(struct intel_encoder *encoder,
>  static void intel_disable_ddi_buf(struct intel_encoder *encoder)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> +	struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
> +	struct intel_dp *intel_dp = &dig_port->dp;
> +	struct intel_crtc_state *crtc_state;
>  	enum port port = encoder->port;
> +

Bogus whitespace.

>  	bool wait = false;
>  	u32 val;
>  
> @@ -3071,6 +3100,9 @@ static void intel_disable_ddi_buf(struct intel_encoder *encoder)
>  	val |= DP_TP_CTL_LINK_TRAIN_PAT1;
>  	I915_WRITE(DP_TP_CTL(port), val);
>  
> +	/* Disable FEC in DP Sink */

Incorrect comment.

> +	intel_dp_disable_fec_state(intel_dp, crtc_state);
> +
>  	if (wait)
>  		intel_wait_ddi_buf_idle(dev_priv, port);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 1cdfa9c5da43..4ed04fad83b7 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1801,6 +1801,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: [v3 0/7] Forward Error Correction

[Ville Syrjälä]

On Tue, Oct 30, 2018 at 05:45:10PM -0700, Anusha Srivatsa wrote:
> 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)
> 
> v3: Add fec as a state to crtc. Move around the code. (Ville)
> 
> v4: s/can_fec/fec_enable; s/intel_dp_can_fec/intel_de_supports_fec;
> Add intel_dp_source supports_fec() (Ville)

One clear thing missing is from this series is the
state readout + PIPE_CONF_CHECK

> 
> This is rebased on top of Manasi's End-to-end DSC
> Implementation: https://patchwork.freedesktop.org/series/47514/
> 
> Anusha Srivatsa (7):
>   i915/dp/fec: Cache the FEC_CAPABLE DPCD register
>   drm/dp/fec: DRM helper for Forward Error Correction
>   i915/dp/fec: Check for FEC Support
>   i915/dp/fec: Add can_fec to the crtc state.
>   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 | 60 ++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_dp.c  | 58 ++++++++++++++++++++++++++++--
>  drivers/gpu/drm/i915/intel_drv.h | 11 ++++++
>  include/drm/drm_dp_helper.h      |  7 ++++
>  5 files changed, 136 insertions(+), 2 deletions(-)
> 
> -- 
> 2.17.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

[Srivatsa, Anusha]

>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>Sent: Wednesday, October 31, 2018 2:08 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>; Navare, Manasi D
><manasi.d.navare@intel.com>
>Subject: Re: [v4 6/7] i915/dp/fec: Configure the Forward Error Correction bits.
>
>On Tue, Oct 30, 2018 at 05:45:16PM -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.
>>
>> v5:
>> - Move the code to the proper spot, according to spec.(Ville)
>> - Use fec state as a check too.
>>
>> 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 | 30 ++++++++++++++++++++++++++++++
>> drivers/gpu/drm/i915/intel_drv.h |  2 ++
>>  3 files changed, 34 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> b/drivers/gpu/drm/i915/i915_reg.h index e85f53cb9cdd..8b1753939299
>> 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -9134,6 +9134,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)
>> @@ -9152,6 +9153,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 efbada95dc4e..f03f44f332c7 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -2889,6 +2889,32 @@ static void intel_ddi_clk_disable(struct
>intel_encoder *encoder)
>>  	}
>>  }
>>
>> +void intel_dp_enable_fec_state(struct intel_dp *intel_dp,
>> +			       const struct intel_crtc_state *crtc_state)
>
>Can be static. intel_ddi_enable_fec() seems like a better name, and we can just
>pass the intel_encoder to this I think. No need to do all the "intel_dp to encoder"
>gynmastics then.
intel_ddi_enable_fec() is a better name. 

>> +{
>> +	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 */
>
>This comment is misleading. There are other reasons besides < DP 1.4 for not
>using FEC. IMO just drop the comment.

Ok.

>> +	if (!crtc_state->fec_enable)
>> +		return;
>> +
>> +	if (!crtc_state->dsc_params.compression_enable)
>> +		return;
>
>That check can be removed now.
No we will still need this. The fec_enable state is not coupled with compression. So we have to check fec state and the compression ....

Anusha 
>> +
>> +	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"); }
>> +
>>  static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
>>  				    const struct intel_crtc_state *crtc_state,
>>  				    const struct drm_connector_state
>*conn_state) @@ -2934,9
>> +2960,13 @@ 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);
>> +
>
>Unrelated whitespace change.
>
>>  	if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
>>  		intel_dp_stop_link_train(intel_dp);
>>
>> +	/* Set FEC state after link training */
>
>Redundant comment.
>
>> +	intel_dp_enable_fec_state(intel_dp, crtc_state);
>> +
>>  	icl_enable_phy_clock_gating(dig_port);
>>
>>  	if (!is_mst)
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>> b/drivers/gpu/drm/i915/intel_drv.h
>> index 2a080205968d..1cdfa9c5da43 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1799,6 +1799,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: [v4 3/7] i915/dp/fec: Check for FEC Support

[Srivatsa, Anusha]

>-----Original Message-----
>From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>Sent: Wednesday, October 31, 2018 2:01 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>; Navare, Manasi D
><manasi.d.navare@intel.com>; Pandiyan, Dhinakaran
><dhinakaran.pandiyan@intel.com>
>Subject: Re: [v4 3/7] i915/dp/fec: Check for FEC Support
>
>On Tue, Oct 30, 2018 at 05:45:13PM -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.
>>
>> v6: pass single field instead of an array for helper function.
>> (manasi)
>>
>> 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>
>> Reviewed-by: Manasi Navare <manasi.d.navare@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 8ae7cf3d4ee1..a344be555dd6
>> 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -680,7 +680,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_capable)) {
>>  			dsc_max_output_bpp =
>>  				intel_dp_dsc_get_output_bpp(max_link_clock,
>>  							    max_lanes,
>> @@ -690,7 +690,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");
>
>Please no debug prints in the mode_valid() that would spam a lot.
>
>Not enough context in the patch for me to see what happens if FEC isn't
>supported. Do we just check against the uncompressed limits?

In cases where we cannot do FEC for DP cases, we should be disabling DSC.
Combining the checking of FEC support (this patch) with the patch 4 of the series which checks for state and disables DSC suitably.

Anusha 
>>  	}
>>
>>  	if ((mode_rate > max_rate && !(dsc_max_output_bpp &&
>> dsc_slice_count)) || @@ -2063,6 +2064,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_capable)) {
>
>Why aren't we checking the source capabilities here as well?
>	
>> +		DRM_DEBUG_KMS("Sink does not support Forward Error
>Correction,
>> +disabling Display Compression\n");
>
>"disabling Display Compression" is not true. We're rejecting the entire thing.
>
>> +		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
>
>--
>Ville Syrjälä
>Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [v4 5/7] drm/i915/fec: Set FEC_READY in FEC_CONFIGURATION

[Srivatsa, Anusha]

>-----Original Message-----
>From: Srivatsa, Anusha
>Sent: Tuesday, October 30, 2018 5:45 PM
>To: intel-gfx@lists.freedesktop.org
>Cc: Srivatsa, Anusha <anusha.srivatsa@intel.com>; Singh, Gaurav K
><gaurav.k.singh@intel.com>; Jani Nikula <jani.nikula@linux.intel.com>; Ville
>Syrjala <ville.syrjala@linux.intel.com>; Navare, Manasi D
><manasi.d.navare@intel.com>
>Subject: [v4 5/7] drm/i915/fec: Set FEC_READY in FEC_CONFIGURATION
>
>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. (r-b Manasi)
>
>v4: Use fec crtc state, before setting FEC_READY bit. (Anusha)
>
>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  | 18
>++++++++++++++++++  drivers/gpu/drm/i915/intel_drv.h |  3 +++
> 3 files changed, 22 insertions(+)
>
>diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>index 1de0a3917d7f..efbada95dc4e 100644
>--- a/drivers/gpu/drm/i915/intel_ddi.c
>+++ b/drivers/gpu/drm/i915/intel_ddi.c
>@@ -2932,6 +2932,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 5ae3855925f3..a750415d1ec0 100644
>--- a/drivers/gpu/drm/i915/intel_dp.c
>+++ b/drivers/gpu/drm/i915/intel_dp.c
>@@ -3058,6 +3058,24 @@ 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)

Manasi, Ville, 
Since enable_fec and disable_fec (next two patches in the series) are being moved to intel_ddi.c, this function might also be moved to intel_ddi.c?

Anusha 
>+{
>+	int ret;
>+
>+	if (!crtc_state->fec_enable)
>+		return;
>+
>+	/* If compression is not enabled, do not set FEC bits */
>+	if (!crtc_state->dsc_params.compression_enable)
>+		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 9f701463219b..2a080205968d 100644
>--- a/drivers/gpu/drm/i915/intel_drv.h
>+++ b/drivers/gpu/drm/i915/intel_drv.h
>@@ -1796,6 +1796,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: [v4 1/7] i915/dp/fec: Cache the FEC_CAPABLE DPCD register

[Manasi Navare]

On Tue, Oct 30, 2018 at 05:45:11PM -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.
> 
> v2: Avoid using memset and array for a single
> field. (Manasi,Jani)
> 
> v3:
> 
> 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  | 9 +++++++++
>  drivers/gpu/drm/i915/intel_drv.h | 1 +
>  2 files changed, 10 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 5a638503e36a..8ae7cf3d4ee1 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4201,6 +4201,9 @@ static void intel_dp_get_dsc_sink_cap(struct intel_dp *intel_dp)
>  	 */
>  	memset(intel_dp->dsc_dpcd, 0, sizeof(intel_dp->dsc_dpcd));
>  
> +	/* Clear fec_capable to avoid using stale values */
> +	intel_dp->fec_capable = 0;
> +
>  	/* Cache the DSC DPCD if eDP or DP rev >= 1.4 */
>  	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x14 ||
>  	    intel_dp->edp_dpcd[0] >= DP_EDP_14) {
> @@ -4213,6 +4216,12 @@ static void intel_dp_get_dsc_sink_cap(struct intel_dp *intel_dp)
>  		DRM_DEBUG_KMS("DSC DPCD: %*ph\n",
>  			      (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)) {
> +			if (drm_dp_dpcd_readb(&intel_dp->aux, DP_FEC_CAPABILITY,
> +					      &intel_dp->fec_capable) < 0)
> +				DRM_ERROR("Failed to read FEC DPCD register\n");

Would be nice to print the value of FEC_CAPABLE in th above DRM_DEBUG_KMS right after DSC DPCD.
With that change:

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

You can make this change, add my r-b , rebase and submit 1st and 2nd patch with CI prefix.

Manasi

> +		}
>  	}
>  }
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 16bbc3768e02..9a94c6544bf5 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1119,6 +1119,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_capable;
>  	/* 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

Re: [v4 1/7] i915/dp/fec: Cache the FEC_CAPABLE DPCD register

[Srivatsa, Anusha]

>-----Original Message-----
>From: Navare, Manasi D
>Sent: Thursday, November 1, 2018 3:31 PM
>To: Srivatsa, Anusha <anusha.srivatsa@intel.com>
>Cc: intel-gfx@lists.freedesktop.org; Jani Nikula <jani.nikula@linux.intel.com>;
>Ville Syrjala <ville.syrjala@linux.intel.com>
>Subject: Re: [v4 1/7] i915/dp/fec: Cache the FEC_CAPABLE DPCD register
>
>On Tue, Oct 30, 2018 at 05:45:11PM -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.
>>
>> v2: Avoid using memset and array for a single field. (Manasi,Jani)
>>
>> v3:
>>
>> 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  | 9 +++++++++
>> drivers/gpu/drm/i915/intel_drv.h | 1 +
>>  2 files changed, 10 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>> b/drivers/gpu/drm/i915/intel_dp.c index 5a638503e36a..8ae7cf3d4ee1
>> 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -4201,6 +4201,9 @@ static void intel_dp_get_dsc_sink_cap(struct intel_dp
>*intel_dp)
>>  	 */
>>  	memset(intel_dp->dsc_dpcd, 0, sizeof(intel_dp->dsc_dpcd));
>>
>> +	/* Clear fec_capable to avoid using stale values */
>> +	intel_dp->fec_capable = 0;
>> +
>>  	/* Cache the DSC DPCD if eDP or DP rev >= 1.4 */
>>  	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x14 ||
>>  	    intel_dp->edp_dpcd[0] >= DP_EDP_14) { @@ -4213,6 +4216,12 @@
>> static void intel_dp_get_dsc_sink_cap(struct intel_dp *intel_dp)
>>  		DRM_DEBUG_KMS("DSC DPCD: %*ph\n",
>>  			      (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)) {
>> +			if (drm_dp_dpcd_readb(&intel_dp->aux,
>DP_FEC_CAPABILITY,
>> +					      &intel_dp->fec_capable) < 0)
>> +				DRM_ERROR("Failed to read FEC DPCD
>register\n");
>
>Would be nice to print the value of FEC_CAPABLE in th above DRM_DEBUG_KMS
>right after DSC DPCD.

So unlike DSC DPCD, the FEC_CAPABLE is just one byte. Would we gain much by printing the value?

Anusha 
>With that change:
>
>Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>
>
>You can make this change, add my r-b , rebase and submit 1st and 2nd patch with
>CI prefix.
>
>Manasi
>
>> +		}
>>  	}
>>  }
>>
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>> b/drivers/gpu/drm/i915/intel_drv.h
>> index 16bbc3768e02..9a94c6544bf5 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -1119,6 +1119,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_capable;
>>  	/* 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

Re: [v4 1/7] i915/dp/fec: Cache the FEC_CAPABLE DPCD register

[Manasi Navare]

On Thu, Nov 01, 2018 at 04:02:48PM -0700, Srivatsa, Anusha wrote:
> 
> 
> >-----Original Message-----
> >From: Navare, Manasi D
> >Sent: Thursday, November 1, 2018 3:31 PM
> >To: Srivatsa, Anusha <anusha.srivatsa@intel.com>
> >Cc: intel-gfx@lists.freedesktop.org; Jani Nikula <jani.nikula@linux.intel.com>;
> >Ville Syrjala <ville.syrjala@linux.intel.com>
> >Subject: Re: [v4 1/7] i915/dp/fec: Cache the FEC_CAPABLE DPCD register
> >
> >On Tue, Oct 30, 2018 at 05:45:11PM -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.
> >>
> >> v2: Avoid using memset and array for a single field. (Manasi,Jani)
> >>
> >> v3:
> >>
> >> 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  | 9 +++++++++
> >> drivers/gpu/drm/i915/intel_drv.h | 1 +
> >>  2 files changed, 10 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c
> >> b/drivers/gpu/drm/i915/intel_dp.c index 5a638503e36a..8ae7cf3d4ee1
> >> 100644
> >> --- a/drivers/gpu/drm/i915/intel_dp.c
> >> +++ b/drivers/gpu/drm/i915/intel_dp.c
> >> @@ -4201,6 +4201,9 @@ static void intel_dp_get_dsc_sink_cap(struct intel_dp
> >*intel_dp)
> >>  	 */
> >>  	memset(intel_dp->dsc_dpcd, 0, sizeof(intel_dp->dsc_dpcd));
> >>
> >> +	/* Clear fec_capable to avoid using stale values */
> >> +	intel_dp->fec_capable = 0;
> >> +
> >>  	/* Cache the DSC DPCD if eDP or DP rev >= 1.4 */
> >>  	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x14 ||
> >>  	    intel_dp->edp_dpcd[0] >= DP_EDP_14) { @@ -4213,6 +4216,12 @@
> >> static void intel_dp_get_dsc_sink_cap(struct intel_dp *intel_dp)
> >>  		DRM_DEBUG_KMS("DSC DPCD: %*ph\n",
> >>  			      (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)) {
> >> +			if (drm_dp_dpcd_readb(&intel_dp->aux,
> >DP_FEC_CAPABILITY,
> >> +					      &intel_dp->fec_capable) < 0)
> >> +				DRM_ERROR("Failed to read FEC DPCD
> >register\n");
> >
> >Would be nice to print the value of FEC_CAPABLE in th above DRM_DEBUG_KMS
> >right after DSC DPCD.
> 
> So unlike DSC DPCD, the FEC_CAPABLE is just one byte. Would we gain much by printing the value?
>

I think it will still be useful since its the contents of DPCD regsiter and will be printed where
we print all DPCDs, DSC DPCD and then this.
Else for debugging, people will need to add a debug print after the macro and rebuild the kernel.

Manasi
 
> Anusha 
> >With that change:
> >
> >Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>
> >
> >You can make this change, add my r-b , rebase and submit 1st and 2nd patch with
> >CI prefix.
> >
> >Manasi
> >
> >> +		}
> >>  	}
> >>  }
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> >> b/drivers/gpu/drm/i915/intel_drv.h
> >> index 16bbc3768e02..9a94c6544bf5 100644
> >> --- a/drivers/gpu/drm/i915/intel_drv.h
> >> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> @@ -1119,6 +1119,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_capable;
> >>  	/* 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

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

[Ville Syrjälä]

On Wed, Oct 31, 2018 at 10:34:31PM +0000, Srivatsa, Anusha wrote:
> 
> 
> >-----Original Message-----
> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> >Sent: Wednesday, October 31, 2018 2:08 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>; Navare, Manasi D
> ><manasi.d.navare@intel.com>
> >Subject: Re: [v4 6/7] i915/dp/fec: Configure the Forward Error Correction bits.
> >
> >On Tue, Oct 30, 2018 at 05:45:16PM -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.
> >>
> >> v5:
> >> - Move the code to the proper spot, according to spec.(Ville)
> >> - Use fec state as a check too.
> >>
> >> 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 | 30 ++++++++++++++++++++++++++++++
> >> drivers/gpu/drm/i915/intel_drv.h |  2 ++
> >>  3 files changed, 34 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h
> >> b/drivers/gpu/drm/i915/i915_reg.h index e85f53cb9cdd..8b1753939299
> >> 100644
> >> --- a/drivers/gpu/drm/i915/i915_reg.h
> >> +++ b/drivers/gpu/drm/i915/i915_reg.h
> >> @@ -9134,6 +9134,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)
> >> @@ -9152,6 +9153,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 efbada95dc4e..f03f44f332c7 100644
> >> --- a/drivers/gpu/drm/i915/intel_ddi.c
> >> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> >> @@ -2889,6 +2889,32 @@ static void intel_ddi_clk_disable(struct
> >intel_encoder *encoder)
> >>  	}
> >>  }
> >>
> >> +void intel_dp_enable_fec_state(struct intel_dp *intel_dp,
> >> +			       const struct intel_crtc_state *crtc_state)
> >
> >Can be static. intel_ddi_enable_fec() seems like a better name, and we can just
> >pass the intel_encoder to this I think. No need to do all the "intel_dp to encoder"
> >gynmastics then.
> intel_ddi_enable_fec() is a better name. 
> 
> >> +{
> >> +	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 */
> >
> >This comment is misleading. There are other reasons besides < DP 1.4 for not
> >using FEC. IMO just drop the comment.
> 
> Ok.
> 
> >> +	if (!crtc_state->fec_enable)
> >> +		return;
> >> +
> >> +	if (!crtc_state->dsc_params.compression_enable)
> >> +		return;
> >
> >That check can be removed now.
> No we will still need this. The fec_enable state is not coupled with compression. So we have to check fec state and the compression ....
> 

Either we enable/disable fec or not. Whatever linkage with DSC only
exists in compute_config().

> Anusha 
> >> +
> >> +	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"); }
> >> +
> >>  static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
> >>  				    const struct intel_crtc_state *crtc_state,
> >>  				    const struct drm_connector_state
> >*conn_state) @@ -2934,9
> >> +2960,13 @@ 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);
> >> +
> >
> >Unrelated whitespace change.
> >
> >>  	if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
> >>  		intel_dp_stop_link_train(intel_dp);
> >>
> >> +	/* Set FEC state after link training */
> >
> >Redundant comment.
> >
> >> +	intel_dp_enable_fec_state(intel_dp, crtc_state);
> >> +
> >>  	icl_enable_phy_clock_gating(dig_port);
> >>
> >>  	if (!is_mst)
> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> >> b/drivers/gpu/drm/i915/intel_drv.h
> >> index 2a080205968d..1cdfa9c5da43 100644
> >> --- a/drivers/gpu/drm/i915/intel_drv.h
> >> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> @@ -1799,6 +1799,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: [v4 6/7] 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, November 2, 2018 4:16 AM
>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>; Navare, Manasi D
><manasi.d.navare@intel.com>
>Subject: Re: [v4 6/7] i915/dp/fec: Configure the Forward Error Correction bits.
>
>On Wed, Oct 31, 2018 at 10:34:31PM +0000, Srivatsa, Anusha wrote:
>>
>>
>> >-----Original Message-----
>> >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
>> >Sent: Wednesday, October 31, 2018 2:08 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>; Navare, Manasi D
>> ><manasi.d.navare@intel.com>
>> >Subject: Re: [v4 6/7] i915/dp/fec: Configure the Forward Error Correction bits.
>> >
>> >On Tue, Oct 30, 2018 at 05:45:16PM -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.
>> >>
>> >> v5:
>> >> - Move the code to the proper spot, according to spec.(Ville)
>> >> - Use fec state as a check too.
>> >>
>> >> 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 | 30
>> >> ++++++++++++++++++++++++++++++ drivers/gpu/drm/i915/intel_drv.h |
>> >> 2 ++
>> >>  3 files changed, 34 insertions(+)
>> >>
>> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> >> b/drivers/gpu/drm/i915/i915_reg.h index e85f53cb9cdd..8b1753939299
>> >> 100644
>> >> --- a/drivers/gpu/drm/i915/i915_reg.h
>> >> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> >> @@ -9134,6 +9134,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)
>> >> @@ -9152,6 +9153,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 efbada95dc4e..f03f44f332c7 100644
>> >> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> >> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> >> @@ -2889,6 +2889,32 @@ static void intel_ddi_clk_disable(struct
>> >intel_encoder *encoder)
>> >>  	}
>> >>  }
>> >>
>> >> +void intel_dp_enable_fec_state(struct intel_dp *intel_dp,
>> >> +			       const struct intel_crtc_state *crtc_state)
>> >
>> >Can be static. intel_ddi_enable_fec() seems like a better name, and
>> >we can just pass the intel_encoder to this I think. No need to do all the
>"intel_dp to encoder"
>> >gynmastics then.
>> intel_ddi_enable_fec() is a better name.
>>
>> >> +{
>> >> +	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 */
>> >
>> >This comment is misleading. There are other reasons besides < DP 1.4
>> >for not using FEC. IMO just drop the comment.
>>
>> Ok.
>>
>> >> +	if (!crtc_state->fec_enable)
>> >> +		return;
>> >> +
>> >> +	if (!crtc_state->dsc_params.compression_enable)
>> >> +		return;
>> >
>> >That check can be removed now.
>> No we will still need this. The fec_enable state is not coupled with compression.
>So we have to check fec state and the compression ....
>>
>
>Either we enable/disable fec or not. Whatever linkage with DSC only exists in
>compute_config().

True, I have changed this in v5.

Anusha
>> Anusha
>> >> +
>> >> +	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"); }
>> >> +
>> >>  static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
>> >>  				    const struct intel_crtc_state *crtc_state,
>> >>  				    const struct drm_connector_state
>> >*conn_state) @@ -2934,9
>> >> +2960,13 @@ 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);
>> >> +
>> >
>> >Unrelated whitespace change.
>> >
>> >>  	if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
>> >>  		intel_dp_stop_link_train(intel_dp);
>> >>
>> >> +	/* Set FEC state after link training */
>> >
>> >Redundant comment.
>> >
>> >> +	intel_dp_enable_fec_state(intel_dp, crtc_state);
>> >> +
>> >>  	icl_enable_phy_clock_gating(dig_port);
>> >>
>> >>  	if (!is_mst)
>> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>> >> b/drivers/gpu/drm/i915/intel_drv.h
>> >> index 2a080205968d..1cdfa9c5da43 100644
>> >> --- a/drivers/gpu/drm/i915/intel_drv.h
>> >> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> >> @@ -1799,6 +1799,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: [v4 7/7] drm/i915/fec: Disable FEC state.

[Manasi Navare]

On Tue, Oct 30, 2018 at 05:45:17PM -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.
> 
> v4:
> - Move the code to the proper spot according to spec (Ville)
> - Use proper checks (manasi)
> 
> 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 | 32 ++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h |  2 ++
>  2 files changed, 34 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index f03f44f332c7..d28280c3b299 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -2915,6 +2915,31 @@ 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;
> +	u8 dsc_en_state;
> +
> +	if (!crtc_state->fec_enable)
> +		return;
> +
> +	if (!crtc_state->dsc_params.compression_enable)
> +		return;
> +
> +	drm_dp_dpcd_readb(&intel_dp->aux, DP_DSC_ENABLE, &dsc_en_state);
> +
> +	if (!dsc_en_state) {
> +		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));
> +	}
> +}
> +
>  static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
>  				    const struct intel_crtc_state *crtc_state,
>  				    const struct drm_connector_state *conn_state)
> @@ -3055,7 +3080,11 @@ static void intel_ddi_pre_enable(struct intel_encoder *encoder,
>  static void intel_disable_ddi_buf(struct intel_encoder *encoder)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> +	struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
> +	struct intel_dp *intel_dp = &dig_port->dp;
> +	struct intel_crtc_state *crtc_state;
>  	enum port port = encoder->port;
> +
>  	bool wait = false;
>  	u32 val;
>  
> @@ -3071,6 +3100,9 @@ static void intel_disable_ddi_buf(struct intel_encoder *encoder)
>  	val |= DP_TP_CTL_LINK_TRAIN_PAT1;
>  	I915_WRITE(DP_TP_CTL(port), val);
>  
> +	/* Disable FEC in DP Sink */
> +	intel_dp_disable_fec_state(intel_dp, crtc_state);

crtc_state is uninitialized here. You need to pass crtc_state to intel_disable_ddi_buf()
or just call intel_dp_disable_fec_state() from the caller of intel_disable_ddi_buf() right
after it.

Manasi

> +
>  	if (wait)
>  		intel_wait_ddi_buf_idle(dev_priv, port);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 1cdfa9c5da43..4ed04fad83b7 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1801,6 +1801,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