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

[Jani Nikula <jani.nikula@intel.com>]

On Fri, 13 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 DISPLAY13 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.
>
> Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c | 13 +++++++++++++
>  drivers/gpu/drm/i915/i915_reg.h         |  1 +
>  2 files changed, 14 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 30c55f980014..6096825a27ca 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -133,6 +133,7 @@ static void intel_dp_set_default_sink_rates(struct intel_dp *intel_dp)
>  /* update sink rates from dpcd */
>  static void intel_dp_set_dpcd_sink_rates(struct intel_dp *intel_dp)

Based on the function name and comment, this function updates the
driver's idea of what rates the sink supports. A quick look at the code
confirms this.

It should be clear that this is not the place to add unrelated DPCD
writes.

>  {
> +	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
>  	static const int dp_rates[] = {
>  		162000, 270000, 540000, 810000
>  	};
> @@ -196,6 +197,18 @@ static void intel_dp_set_dpcd_sink_rates(struct intel_dp *intel_dp)
>  			intel_dp->sink_rates[i++] = 1350000;
>  		if (uhbr_rates & DP_UHBR20)
>  			intel_dp->sink_rates[i++] = 2000000;
> +
> +		/* DP v2.0 SCR on SDP CRC16 for 128b/132b Link Layer */
> +		if (HAS_DP20(i915))
> +			drm_dp_dpcd_writeb(&intel_dp->aux,
> +					   DP_SDP_ERROR_DETECTION,
> +					   DP_SDP_CRC16_128B132B_EN);

HAS_DP20() means the source has DP 2.0 support.

We're in a branch where we can assume the sink also has DP 2.0
support. But at this point we're not sure we'll be using 128b/132b at
all.

I did not look this up in the spec, but I assume this bit is only
supposed to be set when we're actually using a 128b/132b link?

In which case, this should probably be enabled at some point when we're
enabling a 128b/132b link, and at that time the check to use is
intel_dp_is_uhbr(). UHBR and 128b/132b go hand-in-hand, and we won't use
UHBR unless both source and sink support it.

> +		/*
> +		 * VIDEO_DIP_CTL register bit 31 should be set to '0' to not
> +		 * disable SDP CRC. This is applicable for DISPLAY 13. Default
> +		 * value of bit 31 is '0' hence discarging the write
> +		 */
> +		/* TODO: Corrective actions on SDP corruption yet to be defined */

The above might belong in the commit message, but I'm not sure about
their usefulness as comments.

>  	}
>  
>  	intel_dp->num_sink_rates = i;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 8b2cf980f323..77e265f59978 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -2674,6 +2674,7 @@
>  #define   VIDEO_DIP_FREQ_2VSYNC		(2 << 16)
>  #define   VIDEO_DIP_FREQ_MASK		(3 << 16)
>  /* HSW and later: */
> +#define	  VIDEO_DISABLE_SDP_CRC		(1 << 31)

Please read the comment at the top of the file.

>  #define   VIDEO_DIP_ENABLE_DRM_GLK	(1 << 28)
>  #define   PSR_VSC_BIT_7_SET		(1 << 27)
>  #define   VSC_SELECT_MASK		(0x3 << 25)

-- 
Jani Nikula, Intel Open Source Graphics Center