| Message ID | 20230113043653.795183-2-arun.r.murthy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm: Add SDP Error Detection Configuration Register | expand |
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)
> -----Original Message----- > From: Nikula, Jani <jani.nikula@intel.com> > Sent: Friday, January 13, 2023 1:49 PM > To: Murthy, Arun R <arun.r.murthy@intel.com>; intel- > gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org > Cc: Murthy, Arun R <arun.r.murthy@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@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? 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
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) { + 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); + /* + * 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 */ } 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) #define VIDEO_DIP_ENABLE_DRM_GLK (1 << 28) #define PSR_VSC_BIT_7_SET (1 << 27) #define VSC_SELECT_MASK (0x3 << 25)
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(+)