On Fri, 13 Jan 2023, Arun R Murthy <arun.r.murthy@xxxxxxxxx> 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@xxxxxxxxx>
> ---
> 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
