diff mbox series

[v11,09/28] drm/display: hdmi: Add HDMI compute clock helper

Message ID 20240326-kms-hdmi-connector-state-v11-9-c5680ffcf261@kernel.org (mailing list archive)
State New
Headers show
Series drm/connector: Create HDMI Connector infrastructure | expand

Commit Message

Maxime Ripard March 26, 2024, 3:40 p.m. UTC 
A lot of HDMI drivers have some variation of the formula to calculate
the TMDS character rate from a mode, but few of them actually take all
parameters into account.

Let's create a helper to provide that rate taking all parameters into
account.

Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/gpu/drm/display/drm_hdmi_helper.c | 70 +++++++++++++++++++++++++++++++
 include/drm/display/drm_hdmi_helper.h     |  4 ++
 2 files changed, 74 insertions(+)

Comments

Ville Syrjälä April 16, 2024, 1:44 p.m. UTC | #1 
On Tue, Mar 26, 2024 at 04:40:13PM +0100, Maxime Ripard wrote:
> A lot of HDMI drivers have some variation of the formula to calculate
> the TMDS character rate from a mode, but few of them actually take all
> parameters into account.
> 
> Let's create a helper to provide that rate taking all parameters into
> account.
> 
> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
>  drivers/gpu/drm/display/drm_hdmi_helper.c | 70 +++++++++++++++++++++++++++++++
>  include/drm/display/drm_hdmi_helper.h     |  4 ++
>  2 files changed, 74 insertions(+)
> 
> diff --git a/drivers/gpu/drm/display/drm_hdmi_helper.c b/drivers/gpu/drm/display/drm_hdmi_helper.c
> index faf5e9efa7d3..2518dd1a07e7 100644
> --- a/drivers/gpu/drm/display/drm_hdmi_helper.c
> +++ b/drivers/gpu/drm/display/drm_hdmi_helper.c
> @@ -193,5 +193,75 @@ void drm_hdmi_avi_infoframe_content_type(struct hdmi_avi_infoframe *frame,
>  	}
>  
>  	frame->itc = conn_state->content_type != DRM_MODE_CONTENT_TYPE_NO_DATA;
>  }
>  EXPORT_SYMBOL(drm_hdmi_avi_infoframe_content_type);
> +
> +/**
> + * drm_hdmi_compute_mode_clock() - Computes the TMDS Character Rate
> + * @mode: Display mode to compute the clock for
> + * @bpc: Bits per character
> + * @fmt: Output Pixel Format used
> + *
> + * Returns the TMDS Character Rate for a given mode, bpc count and output format.
> + *
> + * RETURNS:
> + * The TMDS Character Rate, in Hertz, or 0 on error.

Everything generally uses kHz. Sticking to common units
would be better.

> + */
> +unsigned long long
> +drm_hdmi_compute_mode_clock(const struct drm_display_mode *mode,
> +			    unsigned int bpc, enum hdmi_colorspace fmt)
> +{
> +	unsigned long long clock = mode->clock * 1000ULL;
> +	unsigned int vic = drm_match_cea_mode(mode);
> +
> +	/*
> +	 * CTA-861-G Spec, section 5.4 - Color Coding and Quantization
> +	 * mandates that VIC 1 always uses 8 bpc.
> +	 */
> +	if (vic == 1 && bpc != 8)
> +		return 0;
> +
> +	/*
> +	 * HDMI 2.0 Spec, section 7.1 - YCbCr 4:2:0 Pixel Encoding
> +	 * specifies that YUV420 encoding is only available for those
> +	 * VICs.
> +	 */
> +	if (fmt == HDMI_COLORSPACE_YUV420 &&
> +	    !(vic == 96 || vic == 97 || vic == 101 ||
> +	      vic == 102 || vic == 106 || vic == 107))
> +		return 0;

I believe that is already outdated. I would just rip this out since the 
sink is anyway required to declare for which timings it will support
4:2:0 via the Y420CMDB/VDB data blocks (see
drm_mode_is_420_{only,also}().

> +
> +	if (fmt == HDMI_COLORSPACE_YUV422) {
> +		/*
> +		 * HDMI 1.4b Spec, section 6.2.3 - Pixel Encoding Requirements
> +		 * specifies that YUV422 is 36-bit only.
> +		 */
> +		if (bpc != 12)
> +			return 0;
> +
> +		/*
> +		 * HDMI 1.0 Spec, section 6.5 - Pixel Encoding
> +		 * specifies that YUV422 requires two 12-bits components per
> +		 * pixel clock, which is equivalent in our calculation to three
> +		 * 8-bits components
> +		 */
> +		bpc = 8;
> +	}
> +
> +	/*
> +	 * HDMI 2.0 Spec, Section 7.1 - YCbCr 4:2:0 Pixel Encoding
> +	 * specifies that YUV420 encoding is carried at a TMDS Character Rate
> +	 * equal to half the pixel clock rate.
> +	 */
> +	if (fmt == HDMI_COLORSPACE_YUV420)
> +		clock = clock / 2;
> +
> +	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
> +		clock = clock * 2;
> +
> +	clock = clock * bpc;
> +	do_div(clock, 8);

IMO one shouldn't use bare do_div(). There are
more sensible wrappers for it.

In this case I would use DIV_ROUND_CLOSEST_ULL().

Although the 64bit math is not even required if you
just stick to kHz like everyone else.

> +
> +	return clock;
> +}
> +EXPORT_SYMBOL(drm_hdmi_compute_mode_clock);
> diff --git a/include/drm/display/drm_hdmi_helper.h b/include/drm/display/drm_hdmi_helper.h
> index 76d234826e22..57e3b18c15ec 100644
> --- a/include/drm/display/drm_hdmi_helper.h
> +++ b/include/drm/display/drm_hdmi_helper.h
> @@ -22,6 +22,10 @@ drm_hdmi_infoframe_set_hdr_metadata(struct hdmi_drm_infoframe *frame,
>  				    const struct drm_connector_state *conn_state);
>  
>  void drm_hdmi_avi_infoframe_content_type(struct hdmi_avi_infoframe *frame,
>  					 const struct drm_connector_state *conn_state);
>  
> +unsigned long long
> +drm_hdmi_compute_mode_clock(const struct drm_display_mode *mode,
> +			    unsigned int bpc, enum hdmi_colorspace fmt);
> +
>  #endif
> 
> -- 
> 2.44.0
Maxime Ripard April 18, 2024, 4:33 p.m. UTC | #2 
On Tue, Apr 16, 2024 at 04:44:14PM +0300, Ville Syrjälä wrote:
> On Tue, Mar 26, 2024 at 04:40:13PM +0100, Maxime Ripard wrote:
> > A lot of HDMI drivers have some variation of the formula to calculate
> > the TMDS character rate from a mode, but few of them actually take all
> > parameters into account.
> > 
> > Let's create a helper to provide that rate taking all parameters into
> > account.
> > 
> > Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > Signed-off-by: Maxime Ripard <mripard@kernel.org>
> > ---
> >  drivers/gpu/drm/display/drm_hdmi_helper.c | 70 +++++++++++++++++++++++++++++++
> >  include/drm/display/drm_hdmi_helper.h     |  4 ++
> >  2 files changed, 74 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/display/drm_hdmi_helper.c b/drivers/gpu/drm/display/drm_hdmi_helper.c
> > index faf5e9efa7d3..2518dd1a07e7 100644
> > --- a/drivers/gpu/drm/display/drm_hdmi_helper.c
> > +++ b/drivers/gpu/drm/display/drm_hdmi_helper.c
> > @@ -193,5 +193,75 @@ void drm_hdmi_avi_infoframe_content_type(struct hdmi_avi_infoframe *frame,
> >  	}
> >  
> >  	frame->itc = conn_state->content_type != DRM_MODE_CONTENT_TYPE_NO_DATA;
> >  }
> >  EXPORT_SYMBOL(drm_hdmi_avi_infoframe_content_type);
> > +
> > +/**
> > + * drm_hdmi_compute_mode_clock() - Computes the TMDS Character Rate
> > + * @mode: Display mode to compute the clock for
> > + * @bpc: Bits per character
> > + * @fmt: Output Pixel Format used
> > + *
> > + * Returns the TMDS Character Rate for a given mode, bpc count and output format.
> > + *
> > + * RETURNS:
> > + * The TMDS Character Rate, in Hertz, or 0 on error.
> 
> Everything generally uses kHz. Sticking to common units
> would be better.

Not everything, no. The clock framework is using Hz for example, and on
drm-misc drivers it's usually going to be the consumer of that field.

And there's almost 200 hits on mode->clock * 1000 in drivers/gpu/drm as
of today, including some in i915. This is a bit less than a third of all
the mode->clock usage, including the one that are unit-neutral (like
comparisons between two mode->clock fields).

Given how the rest of the DRM code is structured, yes, there's going to
be some impedance mismatch, but it's really not as clear cut as you make
it to be.

> > + */
> > +unsigned long long
> > +drm_hdmi_compute_mode_clock(const struct drm_display_mode *mode,
> > +			    unsigned int bpc, enum hdmi_colorspace fmt)
> > +{
> > +	unsigned long long clock = mode->clock * 1000ULL;
> > +	unsigned int vic = drm_match_cea_mode(mode);
> > +
> > +	/*
> > +	 * CTA-861-G Spec, section 5.4 - Color Coding and Quantization
> > +	 * mandates that VIC 1 always uses 8 bpc.
> > +	 */
> > +	if (vic == 1 && bpc != 8)
> > +		return 0;
> > +
> > +	/*
> > +	 * HDMI 2.0 Spec, section 7.1 - YCbCr 4:2:0 Pixel Encoding
> > +	 * specifies that YUV420 encoding is only available for those
> > +	 * VICs.
> > +	 */
> > +	if (fmt == HDMI_COLORSPACE_YUV420 &&
> > +	    !(vic == 96 || vic == 97 || vic == 101 ||
> > +	      vic == 102 || vic == 106 || vic == 107))
> > +		return 0;
> 
> I believe that is already outdated. I would just rip this out since the 
> sink is anyway required to declare for which timings it will support
> 4:2:0 via the Y420CMDB/VDB data blocks (see
> drm_mode_is_420_{only,also}().

Should we use drm_mode_is_420() then or rip it out entirely?

> > +
> > +	if (fmt == HDMI_COLORSPACE_YUV422) {
> > +		/*
> > +		 * HDMI 1.4b Spec, section 6.2.3 - Pixel Encoding Requirements
> > +		 * specifies that YUV422 is 36-bit only.
> > +		 */
> > +		if (bpc != 12)
> > +			return 0;
> > +
> > +		/*
> > +		 * HDMI 1.0 Spec, section 6.5 - Pixel Encoding
> > +		 * specifies that YUV422 requires two 12-bits components per
> > +		 * pixel clock, which is equivalent in our calculation to three
> > +		 * 8-bits components
> > +		 */
> > +		bpc = 8;
> > +	}
> > +
> > +	/*
> > +	 * HDMI 2.0 Spec, Section 7.1 - YCbCr 4:2:0 Pixel Encoding
> > +	 * specifies that YUV420 encoding is carried at a TMDS Character Rate
> > +	 * equal to half the pixel clock rate.
> > +	 */
> > +	if (fmt == HDMI_COLORSPACE_YUV420)
> > +		clock = clock / 2;
> > +
> > +	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
> > +		clock = clock * 2;
> > +
> > +	clock = clock * bpc;
> > +	do_div(clock, 8);
> 
> IMO one shouldn't use bare do_div(). There are
> more sensible wrappers for it.
> 
> In this case I would use DIV_ROUND_CLOSEST_ULL().

Ack.

Thanks!
Maxime
diff mbox series

Patch

diff --git a/drivers/gpu/drm/display/drm_hdmi_helper.c b/drivers/gpu/drm/display/drm_hdmi_helper.c
index faf5e9efa7d3..2518dd1a07e7 100644
--- a/drivers/gpu/drm/display/drm_hdmi_helper.c
+++ b/drivers/gpu/drm/display/drm_hdmi_helper.c
@@ -193,5 +193,75 @@  void drm_hdmi_avi_infoframe_content_type(struct hdmi_avi_infoframe *frame,
 	}
 
 	frame->itc = conn_state->content_type != DRM_MODE_CONTENT_TYPE_NO_DATA;
 }
 EXPORT_SYMBOL(drm_hdmi_avi_infoframe_content_type);
+
+/**
+ * drm_hdmi_compute_mode_clock() - Computes the TMDS Character Rate
+ * @mode: Display mode to compute the clock for
+ * @bpc: Bits per character
+ * @fmt: Output Pixel Format used
+ *
+ * Returns the TMDS Character Rate for a given mode, bpc count and output format.
+ *
+ * RETURNS:
+ * The TMDS Character Rate, in Hertz, or 0 on error.
+ */
+unsigned long long
+drm_hdmi_compute_mode_clock(const struct drm_display_mode *mode,
+			    unsigned int bpc, enum hdmi_colorspace fmt)
+{
+	unsigned long long clock = mode->clock * 1000ULL;
+	unsigned int vic = drm_match_cea_mode(mode);
+
+	/*
+	 * CTA-861-G Spec, section 5.4 - Color Coding and Quantization
+	 * mandates that VIC 1 always uses 8 bpc.
+	 */
+	if (vic == 1 && bpc != 8)
+		return 0;
+
+	/*
+	 * HDMI 2.0 Spec, section 7.1 - YCbCr 4:2:0 Pixel Encoding
+	 * specifies that YUV420 encoding is only available for those
+	 * VICs.
+	 */
+	if (fmt == HDMI_COLORSPACE_YUV420 &&
+	    !(vic == 96 || vic == 97 || vic == 101 ||
+	      vic == 102 || vic == 106 || vic == 107))
+		return 0;
+
+	if (fmt == HDMI_COLORSPACE_YUV422) {
+		/*
+		 * HDMI 1.4b Spec, section 6.2.3 - Pixel Encoding Requirements
+		 * specifies that YUV422 is 36-bit only.
+		 */
+		if (bpc != 12)
+			return 0;
+
+		/*
+		 * HDMI 1.0 Spec, section 6.5 - Pixel Encoding
+		 * specifies that YUV422 requires two 12-bits components per
+		 * pixel clock, which is equivalent in our calculation to three
+		 * 8-bits components
+		 */
+		bpc = 8;
+	}
+
+	/*
+	 * HDMI 2.0 Spec, Section 7.1 - YCbCr 4:2:0 Pixel Encoding
+	 * specifies that YUV420 encoding is carried at a TMDS Character Rate
+	 * equal to half the pixel clock rate.
+	 */
+	if (fmt == HDMI_COLORSPACE_YUV420)
+		clock = clock / 2;
+
+	if (mode->flags & DRM_MODE_FLAG_DBLCLK)
+		clock = clock * 2;
+
+	clock = clock * bpc;
+	do_div(clock, 8);
+
+	return clock;
+}
+EXPORT_SYMBOL(drm_hdmi_compute_mode_clock);
diff --git a/include/drm/display/drm_hdmi_helper.h b/include/drm/display/drm_hdmi_helper.h
index 76d234826e22..57e3b18c15ec 100644
--- a/include/drm/display/drm_hdmi_helper.h
+++ b/include/drm/display/drm_hdmi_helper.h
@@ -22,6 +22,10 @@  drm_hdmi_infoframe_set_hdr_metadata(struct hdmi_drm_infoframe *frame,
 				    const struct drm_connector_state *conn_state);
 
 void drm_hdmi_avi_infoframe_content_type(struct hdmi_avi_infoframe *frame,
 					 const struct drm_connector_state *conn_state);
 
+unsigned long long
+drm_hdmi_compute_mode_clock(const struct drm_display_mode *mode,
+			    unsigned int bpc, enum hdmi_colorspace fmt);
+
 #endif