| Message ID | 20230714053733.2117730-1-arun.r.murthy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/dp: Fix LT debug print in SDP CRC enable | expand |
Any comments? Thanks and Regards, Arun R Murthy -------------------- > -----Original Message----- > From: Murthy, Arun R <arun.r.murthy@intel.com> > Sent: Friday, July 14, 2023 11:08 AM > To: intel-gfx@lists.freedesktop.org > Cc: Murthy, Arun R <arun.r.murthy@intel.com> > Subject: [PATCH] drm/i915/dp: Fix LT debug print in SDP CRC enable > > The debug print for enabling SDP CRC16 is applicable only for DP2.0, but this > debug print was not within the uhbr check and was always printed. > Fis this by adding proper checks and returning. > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> > --- > .../gpu/drm/i915/display/intel_dp_link_training.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > 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 a263773f4d68..4485ef4f8ec6 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c > +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c > @@ -1390,11 +1390,13 @@ void intel_dp_128b132b_sdp_crc16(struct > intel_dp *intel_dp, > * Default value of bit 31 is '0' hence discarding the write > * TODO: Corrective actions on SDP corruption yet to be defined > */ > - if (intel_dp_is_uhbr(crtc_state)) > - /* DP v2.0 SCR on SDP CRC16 for 128b/132b Link Layer */ > - drm_dp_dpcd_writeb(&intel_dp->aux, > - > DP_SDP_ERROR_DETECTION_CONFIGURATION, > - DP_SDP_CRC16_128B132B_EN); > + if (!intel_dp_is_uhbr(crtc_state)) > + return; > + > + /* DP v2.0 SCR on SDP CRC16 for 128b/132b Link Layer */ > + drm_dp_dpcd_writeb(&intel_dp->aux, > + DP_SDP_ERROR_DETECTION_CONFIGURATION, > + DP_SDP_CRC16_128B132B_EN); > > lt_dbg(intel_dp, DP_PHY_DPRX, "DP2.0 SDP CRC16 for 128b/132b > enabled\n"); } > -- > 2.25.1
Hello Arun, > -----Original Message----- > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Arun R > Murthy > Sent: Friday, July 14, 2023 11:08 AM > To: intel-gfx@lists.freedesktop.org > Subject: [Intel-gfx] [PATCH] drm/i915/dp: Fix LT debug print in SDP CRC enable > > The debug print for enabling SDP CRC16 is applicable only for DP2.0, DP2.0 and UHBR? >but this > debug print was not within the uhbr check and was always printed. > Fis this by adding proper checks and returning. Typo. > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> > --- > .../gpu/drm/i915/display/intel_dp_link_training.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > 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 a263773f4d68..4485ef4f8ec6 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c > +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c > @@ -1390,11 +1390,13 @@ void intel_dp_128b132b_sdp_crc16(struct > intel_dp *intel_dp, > * Default value of bit 31 is '0' hence discarding the write > * TODO: Corrective actions on SDP corruption yet to be defined > */ > - if (intel_dp_is_uhbr(crtc_state)) > - /* DP v2.0 SCR on SDP CRC16 for 128b/132b Link Layer */ > - drm_dp_dpcd_writeb(&intel_dp->aux, > - > DP_SDP_ERROR_DETECTION_CONFIGURATION, > - DP_SDP_CRC16_128B132B_EN); > + if (!intel_dp_is_uhbr(crtc_state)) > + return; I see that while calling this function in intel_ddi_pre_enable_dp(), we do have a check for for DP2.0 if (HAS_DP20(dev_priv)) intel_dp_128b132b_sdp_crc16(enc_to_intel_dp(encoder), crtc_state); Should this check be added within the function too for the sake of completion? Regards Chaitanya > + > + /* DP v2.0 SCR on SDP CRC16 for 128b/132b Link Layer */ > + drm_dp_dpcd_writeb(&intel_dp->aux, > + DP_SDP_ERROR_DETECTION_CONFIGURATION, > + DP_SDP_CRC16_128B132B_EN); > > lt_dbg(intel_dp, DP_PHY_DPRX, "DP2.0 SDP CRC16 for 128b/132b > enabled\n"); } > -- > 2.25.1
> -----Original Message----- > From: Borah, Chaitanya Kumar <chaitanya.kumar.borah@intel.com> > Sent: Friday, July 28, 2023 2:48 PM > To: Murthy, Arun R <arun.r.murthy@intel.com> > Cc: intel-gfx@lists.freedesktop.org > Subject: RE: [Intel-gfx] [PATCH] drm/i915/dp: Fix LT debug print in SDP CRC > enable > > Hello Arun, > > > -----Original Message----- > > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of > > Arun R Murthy > > Sent: Friday, July 14, 2023 11:08 AM > > To: intel-gfx@lists.freedesktop.org > > Subject: [Intel-gfx] [PATCH] drm/i915/dp: Fix LT debug print in SDP > > CRC enable > > > > The debug print for enabling SDP CRC16 is applicable only for DP2.0, > > DP2.0 and UHBR? This is a DP2.0 feature that can be enabled on UHBR rates. > > >but this > > debug print was not within the uhbr check and was always printed. > > Fis this by adding proper checks and returning. > > Typo. > > > > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> > > --- > > .../gpu/drm/i915/display/intel_dp_link_training.c | 12 +++++++----- > > 1 file changed, 7 insertions(+), 5 deletions(-) > > > > 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 a263773f4d68..4485ef4f8ec6 100644 > > --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c > > +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c > > @@ -1390,11 +1390,13 @@ void intel_dp_128b132b_sdp_crc16(struct > > intel_dp *intel_dp, > > * Default value of bit 31 is '0' hence discarding the write > > * TODO: Corrective actions on SDP corruption yet to be defined > > */ > > - if (intel_dp_is_uhbr(crtc_state)) > > - /* DP v2.0 SCR on SDP CRC16 for 128b/132b Link Layer */ > > - drm_dp_dpcd_writeb(&intel_dp->aux, > > - > > DP_SDP_ERROR_DETECTION_CONFIGURATION, > > - DP_SDP_CRC16_128B132B_EN); > > + if (!intel_dp_is_uhbr(crtc_state)) > > + return; > > I see that while calling this function in intel_ddi_pre_enable_dp(), we do > have a check for for DP2.0 > > if (HAS_DP20(dev_priv)) > intel_dp_128b132b_sdp_crc16(enc_to_intel_dp(encoder), > crtc_state); > > Should this check be added within the function too for the sake of > completion? > HAS DP20 just checked for the display version number and not UHBR rates. We need to check for UHBR rates and then enable this CRC. Thanks and Regards, Arun R Murthy ------------------- > Regards > > Chaitanya > > > + > > + /* DP v2.0 SCR on SDP CRC16 for 128b/132b Link Layer */ > > + drm_dp_dpcd_writeb(&intel_dp->aux, > > + DP_SDP_ERROR_DETECTION_CONFIGURATION, > > + DP_SDP_CRC16_128B132B_EN); > > > > lt_dbg(intel_dp, DP_PHY_DPRX, "DP2.0 SDP CRC16 for 128b/132b > > enabled\n"); } > > -- > > 2.25.1
Hello Arun, > -----Original Message----- > From: Murthy, Arun R <arun.r.murthy@intel.com> > Sent: Monday, July 31, 2023 11:25 AM > To: Borah, Chaitanya Kumar <chaitanya.kumar.borah@intel.com> > Cc: intel-gfx@lists.freedesktop.org > Subject: RE: [Intel-gfx] [PATCH] drm/i915/dp: Fix LT debug print in SDP CRC > enable > > > -----Original Message----- > > From: Borah, Chaitanya Kumar <chaitanya.kumar.borah@intel.com> > > Sent: Friday, July 28, 2023 2:48 PM > > To: Murthy, Arun R <arun.r.murthy@intel.com> > > Cc: intel-gfx@lists.freedesktop.org > > Subject: RE: [Intel-gfx] [PATCH] drm/i915/dp: Fix LT debug print in > > SDP CRC enable > > > > Hello Arun, > > > > > -----Original Message----- > > > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf > > > Of Arun R Murthy > > > Sent: Friday, July 14, 2023 11:08 AM > > > To: intel-gfx@lists.freedesktop.org > > > Subject: [Intel-gfx] [PATCH] drm/i915/dp: Fix LT debug print in SDP > > > CRC enable > > > > > > The debug print for enabling SDP CRC16 is applicable only for DP2.0, > > > > DP2.0 and UHBR? > > This is a DP2.0 feature that can be enabled on UHBR rates. > > > > > >but this > > > debug print was not within the uhbr check and was always printed. > > > Fis this by adding proper checks and returning. > > > > Typo. > > > > > > > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> > > > --- > > > .../gpu/drm/i915/display/intel_dp_link_training.c | 12 +++++++----- > > > 1 file changed, 7 insertions(+), 5 deletions(-) > > > > > > 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 a263773f4d68..4485ef4f8ec6 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c > > > +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c > > > @@ -1390,11 +1390,13 @@ void intel_dp_128b132b_sdp_crc16(struct > > > intel_dp *intel_dp, > > > * Default value of bit 31 is '0' hence discarding the write > > > * TODO: Corrective actions on SDP corruption yet to be defined > > > */ > > > - if (intel_dp_is_uhbr(crtc_state)) > > > - /* DP v2.0 SCR on SDP CRC16 for 128b/132b Link Layer */ > > > - drm_dp_dpcd_writeb(&intel_dp->aux, > > > - > > > DP_SDP_ERROR_DETECTION_CONFIGURATION, > > > - DP_SDP_CRC16_128B132B_EN); > > > + if (!intel_dp_is_uhbr(crtc_state)) > > > + return; > > > > I see that while calling this function in intel_ddi_pre_enable_dp(), > > we do have a check for for DP2.0 > > > > if (HAS_DP20(dev_priv)) > > intel_dp_128b132b_sdp_crc16(enc_to_intel_dp(encoder), > > crtc_state); > > > > Should this check be added within the function too for the sake of > > completion? > > > > HAS DP20 just checked for the display version number and not UHBR rates. > We need to check for UHBR rates and then enable this CRC. > I was alluding more to the fact that there are two conditions for enabling the CRC. if (!HAS_DP20(dev_priv) || !intel_dp_is_uhbr(crtc_state)) return; But if it is implicit that UHBR will only be supported on DP2.0 or/and this function is not expected to be used anywhere else (and hence without any possibility of this function being called without the HAS_DP20() check), the change looks good to me. Reviewed-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com> Regards Chaitanya > Thanks and Regards, > Arun R Murthy > ------------------- > > > Regards > > > > Chaitanya > > > > > + > > > + /* DP v2.0 SCR on SDP CRC16 for 128b/132b Link Layer */ > > > + drm_dp_dpcd_writeb(&intel_dp->aux, > > > + DP_SDP_ERROR_DETECTION_CONFIGURATION, > > > + DP_SDP_CRC16_128B132B_EN); > > > > > > lt_dbg(intel_dp, DP_PHY_DPRX, "DP2.0 SDP CRC16 for 128b/132b > > > enabled\n"); } > > > -- > > > 2.25.1
On Mon, 31 Jul 2023, "Borah, Chaitanya Kumar" <chaitanya.kumar.borah@intel.com> wrote: > Hello Arun, > >> -----Original Message----- >> From: Murthy, Arun R <arun.r.murthy@intel.com> >> Sent: Monday, July 31, 2023 11:25 AM >> To: Borah, Chaitanya Kumar <chaitanya.kumar.borah@intel.com> >> Cc: intel-gfx@lists.freedesktop.org >> Subject: RE: [Intel-gfx] [PATCH] drm/i915/dp: Fix LT debug print in SDP CRC >> enable >> >> > -----Original Message----- >> > From: Borah, Chaitanya Kumar <chaitanya.kumar.borah@intel.com> >> > Sent: Friday, July 28, 2023 2:48 PM >> > To: Murthy, Arun R <arun.r.murthy@intel.com> >> > Cc: intel-gfx@lists.freedesktop.org >> > Subject: RE: [Intel-gfx] [PATCH] drm/i915/dp: Fix LT debug print in >> > SDP CRC enable >> > >> > Hello Arun, >> > >> > > -----Original Message----- >> > > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf >> > > Of Arun R Murthy >> > > Sent: Friday, July 14, 2023 11:08 AM >> > > To: intel-gfx@lists.freedesktop.org >> > > Subject: [Intel-gfx] [PATCH] drm/i915/dp: Fix LT debug print in SDP >> > > CRC enable >> > > >> > > The debug print for enabling SDP CRC16 is applicable only for DP2.0, >> > >> > DP2.0 and UHBR? >> >> This is a DP2.0 feature that can be enabled on UHBR rates. >> >> > >> > >but this >> > > debug print was not within the uhbr check and was always printed. >> > > Fis this by adding proper checks and returning. >> > >> > Typo. >> > >> > > >> > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> >> > > --- >> > > .../gpu/drm/i915/display/intel_dp_link_training.c | 12 +++++++----- >> > > 1 file changed, 7 insertions(+), 5 deletions(-) >> > > >> > > 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 a263773f4d68..4485ef4f8ec6 100644 >> > > --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c >> > > +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c >> > > @@ -1390,11 +1390,13 @@ void intel_dp_128b132b_sdp_crc16(struct >> > > intel_dp *intel_dp, >> > > * Default value of bit 31 is '0' hence discarding the write >> > > * TODO: Corrective actions on SDP corruption yet to be defined >> > > */ >> > > - if (intel_dp_is_uhbr(crtc_state)) >> > > - /* DP v2.0 SCR on SDP CRC16 for 128b/132b Link Layer */ >> > > - drm_dp_dpcd_writeb(&intel_dp->aux, >> > > - >> > > DP_SDP_ERROR_DETECTION_CONFIGURATION, >> > > - DP_SDP_CRC16_128B132B_EN); >> > > + if (!intel_dp_is_uhbr(crtc_state)) >> > > + return; >> > >> > I see that while calling this function in intel_ddi_pre_enable_dp(), >> > we do have a check for for DP2.0 >> > >> > if (HAS_DP20(dev_priv)) >> > intel_dp_128b132b_sdp_crc16(enc_to_intel_dp(encoder), >> > crtc_state); >> > >> > Should this check be added within the function too for the sake of >> > completion? >> > >> >> HAS DP20 just checked for the display version number and not UHBR rates. >> We need to check for UHBR rates and then enable this CRC. >> > > I was alluding more to the fact that there are two conditions for enabling the CRC. > > if (!HAS_DP20(dev_priv) || !intel_dp_is_uhbr(crtc_state)) > return; > > But if it is implicit that UHBR will only be supported on DP2.0 or/and this function is not > expected to be used anywhere else (and hence without any possibility of this function being > called without the HAS_DP20() check), the change looks good to me. HAS_DP20() should be used as little as possible, basically for platform DP 2.0 support only. In most cases it's UHBR we want to check, and that implies DP 2.0 and 128b/132b. This function is also fine to be called *without* the HAS_DP20() check, and IMO that should just be removed. The crtc state simply can't have a UHBR rate unless HAS_DP20() is true. BR, Jani. > > Reviewed-by: Chaitanya Kumar Borah <chaitanya.kumar.borah@intel.com> > > Regards > > Chaitanya > > >> Thanks and Regards, >> Arun R Murthy >> ------------------- >> >> > Regards >> > >> > Chaitanya >> > >> > > + >> > > + /* DP v2.0 SCR on SDP CRC16 for 128b/132b Link Layer */ >> > > + drm_dp_dpcd_writeb(&intel_dp->aux, >> > > + DP_SDP_ERROR_DETECTION_CONFIGURATION, >> > > + DP_SDP_CRC16_128B132B_EN); >> > > >> > > lt_dbg(intel_dp, DP_PHY_DPRX, "DP2.0 SDP CRC16 for 128b/132b >> > > enabled\n"); } >> > > -- >> > > 2.25.1 >
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 a263773f4d68..4485ef4f8ec6 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c @@ -1390,11 +1390,13 @@ void intel_dp_128b132b_sdp_crc16(struct intel_dp *intel_dp, * Default value of bit 31 is '0' hence discarding the write * TODO: Corrective actions on SDP corruption yet to be defined */ - if (intel_dp_is_uhbr(crtc_state)) - /* DP v2.0 SCR on SDP CRC16 for 128b/132b Link Layer */ - drm_dp_dpcd_writeb(&intel_dp->aux, - DP_SDP_ERROR_DETECTION_CONFIGURATION, - DP_SDP_CRC16_128B132B_EN); + if (!intel_dp_is_uhbr(crtc_state)) + return; + + /* DP v2.0 SCR on SDP CRC16 for 128b/132b Link Layer */ + drm_dp_dpcd_writeb(&intel_dp->aux, + DP_SDP_ERROR_DETECTION_CONFIGURATION, + DP_SDP_CRC16_128B132B_EN); lt_dbg(intel_dp, DP_PHY_DPRX, "DP2.0 SDP CRC16 for 128b/132b enabled\n"); }
The debug print for enabling SDP CRC16 is applicable only for DP2.0, but this debug print was not within the uhbr check and was always printed. Fis this by adding proper checks and returning. Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> --- .../gpu/drm/i915/display/intel_dp_link_training.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)