On Wed, 17 Apr 2024, Imre Deak <imre.deak@xxxxxxxxx> wrote:
> Factor out a function to check for UHBR channel coding support used by a
> follow-up patch in the patchset.
>
> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
> Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@xxxxxxxxx>
> Reviewed-by: Manasi Navare <navaremanasi@xxxxxxxxxxxx>
> Acked-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/display/intel_dp.c | 2 +-
> include/drm/display/drm_dp_helper.h | 6 ++++++
> 2 files changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 23808e9d41d5d..41127069b55e4 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -225,7 +225,7 @@ static void intel_dp_set_dpcd_sink_rates(struct intel_dp *intel_dp)
> * Sink rates for 128b/132b. If set, sink should support all 8b/10b
> * rates and 10 Gbps.
> */
> - if (intel_dp->dpcd[DP_MAIN_LINK_CHANNEL_CODING] & DP_CAP_ANSI_128B132B) {
> + if (drm_dp_uhbr_channel_coding_supported(intel_dp->dpcd)) {
> u8 uhbr_rates = 0;
>
> BUILD_BUG_ON(ARRAY_SIZE(intel_dp->sink_rates) < ARRAY_SIZE(dp_rates) + 3);
> diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h
> index baf9949ff96fc..8a64fe8d97af2 100644
> --- a/include/drm/display/drm_dp_helper.h
> +++ b/include/drm/display/drm_dp_helper.h
> @@ -251,6 +251,12 @@ drm_dp_channel_coding_supported(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
> return dpcd[DP_MAIN_LINK_CHANNEL_CODING] & DP_CAP_ANSI_8B10B;
> }
>
> +static inline bool
> +drm_dp_uhbr_channel_coding_supported(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
> +{
> + return dpcd[DP_MAIN_LINK_CHANNEL_CODING] & DP_CAP_ANSI_128B132B;
> +}
Nitpick, "uhbr channel coding" is not pedantically correct, and it does
rub me the wrong way.
Yes, using 128b/132b channel coding implies UHBR, and UHBR requires
128b/132b channel coding, but they are not the same thing. We do
conflate the two quite a bit in the code, checking for UHBR when we
really mean 128b/132b, but embedding this confusion in the function name
directly is a bit much.
I've named the link training functions drm_dp_128b132b_* in the same
file, and I think this one should be named similarly. Maybe just
drm_dp_128b132b_supported(), and rename
drm_dp_channel_coding_supported() to drm_dp_8b10b_supported() to unify?
BR,
Jani.
> +
> static inline bool
> drm_dp_alternate_scrambler_reset_cap(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
> {
--
Jani Nikula, Intel
