[v7 0/4] Forward Error Correction

[v7 0/4] 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 everytime 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_dp_supports_fec;
Add intel_dp_source supports_fec() (Ville)

v5: Reduce unwanted checks. Pass intel_encoder to fec func
instead of intel_dp. Move code around to suitable place.

v6: Remove warning. rebase.

v7: Dont pass state as argument. Change fec state to take into
account source and sink capability. (Ville, manasi)

Rebased on top of: https://patchwork.freedesktop.org/series/51986/ 

Anusha Srivatsa (4):
  i915/dp/fec: Add fec_enable 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 | 62 +++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_dp.c  | 31 ++++++++++++++--
 drivers/gpu/drm/i915/intel_drv.h |  3 ++
 4 files changed, 92 insertions(+), 6 deletions(-)

-- 
2.19.1

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

[v7 1/4] i915/dp/fec: Add fec_enable to the crtc state.

[Anusha Srivatsa]

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

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)

v3: Check for FEC support along with setting crtc state.

v4: add checks to intel_dp_source_supports_dsc.(manasi)
- Move intel_dp_supports_fec() closer to
intel_dp_supports_dsc() (Anusha)

v5: Move fec check to intel_dp_supports_dsc(Ville)

v6: Remove warning. rebase.

v7: change crtc state to include DP sink and fec capability
of source.(Manasi)

Suggested-by: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: dri-devel@lists.freedesktop.org
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  | 31 +++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_drv.h |  3 +++
 2 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 73c00c5acf14..f764c45deaab 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -545,7 +545,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,
@@ -1710,12 +1710,27 @@ struct link_config_limits {
 	int min_bpp, max_bpp;
 };
 
+static bool intel_dp_source_supports_fec(struct intel_dp *intel_dp,
+					 const struct intel_crtc_state *pipe_config)
+{
+	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);
+
+	return INTEL_GEN(dev_priv) >= 11 && pipe_config->cpu_transcoder != TRANSCODER_A;
+}
+
+static bool intel_dp_supports_fec(struct intel_dp *intel_dp,
+				  const struct intel_crtc_state *pipe_config)
+{
+	return intel_dp_source_supports_fec(intel_dp, pipe_config) &&
+		drm_dp_sink_supports_fec(intel_dp->fec_capable);
+}
+
 static bool intel_dp_source_supports_dsc(struct intel_dp *intel_dp,
 					 const struct intel_crtc_state *pipe_config)
 {
 	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
 
-	/* FIXME: FEC needed for external DP until then reject DSC on DP */
 	if (!intel_dp_is_edp(intel_dp))
 		return false;
 
@@ -1726,6 +1741,9 @@ static bool intel_dp_source_supports_dsc(struct intel_dp *intel_dp,
 static bool intel_dp_supports_dsc(struct intel_dp *intel_dp,
 				  const struct intel_crtc_state *pipe_config)
 {
+	if (!pipe_config->fec_enable)
+		return false;
+
 	if (!intel_dp_source_supports_dsc(intel_dp, pipe_config) ||
 	    !drm_dp_sink_supports_dsc(intel_dp->dsc_dpcd))
 		return false;
@@ -1886,9 +1904,18 @@ 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) &&
+				  intel_dp_supports_fec(intel_dp, pipe_config);
+
 	if (!intel_dp_supports_dsc(intel_dp, pipe_config))
 		return false;
 
+	/* DSC not supported if external DP sink does not support FEC */
+	if (!pipe_config->fec_enable) {
+		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");
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index dd22cdeaa673..997bea5fdf16 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -945,6 +945,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.19.1

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

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

v5: Move to intel_ddi.c
- Make the function static (Anusha)

v6: Dont pass state as a separate argument (Ville)

Cc: dri-devel@lists.freedesktop.org
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 | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 46c1b9e12fbd..850c16200759 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -3051,6 +3051,16 @@ static void icl_program_mg_dp_mode(struct intel_digital_port *intel_dig_port)
 	I915_WRITE(MG_DP_MODE(port, 1), ln1);
 }
 
+static void intel_dp_sink_set_fec_ready(struct intel_dp *intel_dp,
+					const struct intel_crtc_state *crtc_state)
+{
+	if (!crtc_state->fec_enable)
+		return;
+
+	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_FEC_CONFIGURATION, DP_FEC_READY) <= 0)
+		DRM_DEBUG_KMS("Failed to get FEC enabled in sink\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)
@@ -3091,6 +3101,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,
 					      true);
+	intel_dp_sink_set_fec_ready(intel_dp, crtc_state);
 	intel_dp_start_link_train(intel_dp);
 	if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
 		intel_dp_stop_link_train(intel_dp);
-- 
2.19.1

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

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

v6: Pass intel_encoder, instead of intel_dp. (Ville)

v7: Remove unwanted comments (Manasi)

Cc: dri-devel@lists.freedesktop.org
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 | 23 +++++++++++++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 1a84e8f98e66..209b64d2f27a 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -9148,6 +9148,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)
@@ -9166,6 +9167,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 850c16200759..3a62e230ae2c 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -3061,6 +3061,27 @@ static void intel_dp_sink_set_fec_ready(struct intel_dp *intel_dp,
 		DRM_DEBUG_KMS("Failed to get FEC enabled in sink\n");
 }
 
+static void intel_ddi_enable_fec(struct intel_encoder *encoder,
+				 const struct intel_crtc_state *crtc_state)
+{
+	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
+	enum port port = encoder->port;
+	u32 val;
+
+	if (!crtc_state->fec_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)
@@ -3106,6 +3127,8 @@ static void intel_ddi_pre_enable_dp(struct intel_encoder *encoder,
 	if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
 		intel_dp_stop_link_train(intel_dp);
 
+	intel_ddi_enable_fec(encoder, crtc_state);
+
 	icl_enable_phy_clock_gating(dig_port);
 
 	if (!is_mst)
-- 
2.19.1

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[v7 4/4] 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)

v5: Remove unnecessary checks (Ville)

v6: Resolve warnings. Add crtc_state as an argument to
intel_disable_ddi_buf(). (Manasi)

Cc: dri-devel@lists.freedesktop.org
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>
Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c | 28 ++++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 3a62e230ae2c..581f9532d744 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -3082,6 +3082,22 @@ static void intel_ddi_enable_fec(struct intel_encoder *encoder,
 		DRM_ERROR("Timed out waiting for FEC Enable Status\n");
 }
 
+static void intel_ddi_disable_fec_state(struct intel_encoder *encoder,
+					const struct intel_crtc_state *crtc_state)
+{
+	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
+	enum port port = encoder->port;
+	u32 val;
+
+	if (!crtc_state->fec_enable)
+		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));
+}
+
 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)
@@ -3225,7 +3241,8 @@ static void intel_ddi_pre_enable(struct intel_encoder *encoder,
 	}
 }
 
-static void intel_disable_ddi_buf(struct intel_encoder *encoder)
+static void intel_disable_ddi_buf(struct intel_encoder *encoder,
+				  const struct intel_crtc_state *crtc_state)
 {
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	enum port port = encoder->port;
@@ -3244,6 +3261,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_ddi_disable_fec_state(encoder, crtc_state);
+
 	if (wait)
 		intel_wait_ddi_buf_idle(dev_priv, port);
 }
@@ -3267,7 +3287,7 @@ static void intel_ddi_post_disable_dp(struct intel_encoder *encoder,
 		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
 	}
 
-	intel_disable_ddi_buf(encoder);
+	intel_disable_ddi_buf(encoder, old_crtc_state);
 
 	intel_edp_panel_vdd_on(intel_dp);
 	intel_edp_panel_off(intel_dp);
@@ -3290,7 +3310,7 @@ static void intel_ddi_post_disable_hdmi(struct intel_encoder *encoder,
 
 	intel_ddi_disable_pipe_clock(old_crtc_state);
 
-	intel_disable_ddi_buf(encoder);
+	intel_disable_ddi_buf(encoder, old_crtc_state);
 
 	intel_display_power_put(dev_priv, dig_port->ddi_io_power_domain);
 
@@ -3341,7 +3361,7 @@ void intel_ddi_fdi_post_disable(struct intel_encoder *encoder,
 	val &= ~FDI_RX_ENABLE;
 	I915_WRITE(FDI_RX_CTL(PIPE_A), val);
 
-	intel_disable_ddi_buf(encoder);
+	intel_disable_ddi_buf(encoder, old_crtc_state);
 	intel_ddi_clk_disable(encoder);
 
 	val = I915_READ(FDI_RX_MISC(PIPE_A));
-- 
2.19.1

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

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

[Manasi Navare]

On Tue, Nov 06, 2018 at 04:31:19PM -0800, Anusha Srivatsa wrote:
> For DP 1.4 and above, Display Stream compression can be
> enabled only if Forward Error Correctin can be performed.
> 
> 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)
> 
> v3: Check for FEC support along with setting crtc state.
> 
> v4: add checks to intel_dp_source_supports_dsc.(manasi)
> - Move intel_dp_supports_fec() closer to
> intel_dp_supports_dsc() (Anusha)
> 
> v5: Move fec check to intel_dp_supports_dsc(Ville)
> 
> v6: Remove warning. rebase.
> 
> v7: change crtc state to include DP sink and fec capability
> of source.(Manasi)
> 
> Suggested-by: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: dri-devel@lists.freedesktop.org
> 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  | 31 +++++++++++++++++++++++++++++--
>  drivers/gpu/drm/i915/intel_drv.h |  3 +++
>  2 files changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 73c00c5acf14..f764c45deaab 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -545,7 +545,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,
> @@ -1710,12 +1710,27 @@ struct link_config_limits {
>  	int min_bpp, max_bpp;
>  };
>  
> +static bool intel_dp_source_supports_fec(struct intel_dp *intel_dp,
> +					 const struct intel_crtc_state *pipe_config)
> +{
> +	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);
> +
> +	return INTEL_GEN(dev_priv) >= 11 && pipe_config->cpu_transcoder != TRANSCODER_A;
> +}
> +
> +static bool intel_dp_supports_fec(struct intel_dp *intel_dp,
> +				  const struct intel_crtc_state *pipe_config)
> +{
> +	return intel_dp_source_supports_fec(intel_dp, pipe_config) &&
> +		drm_dp_sink_supports_fec(intel_dp->fec_capable);
> +}
> +
>  static bool intel_dp_source_supports_dsc(struct intel_dp *intel_dp,
>  					 const struct intel_crtc_state *pipe_config)
>  {
>  	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
>  
> -	/* FIXME: FEC needed for external DP until then reject DSC on DP */
>  	if (!intel_dp_is_edp(intel_dp))
>  		return false;

No point in keeping this !edp condition here, thats supposed to go with FEC check,
move that to where fec_supports is added.

>  
> @@ -1726,6 +1741,9 @@ static bool intel_dp_source_supports_dsc(struct intel_dp *intel_dp,
>  static bool intel_dp_supports_dsc(struct intel_dp *intel_dp,
>  				  const struct intel_crtc_state *pipe_config)
>  {
> +	if (!pipe_config->fec_enable)
> +		return false;

I think its better to use intel_dp_supports_fec(intel_dp, pipe_config) && !edp instead
of pipe_config->fec_enable because pipe_config->fec_enable state will be set only in dp_compute_config

> +
>  	if (!intel_dp_source_supports_dsc(intel_dp, pipe_config) ||
>  	    !drm_dp_sink_supports_dsc(intel_dp->dsc_dpcd))
>  		return false;
> @@ -1886,9 +1904,18 @@ 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) &&
> +				  intel_dp_supports_fec(intel_dp, pipe_config);

Why is pipe_config->fec_enable set in dsc_compute_config()? This state is totally indepenedent of dsc state
and we are treating this as an independent feature. I think the earlier place of setting this in intel_dp_compute_link_config()
is correct

Manasi

> +
>  	if (!intel_dp_supports_dsc(intel_dp, pipe_config))
>  		return false;
>  
> +	/* DSC not supported if external DP sink does not support FEC */
> +	if (!pipe_config->fec_enable) {
> +		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");
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index dd22cdeaa673..997bea5fdf16 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -945,6 +945,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.19.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [v7 2/4] drm/i915/fec: Set FEC_READY in FEC_CONFIGURATION

[Manasi Navare]

On Tue, Nov 06, 2018 at 04:31:20PM -0800, 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. (r-b Manasi)
> 
> v4: Use fec crtc state, before setting FEC_READY
> bit. (Anusha)
> 
> v5: Move to intel_ddi.c
> - Make the function static (Anusha)
> 
> v6: Dont pass state as a separate argument (Ville)
> 
> Cc: dri-devel@lists.freedesktop.org
> 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>

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

Manasi

> ---
>  drivers/gpu/drm/i915/intel_ddi.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 46c1b9e12fbd..850c16200759 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -3051,6 +3051,16 @@ static void icl_program_mg_dp_mode(struct intel_digital_port *intel_dig_port)
>  	I915_WRITE(MG_DP_MODE(port, 1), ln1);
>  }
>  
> +static void intel_dp_sink_set_fec_ready(struct intel_dp *intel_dp,
> +					const struct intel_crtc_state *crtc_state)
> +{
> +	if (!crtc_state->fec_enable)
> +		return;
> +
> +	if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_FEC_CONFIGURATION, DP_FEC_READY) <= 0)
> +		DRM_DEBUG_KMS("Failed to get FEC enabled in sink\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)
> @@ -3091,6 +3101,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,
>  					      true);
> +	intel_dp_sink_set_fec_ready(intel_dp, crtc_state);
>  	intel_dp_start_link_train(intel_dp);
>  	if (port != PORT_A || INTEL_GEN(dev_priv) >= 9)
>  		intel_dp_stop_link_train(intel_dp);
> -- 
> 2.19.1
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Patchwork]

== Series Details ==

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

== Summary ==

Applying: i915/dp/fec: Add fec_enable to the crtc state.
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: Add fec_enable to the crtc state.
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: [v7 1/4] i915/dp/fec: Add fec_enable to the crtc state.

[Srivatsa, Anusha]

>-----Original Message-----
>From: Navare, Manasi D
>Sent: Tuesday, November 6, 2018 6:40 PM
>To: Srivatsa, Anusha <anusha.srivatsa@intel.com>
>Cc: intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Ville Syrjala
><ville.syrjala@linux.intel.com>; Jani Nikula <jani.nikula@linux.intel.com>
>Subject: Re: [v7 1/4] i915/dp/fec: Add fec_enable to the crtc state.
>
>On Tue, Nov 06, 2018 at 04:31:19PM -0800, Anusha Srivatsa wrote:
>> For DP 1.4 and above, Display Stream compression can be enabled only
>> if Forward Error Correctin can be performed.
>>
>> 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)
>>
>> v3: Check for FEC support along with setting crtc state.
>>
>> v4: add checks to intel_dp_source_supports_dsc.(manasi)
>> - Move intel_dp_supports_fec() closer to
>> intel_dp_supports_dsc() (Anusha)
>>
>> v5: Move fec check to intel_dp_supports_dsc(Ville)
>>
>> v6: Remove warning. rebase.
>>
>> v7: change crtc state to include DP sink and fec capability of
>> source.(Manasi)
>>
>> Suggested-by: Ville Syrjala <ville.syrjala@linux.intel.com>
>> Cc: dri-devel@lists.freedesktop.org
>> 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  | 31 +++++++++++++++++++++++++++++--
>> drivers/gpu/drm/i915/intel_drv.h |  3 +++
>>  2 files changed, 32 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>> b/drivers/gpu/drm/i915/intel_dp.c index 73c00c5acf14..f764c45deaab
>> 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -545,7 +545,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,
>> @@ -1710,12 +1710,27 @@ struct link_config_limits {
>>  	int min_bpp, max_bpp;
>>  };
>>
>> +static bool intel_dp_source_supports_fec(struct intel_dp *intel_dp,
>> +					 const struct intel_crtc_state
>*pipe_config) {
>> +	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);
>> +
>> +	return INTEL_GEN(dev_priv) >= 11 && pipe_config->cpu_transcoder !=
>> +TRANSCODER_A; }
>> +
>> +static bool intel_dp_supports_fec(struct intel_dp *intel_dp,
>> +				  const struct intel_crtc_state *pipe_config) {
>> +	return intel_dp_source_supports_fec(intel_dp, pipe_config) &&
>> +		drm_dp_sink_supports_fec(intel_dp->fec_capable);
>> +}
>> +
>>  static bool intel_dp_source_supports_dsc(struct intel_dp *intel_dp,
>>  					 const struct intel_crtc_state
>*pipe_config)  {
>>  	struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
>>
>> -	/* FIXME: FEC needed for external DP until then reject DSC on DP */
>>  	if (!intel_dp_is_edp(intel_dp))
>>  		return false;
>
>No point in keeping this !edp condition here, thats supposed to go with FEC
>check, move that to where fec_supports is added.
>
>>
>> @@ -1726,6 +1741,9 @@ static bool intel_dp_source_supports_dsc(struct
>> intel_dp *intel_dp,  static bool intel_dp_supports_dsc(struct intel_dp *intel_dp,
>>  				  const struct intel_crtc_state *pipe_config)  {
>> +	if (!pipe_config->fec_enable)
>> +		return false;
>
>I think its better to use intel_dp_supports_fec(intel_dp, pipe_config) && !edp
>instead of pipe_config->fec_enable because pipe_config->fec_enable state will
>be set only in dp_compute_config
>
>> +
>>  	if (!intel_dp_source_supports_dsc(intel_dp, pipe_config) ||
>>  	    !drm_dp_sink_supports_dsc(intel_dp->dsc_dpcd))
>>  		return false;
>> @@ -1886,9 +1904,18 @@ 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) &&
>> +				  intel_dp_supports_fec(intel_dp, pipe_config);
>
>Why is pipe_config->fec_enable set in dsc_compute_config()? This state is totally
>indepenedent of dsc state and we are treating this as an independent feature. I
>think the earlier place of setting this in intel_dp_compute_link_config() is correct

How about, maybe adding the fec_enable state in intel_dp_compute_config(). Most of the pipe_config parameters are being set there. So, by the time we first check for dsc, we know if FEC support is there or not.

>Manasi
>
>> +
>>  	if (!intel_dp_supports_dsc(intel_dp, pipe_config))
>>  		return false;
>>
>> +	/* DSC not supported if external DP sink does not support FEC */
>> +	if (!pipe_config->fec_enable) {
>> +		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"); diff -
>-git
>> a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index dd22cdeaa673..997bea5fdf16 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -945,6 +945,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.19.1
>>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel