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

[Murthy, Arun R]

> -----Original Message-----
> From: Nikula, Jani <jani.nikula at intel.com>
> Sent: Friday, January 13, 2023 1:49 PM
> To: Murthy, Arun R <arun.r.murthy at intel.com>; intel-
> gfx at lists.freedesktop.org; dri-devel at lists.freedesktop.org
> Cc: Murthy, Arun R <arun.r.murthy at intel.com>
> Subject: Re: [PATCH 2/2] i915/display/dp: SDP CRC16 for 128b132b link layer
> 
> On Fri, 13 Jan 2023, Arun R Murthy <arun.r.murthy at 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 at 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?
Yes.

> 
> 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.
Updated and also moved this part under link training init.

> 
> > +		/*
> > +		 * 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.
This comment is just to ensure we keep track on what has to be done on error.

> 
> >  	}
> >
> >  	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.

Will remove this, as for now we are not using this and will add this once the corrective actions are added.

Thanks and Regards,
Arun R Murthy
--------------------

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