From: Maxime Ripard <mripard@kernel.org>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: "Maarten Lankhorst" <maarten.lankhorst@linux.intel.com>,
"Thomas Zimmermann" <tzimmermann@suse.de>,
"David Airlie" <airlied@gmail.com>,
"Daniel Vetter" <daniel@ffwll.ch>,
"Jonathan Corbet" <corbet@lwn.net>,
"Sandy Huang" <hjc@rock-chips.com>,
"Heiko Stübner" <heiko@sntech.de>, "Chen-Yu Tsai" <wens@csie.org>,
"Jernej Skrabec" <jernej.skrabec@gmail.com>,
"Samuel Holland" <samuel@sholland.org>,
"Hans Verkuil" <hverkuil@xs4all.nl>,
"Sebastian Wick" <sebastian.wick@redhat.com>,
dri-devel@lists.freedesktop.org,
linux-arm-kernel@lists.infradead.org, linux-doc@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-media@vger.kernel.org,
linux-rockchip@lists.infradead.org, linux-sunxi@lists.linux.dev,
"Dave Stevenson" <dave.stevenson@raspberrypi.com>
Subject: Re: [PATCH v11 09/28] drm/display: hdmi: Add HDMI compute clock helper
Date: Thu, 18 Apr 2024 18:33:54 +0200 [thread overview]
Message-ID: <20240418-spiritual-loyal-hornet-2fbbfd@houat> (raw)
In-Reply-To: <Zh6Ars8z1ESz-LQO@intel.com>
[-- Attachment #1: Type: text/plain, Size: 4547 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]
next prev parent reply other threads:[~2024-04-18 16:33 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-26 15:40 [PATCH v11 00/28] drm/connector: Create HDMI Connector infrastructure Maxime Ripard
2024-03-26 15:40 ` [PATCH v11 01/28] drm/connector: Introduce an HDMI connector initialization function Maxime Ripard
2024-03-26 15:40 ` [PATCH v11 02/28] drm/mode_object: Export drm_mode_obj_find_prop_id for tests Maxime Ripard
2024-03-26 15:40 ` [PATCH v11 03/28] drm/tests: connector: Add tests for drmm_connector_hdmi_init Maxime Ripard
2024-03-26 15:40 ` [PATCH v11 04/28] drm/connector: hdmi: Create an HDMI sub-state Maxime Ripard
2024-03-26 15:40 ` [PATCH v11 05/28] drm/connector: hdmi: Add output BPC to the connector state Maxime Ripard
2024-03-26 15:40 ` [PATCH v11 06/28] drm/tests: Add output bpc tests Maxime Ripard
2024-03-26 15:40 ` [PATCH v11 07/28] drm/connector: hdmi: Add support for output format Maxime Ripard
2024-03-26 15:40 ` [PATCH v11 08/28] drm/tests: Add output formats tests Maxime Ripard
2024-03-26 15:40 ` [PATCH v11 09/28] drm/display: hdmi: Add HDMI compute clock helper Maxime Ripard
2024-04-16 13:44 ` Ville Syrjälä
2024-04-18 16:33 ` Maxime Ripard [this message]
2024-03-26 15:40 ` [PATCH v11 10/28] drm/tests: Add HDMI TDMS character rate tests Maxime Ripard
2024-03-26 15:40 ` [PATCH v11 11/28] drm/connector: hdmi: Calculate TMDS character rate Maxime Ripard
2024-03-26 15:40 ` [PATCH v11 12/28] drm/tests: Add TDMS character rate connector state tests Maxime Ripard
2024-03-26 15:40 ` [PATCH v11 13/28] drm/connector: hdmi: Add custom hook to filter TMDS character rate Maxime Ripard
2024-03-26 15:40 ` [PATCH v11 14/28] drm/tests: Add HDMI connector rate filter hook tests Maxime Ripard
2024-03-26 15:40 ` [PATCH v11 15/28] drm/connector: hdmi: Compute bpc and format automatically Maxime Ripard
2024-04-16 13:50 ` Ville Syrjälä
2024-04-19 13:01 ` Maxime Ripard
2024-03-26 15:40 ` [PATCH v11 16/28] drm/tests: Add HDMI connector bpc and format tests Maxime Ripard
2024-03-26 15:40 ` [PATCH v11 17/28] drm/connector: hdmi: Add Broadcast RGB property Maxime Ripard
2024-04-16 14:00 ` Ville Syrjälä
2024-04-19 11:36 ` Maxime Ripard
2024-03-26 15:40 ` [PATCH v11 18/28] drm/tests: Add tests for " Maxime Ripard
2024-03-26 15:40 ` [PATCH v11 19/28] drm/connector: hdmi: Add RGB Quantization Range to the connector state Maxime Ripard
2024-04-16 14:09 ` Ville Syrjälä
2024-03-26 15:40 ` [PATCH v11 20/28] drm/tests: Add RGB Quantization tests Maxime Ripard
2024-03-26 15:40 ` [PATCH v11 21/28] drm/connector: hdmi: Add Infoframes generation Maxime Ripard
2024-03-26 15:40 ` [PATCH v11 22/28] drm/tests: Add infoframes test Maxime Ripard
2024-03-26 15:40 ` [PATCH v11 23/28] drm/connector: hdmi: Create Infoframe DebugFS entries Maxime Ripard
2024-03-26 15:40 ` [PATCH v11 24/28] drm/vc4: hdmi: Switch to HDMI connector Maxime Ripard
2024-03-26 15:40 ` [PATCH v11 25/28] drm/vc4: tests: Remove vc4_dummy_plane structure Maxime Ripard
2024-03-26 15:40 ` [PATCH v11 26/28] drm/vc4: tests: Convert to plane creation helper Maxime Ripard
2024-03-26 15:40 ` [PATCH v11 27/28] drm/rockchip: inno_hdmi: Switch to HDMI connector Maxime Ripard
2024-03-26 15:40 ` [PATCH v11 28/28] drm/sun4i: hdmi: " Maxime Ripard
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240418-spiritual-loyal-hornet-2fbbfd@houat \
--to=mripard@kernel.org \
--cc=airlied@gmail.com \
--cc=corbet@lwn.net \
--cc=daniel@ffwll.ch \
--cc=dave.stevenson@raspberrypi.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=heiko@sntech.de \
--cc=hjc@rock-chips.com \
--cc=hverkuil@xs4all.nl \
--cc=jernej.skrabec@gmail.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-doc@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=linux-rockchip@lists.infradead.org \
--cc=linux-sunxi@lists.linux.dev \
--cc=maarten.lankhorst@linux.intel.com \
--cc=samuel@sholland.org \
--cc=sebastian.wick@redhat.com \
--cc=tzimmermann@suse.de \
--cc=ville.syrjala@linux.intel.com \
--cc=wens@csie.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).