From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Jernej Skrabec <jernej.skrabec@siol.net>
Cc: a.hajda@samsung.com, narmstrong@baylibre.com, jonas@kwiboo.se,
airlied@linux.ie, daniel@ffwll.ch,
dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 4/4] drm/bridge: dw-hdmi: Add support for RGB limited range
Date: Mon, 2 Mar 2020 11:53:54 +0200 [thread overview]
Message-ID: <20200302095354.GG11960@pendragon.ideasonboard.com> (raw)
In-Reply-To: <20200229163043.158262-5-jernej.skrabec@siol.net>
Hi Jernej,
Thank you for the patch.
On Sat, Feb 29, 2020 at 05:30:43PM +0100, Jernej Skrabec wrote:
> CEA 861 standard request that RGB quantization range is "limited" for
s/request/requests/
> CEA modes. Support that by adding CSC matrix which downscales values.
Interesting, that's related to what I asked in the review of 2/4 :-)
> This allows to proper color reproduction on TV and PC monitor at the
> same time. In future, override property can be added, like "Broadcast
> RGB" in i915 driver.
>
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> ---
> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 36 +++++++++++++++++++++--
> 1 file changed, 34 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index 3d6021119942..101c90156fa0 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -92,6 +92,12 @@ static const u16 csc_coeff_rgb_in_eitu709[3][4] = {
> { 0x6756, 0x78ab, 0x2000, 0x0200 }
> };
>
> +static const u16 csc_coeff_rgb_limited[3][4] = {
Could you name this to make it explicit that we're converting from RGB
to RGB ? Maybe csc_coeff_rgb_full_to_rgb_limited ?
> + { 0x1B7C, 0x0000, 0x0000, 0x0020 },
> + { 0x0000, 0x1B7C, 0x0000, 0x0020 },
> + { 0x0000, 0x0000, 0x1B7C, 0x0020 }
Lowercase hex constants please.
> +};
> +
> struct hdmi_vmode {
> bool mdataenablepolarity;
>
> @@ -109,6 +115,7 @@ struct hdmi_data_info {
> unsigned int pix_repet_factor;
> unsigned int hdcp_enable;
> struct hdmi_vmode video_mode;
> + bool rgb_limited_range;
> };
>
> struct dw_hdmi_i2c {
> @@ -960,6 +967,13 @@ static int is_color_space_conversion(struct dw_hdmi *hdmi)
> hdmi_bus_fmt_is_rgb(hdmi->hdmi_data.enc_out_bus_format);
> }
>
> +static int is_rgb_downscale_needed(struct dw_hdmi *hdmi)
> +{
> + return hdmi_bus_fmt_is_rgb(hdmi->hdmi_data.enc_in_bus_format) &&
> + hdmi_bus_fmt_is_rgb(hdmi->hdmi_data.enc_out_bus_format) &&
> + hdmi->hdmi_data.rgb_limited_range;
> +}
> +
> static int is_color_space_decimation(struct dw_hdmi *hdmi)
> {
> if (!hdmi_bus_fmt_is_yuv422(hdmi->hdmi_data.enc_out_bus_format))
> @@ -1006,6 +1020,8 @@ static void dw_hdmi_update_csc_coeffs(struct dw_hdmi *hdmi)
> csc_coeff = &csc_coeff_rgb_in_eitu709;
> csc_scale = 0;
> }
> + } else if (is_rgb_downscale_needed(hdmi)) {
> + csc_coeff = &csc_coeff_rgb_limited;
> }
>
> /* The CSC registers are sequential, alternating MSB then LSB */
> @@ -1615,6 +1631,18 @@ static void hdmi_config_AVI(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
> drm_hdmi_avi_infoframe_from_display_mode(&frame,
> &hdmi->connector, mode);
>
> + if (hdmi_bus_fmt_is_rgb(hdmi->hdmi_data.enc_out_bus_format)) {
> + drm_hdmi_avi_infoframe_quant_range(&frame, &hdmi->connector,
> + mode,
> + hdmi->hdmi_data.rgb_limited_range ?
> + HDMI_QUANTIZATION_RANGE_LIMITED :
> + HDMI_QUANTIZATION_RANGE_FULL);
> + } else {
> + frame.quantization_range = HDMI_QUANTIZATION_RANGE_DEFAULT;
> + frame.ycc_quantization_range =
> + HDMI_YCC_QUANTIZATION_RANGE_LIMITED;
> + }
> +
> if (hdmi_bus_fmt_is_yuv444(hdmi->hdmi_data.enc_out_bus_format))
> frame.colorspace = HDMI_COLORSPACE_YUV444;
> else if (hdmi_bus_fmt_is_yuv422(hdmi->hdmi_data.enc_out_bus_format))
> @@ -1990,13 +2018,13 @@ static void dw_hdmi_enable_video_path(struct dw_hdmi *hdmi)
> hdmi_writeb(hdmi, hdmi->mc_clkdis, HDMI_MC_CLKDIS);
>
> /* Enable csc path */
> - if (is_color_space_conversion(hdmi)) {
> + if (is_color_space_conversion(hdmi) || is_rgb_downscale_needed(hdmi)) {
I would fold this change in is_color_space_conversion(), and modify
dw_hdmi_update_csc_coeffs() accordingly with something like
if (!hdmi_bus_fmt_is_rgb(hdmi->hdmi_data.enc_in_bus_format) &&
hdmi_bus_fmt_is_rgb(hdmi->hdmi_data.enc_out_bus_format)) {
if (hdmi->hdmi_data.enc_out_encoding == V4L2_YCBCR_ENC_601)
csc_coeff = &csc_coeff_rgb_out_eitu601;
else
csc_coeff = &csc_coeff_rgb_out_eitu709;
} else if (hdmi_bus_fmt_is_rgb(hdmi->hdmi_data.enc_in_bus_format) &&
!hdmi_bus_fmt_is_rgb(hdmi->hdmi_data.enc_out_bus_format)) {
if (hdmi->hdmi_data.enc_out_encoding == V4L2_YCBCR_ENC_601)
csc_coeff = &csc_coeff_rgb_in_eitu601;
else
csc_coeff = &csc_coeff_rgb_in_eitu709;
csc_scale = 0;
} else if (is_rgb_downscale_needed(hdmi)) {
csc_coeff = &csc_coeff_rgb_limited;
}
> hdmi->mc_clkdis &= ~HDMI_MC_CLKDIS_CSCCLK_DISABLE;
> hdmi_writeb(hdmi, hdmi->mc_clkdis, HDMI_MC_CLKDIS);
> }
>
> /* Enable color space conversion if needed */
> - if (is_color_space_conversion(hdmi))
> + if (is_color_space_conversion(hdmi) || is_rgb_downscale_needed(hdmi))
> hdmi_writeb(hdmi, HDMI_MC_FLOWCTRL_FEED_THROUGH_OFF_CSC_IN_PATH,
> HDMI_MC_FLOWCTRL);
> else
> @@ -2100,6 +2128,10 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
> /* TOFIX: Default to RGB888 output format */
> hdmi->hdmi_data.enc_out_bus_format = MEDIA_BUS_FMT_RGB888_1X24;
>
> + hdmi->hdmi_data.rgb_limited_range = hdmi->sink_is_hdmi &&
> + drm_default_rgb_quant_range(mode) ==
> + HDMI_QUANTIZATION_RANGE_LIMITED;
> +
> hdmi->hdmi_data.pix_repet_factor = 0;
> hdmi->hdmi_data.hdcp_enable = 0;
> hdmi->hdmi_data.video_mode.mdataenablepolarity = true;
--
Regards,
Laurent Pinchart
next prev parent reply other threads:[~2020-03-02 9:54 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-02-29 16:30 [PATCH 0/4] drm/bridge: dw-hdmi: Various updates Jernej Skrabec
2020-02-29 16:30 ` [PATCH 1/4] drm/bridge: dw-hdmi: fix AVI frame colorimetry Jernej Skrabec
2020-03-02 9:21 ` Laurent Pinchart
2020-02-29 16:30 ` [PATCH 2/4] drm/bridge: dw-hdmi: Fix color space conversion detection Jernej Skrabec
2020-03-02 9:26 ` Neil Armstrong
2020-03-02 9:46 ` Jernej Škrabec
2020-03-02 16:42 ` Jernej Škrabec
2020-03-03 10:17 ` Neil Armstrong
2020-03-02 9:27 ` Laurent Pinchart
2020-03-02 12:45 ` Jernej Škrabec
2020-02-29 16:30 ` [PATCH 3/4] drm/bridge: dw-hdmi: do not force "none" scan mode Jernej Skrabec
2020-03-02 9:26 ` Neil Armstrong
2020-03-02 9:37 ` Laurent Pinchart
2020-02-29 16:30 ` [PATCH 4/4] drm/bridge: dw-hdmi: Add support for RGB limited range Jernej Skrabec
2020-03-02 9:53 ` Laurent Pinchart [this message]
2020-03-02 16:48 ` Jernej Škrabec
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=20200302095354.GG11960@pendragon.ideasonboard.com \
--to=laurent.pinchart@ideasonboard.com \
--cc=a.hajda@samsung.com \
--cc=airlied@linux.ie \
--cc=daniel@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
--cc=jernej.skrabec@siol.net \
--cc=jonas@kwiboo.se \
--cc=linux-kernel@vger.kernel.org \
--cc=narmstrong@baylibre.com \
/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 NNTP newsgroup(s).