Re: [Intel-gfx] [PATCH] drm/i915/dp: Fix LT debug print in SDP CRC enable

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

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
>

-- 
Jani Nikula, Intel Open Source Graphics Center