by Ville Syrjälä

[permalink] [raw]

Subject: Re: [PATCH v11 09/28] drm/display: hdmi: Add HDMI compute clock helper

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 <[email protected]>
> Signed-off-by: Maxime Ripard <[email protected]>
> ---
> 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

--
Ville Syrj?l?
Intel

2024-04-18 16:35:26

by Maxime Ripard

[permalink] [raw]

Subject: Re: [PATCH v11 09/28] drm/display: hdmi: Add HDMI compute clock helper

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 <[email protected]>
> > Signed-off-by: Maxime Ripard <[email protected]>
> > ---
> > 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

Attachments:

(No filename) (4.44 kB)
signature.asc (281.00 B)
Download all attachments

2024-04-19 11:37:00

by Maxime Ripard

[permalink] [raw]

Subject: Re: [PATCH v11 17/28] drm/connector: hdmi: Add Broadcast RGB property

On Tue, Apr 16, 2024 at 05:00:26PM +0300, Ville Syrj?l? wrote:
> On Tue, Mar 26, 2024 at 04:40:21PM +0100, Maxime Ripard wrote:
> > The i915 driver has a property to force the RGB range of an HDMI output.
> > The vc4 driver then implemented the same property with the same
> > semantics. KWin has support for it, and a PR for mutter is also there to
> > support it.
>
> Is there a i915 patch to switch over to hdmi.broadcast_rgb? Though
> the "hdmi" name is perhaps not the best idea given this is also a
> thing for DP.

No, there's none yet. I can try to cook one as a follow-up, but I have
no way to test it

Maxime

Attachments:

(No filename) (635.00 B)
signature.asc (281.00 B)
Download all attachments