[Intel-gfx] [PATCHv2 0/2] DP2.0 SDP CRC16 for 128/132b link layer

[Intel-gfx] [PATCHv2 0/2] DP2.0 SDP CRC16 for 128/132b link layer

[Arun R Murthy]

*** BLURB HERE ***

Arun R Murthy (2):
  drm: Add SDP Error Detection Configuration Register
  i915/display/dp: SDP CRC16 for 128b132b link layer

 .../gpu/drm/i915/display/intel_dp_link_training.c    | 12 ++++++++++++
 include/drm/display/drm_dp.h                         |  3 +++
 2 files changed, 15 insertions(+)

-- 
2.25.1

[Intel-gfx] [RESEND PATCHv2 1/2] drm: Add SDP Error Detection Configuration Register

[Arun R Murthy]

DP2.0 E11 defines a new register to facilitate SDP error detection by a
128B/132B capable DPRX device.

v2: Update the macro name to reflect the DP spec(Harry)

Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
Reviewed-by: Harry Wentland <harry.wentland@amd.com>
---
 include/drm/display/drm_dp.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/drm/display/drm_dp.h b/include/drm/display/drm_dp.h
index 632376c291db..358db4a9f167 100644
--- a/include/drm/display/drm_dp.h
+++ b/include/drm/display/drm_dp.h
@@ -692,6 +692,9 @@
 # define DP_FEC_LANE_2_SELECT		    (2 << 4)
 # define DP_FEC_LANE_3_SELECT		    (3 << 4)
 
+#define DP_SDP_ERROR_DETECTION_CONFIGURATION	0x121	/* DP 2.0 E11 */
+#define DP_SDP_CRC16_128B132B_EN		BIT(0)
+
 #define DP_AUX_FRAME_SYNC_VALUE		    0x15c   /* eDP 1.4 */
 # define DP_AUX_FRAME_SYNC_VALID	    (1 << 0)
 
-- 
2.25.1

[Intel-gfx] [RESEND PATCHv2 2/2] i915/display/dp: SDP CRC16 for 128b132b link layer

[Arun R Murthy]

Enable SDP error detection configuration, this will set CRC16 in
128b/132b link layer.
For Display version 13 a hardware bit31 in register VIDEO_DIP_CTL is
added to enable/disable SDP CRC applicable for DP2.0 only, but the
default value of this bit will enable CRC16 in 128b/132b hence
skipping this write.
Corrective actions on SDP corruption is yet to be defined.

v2: Moved the CRC enable to link training init(Jani N)

Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
 .../gpu/drm/i915/display/intel_dp_link_training.c    | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.c b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
index 3d3efcf02011..7064e465423b 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
@@ -1453,4 +1453,16 @@ void intel_dp_start_link_train(struct intel_dp *intel_dp,
 
 	if (!passed)
 		intel_dp_schedule_fallback_link_training(intel_dp, crtc_state);
+
+	/* DP v2.0 SCR on SDP CRC16 for 128b/132b Link Layer */
+	if (intel_dp_is_uhbr(crtc_state) && passed)
+		drm_dp_dpcd_writeb(&intel_dp->aux,
+				   DP_SDP_ERROR_DETECTION_CONFIGURATION,
+				   DP_SDP_CRC16_128B132B_EN);
+		/*
+		 * VIDEO_DIP_CTL register bit 31 should be set to '0' to not
+		 * disable SDP CRC. This is applicable for Display version 13.
+		 * Default value of bit 31 is '0' hence discarding the write
+		 */
+		/* TODO: Corrective actions on SDP corruption yet to be defined */
 }
-- 
2.25.1

Re: [Intel-gfx] [RESEND PATCHv2 2/2] i915/display/dp: SDP CRC16 for 128b132b link layer

[Jani Nikula]

On Fri, 20 Jan 2023, Arun R Murthy <arun.r.murthy@intel.com> wrote:
> Enable SDP error detection configuration, this will set CRC16 in
> 128b/132b link layer.
> For Display version 13 a hardware bit31 in register VIDEO_DIP_CTL is
> added to enable/disable SDP CRC applicable for DP2.0 only, but the
> default value of this bit will enable CRC16 in 128b/132b hence
> skipping this write.
> Corrective actions on SDP corruption is yet to be defined.
>
> v2: Moved the CRC enable to link training init(Jani N)

Yeah, well, I said where this doesn't belong, and I don't think it
really belongs here either. :(

It has nothing to do with link training or intel_dp_start_link_train().

Alas, I'm not really sure what the right place is, I just know this
isn't it. The specs don't give us any clues. The DP specs says
absolutely nothing about when this could or should be written.

*Maybe* in intel_ddi_pre_enable() or intel_mst_pre_enable_dp()? Before
sending any SDPs.


BR,
Jani.

>
> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> ---
>  .../gpu/drm/i915/display/intel_dp_link_training.c    | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.c b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> index 3d3efcf02011..7064e465423b 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> @@ -1453,4 +1453,16 @@ void intel_dp_start_link_train(struct intel_dp *intel_dp,
>  
>  	if (!passed)
>  		intel_dp_schedule_fallback_link_training(intel_dp, crtc_state);
> +
> +	/* DP v2.0 SCR on SDP CRC16 for 128b/132b Link Layer */
> +	if (intel_dp_is_uhbr(crtc_state) && passed)
> +		drm_dp_dpcd_writeb(&intel_dp->aux,
> +				   DP_SDP_ERROR_DETECTION_CONFIGURATION,
> +				   DP_SDP_CRC16_128B132B_EN);
> +		/*
> +		 * VIDEO_DIP_CTL register bit 31 should be set to '0' to not
> +		 * disable SDP CRC. This is applicable for Display version 13.
> +		 * Default value of bit 31 is '0' hence discarding the write
> +		 */
> +		/* TODO: Corrective actions on SDP corruption yet to be defined */
>  }

-- 
Jani Nikula, Intel Open Source Graphics Center

[Intel-gfx] ✗ Fi.CI.SPARSE: warning for DP2.0 SDP CRC16 for 128/132b link layer

[Patchwork]

== Series Details ==

Series: DP2.0 SDP CRC16 for 128/132b link layer
URL   : https://patchwork.freedesktop.org/series/113134/
State : warning

== Summary ==

Error: dim sparse failed
Sparse version: v0.6.2
Fast mode used, each commit won't be checked separately.

[Intel-gfx] ✗ Fi.CI.BAT: failure for DP2.0 SDP CRC16 for 128/132b link layer

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 4256 bytes --]

== Series Details ==

Series: DP2.0 SDP CRC16 for 128/132b link layer
URL   : https://patchwork.freedesktop.org/series/113134/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_12615 -> Patchwork_113134v1
====================================================

Summary
-------

  **FAILURE**

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

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113134v1/index.html

Participating hosts (37 -> 31)
------------------------------

  Missing    (6): fi-rkl-11600 bat-dg1-6 bat-dg2-9 fi-snb-2520m bat-rpls-2 fi-skl-6600u 

Possible new issues
-------------------

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

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live@execlists:
    - fi-bsw-nick:        [PASS][1] -> [INCOMPLETE][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12615/fi-bsw-nick/igt@i915_selftest@live@execlists.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113134v1/fi-bsw-nick/igt@i915_selftest@live@execlists.html

  
#### Suppressed ####

  The following results come from untrusted machines, tests, or statuses.
  They do not affect the overall result.

  * igt@i915_selftest@live@hugepages:
    - {bat-dg2-11}:       [PASS][3] -> [INCOMPLETE][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12615/bat-dg2-11/igt@i915_selftest@live@hugepages.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113134v1/bat-dg2-11/igt@i915_selftest@live@hugepages.html

  
Known issues
------------

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_gttfill@basic:
    - fi-pnv-d510:        [PASS][5] -> [FAIL][6] ([i915#7229])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12615/fi-pnv-d510/igt@gem_exec_gttfill@basic.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113134v1/fi-pnv-d510/igt@gem_exec_gttfill@basic.html

  * igt@i915_selftest@live@gt_heartbeat:
    - fi-apl-guc:         [PASS][7] -> [DMESG-FAIL][8] ([i915#5334])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12615/fi-apl-guc/igt@i915_selftest@live@gt_heartbeat.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113134v1/fi-apl-guc/igt@i915_selftest@live@gt_heartbeat.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@migrate:
    - {bat-dg2-11}:       [DMESG-WARN][9] ([i915#7699]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_12615/bat-dg2-11/igt@i915_selftest@live@migrate.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113134v1/bat-dg2-11/igt@i915_selftest@live@migrate.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [i915#4312]: https://gitlab.freedesktop.org/drm/intel/issues/4312
  [i915#5334]: https://gitlab.freedesktop.org/drm/intel/issues/5334
  [i915#7077]: https://gitlab.freedesktop.org/drm/intel/issues/7077
  [i915#7229]: https://gitlab.freedesktop.org/drm/intel/issues/7229
  [i915#7609]: https://gitlab.freedesktop.org/drm/intel/issues/7609
  [i915#7699]: https://gitlab.freedesktop.org/drm/intel/issues/7699


Build changes
-------------

  * Linux: CI_DRM_12615 -> Patchwork_113134v1

  CI-20190529: 20190529
  CI_DRM_12615: 8692eb28d41136537cd24aaf78f4621ddddbfb3a @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7126: 3aa9364a1e478010aba0d3dfe9183486560ecc60 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_113134v1: 8692eb28d41136537cd24aaf78f4621ddddbfb3a @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

1ec8f0a24447 i915/display/dp: SDP CRC16 for 128b132b link layer
475728c2697f drm: Add SDP Error Detection Configuration Register

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_113134v1/index.html

[-- Attachment #2: Type: text/html, Size: 4805 bytes --]

[Intel-gfx] [PATCHv3 0/2] DP2.0 SDP CRC16 for 128/132b link layer

[Arun R Murthy]

*** BLURB HERE ***

Arun R Murthy (2):
  drm: Add SDP Error Detection Configuration Register
  i915/display/dp: SDP CRC16 for 128b132b link layer

 .../gpu/drm/i915/display/intel_dp_link_training.c    | 12 ++++++++++++
 include/drm/display/drm_dp.h                         |  3 +++
 2 files changed, 15 insertions(+)

-- 
2.25.1

[Intel-gfx] [PATCHv2 1/2] drm: Add SDP Error Detection Configuration Register

[Arun R Murthy]

DP2.0 E11 defines a new register to facilitate SDP error detection by a
128B/132B capable DPRX device.

v2: Update the macro name to reflect the DP spec(Harry)

Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
Reviewed-by: Harry Wentland <harry.wentland@amd.com>
---
 include/drm/display/drm_dp.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/drm/display/drm_dp.h b/include/drm/display/drm_dp.h
index 632376c291db..358db4a9f167 100644
--- a/include/drm/display/drm_dp.h
+++ b/include/drm/display/drm_dp.h
@@ -692,6 +692,9 @@
 # define DP_FEC_LANE_2_SELECT		    (2 << 4)
 # define DP_FEC_LANE_3_SELECT		    (3 << 4)
 
+#define DP_SDP_ERROR_DETECTION_CONFIGURATION	0x121	/* DP 2.0 E11 */
+#define DP_SDP_CRC16_128B132B_EN		BIT(0)
+
 #define DP_AUX_FRAME_SYNC_VALUE		    0x15c   /* eDP 1.4 */
 # define DP_AUX_FRAME_SYNC_VALID	    (1 << 0)
 
-- 
2.25.1

[Intel-gfx] [PATCHv3 2/2] i915/display/dp: SDP CRC16 for 128b132b link layer

[Arun R Murthy]

Enable SDP error detection configuration, this will set CRC16 in
128b/132b link layer.
For Display version 13 a hardware bit31 in register VIDEO_DIP_CTL is
added to enable/disable SDP CRC applicable for DP2.0 only, but the
default value of this bit will enable CRC16 in 128b/132b hence
skipping this write.
Corrective actions on SDP corruption is yet to be defined.

v2: Moved the CRC enable to link training init(Jani N)
v3: Moved crc enable to ddi pre enable <Jani N>

Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
 .../gpu/drm/i915/display/intel_dp_link_training.c    | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.c b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
index 3d3efcf02011..7064e465423b 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
@@ -1453,4 +1453,16 @@ void intel_dp_start_link_train(struct intel_dp *intel_dp,
 
 	if (!passed)
 		intel_dp_schedule_fallback_link_training(intel_dp, crtc_state);
+
+	/* DP v2.0 SCR on SDP CRC16 for 128b/132b Link Layer */
+	if (intel_dp_is_uhbr(crtc_state) && passed)
+		drm_dp_dpcd_writeb(&intel_dp->aux,
+				   DP_SDP_ERROR_DETECTION_CONFIGURATION,
+				   DP_SDP_CRC16_128B132B_EN);
+		/*
+		 * VIDEO_DIP_CTL register bit 31 should be set to '0' to not
+		 * disable SDP CRC. This is applicable for Display version 13.
+		 * Default value of bit 31 is '0' hence discarding the write
+		 */
+		/* TODO: Corrective actions on SDP corruption yet to be defined */
 }
-- 
2.25.1

Re: [Intel-gfx] [PATCHv3 2/2] i915/display/dp: SDP CRC16 for 128b132b link layer

[Jani Nikula]

On Tue, 07 Feb 2023, Arun R Murthy <arun.r.murthy@intel.com> wrote:
> Enable SDP error detection configuration, this will set CRC16 in
> 128b/132b link layer.
> For Display version 13 a hardware bit31 in register VIDEO_DIP_CTL is
> added to enable/disable SDP CRC applicable for DP2.0 only, but the
> default value of this bit will enable CRC16 in 128b/132b hence
> skipping this write.
> Corrective actions on SDP corruption is yet to be defined.
>
> v2: Moved the CRC enable to link training init(Jani N)
> v3: Moved crc enable to ddi pre enable <Jani N>

It's still in intel_dp_start_link_train()...?

BR,
Jani.


>
> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> ---
>  .../gpu/drm/i915/display/intel_dp_link_training.c    | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.c b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> index 3d3efcf02011..7064e465423b 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> @@ -1453,4 +1453,16 @@ void intel_dp_start_link_train(struct intel_dp *intel_dp,
>  
>  	if (!passed)
>  		intel_dp_schedule_fallback_link_training(intel_dp, crtc_state);
> +
> +	/* DP v2.0 SCR on SDP CRC16 for 128b/132b Link Layer */
> +	if (intel_dp_is_uhbr(crtc_state) && passed)
> +		drm_dp_dpcd_writeb(&intel_dp->aux,
> +				   DP_SDP_ERROR_DETECTION_CONFIGURATION,
> +				   DP_SDP_CRC16_128B132B_EN);
> +		/*
> +		 * VIDEO_DIP_CTL register bit 31 should be set to '0' to not
> +		 * disable SDP CRC. This is applicable for Display version 13.
> +		 * Default value of bit 31 is '0' hence discarding the write
> +		 */
> +		/* TODO: Corrective actions on SDP corruption yet to be defined */
>  }

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [Intel-gfx] [PATCHv3 2/2] i915/display/dp: SDP CRC16 for 128b132b link layer

[Jani Nikula]

On Tue, 14 Feb 2023, Jani Nikula <jani.nikula@intel.com> wrote:
> On Tue, 07 Feb 2023, Arun R Murthy <arun.r.murthy@intel.com> wrote:
>> Enable SDP error detection configuration, this will set CRC16 in
>> 128b/132b link layer.
>> For Display version 13 a hardware bit31 in register VIDEO_DIP_CTL is
>> added to enable/disable SDP CRC applicable for DP2.0 only, but the
>> default value of this bit will enable CRC16 in 128b/132b hence
>> skipping this write.
>> Corrective actions on SDP corruption is yet to be defined.
>>
>> v2: Moved the CRC enable to link training init(Jani N)
>> v3: Moved crc enable to ddi pre enable <Jani N>
>
> It's still in intel_dp_start_link_train()...?

Also, please post new versions as new threads instead of in-reply-to. I
don't think patchwork/CI picked this up at all, for example.

BR,
Jani

>
> BR,
> Jani.
>
>
>>
>> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
>> ---
>>  .../gpu/drm/i915/display/intel_dp_link_training.c    | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.c b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
>> index 3d3efcf02011..7064e465423b 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
>> @@ -1453,4 +1453,16 @@ void intel_dp_start_link_train(struct intel_dp *intel_dp,
>>  
>>  	if (!passed)
>>  		intel_dp_schedule_fallback_link_training(intel_dp, crtc_state);
>> +
>> +	/* DP v2.0 SCR on SDP CRC16 for 128b/132b Link Layer */
>> +	if (intel_dp_is_uhbr(crtc_state) && passed)
>> +		drm_dp_dpcd_writeb(&intel_dp->aux,
>> +				   DP_SDP_ERROR_DETECTION_CONFIGURATION,
>> +				   DP_SDP_CRC16_128B132B_EN);
>> +		/*
>> +		 * VIDEO_DIP_CTL register bit 31 should be set to '0' to not
>> +		 * disable SDP CRC. This is applicable for Display version 13.
>> +		 * Default value of bit 31 is '0' hence discarding the write
>> +		 */
>> +		/* TODO: Corrective actions on SDP corruption yet to be defined */
>>  }

-- 
Jani Nikula, Intel Open Source Graphics Center

[Intel-gfx] [PATCHv3 0/2] DP2.0 SDP CRC16 for 128/132b link layer

[Arun R Murthy]

*** BLURB HERE ***

Arun R Murthy (2):
  drm: Add SDP Error Detection Configuration Register
  i915/display/dp: SDP CRC16 for 128b132b link layer

 .../gpu/drm/i915/display/intel_dp_link_training.c    | 12 ++++++++++++
 include/drm/display/drm_dp.h                         |  3 +++
 2 files changed, 15 insertions(+)

-- 
2.25.1

[Intel-gfx] [RESEND PATCHv2 2/2] i915/display/dp: SDP CRC16 for 128b132b link layer

[Arun R Murthy]

Enable SDP error detection configuration, this will set CRC16 in
128b/132b link layer.
For Display version 13 a hardware bit31 in register VIDEO_DIP_CTL is
added to enable/disable SDP CRC applicable for DP2.0 only, but the
default value of this bit will enable CRC16 in 128b/132b hence
skipping this write.
Corrective actions on SDP corruption is yet to be defined.

v2: Moved the CRC enable to link training init(Jani N)
v3: Moved crc enable to ddi pre enable <Jani N>

Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
 drivers/gpu/drm/i915/display/intel_ddi.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
index 254559abedfb..fa995341614d 100644
--- a/drivers/gpu/drm/i915/display/intel_ddi.c
+++ b/drivers/gpu/drm/i915/display/intel_ddi.c
@@ -2330,6 +2330,18 @@ static void tgl_ddi_pre_enable_dp(struct intel_atomic_state *state,
 				 crtc_state->port_clock,
 				 crtc_state->lane_count);
 
+	/* DP v2.0 SCR on SDP CRC16 for 128b/132b Link Layer */
+	if (intel_dp_is_uhbr(crtc_state))
+		drm_dp_dpcd_writeb(&intel_dp->aux,
+				   DP_SDP_ERROR_DETECTION_CONFIGURATION,
+				   DP_SDP_CRC16_128B132B_EN);
+		/*
+		 * VIDEO_DIP_CTL register bit 31 should be set to '0' to not
+		 * disable SDP CRC. This is applicable for Display version 13.
+		 * Default value of bit 31 is '0' hence discarding the write
+		 */
+		/* TODO: Corrective actions on SDP corruption yet to be defined */
+
 	/*
 	 * We only configure what the register value will be here.  Actual
 	 * enabling happens during link training farther down.
-- 
2.25.1

Re: [Intel-gfx] [RESEND PATCHv2 2/2] i915/display/dp: SDP CRC16 for 128b132b link layer

[Jani Nikula]

On Tue, 14 Feb 2023, Arun R Murthy <arun.r.murthy@intel.com> wrote:
> Enable SDP error detection configuration, this will set CRC16 in
> 128b/132b link layer.
> For Display version 13 a hardware bit31 in register VIDEO_DIP_CTL is
> added to enable/disable SDP CRC applicable for DP2.0 only, but the
> default value of this bit will enable CRC16 in 128b/132b hence
> skipping this write.
> Corrective actions on SDP corruption is yet to be defined.
>
> v2: Moved the CRC enable to link training init(Jani N)
> v3: Moved crc enable to ddi pre enable <Jani N>
>
> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_ddi.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c b/drivers/gpu/drm/i915/display/intel_ddi.c
> index 254559abedfb..fa995341614d 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -2330,6 +2330,18 @@ static void tgl_ddi_pre_enable_dp(struct intel_atomic_state *state,
>  				 crtc_state->port_clock,
>  				 crtc_state->lane_count);
>  
> +	/* DP v2.0 SCR on SDP CRC16 for 128b/132b Link Layer */
> +	if (intel_dp_is_uhbr(crtc_state))
> +		drm_dp_dpcd_writeb(&intel_dp->aux,
> +				   DP_SDP_ERROR_DETECTION_CONFIGURATION,
> +				   DP_SDP_CRC16_128B132B_EN);
> +		/*
> +		 * VIDEO_DIP_CTL register bit 31 should be set to '0' to not
> +		 * disable SDP CRC. This is applicable for Display version 13.
> +		 * Default value of bit 31 is '0' hence discarding the write
> +		 */
> +		/* TODO: Corrective actions on SDP corruption yet to be defined */
> +

So yeah, I told you to do this in this function. But look at the
surroundings, does it look like a direct drm_dp_dpcd_writeb() call fits
here?

tgl_ddi_pre_enable_dp() is a function that calls functions, and doesn't
bother with any details. No register or DPCD reads or writes.

Sure, it's a matter of style, and I hate to feel like I'm pushing you
around with this. But the above really needs to be in a separate
function when it's done in tgl_ddi_pre_enable_dp().

It'll also help with placing the comments. You can have *one* block
comment above the function with all the details, TODO notes and
everything, indented at the top level. (Above, the comments are indented
as if they were within a {} block.)

BR,
Jani.




>  	/*
>  	 * We only configure what the register value will be here.  Actual
>  	 * enabling happens during link training farther down.

-- 
Jani Nikula, Intel Open Source Graphics Center

[Intel-gfx] [PATCH 1/2] drm: Add SDP Error Detection Configuration Register

[Arun R Murthy]

DP2.0 E11 defines a new register to facilitate SDP error detection by a
128B/132B capable DPRX device.

Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
 include/drm/display/drm_dp.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/drm/display/drm_dp.h b/include/drm/display/drm_dp.h
index 9bc22a02874d..8bf6f0a60c38 100644
--- a/include/drm/display/drm_dp.h
+++ b/include/drm/display/drm_dp.h
@@ -691,6 +691,9 @@
 # define DP_FEC_LANE_2_SELECT		    (2 << 4)
 # define DP_FEC_LANE_3_SELECT		    (3 << 4)
 
+#define DP_SDP_ERROR_DETECTION		    0x121	/* DP 2.0 E11 */
+#define DP_SDP_CRC16_128B132B_EN	    BIT(0)
+
 #define DP_AUX_FRAME_SYNC_VALUE		    0x15c   /* eDP 1.4 */
 # define DP_AUX_FRAME_SYNC_VALID	    (1 << 0)
 
-- 
2.25.1

[Intel-gfx] [PATCHv2 1/2] drm: Add SDP Error Detection Configuration Register

[Arun R Murthy]

DP2.0 E11 defines a new register to facilitate SDP error detection by a
128B/132B capable DPRX device.

v2: Update the macro name to reflect the DP spec(Harry)

Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
Reviewed-by: Harry Wentland <harry.wentland@amd.com>
---
 include/drm/display/drm_dp.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/drm/display/drm_dp.h b/include/drm/display/drm_dp.h
index 632376c291db..358db4a9f167 100644
--- a/include/drm/display/drm_dp.h
+++ b/include/drm/display/drm_dp.h
@@ -692,6 +692,9 @@
 # define DP_FEC_LANE_2_SELECT		    (2 << 4)
 # define DP_FEC_LANE_3_SELECT		    (3 << 4)
 
+#define DP_SDP_ERROR_DETECTION_CONFIGURATION	0x121	/* DP 2.0 E11 */
+#define DP_SDP_CRC16_128B132B_EN		BIT(0)
+
 #define DP_AUX_FRAME_SYNC_VALUE		    0x15c   /* eDP 1.4 */
 # define DP_AUX_FRAME_SYNC_VALID	    (1 << 0)
 
-- 
2.25.1

[Intel-gfx] [RESEND PATCHv2 2/2] i915/display/dp: SDP CRC16 for 128b132b link layer

[Arun R Murthy]

Enable SDP error detection configuration, this will set CRC16 in
128b/132b link layer.
For Display version 13 a hardware bit31 in register VIDEO_DIP_CTL is
added to enable/disable SDP CRC applicable for DP2.0 only, but the
default value of this bit will enable CRC16 in 128b/132b hence
skipping this write.
Corrective actions on SDP corruption is yet to be defined.

v2: Moved the CRC enable to link training init(Jani N)

Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
---
 .../gpu/drm/i915/display/intel_dp_link_training.c    | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.c b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
index 3d3efcf02011..7064e465423b 100644
--- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
+++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
@@ -1453,4 +1453,16 @@ void intel_dp_start_link_train(struct intel_dp *intel_dp,
 
 	if (!passed)
 		intel_dp_schedule_fallback_link_training(intel_dp, crtc_state);
+
+	/* DP v2.0 SCR on SDP CRC16 for 128b/132b Link Layer */
+	if (intel_dp_is_uhbr(crtc_state) && passed)
+		drm_dp_dpcd_writeb(&intel_dp->aux,
+				   DP_SDP_ERROR_DETECTION_CONFIGURATION,
+				   DP_SDP_CRC16_128B132B_EN);
+		/*
+		 * VIDEO_DIP_CTL register bit 31 should be set to '0' to not
+		 * disable SDP CRC. This is applicable for Display version 13.
+		 * Default value of bit 31 is '0' hence discarding the write
+		 */
+		/* TODO: Corrective actions on SDP corruption yet to be defined */
 }
-- 
2.25.1