2023-12-13 19:51:49

by Alex Bee

Subject: [PATCH 00/11] Add HDMI support for RK3128

This series aims to add support for the display controller (VOP) and the
HDMI controller block of RK3128 (which is very similar to the one found in
RK3036).

The VOP part is very simple - everything we need for HDMI support is
already there. I only needed to split the output selection registers from
RK3036.
The VOP has an IOMMU attached, but it has a serious silicon bug:
Registers can only be written, but not be read. As it's not possible to use
it with the IOMMU driver in it's current state I'm not adding it here and
we have to live with CMA for now - which works fine also. Andy, maybe you
can shed a light on whether there is any way to not have to write the
registers only and hope for the best (a.k.a rockchip,skip-mmu-read). I have
it working here locally - but nothing for upstream (yet).

The inno-hdmi driver currently gets a lot of attention [0-2] and I'm
hooking in now also. I did only the bare minimum to get a correctly colored
picture and a stable signal for RK3128. I leave the cleanups to those other
series and will come back with sound/CEC support later. It shouldn't
interfere to much, only the csc-part must not get dropped completely and
the mode-check-hooks are no longer dead.

Note: Patches are based and tested on next-20231213.

[0] https://lore.kernel.org/all/[email protected]
[1] https://lore.kernel.org/all/[email protected]
[2] https://lore.kernel.org/all/[email protected]

Alex Bee (11):
dt-bindings: display: rockchip,inno-hdmi: Document RK3128 compatible
drm/rockchip: vop: Add output selection registers for RK312x
drm/rockchip: inno_hdmi: Fix video timing
drm/rockchip: inno_hdmi: Correctly setup HDMI quantization range
drm/rockchip: inno_hdmi: Add variant support
drm/rockchip: inno_hdmi: Add RK3128 support
drm/rockchip: inno_hdmi: Add basic mode validation
drm/rockchip: inno_hdmi: Drop custom fill_modes hook
ARM: dts: rockchip: Add display subsystem for RK3128
ARM: dts rockchip: Add HDMI node for RK3128
ARM: dts: rockchip: Enable HDMI output for XPI-3128

.../display/rockchip/rockchip,inno-hdmi.yaml | 30 ++-
.../arm/boot/dts/rockchip/rk3128-xpi-3128.dts | 29 +++
arch/arm/boot/dts/rockchip/rk3128.dtsi | 60 ++++++
drivers/gpu/drm/rockchip/inno_hdmi.c | 203 +++++++++++++++---
drivers/gpu/drm/rockchip/rockchip_vop_reg.c | 13 +-
drivers/gpu/drm/rockchip/rockchip_vop_reg.h | 3 +
6 files changed, 309 insertions(+), 29 deletions(-)


base-commit: 48e8992e33abf054bcc0bb2e77b2d43bb899212e
--
2.43.0

2023-12-13 19:52:00

by Alex Bee

Subject: [PATCH 06/11] drm/rockchip: inno_hdmi: Add RK3128 support

This variant requires the phy reference clock to be enabled before the
DDC block can work and the (initial) DDC bus frequency is calculated
based on the rate of this clock.
Besides the only difference is phy configuration which is required to make
the driver working for this variant as well.

Signed-off-by: Alex Bee <[email protected]>
---
drivers/gpu/drm/rockchip/inno_hdmi.c | 46 +++++++++++++++++++++++++---
1 file changed, 41 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
index 593b184bd0ad..f7f0bec725f9 100644
--- a/drivers/gpu/drm/rockchip/inno_hdmi.c
+++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
@@ -63,6 +63,7 @@ struct inno_hdmi {

int irq;
struct clk *pclk;
+ struct clk *refclk;
void __iomem *regs;

struct drm_connector connector;
@@ -85,6 +86,12 @@ static struct inno_hdmi_phy_config rk3036_hdmi_phy_configs[] = {
{ ~0UL, 0x00, 0x00 }
};

+static struct inno_hdmi_phy_config rk3128_hdmi_phy_configs[] = {
+ { 74250000, 0x3f, 0xaa },
+ { 165000000, 0x5f, 0xaa },
+ { ~0UL, 0x00, 0x00 }
+};
+
static struct inno_hdmi *encoder_to_inno_hdmi(struct drm_encoder *encoder)
{
struct rockchip_encoder *rkencoder = to_rockchip_encoder(encoder);
@@ -930,6 +937,20 @@ static int inno_hdmi_bind(struct device *dev, struct device *master,
return ret;
}

+ hdmi->refclk = devm_clk_get_optional(hdmi->dev, "ref");
+ if (IS_ERR(hdmi->refclk)) {
+ DRM_DEV_ERROR(hdmi->dev, "Unable to get HDMI reference clock\n");
+ ret = PTR_ERR(hdmi->refclk);
+ goto err_disable_pclk;
+ }
+
+ ret = clk_prepare_enable(hdmi->refclk);
+ if (ret) {
+ DRM_DEV_ERROR(hdmi->dev,
+ "Cannot enable HDMI reference clock: %d\n", ret);
+ goto err_disable_pclk;
+ }
+
irq = platform_get_irq(pdev, 0);
if (irq < 0) {
ret = irq;
@@ -946,12 +967,16 @@ static int inno_hdmi_bind(struct device *dev, struct device *master,
}

/*
- * When IP controller haven't configured to an accurate video
- * timing, then the TMDS clock source would be switched to
- * PCLK_HDMI, so we need to init the TMDS rate to PCLK rate,
- * and reconfigure the DDC clock.
+ * When IP controller isn't configured to an accurate
+ * video timing and there is no reference clock available,
+ * then the TMDS clock source would be switched to PCLK_HDMI,
+ * so we need to init the TMDS rate to PCLK rate, and
+ * reconfigure the DDC clock.
*/
- hdmi->tmds_rate = clk_get_rate(hdmi->pclk);
+ if (hdmi->refclk)
+ hdmi->tmds_rate = clk_get_rate(hdmi->refclk);
+ else
+ hdmi->tmds_rate = clk_get_rate(hdmi->pclk);
inno_hdmi_i2c_init(hdmi);

ret = inno_hdmi_register(drm, hdmi);
@@ -976,6 +1001,8 @@ static int inno_hdmi_bind(struct device *dev, struct device *master,
err_put_adapter:
i2c_put_adapter(hdmi->ddc);
err_disable_clk:
+ clk_disable_unprepare(hdmi->refclk);
+err_disable_pclk:
clk_disable_unprepare(hdmi->pclk);
return ret;
}
@@ -989,6 +1016,7 @@ static void inno_hdmi_unbind(struct device *dev, struct device *master,
hdmi->encoder.encoder.funcs->destroy(&hdmi->encoder.encoder);

i2c_put_adapter(hdmi->ddc);
+ clk_disable_unprepare(hdmi->refclk);
clk_disable_unprepare(hdmi->pclk);
}

@@ -1012,10 +1040,18 @@ static const struct inno_hdmi_variant rk3036_inno_hdmi_variant = {
.default_phy_config = &rk3036_hdmi_phy_configs[1],
};

+static const struct inno_hdmi_variant rk3128_inno_hdmi_variant = {
+ .phy_configs = rk3128_hdmi_phy_configs,
+ .default_phy_config = &rk3128_hdmi_phy_configs[1],
+};
+
static const struct of_device_id inno_hdmi_dt_ids[] = {
{ .compatible = "rockchip,rk3036-inno-hdmi",
.data = &rk3036_inno_hdmi_variant,
},
+ { .compatible = "rockchip,rk3128-inno-hdmi",
+ .data = &rk3128_inno_hdmi_variant,
+ },
{},
};
MODULE_DEVICE_TABLE(of, inno_hdmi_dt_ids);
--
2.43.0

2023-12-13 19:52:03

by Alex Bee

Subject: [PATCH 02/11] drm/rockchip: vop: Add output selection registers for RK312x

In contrast to RK3036, RK312x SoCs have multiple output channels such as
RGB (i.e. LVDS TTL), LVDS, DSI and HDMI.

In order to support that, this adds a new vop_output struct for rk3126_vop
with the registers required to enable the appropriate output and setup the
correct polarity.

Signed-off-by: Alex Bee <[email protected]>
---
drivers/gpu/drm/rockchip/rockchip_vop_reg.c | 13 ++++++++++++-
drivers/gpu/drm/rockchip/rockchip_vop_reg.h | 3 +++
2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
index c51ca82320cb..b9ee02061d5b 100644
--- a/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
+++ b/drivers/gpu/drm/rockchip/rockchip_vop_reg.c
@@ -227,11 +227,22 @@ static const struct vop_win_data rk3126_vop_win_data[] = {
.type = DRM_PLANE_TYPE_CURSOR },
};

+static const struct vop_output rk3126_output = {
+ .pin_pol = VOP_REG(RK3036_DSP_CTRL0, 0xf, 4),
+ .hdmi_pin_pol = VOP_REG(RK3126_INT_SCALER, 0x7, 4),
+ .hdmi_en = VOP_REG(RK3036_AXI_BUS_CTRL, 0x1, 22),
+ .hdmi_dclk_pol = VOP_REG(RK3036_AXI_BUS_CTRL, 0x1, 23),
+ .rgb_en = VOP_REG(RK3036_AXI_BUS_CTRL, 0x1, 24),
+ .rgb_dclk_pol = VOP_REG(RK3036_AXI_BUS_CTRL, 0x1, 25),
+ .mipi_en = VOP_REG(RK3036_AXI_BUS_CTRL, 0x1, 28),
+ .mipi_dclk_pol = VOP_REG(RK3036_AXI_BUS_CTRL, 0x1, 29),
+};
+
static const struct vop_data rk3126_vop = {
.intr = &rk3036_intr,
.common = &rk3036_common,
.modeset = &rk3036_modeset,
- .output = &rk3036_output,
+ .output = &rk3126_output,
.win = rk3126_vop_win_data,
.win_size = ARRAY_SIZE(rk3126_vop_win_data),
.max_output = { 1920, 1080 },
diff --git a/drivers/gpu/drm/rockchip/rockchip_vop_reg.h b/drivers/gpu/drm/rockchip/rockchip_vop_reg.h
index 406e981c75bd..fbf1bcc68625 100644
--- a/drivers/gpu/drm/rockchip/rockchip_vop_reg.h
+++ b/drivers/gpu/drm/rockchip/rockchip_vop_reg.h
@@ -872,6 +872,9 @@
/* rk3036 register definition end */

/* rk3126 register definition */
+#define RK3126_INT_SCALER 0x0c
+
+/* win1 register */
#define RK3126_WIN1_MST 0x4c
#define RK3126_WIN1_DSP_INFO 0x50
#define RK3126_WIN1_DSP_ST 0x54
--
2.43.0

2023-12-13 19:52:10

by Alex Bee

Subject: [PATCH 08/11] drm/rockchip: inno_hdmi: Drop custom fill_modes hook

Now that we have proper pixelclock-based validation we can drop the
custom fill_modes hook.
CRTC size validation for the display controller has been added with
Commit 8e140cb60270 ("drm/rockchip: vop: limit maximum resolution to
hardware capabilities")

Signed-off-by: Alex Bee <[email protected]>
---
drivers/gpu/drm/rockchip/inno_hdmi.c | 9 +--------
1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
index 2f839ff31c1c..84520da8c995 100644
--- a/drivers/gpu/drm/rockchip/inno_hdmi.c
+++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
@@ -696,13 +696,6 @@ inno_hdmi_connector_mode_valid(struct drm_connector *connector,
return inno_hdmi_mode_valid(hdmi, mode);
}

-static int
-inno_hdmi_probe_single_connector_modes(struct drm_connector *connector,
- uint32_t maxX, uint32_t maxY)
-{
- return drm_helper_probe_single_connector_modes(connector, 1920, 1080);
-}
-
static void inno_hdmi_connector_destroy(struct drm_connector *connector)
{
drm_connector_unregister(connector);
@@ -710,7 +703,7 @@ static void inno_hdmi_connector_destroy(struct drm_connector *connector)
}

static const struct drm_connector_funcs inno_hdmi_connector_funcs = {
- .fill_modes = inno_hdmi_probe_single_connector_modes,
+ .fill_modes = drm_helper_probe_single_connector_modes,
.detect = inno_hdmi_connector_detect,
.destroy = inno_hdmi_connector_destroy,
.reset = drm_atomic_helper_connector_reset,
--
2.43.0

2023-12-13 19:52:12

by Alex Bee

Subject: [PATCH 07/11] drm/rockchip: inno_hdmi: Add basic mode validation

As per TRM this controller supports pixelclocks starting from 25 MHz. The
maximum supported pixelclocks are defined by the phy configurations we
have. Also it can't support modes that require doubled clocks.
If there is a phy reference clock we can additionally validate against
VESA DMT's recommendations.
Those checks are added to the mode_valid hook of the connector and
encoder's mode_fixup hook.

Signed-off-by: Alex Bee <[email protected]>
---
drivers/gpu/drm/rockchip/inno_hdmi.c | 38 ++++++++++++++++++++++++++--
1 file changed, 36 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
index f7f0bec725f9..2f839ff31c1c 100644
--- a/drivers/gpu/drm/rockchip/inno_hdmi.c
+++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
@@ -38,6 +38,8 @@ struct inno_hdmi_variant {
struct inno_hdmi_phy_config *default_phy_config;
};

+#define INNO_HDMI_MIN_TMDS_CLOCK 25000000U
+
struct hdmi_data_info {
int vic;
bool sink_has_audio;
@@ -572,6 +574,34 @@ static int inno_hdmi_setup(struct inno_hdmi *hdmi,
return 0;
}

+static enum drm_mode_status inno_hdmi_mode_valid(struct inno_hdmi *hdmi,
+ struct drm_display_mode *mode)
+{
+ /* No support for double-clock modes */
+ if (mode->flags & DRM_MODE_FLAG_DBLCLK)
+ return MODE_BAD;
+
+ unsigned int mpixelclk = mode->clock * 1000;
+
+ if (mpixelclk < INNO_HDMI_MIN_TMDS_CLOCK)
+ return MODE_CLOCK_LOW;
+
+ if (inno_hdmi_find_phy_config(hdmi, mpixelclk) < 0)
+ return MODE_CLOCK_HIGH;
+
+ if (hdmi->refclk) {
+ long refclk = clk_round_rate(hdmi->refclk, mpixelclk);
+ unsigned int max_tolerance = mpixelclk / 5000;
+
+ /* Vesa DMT standard mentions +/- 0.5% max tolerance */
+ if (abs(refclk - mpixelclk) > max_tolerance ||
+ mpixelclk - refclk > max_tolerance)
+ return MODE_NOCLOCK;
+ }
+
+ return MODE_OK;
+}
+
static void inno_hdmi_encoder_mode_set(struct drm_encoder *encoder,
struct drm_display_mode *mode,
struct drm_display_mode *adj_mode)
@@ -602,7 +632,9 @@ static bool inno_hdmi_encoder_mode_fixup(struct drm_encoder *encoder,
const struct drm_display_mode *mode,
struct drm_display_mode *adj_mode)
{
- return true;
+ struct inno_hdmi *hdmi = encoder_to_inno_hdmi(encoder);
+
+ return inno_hdmi_mode_valid(hdmi, adj_mode) == MODE_OK;
}

static int
@@ -659,7 +691,9 @@ static enum drm_mode_status
inno_hdmi_connector_mode_valid(struct drm_connector *connector,
struct drm_display_mode *mode)
{
- return MODE_OK;
+ struct inno_hdmi *hdmi = connector_to_inno_hdmi(connector);
+
+ return inno_hdmi_mode_valid(hdmi, mode);
}

static int
--
2.43.0

2023-12-13 19:52:13

by Alex Bee

Subject: [PATCH 05/11] drm/rockchip: inno_hdmi: Add variant support

In preparation to support RK3128's integration of the controller, this
patch adds a simple variant implementation. They mainly differ in the
phy configuration required, so those are part of the match_data.
The values have been taken from downstream. The pixelclocks in there are
meant to be max-inclusive.

Signed-off-by: Alex Bee <[email protected]>
---
drivers/gpu/drm/rockchip/inno_hdmi.c | 68 +++++++++++++++++++++++++++-
1 file changed, 66 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
index 32626a75723c..593b184bd0ad 100644
--- a/drivers/gpu/drm/rockchip/inno_hdmi.c
+++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
@@ -27,6 +27,17 @@

#include "inno_hdmi.h"

+struct inno_hdmi_phy_config {
+ unsigned int pixelclock;
+ u8 pre_emphasis;
+ u8 voltage_level_control;
+};
+
+struct inno_hdmi_variant {
+ struct inno_hdmi_phy_config *phy_configs;
+ struct inno_hdmi_phy_config *default_phy_config;
+};
+
struct hdmi_data_info {
int vic;
bool sink_has_audio;
@@ -64,6 +75,14 @@ struct inno_hdmi {

struct hdmi_data_info hdmi_data;
struct drm_display_mode previous_mode;
+
+ const struct inno_hdmi_variant *variant;
+};
+
+static struct inno_hdmi_phy_config rk3036_hdmi_phy_configs[] = {
+ { 74250000, 0x3f, 0xbb },
+ { 165000000, 0x6f, 0xbb },
+ { ~0UL, 0x00, 0x00 }
};

static struct inno_hdmi *encoder_to_inno_hdmi(struct drm_encoder *encoder)
@@ -176,6 +195,24 @@ static inline void hdmi_modb(struct inno_hdmi *hdmi, u16 offset,
hdmi_writeb(hdmi, offset, temp);
}

+static int inno_hdmi_find_phy_config(struct inno_hdmi *hdmi,
+ unsigned int pixelclk)
+{
+ const struct inno_hdmi_phy_config *phy_configs =
+ hdmi->variant->phy_configs;
+ int i;
+
+ for (i = 0; phy_configs[i].pixelclock != ~0UL; i++) {
+ if (pixelclk <= phy_configs[i].pixelclock)
+ return i;
+ }
+
+ DRM_DEV_DEBUG(hdmi->dev, "No phy configuration for pixelclock %u\n",
+ pixelclk);
+
+ return -EINVAL;
+}
+
static void inno_hdmi_i2c_init(struct inno_hdmi *hdmi)
{
int ddc_bus_freq;
@@ -200,12 +237,25 @@ static void inno_hdmi_sys_power(struct inno_hdmi *hdmi, bool enable)

static void inno_hdmi_set_pwr_mode(struct inno_hdmi *hdmi, int mode)
{
+ int ret;
+ struct inno_hdmi_phy_config *phy_config;
+
switch (mode) {
case NORMAL:
inno_hdmi_sys_power(hdmi, false);

- hdmi_writeb(hdmi, HDMI_PHY_PRE_EMPHASIS, 0x6f);
- hdmi_writeb(hdmi, HDMI_PHY_DRIVER, 0xbb);
+ ret = inno_hdmi_find_phy_config(hdmi, hdmi->tmds_rate);
+ if (ret < 0) {
+ phy_config = hdmi->variant->default_phy_config;
+ DRM_DEV_ERROR(hdmi->dev,
+ "Using default phy configuration for TMDS rate %u",
+ hdmi->tmds_rate);
+ } else {
+ phy_config = &hdmi->variant->phy_configs[ret];
+ }
+
+ hdmi_writeb(hdmi, HDMI_PHY_PRE_EMPHASIS, phy_config->pre_emphasis);
+ hdmi_writeb(hdmi, HDMI_PHY_DRIVER, phy_config->voltage_level_control);

hdmi_writeb(hdmi, HDMI_PHY_SYS_CTL, 0x15);
hdmi_writeb(hdmi, HDMI_PHY_SYS_CTL, 0x14);
@@ -845,6 +895,8 @@ static int inno_hdmi_bind(struct device *dev, struct device *master,
struct platform_device *pdev = to_platform_device(dev);
struct drm_device *drm = data;
struct inno_hdmi *hdmi;
+ const struct inno_hdmi_variant *variant;
+
int irq;
int ret;

@@ -853,6 +905,12 @@ static int inno_hdmi_bind(struct device *dev, struct device *master,
return -ENOMEM;

hdmi->dev = dev;
+
+ variant = of_device_get_match_data(hdmi->dev);
+ if (!variant)
+ return -EINVAL;
+
+ hdmi->variant = variant;
hdmi->drm_dev = drm;

hdmi->regs = devm_platform_ioremap_resource(pdev, 0);
@@ -949,8 +1007,14 @@ static void inno_hdmi_remove(struct platform_device *pdev)
component_del(&pdev->dev, &inno_hdmi_ops);
}

+static const struct inno_hdmi_variant rk3036_inno_hdmi_variant = {
+ .phy_configs = rk3036_hdmi_phy_configs,
+ .default_phy_config = &rk3036_hdmi_phy_configs[1],
+};
+
static const struct of_device_id inno_hdmi_dt_ids[] = {
{ .compatible = "rockchip,rk3036-inno-hdmi",
+ .data = &rk3036_inno_hdmi_variant,
},
{},
};
--
2.43.0

2023-12-13 19:52:18

by Alex Bee

Subject: [PATCH 09/11] ARM: dts: rockchip: Add display subsystem for RK3128

Add vop and display-subsysem nodes to RK3128's device tree.

Signed-off-by: Alex Bee <[email protected]>
---
arch/arm/boot/dts/rockchip/rk3128.dtsi | 27 ++++++++++++++++++++++++++
1 file changed, 27 insertions(+)

diff --git a/arch/arm/boot/dts/rockchip/rk3128.dtsi b/arch/arm/boot/dts/rockchip/rk3128.dtsi
index e2264c40b924..1a3bc8b2bc6e 100644
--- a/arch/arm/boot/dts/rockchip/rk3128.dtsi
+++ b/arch/arm/boot/dts/rockchip/rk3128.dtsi
@@ -115,6 +115,12 @@ opp-1200000000 {
};
};

+ display_subsystem: display-subsystem {
+ compatible = "rockchip,display-subsystem";
+ ports = <&vop_out>;
+ status = "disabled";
+ };
+
gpu_opp_table: opp-table-1 {
compatible = "operating-points-v2";

@@ -246,6 +252,27 @@ power-domain@RK3128_PD_GPU {
};
};

+ vop: vop@1010e000 {
+ compatible = "rockchip,rk3126-vop";
+ reg = <0x1010e000 0x300>;
+ interrupts = <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&cru ACLK_LCDC0>, <&cru DCLK_VOP>,
+ <&cru HCLK_LCDC0>;
+ clock-names = "aclk_vop", "dclk_vop",
+ "hclk_vop";
+ resets = <&cru SRST_VOP_A>, <&cru SRST_VOP_H>,
+ <&cru SRST_VOP_D>;
+ reset-names = "axi", "ahb",
+ "dclk";
+ power-domains = <&power RK3128_PD_VIO>;
+ status = "disabled";
+
+ vop_out: port {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ };
+ };
+
qos_gpu: qos@1012d000 {
compatible = "rockchip,rk3128-qos", "syscon";
reg = <0x1012d000 0x20>;
--
2.43.0

2023-12-13 19:52:18

by Alex Bee

Subject: [PATCH 03/11] drm/rockchip: inno_hdmi: Fix video timing

The controller wants the difference between *total and *sync_start in
the HDMI_VIDEO_EXT_*DELAY registers. Otherwise the signal is very
unstable for certain non-VIC modes.
See downstream commit [0]

[0] https://github.com/rockchip-linux/kernel/commit/8eb559f2502c

Fixes: 412d4ae6b7a5 ("drm/rockchip: hdmi: add Innosilicon HDMI support")
Co-developed-by: Zheng Yang <[email protected]>
Signed-off-by: Zheng Yang <[email protected]>
Signed-off-by: Alex Bee <[email protected]>
---
drivers/gpu/drm/rockchip/inno_hdmi.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
index 6e5b922a121e..345253e033c5 100644
--- a/drivers/gpu/drm/rockchip/inno_hdmi.c
+++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
@@ -412,7 +412,7 @@ static int inno_hdmi_config_video_timing(struct inno_hdmi *hdmi,
hdmi_writeb(hdmi, HDMI_VIDEO_EXT_HBLANK_L, value & 0xFF);
hdmi_writeb(hdmi, HDMI_VIDEO_EXT_HBLANK_H, (value >> 8) & 0xFF);

- value = mode->hsync_start - mode->hdisplay;
+ value = mode->htotal - mode->hsync_start;
hdmi_writeb(hdmi, HDMI_VIDEO_EXT_HDELAY_L, value & 0xFF);
hdmi_writeb(hdmi, HDMI_VIDEO_EXT_HDELAY_H, (value >> 8) & 0xFF);

@@ -427,7 +427,7 @@ static int inno_hdmi_config_video_timing(struct inno_hdmi *hdmi,
value = mode->vtotal - mode->vdisplay;
hdmi_writeb(hdmi, HDMI_VIDEO_EXT_VBLANK, value & 0xFF);

- value = mode->vsync_start - mode->vdisplay;
+ value = mode->vtotal - mode->vsync_start;
hdmi_writeb(hdmi, HDMI_VIDEO_EXT_VDELAY, value & 0xFF);

value = mode->vsync_end - mode->vsync_start;
--
2.43.0

2023-12-13 19:52:19

by Alex Bee

Subject: [PATCH 10/11] ARM: dts rockchip: Add HDMI node for RK3128

RK3128 has Innosilicon based HDMI TX controller similar to the one found in
RK3036.
Add it and the respective port nodes to the SoC device tree.

Signed-off-by: Alex Bee <[email protected]>
---
arch/arm/boot/dts/rockchip/rk3128.dtsi | 33 ++++++++++++++++++++++++++
1 file changed, 33 insertions(+)

diff --git a/arch/arm/boot/dts/rockchip/rk3128.dtsi b/arch/arm/boot/dts/rockchip/rk3128.dtsi
index 1a3bc8b2bc6e..fb98873fd94e 100644
--- a/arch/arm/boot/dts/rockchip/rk3128.dtsi
+++ b/arch/arm/boot/dts/rockchip/rk3128.dtsi
@@ -270,6 +270,11 @@ vop: vop@1010e000 {
vop_out: port {
#address-cells = <1>;
#size-cells = <0>;
+
+ vop_out_hdmi: endpoint@0 {
+ reg = <0>;
+ remote-endpoint = <&hdmi_in_vop>;
+ };
};
};

@@ -463,6 +468,34 @@ usb2phy_otg: otg-port {
};
};

+ hdmi: hdmi@20034000 {
+ compatible = "rockchip,rk3128-inno-hdmi";
+ reg = <0x20034000 0x4000>;
+ interrupts = <GIC_SPI 45 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&cru PCLK_HDMI>, <&cru DCLK_VOP>;
+ clock-names = "pclk", "ref";
+ pinctrl-names = "default";
+ pinctrl-0 = <&hdmii2c_xfer &hdmi_hpd &hdmi_cec>;
+ power-domains = <&power RK3128_PD_VIO>;
+ status = "disabled";
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ hdmi_in: port@0 {
+ reg = <0>;
+ hdmi_in_vop: endpoint {
+ remote-endpoint = <&vop_out_hdmi>;
+ };
+ };
+
+ hdmi_out: port@1 {
+ reg = <1>;
+ };
+ };
+ };
+
timer0: timer@20044000 {
compatible = "rockchip,rk3128-timer", "rockchip,rk3288-timer";
reg = <0x20044000 0x20>;
--
2.43.0

2023-12-13 19:52:20

by Alex Bee

Subject: [PATCH 04/11] drm/rockchip: inno_hdmi: Correctly setup HDMI quantization range

The display controller will always give full range RGB regardless of the
mode set, but HDMI requires certain modes to be transmitted in limited
range RGB. This is especially required for HDMI sinks which do not support
non-standard quantization ranges.

This enables color space conversion for those modes and sets the
quantization range accordingly in the AVI infoframe.

Fixes: 412d4ae6b7a5 ("drm/rockchip: hdmi: add Innosilicon HDMI support")
Signed-off-by: Alex Bee <[email protected]>
---
drivers/gpu/drm/rockchip/inno_hdmi.c | 40 ++++++++++++++++++++++------
1 file changed, 32 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
index 345253e033c5..32626a75723c 100644
--- a/drivers/gpu/drm/rockchip/inno_hdmi.c
+++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
@@ -33,6 +33,7 @@ struct hdmi_data_info {
unsigned int enc_in_format;
unsigned int enc_out_format;
unsigned int colorimetry;
+ bool rgb_limited_range;
};

struct inno_hdmi_i2c {
@@ -308,6 +309,18 @@ static int inno_hdmi_config_video_avi(struct inno_hdmi *hdmi,
else
frame.avi.colorspace = HDMI_COLORSPACE_RGB;

+ if (hdmi->hdmi_data.enc_out_format == HDMI_COLORSPACE_RGB) {
+ drm_hdmi_avi_infoframe_quant_range(&frame.avi,
+ &hdmi->connector, mode,
+ hdmi->hdmi_data.rgb_limited_range ?
+ HDMI_QUANTIZATION_RANGE_LIMITED :
+ HDMI_QUANTIZATION_RANGE_FULL);
+ } else {
+ frame.avi.quantization_range = HDMI_QUANTIZATION_RANGE_DEFAULT;
+ frame.avi.ycc_quantization_range =
+ HDMI_YCC_QUANTIZATION_RANGE_LIMITED;
+ }
+
return inno_hdmi_upload_frame(hdmi, rc, &frame, INFOFRAME_AVI, 0, 0, 0);
}

@@ -334,14 +347,22 @@ static int inno_hdmi_config_video_csc(struct inno_hdmi *hdmi)
if (data->enc_in_format == data->enc_out_format) {
if ((data->enc_in_format == HDMI_COLORSPACE_RGB) ||
(data->enc_in_format >= HDMI_COLORSPACE_YUV444)) {
- value = v_SOF_DISABLE | v_COLOR_DEPTH_NOT_INDICATED(1);
- hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL3, value);
-
- hdmi_modb(hdmi, HDMI_VIDEO_CONTRL,
- m_VIDEO_AUTO_CSC | m_VIDEO_C0_C2_SWAP,
- v_VIDEO_AUTO_CSC(AUTO_CSC_DISABLE) |
- v_VIDEO_C0_C2_SWAP(C0_C2_CHANGE_DISABLE));
- return 0;
+ if (data->enc_in_format == HDMI_COLORSPACE_RGB &&
+ data->enc_out_format == HDMI_COLORSPACE_RGB &&
+ hdmi->hdmi_data.rgb_limited_range) {
+ csc_mode = CSC_RGB_0_255_TO_RGB_16_235_8BIT;
+ auto_csc = AUTO_CSC_DISABLE;
+ c0_c2_change = C0_C2_CHANGE_DISABLE;
+ csc_enable = v_CSC_ENABLE;
+ } else {
+ value = v_SOF_DISABLE | v_COLOR_DEPTH_NOT_INDICATED(1);
+ hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL3, value);
+ hdmi_modb(hdmi, HDMI_VIDEO_CONTRL,
+ m_VIDEO_AUTO_CSC | m_VIDEO_C0_C2_SWAP,
+ v_VIDEO_AUTO_CSC(AUTO_CSC_DISABLE) |
+ v_VIDEO_C0_C2_SWAP(C0_C2_CHANGE_DISABLE));
+ return 0;
+ }
}
}

@@ -458,6 +479,9 @@ static int inno_hdmi_setup(struct inno_hdmi *hdmi,
else
hdmi->hdmi_data.colorimetry = HDMI_COLORIMETRY_ITU_709;

+ hdmi->hdmi_data.rgb_limited_range =
+ drm_default_rgb_quant_range(mode) == HDMI_QUANTIZATION_RANGE_LIMITED;
+
/* Mute video and audio output */
hdmi_modb(hdmi, HDMI_AV_MUTE, m_AUDIO_MUTE | m_VIDEO_BLACK,
v_AUDIO_MUTE(1) | v_VIDEO_MUTE(1));
--
2.43.0

2023-12-13 19:52:28

by Alex Bee

Subject: [PATCH 11/11] ARM: dts: rockchip: Enable HDMI output for XPI-3128

Add an hdmi-connector node and enable the hdmi, display-subsystem and vop
nodes.

Signed-off-by: Alex Bee <[email protected]>
---
.../arm/boot/dts/rockchip/rk3128-xpi-3128.dts | 29 +++++++++++++++++++
1 file changed, 29 insertions(+)

diff --git a/arch/arm/boot/dts/rockchip/rk3128-xpi-3128.dts b/arch/arm/boot/dts/rockchip/rk3128-xpi-3128.dts
index 03a97881519a..21c1678f4e91 100644
--- a/arch/arm/boot/dts/rockchip/rk3128-xpi-3128.dts
+++ b/arch/arm/boot/dts/rockchip/rk3128-xpi-3128.dts
@@ -47,6 +47,17 @@ dc_5v: dc-5v-regulator {
regulator-boot-on;
};

+ hdmi-connnector {
+ compatible = "hdmi-connector";
+ type = "a";
+
+ port {
+ hdmi_connector_in: endpoint {
+ remote-endpoint = <&hdmi_connector_out>;
+ };
+ };
+ };
+
/*
* This is a vbus-supply, which also supplies the GL852G usb hub,
* thus has to be always-on
@@ -239,6 +250,10 @@ &cpu0 {
cpu-supply = <&vdd_arm>;
};

+&display_subsystem {
+ status = "okay";
+};
+
&emmc {
bus-width = <8>;
vmmc-supply = <&vcc_io>;
@@ -328,6 +343,16 @@ &gpu {
status = "okay";
};

+&hdmi {
+ status = "okay";
+};
+
+&hdmi_out {
+ hdmi_connector_out: endpoint {
+ remote-endpoint = <&hdmi_connector_in>;
+ };
+};
+
&mdio {
phy0: ethernet-phy@1 {
compatible = "ethernet-phy-ieee802.3-c22";
@@ -423,3 +448,7 @@ &usb2phy_host {
&usb2phy_otg {
status = "okay";
};
+
+&vop {
+ status = "okay";
+};
--
2.43.0

2023-12-14 07:56:50

by Maxime Ripard

Subject: Re: [PATCH 04/11] drm/rockchip: inno_hdmi: Correctly setup HDMI quantization range

Hi Alex,

Thanks for working on this!

On Wed, Dec 13, 2023 at 08:51:18PM +0100, Alex Bee wrote:
> The display controller will always give full range RGB regardless of the
> mode set, but HDMI requires certain modes to be transmitted in limited
> range RGB. This is especially required for HDMI sinks which do not support
> non-standard quantization ranges.
>
> This enables color space conversion for those modes and sets the
> quantization range accordingly in the AVI infoframe.
>
> Fixes: 412d4ae6b7a5 ("drm/rockchip: hdmi: add Innosilicon HDMI support")
> Signed-off-by: Alex Bee <[email protected]>
> ---
> drivers/gpu/drm/rockchip/inno_hdmi.c | 40 ++++++++++++++++++++++------
> 1 file changed, 32 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
> index 345253e033c5..32626a75723c 100644
> --- a/drivers/gpu/drm/rockchip/inno_hdmi.c
> +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
> @@ -33,6 +33,7 @@ struct hdmi_data_info {
> unsigned int enc_in_format;
> unsigned int enc_out_format;
> unsigned int colorimetry;
> + bool rgb_limited_range;
> };
>
> struct inno_hdmi_i2c {
> @@ -308,6 +309,18 @@ static int inno_hdmi_config_video_avi(struct inno_hdmi *hdmi,
> else
> frame.avi.colorspace = HDMI_COLORSPACE_RGB;
>
> + if (hdmi->hdmi_data.enc_out_format == HDMI_COLORSPACE_RGB) {
> + drm_hdmi_avi_infoframe_quant_range(&frame.avi,
> + &hdmi->connector, mode,
> + hdmi->hdmi_data.rgb_limited_range ?
> + HDMI_QUANTIZATION_RANGE_LIMITED :
> + HDMI_QUANTIZATION_RANGE_FULL);
> + } else {
> + frame.avi.quantization_range = HDMI_QUANTIZATION_RANGE_DEFAULT;
> + frame.avi.ycc_quantization_range =
> + HDMI_YCC_QUANTIZATION_RANGE_LIMITED;
> + }
> +
> return inno_hdmi_upload_frame(hdmi, rc, &frame, INFOFRAME_AVI, 0, 0, 0);
> }
>
> @@ -334,14 +347,22 @@ static int inno_hdmi_config_video_csc(struct inno_hdmi *hdmi)
> if (data->enc_in_format == data->enc_out_format) {
> if ((data->enc_in_format == HDMI_COLORSPACE_RGB) ||
> (data->enc_in_format >= HDMI_COLORSPACE_YUV444)) {
> - value = v_SOF_DISABLE | v_COLOR_DEPTH_NOT_INDICATED(1);
> - hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL3, value);
> -
> - hdmi_modb(hdmi, HDMI_VIDEO_CONTRL,
> - m_VIDEO_AUTO_CSC | m_VIDEO_C0_C2_SWAP,
> - v_VIDEO_AUTO_CSC(AUTO_CSC_DISABLE) |
> - v_VIDEO_C0_C2_SWAP(C0_C2_CHANGE_DISABLE));
> - return 0;
> + if (data->enc_in_format == HDMI_COLORSPACE_RGB &&
> + data->enc_out_format == HDMI_COLORSPACE_RGB &&
> + hdmi->hdmi_data.rgb_limited_range) {
> + csc_mode = CSC_RGB_0_255_TO_RGB_16_235_8BIT;
> + auto_csc = AUTO_CSC_DISABLE;
> + c0_c2_change = C0_C2_CHANGE_DISABLE;
> + csc_enable = v_CSC_ENABLE;
> + } else {
> + value = v_SOF_DISABLE | v_COLOR_DEPTH_NOT_INDICATED(1);
> + hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL3, value);
> + hdmi_modb(hdmi, HDMI_VIDEO_CONTRL,
> + m_VIDEO_AUTO_CSC | m_VIDEO_C0_C2_SWAP,
> + v_VIDEO_AUTO_CSC(AUTO_CSC_DISABLE) |
> + v_VIDEO_C0_C2_SWAP(C0_C2_CHANGE_DISABLE));
> + return 0;
> + }
> }
> }
>
> @@ -458,6 +479,9 @@ static int inno_hdmi_setup(struct inno_hdmi *hdmi,
> else
> hdmi->hdmi_data.colorimetry = HDMI_COLORIMETRY_ITU_709;
>
> + hdmi->hdmi_data.rgb_limited_range =
> + drm_default_rgb_quant_range(mode) == HDMI_QUANTIZATION_RANGE_LIMITED;
> +

This patch conflicts heavily with my inno_hdmi patches here (patches 22 to 38):
https://lore.kernel.org/dri-devel/[email protected]/

I would appreciate if you could test and merge them into your series.

In particular, there's no need to store the range here: enc_out_format
is always RGB, so you'll always end up calling
drm_hdmi_avi_infoframe_quant_range(), and you'll always have the same csc values.

Maxime

Attachments:

2023-12-14 08:07:16

by Maxime Ripard

Subject: Re: [PATCH 07/11] drm/rockchip: inno_hdmi: Add basic mode validation

Hi,

On Wed, Dec 13, 2023 at 08:51:21PM +0100, Alex Bee wrote:
> As per TRM this controller supports pixelclocks starting from 25 MHz. The
> maximum supported pixelclocks are defined by the phy configurations we
> have. Also it can't support modes that require doubled clocks.
> If there is a phy reference clock we can additionally validate against
> VESA DMT's recommendations.
> Those checks are added to the mode_valid hook of the connector and
> encoder's mode_fixup hook.
>
> Signed-off-by: Alex Bee <[email protected]>
> ---
> drivers/gpu/drm/rockchip/inno_hdmi.c | 38 ++++++++++++++++++++++++++--
> 1 file changed, 36 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
> index f7f0bec725f9..2f839ff31c1c 100644
> --- a/drivers/gpu/drm/rockchip/inno_hdmi.c
> +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
> @@ -38,6 +38,8 @@ struct inno_hdmi_variant {
> struct inno_hdmi_phy_config *default_phy_config;
> };
>
> +#define INNO_HDMI_MIN_TMDS_CLOCK 25000000U
> +
> struct hdmi_data_info {
> int vic;
> bool sink_has_audio;
> @@ -572,6 +574,34 @@ static int inno_hdmi_setup(struct inno_hdmi *hdmi,
> return 0;
> }
>
> +static enum drm_mode_status inno_hdmi_mode_valid(struct inno_hdmi *hdmi,
> + struct drm_display_mode *mode)
> +{

So, mode_valid is only called to filter out the modes retrieved by
get_modes, but it won't be called when userspace programs a mode. That's
atomic_check's job.

So you probably want to create a shared function between atomic_check
and mode_valid, and call it from both places (or call mode_valid from
atomic_check).

> + /* No support for double-clock modes */
> + if (mode->flags & DRM_MODE_FLAG_DBLCLK)
> + return MODE_BAD;
> +
> + unsigned int mpixelclk = mode->clock * 1000;

Variables should be declared at the top of the function.

> + if (mpixelclk < INNO_HDMI_MIN_TMDS_CLOCK)
> + return MODE_CLOCK_LOW;

You probably want to check the max TMDS clock too?

> + if (inno_hdmi_find_phy_config(hdmi, mpixelclk) < 0)
> + return MODE_CLOCK_HIGH;
> +
> + if (hdmi->refclk) {
> + long refclk = clk_round_rate(hdmi->refclk, mpixelclk);
> + unsigned int max_tolerance = mpixelclk / 5000;
> +
> + /* Vesa DMT standard mentions +/- 0.5% max tolerance */
> + if (abs(refclk - mpixelclk) > max_tolerance ||
> + mpixelclk - refclk > max_tolerance;
> + return MODE_NOCLOCK;

You should use abs_diff here. abs() will get confused by the unsigned vs
signed comparison.

> + }
> +
> + return MODE_OK;
> +}
> +
> static void inno_hdmi_encoder_mode_set(struct drm_encoder *encoder,
> struct drm_display_mode *mode,
> struct drm_display_mode *adj_mode)
> @@ -602,7 +632,9 @@ static bool inno_hdmi_encoder_mode_fixup(struct drm_encoder *encoder,
> const struct drm_display_mode *mode,
> struct drm_display_mode *adj_mode)
> {
> - return true;
> + struct inno_hdmi *hdmi = encoder_to_inno_hdmi(encoder);
> +
> + return inno_hdmi_mode_valid(hdmi, adj_mode) == MODE_OK;
> }

Why do you call mode_valid in mode_fixup?

Maxime

Attachments:

2023-12-14 11:12:26

by Alex Bee

Subject: Re: [PATCH 04/11] drm/rockchip: inno_hdmi: Correctly setup HDMI quantization range

Hi Maxime

Am 14.12.23 um 08:56 schrieb Maxime Ripard:
> Hi Alex,
>
> Thanks for working on this!
>
> On Wed, Dec 13, 2023 at 08:51:18PM +0100, Alex Bee wrote:
>> The display controller will always give full range RGB regardless of the
>> mode set, but HDMI requires certain modes to be transmitted in limited
>> range RGB. This is especially required for HDMI sinks which do not support
>> non-standard quantization ranges.
>>
>> This enables color space conversion for those modes and sets the
>> quantization range accordingly in the AVI infoframe.
>>
>> Fixes: 412d4ae6b7a5 ("drm/rockchip: hdmi: add Innosilicon HDMI support")
>> Signed-off-by: Alex Bee <[email protected]>
>> ---
>> drivers/gpu/drm/rockchip/inno_hdmi.c | 40 ++++++++++++++++++++++------
>> 1 file changed, 32 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
>> index 345253e033c5..32626a75723c 100644
>> --- a/drivers/gpu/drm/rockchip/inno_hdmi.c
>> +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
>> @@ -33,6 +33,7 @@ struct hdmi_data_info {
>> unsigned int enc_in_format;
>> unsigned int enc_out_format;
>> unsigned int colorimetry;
>> + bool rgb_limited_range;
>> };
>>
>> struct inno_hdmi_i2c {
>> @@ -308,6 +309,18 @@ static int inno_hdmi_config_video_avi(struct inno_hdmi *hdmi,
>> else
>> frame.avi.colorspace = HDMI_COLORSPACE_RGB;
>>
>> + if (hdmi->hdmi_data.enc_out_format == HDMI_COLORSPACE_RGB) {
>> + drm_hdmi_avi_infoframe_quant_range(&frame.avi,
>> + &hdmi->connector, mode,
>> + hdmi->hdmi_data.rgb_limited_range ?
>> + HDMI_QUANTIZATION_RANGE_LIMITED :
>> + HDMI_QUANTIZATION_RANGE_FULL);
>> + } else {
>> + frame.avi.quantization_range = HDMI_QUANTIZATION_RANGE_DEFAULT;
>> + frame.avi.ycc_quantization_range =
>> + HDMI_YCC_QUANTIZATION_RANGE_LIMITED;
>> + }
>> +
>> return inno_hdmi_upload_frame(hdmi, rc, &frame, INFOFRAME_AVI, 0, 0, 0);
>> }
>>
>> @@ -334,14 +347,22 @@ static int inno_hdmi_config_video_csc(struct inno_hdmi *hdmi)
>> if (data->enc_in_format == data->enc_out_format) {
>> if ((data->enc_in_format == HDMI_COLORSPACE_RGB) ||
>> (data->enc_in_format >= HDMI_COLORSPACE_YUV444)) {
>> - value = v_SOF_DISABLE | v_COLOR_DEPTH_NOT_INDICATED(1);
>> - hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL3, value);
>> -
>> - hdmi_modb(hdmi, HDMI_VIDEO_CONTRL,
>> - m_VIDEO_AUTO_CSC | m_VIDEO_C0_C2_SWAP,
>> - v_VIDEO_AUTO_CSC(AUTO_CSC_DISABLE) |
>> - v_VIDEO_C0_C2_SWAP(C0_C2_CHANGE_DISABLE));
>> - return 0;
>> + if (data->enc_in_format == HDMI_COLORSPACE_RGB &&
>> + data->enc_out_format == HDMI_COLORSPACE_RGB &&
>> + hdmi->hdmi_data.rgb_limited_range) {
>> + csc_mode = CSC_RGB_0_255_TO_RGB_16_235_8BIT;
>> + auto_csc = AUTO_CSC_DISABLE;
>> + c0_c2_change = C0_C2_CHANGE_DISABLE;
>> + csc_enable = v_CSC_ENABLE;
>> + } else {
>> + value = v_SOF_DISABLE | v_COLOR_DEPTH_NOT_INDICATED(1);
>> + hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL3, value);
>> + hdmi_modb(hdmi, HDMI_VIDEO_CONTRL,
>> + m_VIDEO_AUTO_CSC | m_VIDEO_C0_C2_SWAP,
>> + v_VIDEO_AUTO_CSC(AUTO_CSC_DISABLE) |
>> + v_VIDEO_C0_C2_SWAP(C0_C2_CHANGE_DISABLE));
>> + return 0;
>> + }
>> }
>> }
>>
>> @@ -458,6 +479,9 @@ static int inno_hdmi_setup(struct inno_hdmi *hdmi,
>> else
>> hdmi->hdmi_data.colorimetry = HDMI_COLORIMETRY_ITU_709;
>>
>> + hdmi->hdmi_data.rgb_limited_range =
>> + drm_default_rgb_quant_range(mode) == HDMI_QUANTIZATION_RANGE_LIMITED;
>> +
> This patch conflicts heavily with my inno_hdmi patches here (patches 22 to 38):
> https://lore.kernel.org/dri-devel/[email protected]/
I'm aware of that and I mentioned it in the cover letter. Your series is
not merged yet and it didn't get much feedback so far. What is the
status there? Especially because you are removing things from inno-hdmi
driver (which aren't really required to remove there) which will I have
to reintroduce.

> I would appreciate if you could test and merge them into your series.
>
> In particular, there's no need to store the range here: enc_out_format
rgb_limited_range is currently not only used for csc, but also for for
infoframe creation. So it makes sense to have this stored  to avoid
calling drm_default_rgb_quant_range twice.

> is always RGB, so you'll always end up calling
> drm_hdmi_avi_infoframe_quant_range(), and you'll always have the same csc values.

I don't think that's true. Non-VIC modes like 800x600 or 1024x764 want
full range RGB and csc must not be done in those cases.

Alex

>
> Maxime

2023-12-14 11:17:54

by Alex Bee

Subject: Re: [PATCH 07/11] drm/rockchip: inno_hdmi: Add basic mode validation

Hi Maxime,

Am 14.12.23 um 09:05 schrieb Maxime Ripard:
> Hi,
>
> On Wed, Dec 13, 2023 at 08:51:21PM +0100, Alex Bee wrote:
>> As per TRM this controller supports pixelclocks starting from 25 MHz. The
>> maximum supported pixelclocks are defined by the phy configurations we
>> have. Also it can't support modes that require doubled clocks.
>> If there is a phy reference clock we can additionally validate against
>> VESA DMT's recommendations.
>> Those checks are added to the mode_valid hook of the connector and
>> encoder's mode_fixup hook.
>>
>> Signed-off-by: Alex Bee <[email protected]>
>> ---
>> drivers/gpu/drm/rockchip/inno_hdmi.c | 38 ++++++++++++++++++++++++++--
>> 1 file changed, 36 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
>> index f7f0bec725f9..2f839ff31c1c 100644
>> --- a/drivers/gpu/drm/rockchip/inno_hdmi.c
>> +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
>> @@ -38,6 +38,8 @@ struct inno_hdmi_variant {
>> struct inno_hdmi_phy_config *default_phy_config;
>> };
>>
>> +#define INNO_HDMI_MIN_TMDS_CLOCK 25000000U
>> +
>> struct hdmi_data_info {
>> int vic;
>> bool sink_has_audio;
>> @@ -572,6 +574,34 @@ static int inno_hdmi_setup(struct inno_hdmi *hdmi,
>> return 0;
>> }
>>
>> +static enum drm_mode_status inno_hdmi_mode_valid(struct inno_hdmi *hdmi,
>> + struct drm_display_mode *mode)
>> +{
> So, mode_valid is only called to filter out the modes retrieved by
> get_modes, but it won't be called when userspace programs a mode. That's
> atomic_check's job.
>
> So you probably want to create a shared function between atomic_check
> and mode_valid, and call it from both places (or call mode_valid from
> atomic_check).
This actually _is_ a shared function called in
inno_hdmi_connector_mode_valid and inno_hdmi_encoder_mode_fixup. Yes, I 
probably should use it in atomic_check _also_.
>
>> + /* No support for double-clock modes */
>> + if (mode->flags & DRM_MODE_FLAG_DBLCLK)
>> + return MODE_BAD;
>> +
>> + unsigned int mpixelclk = mode->clock * 1000;
> Variables should be declared at the top of the function.
Oookay ... can move them.
>> + if (mpixelclk < INNO_HDMI_MIN_TMDS_CLOCK)
>> + return MODE_CLOCK_LOW;
> You probably want to check the max TMDS clock too?
Not sure what you mean here. For the currently supported formats of this
driver (rgb only) tmds clock and pixel clock are always the same.
>
>> + if (inno_hdmi_find_phy_config(hdmi, mpixelclk) < 0)
>> + return MODE_CLOCK_HIGH;
>> +
>> + if (hdmi->refclk) {
>> + long refclk = clk_round_rate(hdmi->refclk, mpixelclk);
>> + unsigned int max_tolerance = mpixelclk / 5000;
>> +
>> + /* Vesa DMT standard mentions +/- 0.5% max tolerance */
>> + if (abs(refclk - mpixelclk) > max_tolerance ||
>> + mpixelclk - refclk > max_tolerance;
>> + return MODE_NOCLOCK;
> You should use abs_diff here. abs() will get confused by the unsigned vs
> signed comparison.
Ack.
>
>> + }
>> +
>> + return MODE_OK;
>> +}
>> +
>> static void inno_hdmi_encoder_mode_set(struct drm_encoder *encoder,
>> struct drm_display_mode *mode,
>> struct drm_display_mode *adj_mode)
>> @@ -602,7 +632,9 @@ static bool inno_hdmi_encoder_mode_fixup(struct drm_encoder *encoder,
>> const struct drm_display_mode *mode,
>> struct drm_display_mode *adj_mode)
>> {
>> - return true;
>> + struct inno_hdmi *hdmi = encoder_to_inno_hdmi(encoder);
>> +
>> + return inno_hdmi_mode_valid(hdmi, adj_mode) == MODE_OK;
>> }
> Why do you call mode_valid in mode_fixup?

I'm calling the shared function you asked me to introduce
(inno_hdmi_connector_mode_valid != inno_mode_valid)

Alex

>
> Maxime

2023-12-14 11:18:23

by Heiko Stuebner

Subject: Re: [PATCH 04/11] drm/rockchip: inno_hdmi: Correctly setup HDMI quantization range

Am Donnerstag, 14. Dezember 2023, 12:12:08 CET schrieb Alex Bee:
> Hi Maxime
>
> Am 14.12.23 um 08:56 schrieb Maxime Ripard:
> > Hi Alex,
> >
> > Thanks for working on this!
> >
> > On Wed, Dec 13, 2023 at 08:51:18PM +0100, Alex Bee wrote:
> >> The display controller will always give full range RGB regardless of the
> >> mode set, but HDMI requires certain modes to be transmitted in limited
> >> range RGB. This is especially required for HDMI sinks which do not support
> >> non-standard quantization ranges.
> >>
> >> This enables color space conversion for those modes and sets the
> >> quantization range accordingly in the AVI infoframe.
> >>
> >> Fixes: 412d4ae6b7a5 ("drm/rockchip: hdmi: add Innosilicon HDMI support")
> >> Signed-off-by: Alex Bee <[email protected]>
> >> ---
> >> drivers/gpu/drm/rockchip/inno_hdmi.c | 40 ++++++++++++++++++++++------
> >> 1 file changed, 32 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
> >> index 345253e033c5..32626a75723c 100644
> >> --- a/drivers/gpu/drm/rockchip/inno_hdmi.c
> >> +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
> >> @@ -33,6 +33,7 @@ struct hdmi_data_info {
> >> unsigned int enc_in_format;
> >> unsigned int enc_out_format;
> >> unsigned int colorimetry;
> >> + bool rgb_limited_range;
> >> };
> >>
> >> struct inno_hdmi_i2c {
> >> @@ -308,6 +309,18 @@ static int inno_hdmi_config_video_avi(struct inno_hdmi *hdmi,
> >> else
> >> frame.avi.colorspace = HDMI_COLORSPACE_RGB;
> >>
> >> + if (hdmi->hdmi_data.enc_out_format == HDMI_COLORSPACE_RGB) {
> >> + drm_hdmi_avi_infoframe_quant_range(&frame.avi,
> >> + &hdmi->connector, mode,
> >> + hdmi->hdmi_data.rgb_limited_range ?
> >> + HDMI_QUANTIZATION_RANGE_LIMITED :
> >> + HDMI_QUANTIZATION_RANGE_FULL);
> >> + } else {
> >> + frame.avi.quantization_range = HDMI_QUANTIZATION_RANGE_DEFAULT;
> >> + frame.avi.ycc_quantization_range =
> >> + HDMI_YCC_QUANTIZATION_RANGE_LIMITED;
> >> + }
> >> +
> >> return inno_hdmi_upload_frame(hdmi, rc, &frame, INFOFRAME_AVI, 0, 0, 0);
> >> }
> >>
> >> @@ -334,14 +347,22 @@ static int inno_hdmi_config_video_csc(struct inno_hdmi *hdmi)
> >> if (data->enc_in_format == data->enc_out_format) {
> >> if ((data->enc_in_format == HDMI_COLORSPACE_RGB) ||
> >> (data->enc_in_format >= HDMI_COLORSPACE_YUV444)) {
> >> - value = v_SOF_DISABLE | v_COLOR_DEPTH_NOT_INDICATED(1);
> >> - hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL3, value);
> >> -
> >> - hdmi_modb(hdmi, HDMI_VIDEO_CONTRL,
> >> - m_VIDEO_AUTO_CSC | m_VIDEO_C0_C2_SWAP,
> >> - v_VIDEO_AUTO_CSC(AUTO_CSC_DISABLE) |
> >> - v_VIDEO_C0_C2_SWAP(C0_C2_CHANGE_DISABLE));
> >> - return 0;
> >> + if (data->enc_in_format == HDMI_COLORSPACE_RGB &&
> >> + data->enc_out_format == HDMI_COLORSPACE_RGB &&
> >> + hdmi->hdmi_data.rgb_limited_range) {
> >> + csc_mode = CSC_RGB_0_255_TO_RGB_16_235_8BIT;
> >> + auto_csc = AUTO_CSC_DISABLE;
> >> + c0_c2_change = C0_C2_CHANGE_DISABLE;
> >> + csc_enable = v_CSC_ENABLE;
> >> + } else {
> >> + value = v_SOF_DISABLE | v_COLOR_DEPTH_NOT_INDICATED(1);
> >> + hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL3, value);
> >> + hdmi_modb(hdmi, HDMI_VIDEO_CONTRL,
> >> + m_VIDEO_AUTO_CSC | m_VIDEO_C0_C2_SWAP,
> >> + v_VIDEO_AUTO_CSC(AUTO_CSC_DISABLE) |
> >> + v_VIDEO_C0_C2_SWAP(C0_C2_CHANGE_DISABLE));
> >> + return 0;
> >> + }
> >> }
> >> }
> >>
> >> @@ -458,6 +479,9 @@ static int inno_hdmi_setup(struct inno_hdmi *hdmi,
> >> else
> >> hdmi->hdmi_data.colorimetry = HDMI_COLORIMETRY_ITU_709;
> >>
> >> + hdmi->hdmi_data.rgb_limited_range =
> >> + drm_default_rgb_quant_range(mode) == HDMI_QUANTIZATION_RANGE_LIMITED;
> >> +
> > This patch conflicts heavily with my inno_hdmi patches here (patches 22 to 38):
> > https://lore.kernel.org/dri-devel/[email protected]/
> I'm aware of that and I mentioned it in the cover letter. Your series is
> not merged yet and it didn't get much feedback so far. What is the
> status there? Especially because you are removing things from inno-hdmi
> driver (which aren't really required to remove there) which will I have
> to reintroduce.

Sadly I haven't found the time to look closer at Maxime's series so far,
but I got the impression that it separates into multiple cleanup steps
for a number of controllers.

So the sentence below suggests that Maxime wanted you to pick the
appropriate patches from there and incorporate them into your series
(as it looks like you developed a nice working knowledge of the inno-hdmi
driver). So there is no need to first drop and then reintroduce stuff, but
there may be other interesting cleanups there.

> > I would appreciate if you could test and merge them into your series.


Heiko

> > In particular, there's no need to store the range here: enc_out_format
> rgb_limited_range is currently not only used for csc, but also for for
> infoframe creation. So it makes sense to have this stored to avoid
> calling drm_default_rgb_quant_range twice.
>
> > is always RGB, so you'll always end up calling
> > drm_hdmi_avi_infoframe_quant_range(), and you'll always have the same csc values.
>
> I don't think that's true. Non-VIC modes like 800x600 or 1024x764 want
> full range RGB and csc must not be done in those cases.

2023-12-14 11:33:57

by Maxime Ripard

Subject: Re: [PATCH 04/11] drm/rockchip: inno_hdmi: Correctly setup HDMI quantization range

On Thu, Dec 14, 2023 at 12:12:08PM +0100, Alex Bee wrote:
> Hi Maxime
>
> Am 14.12.23 um 08:56 schrieb Maxime Ripard:
> > Hi Alex,
> >
> > Thanks for working on this!
> >
> > On Wed, Dec 13, 2023 at 08:51:18PM +0100, Alex Bee wrote:
> > > The display controller will always give full range RGB regardless of the
> > > mode set, but HDMI requires certain modes to be transmitted in limited
> > > range RGB. This is especially required for HDMI sinks which do not support
> > > non-standard quantization ranges.
> > >
> > > This enables color space conversion for those modes and sets the
> > > quantization range accordingly in the AVI infoframe.
> > >
> > > Fixes: 412d4ae6b7a5 ("drm/rockchip: hdmi: add Innosilicon HDMI support")
> > > Signed-off-by: Alex Bee <[email protected]>
> > > ---
> > > drivers/gpu/drm/rockchip/inno_hdmi.c | 40 ++++++++++++++++++++++------
> > > 1 file changed, 32 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
> > > index 345253e033c5..32626a75723c 100644
> > > --- a/drivers/gpu/drm/rockchip/inno_hdmi.c
> > > +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
> > > @@ -33,6 +33,7 @@ struct hdmi_data_info {
> > > unsigned int enc_in_format;
> > > unsigned int enc_out_format;
> > > unsigned int colorimetry;
> > > + bool rgb_limited_range;
> > > };
> > > struct inno_hdmi_i2c {
> > > @@ -308,6 +309,18 @@ static int inno_hdmi_config_video_avi(struct inno_hdmi *hdmi,
> > > else
> > > frame.avi.colorspace = HDMI_COLORSPACE_RGB;
> > > + if (hdmi->hdmi_data.enc_out_format == HDMI_COLORSPACE_RGB) {
> > > + drm_hdmi_avi_infoframe_quant_range(&frame.avi,
> > > + &hdmi->connector, mode,
> > > + hdmi->hdmi_data.rgb_limited_range ?
> > > + HDMI_QUANTIZATION_RANGE_LIMITED :
> > > + HDMI_QUANTIZATION_RANGE_FULL);
> > > + } else {
> > > + frame.avi.quantization_range = HDMI_QUANTIZATION_RANGE_DEFAULT;
> > > + frame.avi.ycc_quantization_range =
> > > + HDMI_YCC_QUANTIZATION_RANGE_LIMITED;
> > > + }
> > > +
> > > return inno_hdmi_upload_frame(hdmi, rc, &frame, INFOFRAME_AVI, 0, 0, 0);
> > > }
> > > @@ -334,14 +347,22 @@ static int inno_hdmi_config_video_csc(struct inno_hdmi *hdmi)
> > > if (data->enc_in_format == data->enc_out_format) {
> > > if ((data->enc_in_format == HDMI_COLORSPACE_RGB) ||
> > > (data->enc_in_format >= HDMI_COLORSPACE_YUV444)) {
> > > - value = v_SOF_DISABLE | v_COLOR_DEPTH_NOT_INDICATED(1);
> > > - hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL3, value);
> > > -
> > > - hdmi_modb(hdmi, HDMI_VIDEO_CONTRL,
> > > - m_VIDEO_AUTO_CSC | m_VIDEO_C0_C2_SWAP,
> > > - v_VIDEO_AUTO_CSC(AUTO_CSC_DISABLE) |
> > > - v_VIDEO_C0_C2_SWAP(C0_C2_CHANGE_DISABLE));
> > > - return 0;
> > > + if (data->enc_in_format == HDMI_COLORSPACE_RGB &&
> > > + data->enc_out_format == HDMI_COLORSPACE_RGB &&
> > > + hdmi->hdmi_data.rgb_limited_range) {
> > > + csc_mode = CSC_RGB_0_255_TO_RGB_16_235_8BIT;
> > > + auto_csc = AUTO_CSC_DISABLE;
> > > + c0_c2_change = C0_C2_CHANGE_DISABLE;
> > > + csc_enable = v_CSC_ENABLE;
> > > + } else {
> > > + value = v_SOF_DISABLE | v_COLOR_DEPTH_NOT_INDICATED(1);
> > > + hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL3, value);
> > > + hdmi_modb(hdmi, HDMI_VIDEO_CONTRL,
> > > + m_VIDEO_AUTO_CSC | m_VIDEO_C0_C2_SWAP,
> > > + v_VIDEO_AUTO_CSC(AUTO_CSC_DISABLE) |
> > > + v_VIDEO_C0_C2_SWAP(C0_C2_CHANGE_DISABLE));
> > > + return 0;
> > > + }
> > > }
> > > }
> > > @@ -458,6 +479,9 @@ static int inno_hdmi_setup(struct inno_hdmi *hdmi,
> > > else
> > > hdmi->hdmi_data.colorimetry = HDMI_COLORIMETRY_ITU_709;
> > > + hdmi->hdmi_data.rgb_limited_range =
> > > + drm_default_rgb_quant_range(mode) == HDMI_QUANTIZATION_RANGE_LIMITED;
> > > +
> > This patch conflicts heavily with my inno_hdmi patches here (patches 22 to 38):
> > https://lore.kernel.org/dri-devel/[email protected]/
>
> I'm aware of that and I mentioned it in the cover letter.

Sorry, I missed that part.

> Your series is not merged yet and it didn't get much feedback so far.
> What is the status there?

It didn't have much reviews, but I'll hope to change that. For the
patches 22 to 38 though, it doesn't really matter. Those changes are
self-contained and can be applied as is outside of the series.

> Especially because you are removing things from inno-hdmi driver (which
> aren't really required to remove there) which will I have to reintroduce.

I'm not entirely sure which part I remove that are actually going to be
used here.

> > I would appreciate if you could test and merge them into your series.
> >
> > In particular, there's no need to store the range here: enc_out_format
>
> rgb_limited_range is currently not only used for csc, but also for for
> infoframe creation. So it makes sense to have this stored? to avoid calling
> drm_default_rgb_quant_range twice.

You're right, I missed one. Still, it shouldn't be stored in the
hdmi_data_info structure, it's tied to the mode, and the mode is part of
the state, so it's not a property to a given device, but it's tied to
the connector state.

So if you want to do so, you should really create a custom state
structure and store the range there, just like vc4 is doing for example.

Maxime

Attachments:

2023-12-14 11:37:23

by Maxime Ripard

Subject: Re: [PATCH 04/11] drm/rockchip: inno_hdmi: Correctly setup HDMI quantization range

On Thu, Dec 14, 2023 at 12:17:34PM +0100, Heiko St?bner wrote:
> Am Donnerstag, 14. Dezember 2023, 12:12:08 CET schrieb Alex Bee:
> > Hi Maxime
> >
> > Am 14.12.23 um 08:56 schrieb Maxime Ripard:
> > > Hi Alex,
> > >
> > > Thanks for working on this!
> > >
> > > On Wed, Dec 13, 2023 at 08:51:18PM +0100, Alex Bee wrote:
> > >> The display controller will always give full range RGB regardless of the
> > >> mode set, but HDMI requires certain modes to be transmitted in limited
> > >> range RGB. This is especially required for HDMI sinks which do not support
> > >> non-standard quantization ranges.
> > >>
> > >> This enables color space conversion for those modes and sets the
> > >> quantization range accordingly in the AVI infoframe.
> > >>
> > >> Fixes: 412d4ae6b7a5 ("drm/rockchip: hdmi: add Innosilicon HDMI support")
> > >> Signed-off-by: Alex Bee <[email protected]>
> > >> ---
> > >> drivers/gpu/drm/rockchip/inno_hdmi.c | 40 ++++++++++++++++++++++------
> > >> 1 file changed, 32 insertions(+), 8 deletions(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
> > >> index 345253e033c5..32626a75723c 100644
> > >> --- a/drivers/gpu/drm/rockchip/inno_hdmi.c
> > >> +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
> > >> @@ -33,6 +33,7 @@ struct hdmi_data_info {
> > >> unsigned int enc_in_format;
> > >> unsigned int enc_out_format;
> > >> unsigned int colorimetry;
> > >> + bool rgb_limited_range;
> > >> };
> > >>
> > >> struct inno_hdmi_i2c {
> > >> @@ -308,6 +309,18 @@ static int inno_hdmi_config_video_avi(struct inno_hdmi *hdmi,
> > >> else
> > >> frame.avi.colorspace = HDMI_COLORSPACE_RGB;
> > >>
> > >> + if (hdmi->hdmi_data.enc_out_format == HDMI_COLORSPACE_RGB) {
> > >> + drm_hdmi_avi_infoframe_quant_range(&frame.avi,
> > >> + &hdmi->connector, mode,
> > >> + hdmi->hdmi_data.rgb_limited_range ?
> > >> + HDMI_QUANTIZATION_RANGE_LIMITED :
> > >> + HDMI_QUANTIZATION_RANGE_FULL);
> > >> + } else {
> > >> + frame.avi.quantization_range = HDMI_QUANTIZATION_RANGE_DEFAULT;
> > >> + frame.avi.ycc_quantization_range =
> > >> + HDMI_YCC_QUANTIZATION_RANGE_LIMITED;
> > >> + }
> > >> +
> > >> return inno_hdmi_upload_frame(hdmi, rc, &frame, INFOFRAME_AVI, 0, 0, 0);
> > >> }
> > >>
> > >> @@ -334,14 +347,22 @@ static int inno_hdmi_config_video_csc(struct inno_hdmi *hdmi)
> > >> if (data->enc_in_format == data->enc_out_format) {
> > >> if ((data->enc_in_format == HDMI_COLORSPACE_RGB) ||
> > >> (data->enc_in_format >= HDMI_COLORSPACE_YUV444)) {
> > >> - value = v_SOF_DISABLE | v_COLOR_DEPTH_NOT_INDICATED(1);
> > >> - hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL3, value);
> > >> -
> > >> - hdmi_modb(hdmi, HDMI_VIDEO_CONTRL,
> > >> - m_VIDEO_AUTO_CSC | m_VIDEO_C0_C2_SWAP,
> > >> - v_VIDEO_AUTO_CSC(AUTO_CSC_DISABLE) |
> > >> - v_VIDEO_C0_C2_SWAP(C0_C2_CHANGE_DISABLE));
> > >> - return 0;
> > >> + if (data->enc_in_format == HDMI_COLORSPACE_RGB &&
> > >> + data->enc_out_format == HDMI_COLORSPACE_RGB &&
> > >> + hdmi->hdmi_data.rgb_limited_range) {
> > >> + csc_mode = CSC_RGB_0_255_TO_RGB_16_235_8BIT;
> > >> + auto_csc = AUTO_CSC_DISABLE;
> > >> + c0_c2_change = C0_C2_CHANGE_DISABLE;
> > >> + csc_enable = v_CSC_ENABLE;
> > >> + } else {
> > >> + value = v_SOF_DISABLE | v_COLOR_DEPTH_NOT_INDICATED(1);
> > >> + hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL3, value);
> > >> + hdmi_modb(hdmi, HDMI_VIDEO_CONTRL,
> > >> + m_VIDEO_AUTO_CSC | m_VIDEO_C0_C2_SWAP,
> > >> + v_VIDEO_AUTO_CSC(AUTO_CSC_DISABLE) |
> > >> + v_VIDEO_C0_C2_SWAP(C0_C2_CHANGE_DISABLE));
> > >> + return 0;
> > >> + }
> > >> }
> > >> }
> > >>
> > >> @@ -458,6 +479,9 @@ static int inno_hdmi_setup(struct inno_hdmi *hdmi,
> > >> else
> > >> hdmi->hdmi_data.colorimetry = HDMI_COLORIMETRY_ITU_709;
> > >>
> > >> + hdmi->hdmi_data.rgb_limited_range =
> > >> + drm_default_rgb_quant_range(mode) == HDMI_QUANTIZATION_RANGE_LIMITED;
> > >> +
> > > This patch conflicts heavily with my inno_hdmi patches here (patches 22 to 38):
> > > https://lore.kernel.org/dri-devel/[email protected]/
> > I'm aware of that and I mentioned it in the cover letter. Your series is
> > not merged yet and it didn't get much feedback so far. What is the
> > status there? Especially because you are removing things from inno-hdmi
> > driver (which aren't really required to remove there) which will I have
> > to reintroduce.
>
> Sadly I haven't found the time to look closer at Maxime's series so far,
> but I got the impression that it separates into multiple cleanup steps
> for a number of controllers.

Yeah, one of the previous version comment was to support more
controllers than vc4, which is fair. So I ended up reworking and
converting multiple controllers, but most of the clean up changes can be
applied outside of that series just fine.

I just didn't find someone to test / review them yet :)

Maxime

Attachments:

2023-12-14 11:41:55

by Maxime Ripard

Subject: Re: [PATCH 07/11] drm/rockchip: inno_hdmi: Add basic mode validation

On Thu, Dec 14, 2023 at 12:17:39PM +0100, Alex Bee wrote:
> Hi Maxime,
>
> Am 14.12.23 um 09:05 schrieb Maxime Ripard:
> > Hi,
> >
> > On Wed, Dec 13, 2023 at 08:51:21PM +0100, Alex Bee wrote:
> > > As per TRM this controller supports pixelclocks starting from 25 MHz. The
> > > maximum supported pixelclocks are defined by the phy configurations we
> > > have. Also it can't support modes that require doubled clocks.
> > > If there is a phy reference clock we can additionally validate against
> > > VESA DMT's recommendations.
> > > Those checks are added to the mode_valid hook of the connector and
> > > encoder's mode_fixup hook.
> > >
> > > Signed-off-by: Alex Bee <[email protected]>
> > > ---
> > > drivers/gpu/drm/rockchip/inno_hdmi.c | 38 ++++++++++++++++++++++++++--
> > > 1 file changed, 36 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
> > > index f7f0bec725f9..2f839ff31c1c 100644
> > > --- a/drivers/gpu/drm/rockchip/inno_hdmi.c
> > > +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
> > > @@ -38,6 +38,8 @@ struct inno_hdmi_variant {
> > > struct inno_hdmi_phy_config *default_phy_config;
> > > };
> > > +#define INNO_HDMI_MIN_TMDS_CLOCK 25000000U
> > > +
> > > struct hdmi_data_info {
> > > int vic;
> > > bool sink_has_audio;
> > > @@ -572,6 +574,34 @@ static int inno_hdmi_setup(struct inno_hdmi *hdmi,
> > > return 0;
> > > }
> > > +static enum drm_mode_status inno_hdmi_mode_valid(struct inno_hdmi *hdmi,
> > > + struct drm_display_mode *mode)
> > > +{
> > So, mode_valid is only called to filter out the modes retrieved by
> > get_modes, but it won't be called when userspace programs a mode. That's
> > atomic_check's job.
> >
> > So you probably want to create a shared function between atomic_check
> > and mode_valid, and call it from both places (or call mode_valid from
> > atomic_check).
>
> This actually _is_ a shared function called in
> inno_hdmi_connector_mode_valid and inno_hdmi_encoder_mode_fixup. Yes, I?
> probably should use it in atomic_check _also_.

Yeah, I saw that later and forgot to rephrase.

> > > + /* No support for double-clock modes */
> > > + if (mode->flags & DRM_MODE_FLAG_DBLCLK)
> > > + return MODE_BAD;
> > > +
> > > + unsigned int mpixelclk = mode->clock * 1000;
> > Variables should be declared at the top of the function.
> Oookay ... can move them.
> > > + if (mpixelclk < INNO_HDMI_MIN_TMDS_CLOCK)
> > > + return MODE_CLOCK_LOW;
> > You probably want to check the max TMDS clock too?
>
> Not sure what you mean here. For the currently supported formats of this
> driver (rgb only) tmds clock and pixel clock are always the same.

I mean that your controller has a maximum TMDS rate it supports too
(probably something like 340MHz). You should also filter out the modes
that have a pixel clock higher than the one you can reach.

> > > @@ -602,7 +632,9 @@ static bool inno_hdmi_encoder_mode_fixup(struct drm_encoder *encoder,
> > > const struct drm_display_mode *mode,
> > > struct drm_display_mode *adj_mode)
> > > {
> > > - return true;
> > > + struct inno_hdmi *hdmi = encoder_to_inno_hdmi(encoder);
> > > +
> > > + return inno_hdmi_mode_valid(hdmi, adj_mode) == MODE_OK;
> > > }
> > Why do you call mode_valid in mode_fixup?
>
> I'm calling the shared function you asked me to introduce
> (inno_hdmi_connector_mode_valid != inno_mode_valid)

That's the mode_fixup part that I'm focused on :)

mode_fixup is the legacy function to adjust the mode to the controller
capabilities. It's optional, and you're not adjusting anything here,
just testing the same thing mode_valid did.

mode_valid has been superseeded by atomic_check anyway, so just drop
mode_valid and use your function in atomic_check like we discussed.

Maxime

Attachments:

2023-12-14 11:53:52

by Andy Yan

Subject: Re:Re: [PATCH 04/11] drm/rockchip: inno_hdmi: Correctly setup HDMI quantization range

Hi：

在 2023-12-14 19:36:05，"Maxime Ripard" <[email protected]> 写道：
>On Thu, Dec 14, 2023 at 12:17:34PM +0100, Heiko Stübner wrote:
>> Am Donnerstag, 14. Dezember 2023, 12:12:08 CET schrieb Alex Bee:
>> > Hi Maxime
>> >
>> > Am 14.12.23 um 08:56 schrieb Maxime Ripard:
>> > > Hi Alex,
>> > >
>> > > Thanks for working on this!
>> > >
>> > > On Wed, Dec 13, 2023 at 08:51:18PM +0100, Alex Bee wrote:
>> > >> The display controller will always give full range RGB regardless of the
>> > >> mode set, but HDMI requires certain modes to be transmitted in limited
>> > >> range RGB. This is especially required for HDMI sinks which do not support
>> > >> non-standard quantization ranges.
>> > >>
>> > >> This enables color space conversion for those modes and sets the
>> > >> quantization range accordingly in the AVI infoframe.
>> > >>
>> > >> Fixes: 412d4ae6b7a5 ("drm/rockchip: hdmi: add Innosilicon HDMI support")
>> > >> Signed-off-by: Alex Bee <[email protected]>
>> > >> ---
>> > >> drivers/gpu/drm/rockchip/inno_hdmi.c | 40 ++++++++++++++++++++++------
>> > >> 1 file changed, 32 insertions(+), 8 deletions(-)
>> > >>
>> > >> diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
>> > >> index 345253e033c5..32626a75723c 100644
>> > >> --- a/drivers/gpu/drm/rockchip/inno_hdmi.c
>> > >> +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
>> > >> @@ -33,6 +33,7 @@ struct hdmi_data_info {
>> > >> unsigned int enc_in_format;
>> > >> unsigned int enc_out_format;
>> > >> unsigned int colorimetry;
>> > >> + bool rgb_limited_range;
>> > >> };
>> > >>
>> > >> struct inno_hdmi_i2c {
>> > >> @@ -308,6 +309,18 @@ static int inno_hdmi_config_video_avi(struct inno_hdmi *hdmi,
>> > >> else
>> > >> frame.avi.colorspace = HDMI_COLORSPACE_RGB;
>> > >>
>> > >> + if (hdmi->hdmi_data.enc_out_format == HDMI_COLORSPACE_RGB) {
>> > >> + drm_hdmi_avi_infoframe_quant_range(&frame.avi,
>> > >> + &hdmi->connector, mode,
>> > >> + hdmi->hdmi_data.rgb_limited_range ?
>> > >> + HDMI_QUANTIZATION_RANGE_LIMITED :
>> > >> + HDMI_QUANTIZATION_RANGE_FULL);
>> > >> + } else {
>> > >> + frame.avi.quantization_range = HDMI_QUANTIZATION_RANGE_DEFAULT;
>> > >> + frame.avi.ycc_quantization_range =
>> > >> + HDMI_YCC_QUANTIZATION_RANGE_LIMITED;
>> > >> + }
>> > >> +
>> > >> return inno_hdmi_upload_frame(hdmi, rc, &frame, INFOFRAME_AVI, 0, 0, 0);
>> > >> }
>> > >>
>> > >> @@ -334,14 +347,22 @@ static int inno_hdmi_config_video_csc(struct inno_hdmi *hdmi)
>> > >> if (data->enc_in_format == data->enc_out_format) {
>> > >> if ((data->enc_in_format == HDMI_COLORSPACE_RGB) ||
>> > >> (data->enc_in_format >= HDMI_COLORSPACE_YUV444)) {
>> > >> - value = v_SOF_DISABLE | v_COLOR_DEPTH_NOT_INDICATED(1);
>> > >> - hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL3, value);
>> > >> -
>> > >> - hdmi_modb(hdmi, HDMI_VIDEO_CONTRL,
>> > >> - m_VIDEO_AUTO_CSC | m_VIDEO_C0_C2_SWAP,
>> > >> - v_VIDEO_AUTO_CSC(AUTO_CSC_DISABLE) |
>> > >> - v_VIDEO_C0_C2_SWAP(C0_C2_CHANGE_DISABLE));
>> > >> - return 0;
>> > >> + if (data->enc_in_format == HDMI_COLORSPACE_RGB &&
>> > >> + data->enc_out_format == HDMI_COLORSPACE_RGB &&
>> > >> + hdmi->hdmi_data.rgb_limited_range) {
>> > >> + csc_mode = CSC_RGB_0_255_TO_RGB_16_235_8BIT;
>> > >> + auto_csc = AUTO_CSC_DISABLE;
>> > >> + c0_c2_change = C0_C2_CHANGE_DISABLE;
>> > >> + csc_enable = v_CSC_ENABLE;
>> > >> + } else {
>> > >> + value = v_SOF_DISABLE | v_COLOR_DEPTH_NOT_INDICATED(1);
>> > >> + hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL3, value);
>> > >> + hdmi_modb(hdmi, HDMI_VIDEO_CONTRL,
>> > >> + m_VIDEO_AUTO_CSC | m_VIDEO_C0_C2_SWAP,
>> > >> + v_VIDEO_AUTO_CSC(AUTO_CSC_DISABLE) |
>> > >> + v_VIDEO_C0_C2_SWAP(C0_C2_CHANGE_DISABLE));
>> > >> + return 0;
>> > >> + }
>> > >> }
>> > >> }
>> > >>
>> > >> @@ -458,6 +479,9 @@ static int inno_hdmi_setup(struct inno_hdmi *hdmi,
>> > >> else
>> > >> hdmi->hdmi_data.colorimetry = HDMI_COLORIMETRY_ITU_709;
>> > >>
>> > >> + hdmi->hdmi_data.rgb_limited_range =
>> > >> + drm_default_rgb_quant_range(mode) == HDMI_QUANTIZATION_RANGE_LIMITED;
>> > >> +
>> > > This patch conflicts heavily with my inno_hdmi patches here (patches 22 to 38):
>> > > https://lore.kernel.org/dri-devel/[email protected]/
>> > I'm aware of that and I mentioned it in the cover letter. Your series is
>> > not merged yet and it didn't get much feedback so far. What is the
>> > status there? Especially because you are removing things from inno-hdmi
>> > driver (which aren't really required to remove there) which will I have
>> > to reintroduce.
>>
>> Sadly I haven't found the time to look closer at Maxime's series so far,
>> but I got the impression that it separates into multiple cleanup steps
>> for a number of controllers.
>
>Yeah, one of the previous version comment was to support more
>controllers than vc4, which is fair. So I ended up reworking and
>converting multiple controllers, but most of the clean up changes can be
>applied outside of that series just fine.
>
>I just didn't find someone to test / review them yet :)

I will try to bring up my rk3036 kylin board whith mainline kernel this weekend， then I can do some tests.

>
>Maxime

2023-12-14 13:05:43

by Alex Bee

Subject: Re: [PATCH 07/11] drm/rockchip: inno_hdmi: Add basic mode validation

Am 14.12.23 um 12:40 schrieb Maxime Ripard:
> On Thu, Dec 14, 2023 at 12:17:39PM +0100, Alex Bee wrote:
>> Hi Maxime,
>>
>> Am 14.12.23 um 09:05 schrieb Maxime Ripard:
>>> Hi,
>>>
>>> On Wed, Dec 13, 2023 at 08:51:21PM +0100, Alex Bee wrote:
>>>> As per TRM this controller supports pixelclocks starting from 25 MHz. The
>>>> maximum supported pixelclocks are defined by the phy configurations we
>>>> have. Also it can't support modes that require doubled clocks.
>>>> If there is a phy reference clock we can additionally validate against
>>>> VESA DMT's recommendations.
>>>> Those checks are added to the mode_valid hook of the connector and
>>>> encoder's mode_fixup hook.
>>>>
>>>> Signed-off-by: Alex Bee <[email protected]>
>>>> ---
>>>> drivers/gpu/drm/rockchip/inno_hdmi.c | 38 ++++++++++++++++++++++++++--
>>>> 1 file changed, 36 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
>>>> index f7f0bec725f9..2f839ff31c1c 100644
>>>> --- a/drivers/gpu/drm/rockchip/inno_hdmi.c
>>>> +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
>>>> @@ -38,6 +38,8 @@ struct inno_hdmi_variant {
>>>> struct inno_hdmi_phy_config *default_phy_config;
>>>> };
>>>> +#define INNO_HDMI_MIN_TMDS_CLOCK 25000000U
>>>> +
>>>> struct hdmi_data_info {
>>>> int vic;
>>>> bool sink_has_audio;
>>>> @@ -572,6 +574,34 @@ static int inno_hdmi_setup(struct inno_hdmi *hdmi,
>>>> return 0;
>>>> }
>>>> +static enum drm_mode_status inno_hdmi_mode_valid(struct inno_hdmi *hdmi,
>>>> + struct drm_display_mode *mode)
>>>> +{
>>> So, mode_valid is only called to filter out the modes retrieved by
>>> get_modes, but it won't be called when userspace programs a mode. That's
>>> atomic_check's job.
>>>
>>> So you probably want to create a shared function between atomic_check
>>> and mode_valid, and call it from both places (or call mode_valid from
>>> atomic_check).
>> This actually _is_ a shared function called in
>> inno_hdmi_connector_mode_valid and inno_hdmi_encoder_mode_fixup. Yes, I
>> probably should use it in atomic_check _also_.
> Yeah, I saw that later and forgot to rephrase.
>
>>>> + /* No support for double-clock modes */
>>>> + if (mode->flags & DRM_MODE_FLAG_DBLCLK)
>>>> + return MODE_BAD;
>>>> +
>>>> + unsigned int mpixelclk = mode->clock * 1000;
>>> Variables should be declared at the top of the function.
>> Oookay ... can move them.
>>>> + if (mpixelclk < INNO_HDMI_MIN_TMDS_CLOCK)
>>>> + return MODE_CLOCK_LOW;
>>> You probably want to check the max TMDS clock too?
>> Not sure what you mean here. For the currently supported formats of this
>> driver (rgb only) tmds clock and pixel clock are always the same.
> I mean that your controller has a maximum TMDS rate it supports too
> (probably something like 340MHz). You should also filter out the modes
> that have a pixel clock higher than the one you can reach.
Yes it does have it and it is defined by the phy configurations that do
exist. The mode is validated against those exactly below this statement.
(See commit message, btw.)
>
>>>> @@ -602,7 +632,9 @@ static bool inno_hdmi_encoder_mode_fixup(struct drm_encoder *encoder,
>>>> const struct drm_display_mode *mode,
>>>> struct drm_display_mode *adj_mode)
>>>> {
>>>> - return true;
>>>> + struct inno_hdmi *hdmi = encoder_to_inno_hdmi(encoder);
>>>> +
>>>> + return inno_hdmi_mode_valid(hdmi, adj_mode) == MODE_OK;
>>>> }
>>> Why do you call mode_valid in mode_fixup?
>> I'm calling the shared function you asked me to introduce
>> (inno_hdmi_connector_mode_valid != inno_mode_valid)
> That's the mode_fixup part that I'm focused on :)
>
> mode_fixup is the legacy function to adjust the mode to the controller
> capabilities. It's optional, and you're not adjusting anything here,
> just testing the same thing mode_valid did.
>
> mode_valid has been superseeded by atomic_check anyway, so just drop
> mode_valid and use your function in atomic_check like we discussed.

OK.

I just read that mode_fixup won't be called at all if atomic_check
exists. I will drop here and call in atomic_check only.

Alex

> Maxime

2023-12-14 14:06:54

by Alex Bee

Subject: Re: [PATCH 04/11] drm/rockchip: inno_hdmi: Correctly setup HDMI quantization range

Am 14.12.23 um 12:33 schrieb Maxime Ripard:
> On Thu, Dec 14, 2023 at 12:12:08PM +0100, Alex Bee wrote:
>> Hi Maxime
>>
>> Am 14.12.23 um 08:56 schrieb Maxime Ripard:
>>> Hi Alex,
>>>
>>> Thanks for working on this!
>>>
>>> On Wed, Dec 13, 2023 at 08:51:18PM +0100, Alex Bee wrote:
>>>> The display controller will always give full range RGB regardless of the
>>>> mode set, but HDMI requires certain modes to be transmitted in limited
>>>> range RGB. This is especially required for HDMI sinks which do not support
>>>> non-standard quantization ranges.
>>>>
>>>> This enables color space conversion for those modes and sets the
>>>> quantization range accordingly in the AVI infoframe.
>>>>
>>>> Fixes: 412d4ae6b7a5 ("drm/rockchip: hdmi: add Innosilicon HDMI support")
>>>> Signed-off-by: Alex Bee <[email protected]>
>>>> ---
>>>> drivers/gpu/drm/rockchip/inno_hdmi.c | 40 ++++++++++++++++++++++------
>>>> 1 file changed, 32 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
>>>> index 345253e033c5..32626a75723c 100644
>>>> --- a/drivers/gpu/drm/rockchip/inno_hdmi.c
>>>> +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
>>>> @@ -33,6 +33,7 @@ struct hdmi_data_info {
>>>> unsigned int enc_in_format;
>>>> unsigned int enc_out_format;
>>>> unsigned int colorimetry;
>>>> + bool rgb_limited_range;
>>>> };
>>>> struct inno_hdmi_i2c {
>>>> @@ -308,6 +309,18 @@ static int inno_hdmi_config_video_avi(struct inno_hdmi *hdmi,
>>>> else
>>>> frame.avi.colorspace = HDMI_COLORSPACE_RGB;
>>>> + if (hdmi->hdmi_data.enc_out_format == HDMI_COLORSPACE_RGB) {
>>>> + drm_hdmi_avi_infoframe_quant_range(&frame.avi,
>>>> + &hdmi->connector, mode,
>>>> + hdmi->hdmi_data.rgb_limited_range ?
>>>> + HDMI_QUANTIZATION_RANGE_LIMITED :
>>>> + HDMI_QUANTIZATION_RANGE_FULL);
>>>> + } else {
>>>> + frame.avi.quantization_range = HDMI_QUANTIZATION_RANGE_DEFAULT;
>>>> + frame.avi.ycc_quantization_range =
>>>> + HDMI_YCC_QUANTIZATION_RANGE_LIMITED;
>>>> + }
>>>> +
>>>> return inno_hdmi_upload_frame(hdmi, rc, &frame, INFOFRAME_AVI, 0, 0, 0);
>>>> }
>>>> @@ -334,14 +347,22 @@ static int inno_hdmi_config_video_csc(struct inno_hdmi *hdmi)
>>>> if (data->enc_in_format == data->enc_out_format) {
>>>> if ((data->enc_in_format == HDMI_COLORSPACE_RGB) ||
>>>> (data->enc_in_format >= HDMI_COLORSPACE_YUV444)) {
>>>> - value = v_SOF_DISABLE | v_COLOR_DEPTH_NOT_INDICATED(1);
>>>> - hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL3, value);
>>>> -
>>>> - hdmi_modb(hdmi, HDMI_VIDEO_CONTRL,
>>>> - m_VIDEO_AUTO_CSC | m_VIDEO_C0_C2_SWAP,
>>>> - v_VIDEO_AUTO_CSC(AUTO_CSC_DISABLE) |
>>>> - v_VIDEO_C0_C2_SWAP(C0_C2_CHANGE_DISABLE));
>>>> - return 0;
>>>> + if (data->enc_in_format == HDMI_COLORSPACE_RGB &&
>>>> + data->enc_out_format == HDMI_COLORSPACE_RGB &&
>>>> + hdmi->hdmi_data.rgb_limited_range) {
>>>> + csc_mode = CSC_RGB_0_255_TO_RGB_16_235_8BIT;
>>>> + auto_csc = AUTO_CSC_DISABLE;
>>>> + c0_c2_change = C0_C2_CHANGE_DISABLE;
>>>> + csc_enable = v_CSC_ENABLE;
>>>> + } else {
>>>> + value = v_SOF_DISABLE | v_COLOR_DEPTH_NOT_INDICATED(1);
>>>> + hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL3, value);
>>>> + hdmi_modb(hdmi, HDMI_VIDEO_CONTRL,
>>>> + m_VIDEO_AUTO_CSC | m_VIDEO_C0_C2_SWAP,
>>>> + v_VIDEO_AUTO_CSC(AUTO_CSC_DISABLE) |
>>>> + v_VIDEO_C0_C2_SWAP(C0_C2_CHANGE_DISABLE));
>>>> + return 0;
>>>> + }
>>>> }
>>>> }
>>>> @@ -458,6 +479,9 @@ static int inno_hdmi_setup(struct inno_hdmi *hdmi,
>>>> else
>>>> hdmi->hdmi_data.colorimetry = HDMI_COLORIMETRY_ITU_709;
>>>> + hdmi->hdmi_data.rgb_limited_range =
>>>> + drm_default_rgb_quant_range(mode) == HDMI_QUANTIZATION_RANGE_LIMITED;
>>>> +
>>> This patch conflicts heavily with my inno_hdmi patches here (patches 22 to 38):
>>> https://lore.kernel.org/dri-devel/[email protected]/
>> I'm aware of that and I mentioned it in the cover letter.
> Sorry, I missed that part.
>
>> Your series is not merged yet and it didn't get much feedback so far.
>> What is the status there?
> It didn't have much reviews, but I'll hope to change that. For the
> patches 22 to 38 though, it doesn't really matter. Those changes are
> self-contained and can be applied as is outside of the series.
>
>> Especially because you are removing things from inno-hdmi driver (which
>> aren't really required to remove there) which will I have to reintroduce.
> I'm not entirely sure which part I remove that are actually going to be
> used here.
I'm refering to [PATCH v5 33/44] which completly removes csc coeffs but
this series needs the CSC_RGB_0_255_TO_RGB_16_235_8BIT  coeffs and 
[PATCH v5 29/44] which removes writing csc_coeffs to the hardware.
>
>>> I would appreciate if you could test and merge them into your series.
>>>
>>> In particular, there's no need to store the range here: enc_out_format
>> rgb_limited_range is currently not only used for csc, but also for for
>> infoframe creation. So it makes sense to have this stored  to avoid calling
>> drm_default_rgb_quant_range twice.
> You're right, I missed one. Still, it shouldn't be stored in the
> hdmi_data_info structure, it's tied to the mode, and the mode is part of
> the state, so it's not a property to a given device, but it's tied to
> the connector state.
>
> So if you want to do so, you should really create a custom state
> structure and store the range there, just like vc4 is doing for example.

OK - I'll check.

Alex

>
> Maxime

2023-12-14 16:35:48

by Alex Bee

Subject: Re: [PATCH 04/11] drm/rockchip: inno_hdmi: Correctly setup HDMI quantization range

Hi Heiko, Hi Maxime,

Am 14.12.23 um 12:36 schrieb Maxime Ripard:
> On Thu, Dec 14, 2023 at 12:17:34PM +0100, Heiko Stübner wrote:
>> Am Donnerstag, 14. Dezember 2023, 12:12:08 CET schrieb Alex Bee:
>>> Hi Maxime
>>>
>>> Am 14.12.23 um 08:56 schrieb Maxime Ripard:
>>>> Hi Alex,
>>>>
>>>> Thanks for working on this!
>>>>
>>>> On Wed, Dec 13, 2023 at 08:51:18PM +0100, Alex Bee wrote:
>>>>> The display controller will always give full range RGB regardless of the
>>>>> mode set, but HDMI requires certain modes to be transmitted in limited
>>>>> range RGB. This is especially required for HDMI sinks which do not support
>>>>> non-standard quantization ranges.
>>>>>
>>>>> This enables color space conversion for those modes and sets the
>>>>> quantization range accordingly in the AVI infoframe.
>>>>>
>>>>> Fixes: 412d4ae6b7a5 ("drm/rockchip: hdmi: add Innosilicon HDMI support")
>>>>> Signed-off-by: Alex Bee <[email protected]>
>>>>> ---
>>>>> drivers/gpu/drm/rockchip/inno_hdmi.c | 40 ++++++++++++++++++++++------
>>>>> 1 file changed, 32 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
>>>>> index 345253e033c5..32626a75723c 100644
>>>>> --- a/drivers/gpu/drm/rockchip/inno_hdmi.c
>>>>> +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
>>>>> @@ -33,6 +33,7 @@ struct hdmi_data_info {
>>>>> unsigned int enc_in_format;
>>>>> unsigned int enc_out_format;
>>>>> unsigned int colorimetry;
>>>>> + bool rgb_limited_range;
>>>>> };
>>>>>
>>>>> struct inno_hdmi_i2c {
>>>>> @@ -308,6 +309,18 @@ static int inno_hdmi_config_video_avi(struct inno_hdmi *hdmi,
>>>>> else
>>>>> frame.avi.colorspace = HDMI_COLORSPACE_RGB;
>>>>>
>>>>> + if (hdmi->hdmi_data.enc_out_format == HDMI_COLORSPACE_RGB) {
>>>>> + drm_hdmi_avi_infoframe_quant_range(&frame.avi,
>>>>> + &hdmi->connector, mode,
>>>>> + hdmi->hdmi_data.rgb_limited_range ?
>>>>> + HDMI_QUANTIZATION_RANGE_LIMITED :
>>>>> + HDMI_QUANTIZATION_RANGE_FULL);
>>>>> + } else {
>>>>> + frame.avi.quantization_range = HDMI_QUANTIZATION_RANGE_DEFAULT;
>>>>> + frame.avi.ycc_quantization_range =
>>>>> + HDMI_YCC_QUANTIZATION_RANGE_LIMITED;
>>>>> + }
>>>>> +
>>>>> return inno_hdmi_upload_frame(hdmi, rc, &frame, INFOFRAME_AVI, 0, 0, 0);
>>>>> }
>>>>>
>>>>> @@ -334,14 +347,22 @@ static int inno_hdmi_config_video_csc(struct inno_hdmi *hdmi)
>>>>> if (data->enc_in_format == data->enc_out_format) {
>>>>> if ((data->enc_in_format == HDMI_COLORSPACE_RGB) ||
>>>>> (data->enc_in_format >= HDMI_COLORSPACE_YUV444)) {
>>>>> - value = v_SOF_DISABLE | v_COLOR_DEPTH_NOT_INDICATED(1);
>>>>> - hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL3, value);
>>>>> -
>>>>> - hdmi_modb(hdmi, HDMI_VIDEO_CONTRL,
>>>>> - m_VIDEO_AUTO_CSC | m_VIDEO_C0_C2_SWAP,
>>>>> - v_VIDEO_AUTO_CSC(AUTO_CSC_DISABLE) |
>>>>> - v_VIDEO_C0_C2_SWAP(C0_C2_CHANGE_DISABLE));
>>>>> - return 0;
>>>>> + if (data->enc_in_format == HDMI_COLORSPACE_RGB &&
>>>>> + data->enc_out_format == HDMI_COLORSPACE_RGB &&
>>>>> + hdmi->hdmi_data.rgb_limited_range) {
>>>>> + csc_mode = CSC_RGB_0_255_TO_RGB_16_235_8BIT;
>>>>> + auto_csc = AUTO_CSC_DISABLE;
>>>>> + c0_c2_change = C0_C2_CHANGE_DISABLE;
>>>>> + csc_enable = v_CSC_ENABLE;
>>>>> + } else {
>>>>> + value = v_SOF_DISABLE | v_COLOR_DEPTH_NOT_INDICATED(1);
>>>>> + hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL3, value);
>>>>> + hdmi_modb(hdmi, HDMI_VIDEO_CONTRL,
>>>>> + m_VIDEO_AUTO_CSC | m_VIDEO_C0_C2_SWAP,
>>>>> + v_VIDEO_AUTO_CSC(AUTO_CSC_DISABLE) |
>>>>> + v_VIDEO_C0_C2_SWAP(C0_C2_CHANGE_DISABLE));
>>>>> + return 0;
>>>>> + }
>>>>> }
>>>>> }
>>>>>
>>>>> @@ -458,6 +479,9 @@ static int inno_hdmi_setup(struct inno_hdmi *hdmi,
>>>>> else
>>>>> hdmi->hdmi_data.colorimetry = HDMI_COLORIMETRY_ITU_709;
>>>>>
>>>>> + hdmi->hdmi_data.rgb_limited_range =
>>>>> + drm_default_rgb_quant_range(mode) == HDMI_QUANTIZATION_RANGE_LIMITED;
>>>>> +
>>>> This patch conflicts heavily with my inno_hdmi patches here (patches 22 to 38):
>>>> https://lore.kernel.org/dri-devel/[email protected]/
>>> I'm aware of that and I mentioned it in the cover letter. Your series is
>>> not merged yet and it didn't get much feedback so far. What is the
>>> status there? Especially because you are removing things from inno-hdmi
>>> driver (which aren't really required to remove there) which will I have
>>> to reintroduce.
>> Sadly I haven't found the time to look closer at Maxime's series so far,
>> but I got the impression that it separates into multiple cleanup steps
>> for a number of controllers.
> Yeah, one of the previous version comment was to support more
> controllers than vc4, which is fair. So I ended up reworking and
> converting multiple controllers, but most of the clean up changes can be
> applied outside of that series just fine.
>
> I just didn't find someone to test / review them yet :)

I'm not exactly sure how to proceed here. Do you want me to:

- base my patches on top of Maxime's series and reintroduce csc things
(of course only those which touch inno-hdmi w/o the framwork changes)

- only apply the patches that do not touch csc things and base my
series  on top of that

- adapt Maxime's patches so that RGB full to RGB limited stays in there

- wait with resend until Maxime's series is merged and reintroduce csc
after that

- something else

?

Please advise.

Thanks,

Alex

> Maxime

2023-12-15 12:28:21

by Maxime Ripard

Subject: Re: [PATCH 04/11] drm/rockchip: inno_hdmi: Correctly setup HDMI quantization range

On Thu, Dec 14, 2023 at 03:05:59PM +0100, Alex Bee wrote:
>
> Am 14.12.23 um 12:33 schrieb Maxime Ripard:
> > On Thu, Dec 14, 2023 at 12:12:08PM +0100, Alex Bee wrote:
> > > Hi Maxime
> > >
> > > Am 14.12.23 um 08:56 schrieb Maxime Ripard:
> > > > Hi Alex,
> > > >
> > > > Thanks for working on this!
> > > >
> > > > On Wed, Dec 13, 2023 at 08:51:18PM +0100, Alex Bee wrote:
> > > > > The display controller will always give full range RGB regardless of the
> > > > > mode set, but HDMI requires certain modes to be transmitted in limited
> > > > > range RGB. This is especially required for HDMI sinks which do not support
> > > > > non-standard quantization ranges.
> > > > >
> > > > > This enables color space conversion for those modes and sets the
> > > > > quantization range accordingly in the AVI infoframe.
> > > > >
> > > > > Fixes: 412d4ae6b7a5 ("drm/rockchip: hdmi: add Innosilicon HDMI support")
> > > > > Signed-off-by: Alex Bee <[email protected]>
> > > > > ---
> > > > > drivers/gpu/drm/rockchip/inno_hdmi.c | 40 ++++++++++++++++++++++------
> > > > > 1 file changed, 32 insertions(+), 8 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
> > > > > index 345253e033c5..32626a75723c 100644
> > > > > --- a/drivers/gpu/drm/rockchip/inno_hdmi.c
> > > > > +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
> > > > > @@ -33,6 +33,7 @@ struct hdmi_data_info {
> > > > > unsigned int enc_in_format;
> > > > > unsigned int enc_out_format;
> > > > > unsigned int colorimetry;
> > > > > + bool rgb_limited_range;
> > > > > };
> > > > > struct inno_hdmi_i2c {
> > > > > @@ -308,6 +309,18 @@ static int inno_hdmi_config_video_avi(struct inno_hdmi *hdmi,
> > > > > else
> > > > > frame.avi.colorspace = HDMI_COLORSPACE_RGB;
> > > > > + if (hdmi->hdmi_data.enc_out_format == HDMI_COLORSPACE_RGB) {
> > > > > + drm_hdmi_avi_infoframe_quant_range(&frame.avi,
> > > > > + &hdmi->connector, mode,
> > > > > + hdmi->hdmi_data.rgb_limited_range ?
> > > > > + HDMI_QUANTIZATION_RANGE_LIMITED :
> > > > > + HDMI_QUANTIZATION_RANGE_FULL);
> > > > > + } else {
> > > > > + frame.avi.quantization_range = HDMI_QUANTIZATION_RANGE_DEFAULT;
> > > > > + frame.avi.ycc_quantization_range =
> > > > > + HDMI_YCC_QUANTIZATION_RANGE_LIMITED;
> > > > > + }
> > > > > +
> > > > > return inno_hdmi_upload_frame(hdmi, rc, &frame, INFOFRAME_AVI, 0, 0, 0);
> > > > > }
> > > > > @@ -334,14 +347,22 @@ static int inno_hdmi_config_video_csc(struct inno_hdmi *hdmi)
> > > > > if (data->enc_in_format == data->enc_out_format) {
> > > > > if ((data->enc_in_format == HDMI_COLORSPACE_RGB) ||
> > > > > (data->enc_in_format >= HDMI_COLORSPACE_YUV444)) {
> > > > > - value = v_SOF_DISABLE | v_COLOR_DEPTH_NOT_INDICATED(1);
> > > > > - hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL3, value);
> > > > > -
> > > > > - hdmi_modb(hdmi, HDMI_VIDEO_CONTRL,
> > > > > - m_VIDEO_AUTO_CSC | m_VIDEO_C0_C2_SWAP,
> > > > > - v_VIDEO_AUTO_CSC(AUTO_CSC_DISABLE) |
> > > > > - v_VIDEO_C0_C2_SWAP(C0_C2_CHANGE_DISABLE));
> > > > > - return 0;
> > > > > + if (data->enc_in_format == HDMI_COLORSPACE_RGB &&
> > > > > + data->enc_out_format == HDMI_COLORSPACE_RGB &&
> > > > > + hdmi->hdmi_data.rgb_limited_range) {
> > > > > + csc_mode = CSC_RGB_0_255_TO_RGB_16_235_8BIT;
> > > > > + auto_csc = AUTO_CSC_DISABLE;
> > > > > + c0_c2_change = C0_C2_CHANGE_DISABLE;
> > > > > + csc_enable = v_CSC_ENABLE;
> > > > > + } else {
> > > > > + value = v_SOF_DISABLE | v_COLOR_DEPTH_NOT_INDICATED(1);
> > > > > + hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL3, value);
> > > > > + hdmi_modb(hdmi, HDMI_VIDEO_CONTRL,
> > > > > + m_VIDEO_AUTO_CSC | m_VIDEO_C0_C2_SWAP,
> > > > > + v_VIDEO_AUTO_CSC(AUTO_CSC_DISABLE) |
> > > > > + v_VIDEO_C0_C2_SWAP(C0_C2_CHANGE_DISABLE));
> > > > > + return 0;
> > > > > + }
> > > > > }
> > > > > }
> > > > > @@ -458,6 +479,9 @@ static int inno_hdmi_setup(struct inno_hdmi *hdmi,
> > > > > else
> > > > > hdmi->hdmi_data.colorimetry = HDMI_COLORIMETRY_ITU_709;
> > > > > + hdmi->hdmi_data.rgb_limited_range =
> > > > > + drm_default_rgb_quant_range(mode) == HDMI_QUANTIZATION_RANGE_LIMITED;
> > > > > +
> > > > This patch conflicts heavily with my inno_hdmi patches here (patches 22 to 38):
> > > > https://lore.kernel.org/dri-devel/[email protected]/
> > > I'm aware of that and I mentioned it in the cover letter.
> > Sorry, I missed that part.
> >
> > > Your series is not merged yet and it didn't get much feedback so far.
> > > What is the status there?
> > It didn't have much reviews, but I'll hope to change that. For the
> > patches 22 to 38 though, it doesn't really matter. Those changes are
> > self-contained and can be applied as is outside of the series.
> >
> > > Especially because you are removing things from inno-hdmi driver (which
> > > aren't really required to remove there) which will I have to reintroduce.
> > I'm not entirely sure which part I remove that are actually going to be
> > used here.
>
> I'm refering to [PATCH v5 33/44] which completly removes csc coeffs but this
> series needs the CSC_RGB_0_255_TO_RGB_16_235_8BIT? coeffs and? [PATCH v5
> 29/44] which removes writing csc_coeffs to the hardware.

Oh, right, feel free to drop those

Maxime

Attachments:

2023-12-15 12:29:15

by Maxime Ripard

Subject: Re: [PATCH 04/11] drm/rockchip: inno_hdmi: Correctly setup HDMI quantization range

On Thu, Dec 14, 2023 at 05:34:59PM +0100, Alex Bee wrote:
> Hi Heiko, Hi Maxime,
>
> Am 14.12.23 um 12:36 schrieb Maxime Ripard:
> > On Thu, Dec 14, 2023 at 12:17:34PM +0100, Heiko St?bner wrote:
> > > Am Donnerstag, 14. Dezember 2023, 12:12:08 CET schrieb Alex Bee:
> > > > Hi Maxime
> > > >
> > > > Am 14.12.23 um 08:56 schrieb Maxime Ripard:
> > > > > Hi Alex,
> > > > >
> > > > > Thanks for working on this!
> > > > >
> > > > > On Wed, Dec 13, 2023 at 08:51:18PM +0100, Alex Bee wrote:
> > > > > > The display controller will always give full range RGB regardless of the
> > > > > > mode set, but HDMI requires certain modes to be transmitted in limited
> > > > > > range RGB. This is especially required for HDMI sinks which do not support
> > > > > > non-standard quantization ranges.
> > > > > >
> > > > > > This enables color space conversion for those modes and sets the
> > > > > > quantization range accordingly in the AVI infoframe.
> > > > > >
> > > > > > Fixes: 412d4ae6b7a5 ("drm/rockchip: hdmi: add Innosilicon HDMI support")
> > > > > > Signed-off-by: Alex Bee <[email protected]>
> > > > > > ---
> > > > > > drivers/gpu/drm/rockchip/inno_hdmi.c | 40 ++++++++++++++++++++++------
> > > > > > 1 file changed, 32 insertions(+), 8 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/rockchip/inno_hdmi.c b/drivers/gpu/drm/rockchip/inno_hdmi.c
> > > > > > index 345253e033c5..32626a75723c 100644
> > > > > > --- a/drivers/gpu/drm/rockchip/inno_hdmi.c
> > > > > > +++ b/drivers/gpu/drm/rockchip/inno_hdmi.c
> > > > > > @@ -33,6 +33,7 @@ struct hdmi_data_info {
> > > > > > unsigned int enc_in_format;
> > > > > > unsigned int enc_out_format;
> > > > > > unsigned int colorimetry;
> > > > > > + bool rgb_limited_range;
> > > > > > };
> > > > > > struct inno_hdmi_i2c {
> > > > > > @@ -308,6 +309,18 @@ static int inno_hdmi_config_video_avi(struct inno_hdmi *hdmi,
> > > > > > else
> > > > > > frame.avi.colorspace = HDMI_COLORSPACE_RGB;
> > > > > > + if (hdmi->hdmi_data.enc_out_format == HDMI_COLORSPACE_RGB) {
> > > > > > + drm_hdmi_avi_infoframe_quant_range(&frame.avi,
> > > > > > + &hdmi->connector, mode,
> > > > > > + hdmi->hdmi_data.rgb_limited_range ?
> > > > > > + HDMI_QUANTIZATION_RANGE_LIMITED :
> > > > > > + HDMI_QUANTIZATION_RANGE_FULL);
> > > > > > + } else {
> > > > > > + frame.avi.quantization_range = HDMI_QUANTIZATION_RANGE_DEFAULT;
> > > > > > + frame.avi.ycc_quantization_range =
> > > > > > + HDMI_YCC_QUANTIZATION_RANGE_LIMITED;
> > > > > > + }
> > > > > > +
> > > > > > return inno_hdmi_upload_frame(hdmi, rc, &frame, INFOFRAME_AVI, 0, 0, 0);
> > > > > > }
> > > > > > @@ -334,14 +347,22 @@ static int inno_hdmi_config_video_csc(struct inno_hdmi *hdmi)
> > > > > > if (data->enc_in_format == data->enc_out_format) {
> > > > > > if ((data->enc_in_format == HDMI_COLORSPACE_RGB) ||
> > > > > > (data->enc_in_format >= HDMI_COLORSPACE_YUV444)) {
> > > > > > - value = v_SOF_DISABLE | v_COLOR_DEPTH_NOT_INDICATED(1);
> > > > > > - hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL3, value);
> > > > > > -
> > > > > > - hdmi_modb(hdmi, HDMI_VIDEO_CONTRL,
> > > > > > - m_VIDEO_AUTO_CSC | m_VIDEO_C0_C2_SWAP,
> > > > > > - v_VIDEO_AUTO_CSC(AUTO_CSC_DISABLE) |
> > > > > > - v_VIDEO_C0_C2_SWAP(C0_C2_CHANGE_DISABLE));
> > > > > > - return 0;
> > > > > > + if (data->enc_in_format == HDMI_COLORSPACE_RGB &&
> > > > > > + data->enc_out_format == HDMI_COLORSPACE_RGB &&
> > > > > > + hdmi->hdmi_data.rgb_limited_range) {
> > > > > > + csc_mode = CSC_RGB_0_255_TO_RGB_16_235_8BIT;
> > > > > > + auto_csc = AUTO_CSC_DISABLE;
> > > > > > + c0_c2_change = C0_C2_CHANGE_DISABLE;
> > > > > > + csc_enable = v_CSC_ENABLE;
> > > > > > + } else {
> > > > > > + value = v_SOF_DISABLE | v_COLOR_DEPTH_NOT_INDICATED(1);
> > > > > > + hdmi_writeb(hdmi, HDMI_VIDEO_CONTRL3, value);
> > > > > > + hdmi_modb(hdmi, HDMI_VIDEO_CONTRL,
> > > > > > + m_VIDEO_AUTO_CSC | m_VIDEO_C0_C2_SWAP,
> > > > > > + v_VIDEO_AUTO_CSC(AUTO_CSC_DISABLE) |
> > > > > > + v_VIDEO_C0_C2_SWAP(C0_C2_CHANGE_DISABLE));
> > > > > > + return 0;
> > > > > > + }
> > > > > > }
> > > > > > }
> > > > > > @@ -458,6 +479,9 @@ static int inno_hdmi_setup(struct inno_hdmi *hdmi,
> > > > > > else
> > > > > > hdmi->hdmi_data.colorimetry = HDMI_COLORIMETRY_ITU_709;
> > > > > > + hdmi->hdmi_data.rgb_limited_range =
> > > > > > + drm_default_rgb_quant_range(mode) == HDMI_QUANTIZATION_RANGE_LIMITED;
> > > > > > +
> > > > > This patch conflicts heavily with my inno_hdmi patches here (patches 22 to 38):
> > > > > https://lore.kernel.org/dri-devel/[email protected]/
> > > > I'm aware of that and I mentioned it in the cover letter. Your series is
> > > > not merged yet and it didn't get much feedback so far. What is the
> > > > status there? Especially because you are removing things from inno-hdmi
> > > > driver (which aren't really required to remove there) which will I have
> > > > to reintroduce.
> > > Sadly I haven't found the time to look closer at Maxime's series so far,
> > > but I got the impression that it separates into multiple cleanup steps
> > > for a number of controllers.
> > Yeah, one of the previous version comment was to support more
> > controllers than vc4, which is fair. So I ended up reworking and
> > converting multiple controllers, but most of the clean up changes can be
> > applied outside of that series just fine.
> >
> > I just didn't find someone to test / review them yet :)
>
> I'm not exactly sure how to proceed here. Do you want me to:
>
> - base my patches on top of Maxime's series and reintroduce csc things (of
> course only those which touch inno-hdmi w/o the framwork changes)
>
> - only apply the patches that do not touch csc things and base my series? on
> top of that
>
> - adapt Maxime's patches so that RGB full to RGB limited stays in there
>
> - wait with resend until Maxime's series is merged and reintroduce csc after
> that
>
> - something else
>
> ?

2 or 3, at your discretion

Maxime

Attachments: