diff mbox series

[v2,07/11] drm/dp: Add drm_dp_uhbr_channel_coding_supported()

Message ID 20240416221010.376865-8-imre.deak@intel.com (mailing list archive)
State New, archived
Headers show
Series [v2,01/11] drm/i915/dp: Fix DSC line buffer depth programming | expand

Commit Message

Imre Deak April 16, 2024, 10:10 p.m. UTC 
Factor out a function to check for UHBR channel coding support used by a
follow-up patch in the patchset.

Cc: dri-devel@lists.freedesktop.org
Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
Reviewed-by: Manasi Navare <navaremanasi@chromium.org>
Acked-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dp.c | 2 +-
 include/drm/display/drm_dp_helper.h     | 6 ++++++
 2 files changed, 7 insertions(+), 1 deletion(-)

Comments

Jani Nikula April 17, 2024, 9:21 a.m. UTC | #1 
On Wed, 17 Apr 2024, Imre Deak <imre.deak@intel.com> wrote:
> Factor out a function to check for UHBR channel coding support used by a
> follow-up patch in the patchset.
>
> Cc: dri-devel@lists.freedesktop.org
> Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> Reviewed-by: Manasi Navare <navaremanasi@chromium.org>
> Acked-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  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])
>  {
Imre Deak April 17, 2024, 11:49 a.m. UTC | #2 
On Wed, Apr 17, 2024 at 12:21:58PM +0300, Jani Nikula wrote:
> On Wed, 17 Apr 2024, Imre Deak <imre.deak@intel.com> wrote:
> > Factor out a function to check for UHBR channel coding support used by a
> > follow-up patch in the patchset.
> >
> > Cc: dri-devel@lists.freedesktop.org
> > Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> > Reviewed-by: Manasi Navare <navaremanasi@chromium.org>
> > Acked-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  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?

Ok, makes sense, will rename it to drm_dp_128b132b_supported() and
can do the same - as a follow-up - for 8b10b.

> BR,
> Jani.
> 
> 
> 
> > +
> >  static inline bool
> >  drm_dp_alternate_scrambler_reset_cap(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
> >  {
> 
> -- 
> Jani Nikula, Intel
diff mbox series

Patch

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;
+}
+
 static inline bool
 drm_dp_alternate_scrambler_reset_cap(const u8 dpcd[DP_RECEIVER_CAP_SIZE])
 {