[PATCH v16 0/4] MIPS: JZ4780 and CI20 HDMI

[PATCH v16 0/4] MIPS: JZ4780 and CI20 HDMI

[H. Nikolaus Schaller]

PATCH V16 2022-02-26 18:13:02:
* fixed and renamed dw-hdmi bus negotiation patch (by narmstrong@baylibre.com)
* reordered and merged HPD fix (suggested by paul@crapouillou.net)
* fixed MODULE_ALIAS for dw-hdmi-ingenic (reported by paul@crapouillou.net)
* dropped some already merged commits from the series

PATCH V15 2022-02-12 16:50:54:
* remove already (elsewhere) merged commits (suggested by paul@crapouillou.net)
* clarify commit message for (now) 1/7 ((suggested by paul@crapouillou.net))

PATCH V14 2022-02-12 15:19:25:
* make compatible to c03d0b52ff71d5 ("drm/connector: Fix typo in output format")
* move "dw-hdmi/ingenic-dw-hdmi: repair interworking with hdmi-connector" before
  drm/ingenic: Add dw-hdmi driver specialization for jz4780 (by paul@crapouillou.net)
* split introduction of dw_hdmi_enable_poll() into separate patch
* explicitly mark plane f0 as not working in jz4780 (suggested by paul@crapouillou.net)
* drop 1/9 since it is now in drm-misc/drm-misc-next

PATCH V13 2022-02-02 17:31:22:
* 7/9: remove call to gpiod_set_value() because of GPIOD_OUT_HIGH (by paul@crapouillou.net)
* 4/9: replace ".." by "." (by paul@crapouillou.net)
* 3/9: remove old hdmi-5v-power in the example (by paul@crapouillou.net)
* 2/9: disable handling of plane f0 only for jz4780 (by paul@crapouillou.net)

PATCH V12 2022-01-31 13:26:54:
This version reworks how hdmi ddc power is controlled by connector and not
by ddc/hdmi bridge driver.

Also some patches of the previous version of this series have been removed
since they are already applied to mips-next/linux/next/v5.17-rc1.

Fixes and changes:

- repair interworking of dw-hdmi with connector-hdmi (by hns@goldelico.com)
- fix JZ_REG_LCD_OSDC setup for jz4780 (by hns@goldelico.com and paul@crapouillou.net)
- adjustments for ci20.dts to use connector gpio for +5v (suggested by several)
- to add control of "ddc-en-gpios" to hdmi-connector driver (by hns@goldelico.com)
- regulator code removed because we now use the "ddc-en-gpios" of the connector
  driver (suggested by paul@crapouillou.net)
- bindings: addition of "ddc-i2c-bus" and "hdmi-5v-supply" removed (suggested by robh+dt@kernel.org)
- rebase on v5.17-rc2

PATCH V11 2021-12-02 19:39:52:
- patch 4/8: change devm_regulator_get_optional to devm_regulator_get and
             remove NULL check (requested by broonie@kernel.org)
- patch 3/8: make hdmi-5v-supply required (requested by broonie@kernel.org)

PATCH V10 2021-11-30 22:26:41:
- patch 3/8: fix $id and $ref paths (found by robh@kernel.org)

PATCH V9 2021-11-24 22:29:14:
- patch 6/8: remove optional <0> for assigned-clocks and unintentionally included "unwedge" setup (found by paul@crapouillou.net)
- patch 4/8: some cosmetics
             make regulator enable/disable only if not NULL (found by paul@crapouillou.net)
             simplify/fix error handling and driver cleanup on remove (proposed by paul@crapouillou.net)
- patch 3/8: fix #include path in example (found by paul@crapouillou.net)
             fix missing "i" in unevaluatedProperties (found by robh@kernel.org)
             fix 4 spaces indentation for required: property (found by robh@kernel.org)

PATCH V8 2021-11-23 19:14:00:
- fix a bad editing result from patch 2/8 (found by paul@crapouillou.net)

PATCH V7 2021-11-23 18:46:23:
- changed gpio polarity of hdmi_power to 0 (suggested by paul@crapouillou.net)
- fixed LCD1 irq number (bug found by paul@crapouillou.net)
- removed "- 4" for calculating max_register (suggested by paul@crapouillou.net)
- use unevaluatedPropertes instead of additionalProperties (suggested by robh@kernel.org)
- moved and renamed ingenic,jz4780-hdmi.yaml (suggested by robh@kernel.org)
- adjusted assigned-clocks changes to upstream which added some for SSI (by hns@goldelico.com)
- rebased and tested with v5.16-rc2 + patch set drm/ingenic by paul@crapouillou.net (by hns@goldelico.com)

PATCH V6 2021-11-10 20:43:33:
- changed CONFIG_DRM_INGENIC_DW_HDMI to "m" (by hns@goldelico.com)
- made ingenic-dw-hdmi an independent platform driver which can be compiled as module
  and removed error patch fixes for IPU (suggested by paul@crapouillou.net)
- moved assigned-clocks from jz4780.dtsi to ci20.dts (suggested by paul@crapouillou.net)
- fixed reg property in jz4780.dtsi to cover all registers incl. gamma and vee (by hns@goldelico.com)
- added a base patch to calculate regmap size from DTS reg property (requested by paul@crapouillou.net)
- restored resetting all bits except one in LCDOSDC (requested by paul@crapouillou.net)
- clarified setting of cpos (suggested by paul@crapouillou.net)
- moved bindings definition for ddc-i2c-bus (suggested by paul@crapouillou.net)
- simplified mask definitions for JZ_LCD_DESSIZE (requested by paul@crapouillou.net)
- removed setting alpha premultiplication (suggested by paul@crapouillou.net)
- removed some comments (suggested by paul@crapouillou.net)

PATCH V5 2021-10-05 14:28:44:
- dropped mode_fixup and timings support in dw-hdmi as it is no longer needed in this V5 (by hns@goldelico.com)
- dropped "drm/ingenic: add some jz4780 specific features" (stimulated by paul@crapouillou.net)
- fixed typo in commit subject: "synopsis" -> "synopsys" (by hns@goldelico.com)
- swapped clocks in jz4780.dtsi to match synopsys,dw-hdmi.yaml (by hns@goldelico.com)
- improved, simplified, fixed, dtbschecked ingenic-jz4780-hdmi.yaml and made dependent of bridge/synopsys,dw-hdmi.yaml (based on suggestions by maxime@cerno.tech)
- fixed binding vs. driver&DTS use of hdmi-5v regulator (suggested by maxime@cerno.tech)
- dropped "drm/bridge: synopsis: Fix to properly handle HPD" - was a no longer needed workaround for a previous version
  (suggested by maxime@cerno.tech)

PATCH V4 2021-09-27 18:44:38:
- fix setting output_port = 1 (issue found by paul@crapouillou.net)
- ci20.dts: convert to use hdmi-connector (by hns@goldelico.com)
- add a hdmi-regulator to control +5V power (by hns@goldelico.com)
- added a fix to dw-hdmi to call drm_kms_helper_hotplug_event on plugin event detection (by hns@goldelico.com)
- always allocate extended descriptor but initialize only for jz4780 (by hns@goldelico.com)
- updated to work on top of "[PATCH v3 0/6] drm/ingenic: Various improvements v3" (by paul@crapouillou.net)
- rebased to v5.13-rc3

PATCH V3 2021-08-08 07:10:50:
This series adds HDMI support for JZ4780 and CI20 board (and fixes one IPU related issue in registration error path)
- [patch 1/8] switched from mode_fixup to atomic_check (suggested by robert.foss@linaro.org)
  - the call to the dw-hdmi specialization is still called mode_fixup
- [patch 3/8] diverse fixes for ingenic-drm-drv (suggested by paul@crapouillou.net)
  - factor out some non-HDMI features of the jz4780 into a separate patch
  - multiple fixes around max height
  - do not change regmap config but a copy on stack
  - define some constants
  - factor out fixing of drm_init error path for IPU into separate patch
  - use FIELD_PREP()
- [patch 8/8] conversion to component framework dropped (suggested by Laurent.pinchart@ideasonboard.com and paul@crapouillou.net)

PATCH V2 2021-08-05 16:08:05:
- code and commit messages revisited for checkpatch warnings
- rebased on v5.14-rc4
- include (failed, hence RFC 8/8) attempt to convert to component framework
  (was suggested by Paul Cercueil <paul@crapouillou.net> a while ago)

This series adds HDMI support for JZ4780 and CI20 board



H. Nikolaus Schaller (3):
  drm/bridge: dw-hdmi: introduce dw_hdmi_enable_poll()
  drm/bridge: display-connector: add ddc-en gpio support
  drm/bridge: dw-hdmi: fix bus formats negotiation for 8 bit modes

Paul Boddie (1):
  drm/ingenic: Add dw-hdmi driver specialization for jz4780

 drivers/gpu/drm/bridge/display-connector.c |  15 +++
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c  |  19 +++-
 drivers/gpu/drm/ingenic/Kconfig            |   9 ++
 drivers/gpu/drm/ingenic/Makefile           |   1 +
 drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c  | 105 +++++++++++++++++++++
 include/drm/bridge/dw_hdmi.h               |   1 +
 6 files changed, 146 insertions(+), 4 deletions(-)
 create mode 100644 drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c

-- 
2.33.0

[PATCH v16 1/4] drm/bridge: dw-hdmi: introduce dw_hdmi_enable_poll()

[H. Nikolaus Schaller]

so that specialization drivers like ingenic-dw-hdmi can enable polling.

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 9 +++++++++
 include/drm/bridge/dw_hdmi.h              | 1 +
 2 files changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index 4befc104d2200..43e375da131e8 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -3217,6 +3217,15 @@ static int dw_hdmi_parse_dt(struct dw_hdmi *hdmi)
 	return 0;
 }
 
+void dw_hdmi_enable_poll(struct dw_hdmi *hdmi, bool enable)
+{
+	if (hdmi->bridge.dev)
+		hdmi->bridge.dev->mode_config.poll_enabled = enable;
+	else
+		dev_warn(hdmi->dev, "no hdmi->bridge.dev");
+}
+EXPORT_SYMBOL_GPL(dw_hdmi_enable_poll);
+
 struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev,
 			      const struct dw_hdmi_plat_data *plat_data)
 {
diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
index 2a1f85f9a8a3f..963960794b40e 100644
--- a/include/drm/bridge/dw_hdmi.h
+++ b/include/drm/bridge/dw_hdmi.h
@@ -196,5 +196,6 @@ enum drm_connector_status dw_hdmi_phy_read_hpd(struct dw_hdmi *hdmi,
 void dw_hdmi_phy_update_hpd(struct dw_hdmi *hdmi, void *data,
 			    bool force, bool disabled, bool rxsense);
 void dw_hdmi_phy_setup_hpd(struct dw_hdmi *hdmi, void *data);
+void dw_hdmi_enable_poll(struct dw_hdmi *hdmi, bool enable);
 
 #endif /* __IMX_HDMI_H__ */
-- 
2.33.0

[PATCH v16 2/4] drm/ingenic: Add dw-hdmi driver specialization for jz4780

[H. Nikolaus Schaller]

From: Paul Boddie <paul@boddie.org.uk>

A specialisation of the generic Synopsys HDMI driver is employed for
JZ4780 HDMI support. This requires a new driver, plus device tree and
configuration modifications.

Here we add Kconfig DRM_INGENIC_DW_HDMI, Makefile and driver code.

Note that there is no hpd-gpio installed on the CI20 board HDMI
connector. Hence there is no hpd detection by the connector driver
and we have to enable polling in the dw-hdmi core driver.

For that we need to set .poll_enabled but that struct component
can only be accessed by core code. Hence we use the public
setter function drm_kms_helper_hotplug_event() introduced before.

Signed-off-by: Paul Boddie <paul@boddie.org.uk>
Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 drivers/gpu/drm/ingenic/Kconfig           |   9 ++
 drivers/gpu/drm/ingenic/Makefile          |   1 +
 drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c | 105 ++++++++++++++++++++++
 3 files changed, 115 insertions(+)
 create mode 100644 drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c

diff --git a/drivers/gpu/drm/ingenic/Kconfig b/drivers/gpu/drm/ingenic/Kconfig
index 001f59fb06d56..090830bcbde7f 100644
--- a/drivers/gpu/drm/ingenic/Kconfig
+++ b/drivers/gpu/drm/ingenic/Kconfig
@@ -24,4 +24,13 @@ config DRM_INGENIC_IPU
 
 	  The Image Processing Unit (IPU) will appear as a second primary plane.
 
+config DRM_INGENIC_DW_HDMI
+	tristate "Ingenic specific support for Synopsys DW HDMI"
+	depends on MACH_JZ4780
+	select DRM_DW_HDMI
+	help
+	  Choose this option to enable Synopsys DesignWare HDMI based driver.
+	  If you want to enable HDMI on Ingenic JZ4780 based SoC, you should
+	  select this option.
+
 endif
diff --git a/drivers/gpu/drm/ingenic/Makefile b/drivers/gpu/drm/ingenic/Makefile
index d313326bdddbb..f10cc1c5a5f22 100644
--- a/drivers/gpu/drm/ingenic/Makefile
+++ b/drivers/gpu/drm/ingenic/Makefile
@@ -1,3 +1,4 @@
 obj-$(CONFIG_DRM_INGENIC) += ingenic-drm.o
 ingenic-drm-y = ingenic-drm-drv.o
 ingenic-drm-$(CONFIG_DRM_INGENIC_IPU) += ingenic-ipu.o
+obj-$(CONFIG_DRM_INGENIC_DW_HDMI) += ingenic-dw-hdmi.o
diff --git a/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c b/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c
new file mode 100644
index 0000000000000..508d4b6bfa243
--- /dev/null
+++ b/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c
@@ -0,0 +1,105 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (C) 2011-2013 Freescale Semiconductor, Inc.
+ * Copyright (C) 2019, 2020 Paul Boddie <paul@boddie.org.uk>
+ *
+ * Derived from dw_hdmi-imx.c with i.MX portions removed.
+ */
+
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+
+#include <drm/bridge/dw_hdmi.h>
+#include <drm/drm_of.h>
+#include <drm/drm_print.h>
+
+static const struct dw_hdmi_mpll_config ingenic_mpll_cfg[] = {
+	{ 45250000,  { { 0x01e0, 0x0000 }, { 0x21e1, 0x0000 }, { 0x41e2, 0x0000 } } },
+	{ 92500000,  { { 0x0140, 0x0005 }, { 0x2141, 0x0005 }, { 0x4142, 0x0005 } } },
+	{ 148500000, { { 0x00a0, 0x000a }, { 0x20a1, 0x000a }, { 0x40a2, 0x000a } } },
+	{ 216000000, { { 0x00a0, 0x000a }, { 0x2001, 0x000f }, { 0x4002, 0x000f } } },
+	{ ~0UL,      { { 0x0000, 0x0000 }, { 0x0000, 0x0000 }, { 0x0000, 0x0000 } } }
+};
+
+static const struct dw_hdmi_curr_ctrl ingenic_cur_ctr[] = {
+	/*pixelclk     bpp8    bpp10   bpp12 */
+	{ 54000000,  { 0x091c, 0x091c, 0x06dc } },
+	{ 58400000,  { 0x091c, 0x06dc, 0x06dc } },
+	{ 72000000,  { 0x06dc, 0x06dc, 0x091c } },
+	{ 74250000,  { 0x06dc, 0x0b5c, 0x091c } },
+	{ 118800000, { 0x091c, 0x091c, 0x06dc } },
+	{ 216000000, { 0x06dc, 0x0b5c, 0x091c } },
+	{ ~0UL,      { 0x0000, 0x0000, 0x0000 } },
+};
+
+/*
+ * Resistance term 133Ohm Cfg
+ * PREEMP config 0.00
+ * TX/CK level 10
+ */
+static const struct dw_hdmi_phy_config ingenic_phy_config[] = {
+	/*pixelclk   symbol   term   vlev */
+	{ 216000000, 0x800d, 0x0005, 0x01ad},
+	{ ~0UL,      0x0000, 0x0000, 0x0000}
+};
+
+static enum drm_mode_status
+ingenic_dw_hdmi_mode_valid(struct dw_hdmi *hdmi, void *data,
+			   const struct drm_display_info *info,
+			   const struct drm_display_mode *mode)
+{
+	if (mode->clock < 13500)
+		return MODE_CLOCK_LOW;
+	/* FIXME: Hardware is capable of 270MHz, but setup data is missing. */
+	if (mode->clock > 216000)
+		return MODE_CLOCK_HIGH;
+
+	dw_hdmi_enable_poll(hdmi, true);
+
+	return MODE_OK;
+}
+
+static struct dw_hdmi_plat_data ingenic_dw_hdmi_plat_data = {
+	.mpll_cfg   = ingenic_mpll_cfg,
+	.cur_ctr    = ingenic_cur_ctr,
+	.phy_config = ingenic_phy_config,
+	.mode_valid = ingenic_dw_hdmi_mode_valid,
+	.output_port	= 1,
+};
+
+static const struct of_device_id ingenic_dw_hdmi_dt_ids[] = {
+	{ .compatible = "ingenic,jz4780-dw-hdmi" },
+	{ /* Sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, ingenic_dw_hdmi_dt_ids);
+
+static void ingenic_dw_hdmi_cleanup(void *data)
+{
+	struct dw_hdmi *hdmi = (struct dw_hdmi *)data;
+
+	dw_hdmi_remove(hdmi);
+}
+
+static int ingenic_dw_hdmi_probe(struct platform_device *pdev)
+{
+	struct dw_hdmi *hdmi;
+
+	hdmi = dw_hdmi_probe(pdev, &ingenic_dw_hdmi_plat_data);
+	if (IS_ERR(hdmi))
+		return PTR_ERR(hdmi);
+
+	return devm_add_action_or_reset(&pdev->dev, ingenic_dw_hdmi_cleanup, hdmi);
+}
+
+static struct platform_driver ingenic_dw_hdmi_driver = {
+	.probe  = ingenic_dw_hdmi_probe,
+	.driver = {
+		.name = "dw-hdmi-ingenic",
+		.of_match_table = ingenic_dw_hdmi_dt_ids,
+	},
+};
+module_platform_driver(ingenic_dw_hdmi_driver);
+
+MODULE_DESCRIPTION("JZ4780 Specific DW-HDMI Driver Extension");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:dw-hdmi-ingenic");
-- 
2.33.0

[PATCH v16 3/4] drm/bridge: display-connector: add ddc-en gpio support

[H. Nikolaus Schaller]

"hdmi-connector.yaml" bindings defines an optional property
"ddc-en-gpios" for a single gpio to enable DDC operation.

Usually this controls +5V power on the HDMI connector.
This +5V may also be needed for HPD.

This was not reflected in code.

Now, the driver activates the ddc gpio after probe and
deactivates after remove so it is "almost on".

But only if this driver is loaded (and not e.g. blacklisted
as module).

Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 drivers/gpu/drm/bridge/display-connector.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/bridge/display-connector.c b/drivers/gpu/drm/bridge/display-connector.c
index d24f5b90feabf..e4d52a7e31b71 100644
--- a/drivers/gpu/drm/bridge/display-connector.c
+++ b/drivers/gpu/drm/bridge/display-connector.c
@@ -24,6 +24,7 @@ struct display_connector {
 	int			hpd_irq;
 
 	struct regulator	*dp_pwr;
+	struct gpio_desc	*ddc_en;
 };
 
 static inline struct display_connector *
@@ -345,6 +346,17 @@ static int display_connector_probe(struct platform_device *pdev)
 		}
 	}
 
+	/* enable DDC */
+	if (type == DRM_MODE_CONNECTOR_HDMIA) {
+		conn->ddc_en = devm_gpiod_get_optional(&pdev->dev, "ddc-en",
+						       GPIOD_OUT_HIGH);
+
+		if (IS_ERR(conn->ddc_en)) {
+			dev_err(&pdev->dev, "Couldn't get ddc-en gpio\n");
+			return PTR_ERR(conn->ddc_en);
+		}
+	}
+
 	conn->bridge.funcs = &display_connector_bridge_funcs;
 	conn->bridge.of_node = pdev->dev.of_node;
 
@@ -373,6 +385,9 @@ static int display_connector_remove(struct platform_device *pdev)
 {
 	struct display_connector *conn = platform_get_drvdata(pdev);
 
+	if (conn->ddc_en)
+		gpiod_set_value(conn->ddc_en, 0);
+
 	if (conn->dp_pwr)
 		regulator_disable(conn->dp_pwr);
 
-- 
2.33.0

[PATCH v16 4/4] drm/bridge: dw-hdmi: fix bus formats negotiation for 8 bit modes

[H. Nikolaus Schaller]

Commit 7cd70656d1285b ("drm/bridge: display-connector: implement bus fmts callbacks")

introduced a new mechanism to negotiate bus formats between hdmi connectors
and bridges which is to be used e.g. for the jz4780 based CI20 board.

In this case dw-hdmi sets up a list of formats in
dw_hdmi_bridge_atomic_get_output_bus_fmts().

This includes e.g. MEDIA_BUS_FMT_UYVY8_1X16 which is chosen for the CI20 but
only produces a black screen.

Analysis revealed an omission in

Commit 6c3c719936dafe ("drm/bridge: synopsys: dw-hdmi: add bus format negociation")

to check for 8 bit with when adding UYVY8 or YUV8 formats.

This fix is based on the observation that max_bpc = 0 when running this
function while info->bpc = 8.

Adding the proposed patch makes the jz4780/CI20 panel work again with default
MEDIA_BUS_FMT_RGB888_1X24 mode.

Fixes: 7cd70656d1285b ("drm/bridge: display-connector: implement bus fmts callbacks")
Fixes: 6c3c719936dafe ("drm/bridge: synopsys: dw-hdmi: add bus format negociation")
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index 43e375da131e8..c08e2cc96584c 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -2621,11 +2621,13 @@ static u32 *dw_hdmi_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge,
 		output_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30;
 	}
 
-	if (info->color_formats & DRM_COLOR_FORMAT_YCBCR422)
-		output_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16;
+	if (max_bpc >= 8 && info->bpc >= 8) {
+		if (info->color_formats & DRM_COLOR_FORMAT_YCBCR422)
+			output_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16;
 
-	if (info->color_formats & DRM_COLOR_FORMAT_YCBCR444)
-		output_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24;
+		if (info->color_formats & DRM_COLOR_FORMAT_YCBCR444)
+			output_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24;
+	}
 
 	/* Default 8bit RGB fallback */
 	output_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24;
-- 
2.33.0

Re: [PATCH v16 4/4] drm/bridge: dw-hdmi: fix bus formats negotiation for 8 bit modes

[Neil Armstrong]

Hi,

On 26/02/2022 18:13, H. Nikolaus Schaller wrote:
> Commit 7cd70656d1285b ("drm/bridge: display-connector: implement bus fmts callbacks")
> 
> introduced a new mechanism to negotiate bus formats between hdmi connectors
> and bridges which is to be used e.g. for the jz4780 based CI20 board.
> 
> In this case dw-hdmi sets up a list of formats in
> dw_hdmi_bridge_atomic_get_output_bus_fmts().
> 
> This includes e.g. MEDIA_BUS_FMT_UYVY8_1X16 which is chosen for the CI20 but
> only produces a black screen.
> 
> Analysis revealed an omission in
> 
> Commit 6c3c719936dafe ("drm/bridge: synopsys: dw-hdmi: add bus format negociation")
> 
> to check for 8 bit with when adding UYVY8 or YUV8 formats.
> 
> This fix is based on the observation that max_bpc = 0 when running this
> function while info->bpc = 8.

In fact if bpc = 0, it should be considered as 8, so the issue is elsewhere.

> 
> Adding the proposed patch makes the jz4780/CI20 panel work again with default
> MEDIA_BUS_FMT_RGB888_1X24 mode.
> 
> Fixes: 7cd70656d1285b ("drm/bridge: display-connector: implement bus fmts callbacks")
> Fixes: 6c3c719936dafe ("drm/bridge: synopsys: dw-hdmi: add bus format negociation")
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
>   drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 10 ++++++----
>   1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index 43e375da131e8..c08e2cc96584c 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -2621,11 +2621,13 @@ static u32 *dw_hdmi_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge,
>   		output_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30;
>   	}
>   
> -	if (info->color_formats & DRM_COLOR_FORMAT_YCBCR422)
> -		output_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16;
> +	if (max_bpc >= 8 && info->bpc >= 8) {
> +		if (info->color_formats & DRM_COLOR_FORMAT_YCBCR422)
> +			output_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16;
>   
> -	if (info->color_formats & DRM_COLOR_FORMAT_YCBCR444)
> -		output_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24;
> +		if (info->color_formats & DRM_COLOR_FORMAT_YCBCR444)
> +			output_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24;
> +	}

It should not select YUV here if it's not possible, so something is wrong.

Can you check if https://lore.kernel.org/r/20220119123656.1456355-2-narmstrong@baylibre.com fixes this issue instead ?

Neil

>   
>   	/* Default 8bit RGB fallback */
>   	output_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24;

Re: [PATCH v16 4/4] drm/bridge: dw-hdmi: fix bus formats negotiation for 8 bit modes

[H. Nikolaus Schaller]

Hi Neil,


> Am 01.03.2022 um 10:18 schrieb Neil Armstrong <narmstrong@baylibre.com>:
> 
> Hi,
> 
> On 26/02/2022 18:13, H. Nikolaus Schaller wrote:
>> Commit 7cd70656d1285b ("drm/bridge: display-connector: implement bus fmts callbacks")
>> introduced a new mechanism to negotiate bus formats between hdmi connectors
>> and bridges which is to be used e.g. for the jz4780 based CI20 board.
>> In this case dw-hdmi sets up a list of formats in
>> dw_hdmi_bridge_atomic_get_output_bus_fmts().
>> This includes e.g. MEDIA_BUS_FMT_UYVY8_1X16 which is chosen for the CI20 but
>> only produces a black screen.
>> Analysis revealed an omission in
>> Commit 6c3c719936dafe ("drm/bridge: synopsys: dw-hdmi: add bus format negociation")
>> to check for 8 bit with when adding UYVY8 or YUV8 formats.
>> This fix is based on the observation that max_bpc = 0 when running this
>> function while info->bpc = 8.
> 
> In fact if bpc = 0, it should be considered as 8, so the issue is elsewhere.
> 
>> Adding the proposed patch makes the jz4780/CI20 panel work again with default
>> MEDIA_BUS_FMT_RGB888_1X24 mode.
>> Fixes: 7cd70656d1285b ("drm/bridge: display-connector: implement bus fmts callbacks")
>> Fixes: 6c3c719936dafe ("drm/bridge: synopsys: dw-hdmi: add bus format negociation")
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> ---
>>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> index 43e375da131e8..c08e2cc96584c 100644
>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> @@ -2621,11 +2621,13 @@ static u32 *dw_hdmi_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge,
>>  		output_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30;
>>  	}
>>  -	if (info->color_formats & DRM_COLOR_FORMAT_YCBCR422)
>> -		output_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16;
>> +	if (max_bpc >= 8 && info->bpc >= 8) {
>> +		if (info->color_formats & DRM_COLOR_FORMAT_YCBCR422)
>> +			output_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16;
>>  -	if (info->color_formats & DRM_COLOR_FORMAT_YCBCR444)
>> -		output_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24;
>> +		if (info->color_formats & DRM_COLOR_FORMAT_YCBCR444)
>> +			output_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24;
>> +	}
> 
> It should not select YUV here if it's not possible, so something is wrong.
> 
> Can you check if https://lore.kernel.org/r/20220119123656.1456355-2-narmstrong@baylibre.com fixes this issue instead ?

Well, I had to manually fix it to be appliable to drm-misc/drm-misc-next
and specifically:

c03d0b52ff71 ("drm/connector: Fix typo in output format")

My resulting patch is attached.

Unfortunately it did not work.

I added a printk for hdmi->sink_is_hdmi. This returns 1. Which IMHO is to be expected
since I am using a HDMI connector and panel... So your patch will still add the UYVY formats.

Either the synposys module inside the jz4780 or the panel does not understand them.

Here is the EDID. Unfortunately it does not pretty print the extended descriptors for UYVY etc.
so that I don't know the exact capabilities of the panel. And what I am not sure is if the
jz4780 SoC can convert to UYVY or how it can.

root@letux:~# parse-edid </sys/devices/platform/13050000.lcdc0/drm/card0/card0-HDMI-A-1/edid
Checksum Correct

Section "Monitor"
        Identifier "LEN L1950wD"
        ModelName "LEN L1950wD"
        VendorName "LEN"
        # Monitor Manufactured week 34 of 2011
        # EDID version 1.3
        # Digital Display
        DisplaySize 410 260
        Gamma 2.20
        Option "DPMS" "true"
        Horizsync 30-81
        VertRefresh 50-76
        # Maximum pixel clock is 140MHz
        #Not giving standard mode: 1152x864, 75Hz
        #Not giving standard mode: 1280x720, 60Hz
        #Not giving standard mode: 1280x1024, 60Hz
        #Not giving standard mode: 1280x1024, 60Hz
        #Not giving standard mode: 1280x1024, 60Hz
        #Not giving standard mode: 1440x900, 60Hz
        #Not giving standard mode: 1440x900, 75Hz
        #Not giving standard mode: 1920x1080, 60Hz

        #Extension block found. Parsing...
        Modeline        "Mode 15" -hsync -vsync 
        Modeline        "Mode 0" -hsync +vsync 
        Modeline        "Mode 1" 27.027 1440 1478 1602 1716 480 484 487 525 -hsync -vsync interlace
        Modeline        "Mode 2" 27.027 1440 1478 1602 1716 480 484 487 525 -hsync -vsync interlace
        Modeline        "Mode 3" 27.027 720 736 798 858 480 489 495 525 -hsync -vsync
        Modeline        "Mode 4" 27.027 720 736 798 858 480 489 495 525 -hsync -vsync
        Modeline        "Mode 5" 27.000 1440 1464 1590 1728 576 578 581 625 -hsync -vsync interlace
        Modeline        "Mode 6" 27.000 1440 1464 1590 1728 576 578 581 625 -hsync -vsync interlace
        Modeline        "Mode 7" 27.000 720 732 796 864 576 581 586 625 -hsync -vsync
        Modeline        "Mode 8" 27.000 720 732 796 864 576 581 586 625 -hsync -vsync
        Modeline        "Mode 9" 74.250 1280 1720 1760 1980 720 725 730 750 +hsync +vsync
        Modeline        "Mode 10" 74.250 1280 1390 1420 1650 720 725 730 750 +hsync +vsync
        Modeline        "Mode 11" 74.250 1920 2448 2492 2640 1080 1082 1089 1125 +hsync +vsync interlace
        Modeline        "Mode 12" 74.250 1920 2008 2052 2200 1080 1082 1087 1125 +hsync +vsync interlace
        Modeline        "Mode 13" 148.500 1920 2448 2492 2640 1080 1084 1089 1125 +hsync +vsync
        Modeline        "Mode 14" 148.500 1920 2008 2052 2200 1080 1084 1089 1125 +hsync +vsync
        Modeline        "Mode 16" +hsync +vsync interlace
        Modeline        "Mode 17" +hsync +vsync interlace
        Modeline        "Mode 18" +hsync +vsync 
        Option "PreferredMode" "Mode 15"
EndSection
root@letux:~# xxd /sys/devices/platform/13050000.lcdc0/drm/card0/card0-HDMI-A-1/ 
00000000: 00ff ffff ffff ff00 30ae 8610 0101 0101  ........0.......
00000010: 2215 0103 8029 1a78 eee5 b5a3 5549 9927  "....).x....UI.'
00000020: 1350 54af ef00 714f 81c0 8180 8180 8180  .PT...qO........
00000030: 9500 950f d1c0 2413 0020 4158 1620 050d  ......$.. AX. ..
00000040: 2300 ffff 0000 001c 0000 00fc 004c 454e  #............LEN
00000050: 204c 3139 3530 7744 0a20 0000 00fd 0032   L1950wD. .....2
00000060: 4c1e 510e 000a 2020 2020 2020 0000 00ff  L.Q...      ....
00000070: 0042 3334 3332 3834 350a 2020 2020 0101  .B3432845.    ..
00000080: 0203 2171 4e06 0702 0315 9611 1213 0414  ..!qN...........
00000090: 051f 9023 0907 0783 0100 0065 030c 0010  ...#.......e....
000000a0: 008c 0ad0 9020 4031 200c 4055 00b9 8821  ..... @1 .@U...!
000000b0: 0000 1801 1d80 1871 1c16 2058 2c25 00b9  .......q.. X,%..
000000c0: 8821 0000 9e01 1d80 d072 1c16 2010 2c25  .!.......r.. .,%
000000d0: 80b9 8821 0000 9e01 1d00 bc52 d01e 20b8  ...!.......R.. .
000000e0: 2855 40b9 8821 0000 1e02 3a80 d072 382d  (U@..!....:..r8-
000000f0: 4010 2c45 80b9 8821 0000 1e00 0000 00d0  @.,E...!........
root@letux:~# root@letux:~# dmesg|grep dw.hdmi
[    9.622138] dw-hdmi-ingenic 10180000.hdmi: Detected HDMI TX controller v1.31a with HDCP (DWC HDMI 3D TX PHY)
[    9.727840] dw-hdmi-ingenic 10180000.hdmi: registered DesignWare HDMI I2C bus driver
[   10.103864] dw_hdmi_bridge_atomic_get_output_bus_fmts: hdmi->sink_is_hdmi=1

So please let me know which parameters I should try to printk()...

BR and thanks,
Nikolaus


------

From c84a3c4a500684e57b1243fe5386696c48fa1e1b Mon Sep 17 00:00:00 2001
From: Neil Armstrong <narmstrong@baylibre.com>
Date: Wed, 19 Jan 2022 13:36:56 +0100
Subject: [PATCH] drm/bridge: dw-hdmi: filter out YUV output formats when DVI

When the display is not an HDMI sink, only the RGB output format is
valid. Thus stop returning YUV output formats when sink is not HDMI.

Fixes: 6c3c719936da ("drm/bridge: synopsys: dw-hdmi: add bus format negociation")
Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index 43e375da131e8..0ec0cbe448e05 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -2538,6 +2538,7 @@ static u32 *dw_hdmi_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge,
        struct drm_connector *conn = conn_state->connector;
        struct drm_display_info *info = &conn->display_info;
        struct drm_display_mode *mode = &crtc_state->mode;
+       struct dw_hdmi *hdmi = bridge->driver_private;
        u8 max_bpc = conn_state->max_requested_bpc;
        bool is_hdmi2_sink = info->hdmi.scdc.supported ||
                             (info->color_formats & DRM_COLOR_FORMAT_YCBCR420);
@@ -2564,7 +2565,7 @@ static u32 *dw_hdmi_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge,
         * If the current mode enforces 4:2:0, force the output but format
         * to 4:2:0 and do not add the YUV422/444/RGB formats
         */
-       if (conn->ycbcr_420_allowed &&
+       if (hdmi->sink_is_hdmi && conn->ycbcr_420_allowed &&
            (drm_mode_is_420_only(info, mode) ||
             (is_hdmi2_sink && drm_mode_is_420_also(info, mode)))) {
 
@@ -2595,36 +2596,36 @@ static u32 *dw_hdmi_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge,
         */
 
        if (max_bpc >= 16 && info->bpc == 16) {
-               if (info->color_formats & DRM_COLOR_FORMAT_YCBCR444)
+               if (hdmi->sink_is_hdmi && info->color_formats & DRM_COLOR_FORMAT_YCBCR444)
                        output_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48;
 
                output_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48;
        }
 
        if (max_bpc >= 12 && info->bpc >= 12) {
-               if (info->color_formats & DRM_COLOR_FORMAT_YCBCR422)
+               if (hdmi->sink_is_hdmi && info->color_formats & DRM_COLOR_FORMAT_YCBCR422)
                        output_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24;
 
-               if (info->color_formats & DRM_COLOR_FORMAT_YCBCR444)
+               if (hdmi->sink_is_hdmi && info->color_formats & DRM_COLOR_FORMAT_YCBCR444)
                        output_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36;
 
                output_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36;
        }
 
        if (max_bpc >= 10 && info->bpc >= 10) {
-               if (info->color_formats & DRM_COLOR_FORMAT_YCBCR422)
+               if (hdmi->sink_is_hdmi && info->color_formats & DRM_COLOR_FORMAT_YCBCR422)
                        output_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20;
 
-               if (info->color_formats & DRM_COLOR_FORMAT_YCBCR444)
+               if (hdmi->sink_is_hdmi && info->color_formats & DRM_COLOR_FORMAT_YCBCR444)
                        output_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30;
 
                output_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30;
        }
 
-       if (info->color_formats & DRM_COLOR_FORMAT_YCBCR422)
+       if (hdmi->sink_is_hdmi && info->color_formats & DRM_COLOR_FORMAT_YCBCR422)
                output_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16;
 
-       if (info->color_formats & DRM_COLOR_FORMAT_YCBCR444)
+       if (hdmi->sink_is_hdmi && info->color_formats & DRM_COLOR_FORMAT_YCBCR444)
                output_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24;
 
        /* Default 8bit RGB fallback */
-- 
2.33.0

Re: [PATCH v16 4/4] drm/bridge: dw-hdmi: fix bus formats negotiation for 8 bit modes

[Neil Armstrong]

H,

On 01/03/2022 21:37, H. Nikolaus Schaller wrote:
> Hi Neil,
> 
> 
>> Am 01.03.2022 um 10:18 schrieb Neil Armstrong <narmstrong@baylibre.com>:
>>
>> Hi,
>>
>> On 26/02/2022 18:13, H. Nikolaus Schaller wrote:
>>> Commit 7cd70656d1285b ("drm/bridge: display-connector: implement bus fmts callbacks")
>>> introduced a new mechanism to negotiate bus formats between hdmi connectors
>>> and bridges which is to be used e.g. for the jz4780 based CI20 board.
>>> In this case dw-hdmi sets up a list of formats in
>>> dw_hdmi_bridge_atomic_get_output_bus_fmts().
>>> This includes e.g. MEDIA_BUS_FMT_UYVY8_1X16 which is chosen for the CI20 but
>>> only produces a black screen.
>>> Analysis revealed an omission in
>>> Commit 6c3c719936dafe ("drm/bridge: synopsys: dw-hdmi: add bus format negociation")
>>> to check for 8 bit with when adding UYVY8 or YUV8 formats.
>>> This fix is based on the observation that max_bpc = 0 when running this
>>> function while info->bpc = 8.
>>
>> In fact if bpc = 0, it should be considered as 8, so the issue is elsewhere.
>>
>>> Adding the proposed patch makes the jz4780/CI20 panel work again with default
>>> MEDIA_BUS_FMT_RGB888_1X24 mode.
>>> Fixes: 7cd70656d1285b ("drm/bridge: display-connector: implement bus fmts callbacks")
>>> Fixes: 6c3c719936dafe ("drm/bridge: synopsys: dw-hdmi: add bus format negociation")
>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>>> ---
>>>   drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 10 ++++++----
>>>   1 file changed, 6 insertions(+), 4 deletions(-)
>>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>> index 43e375da131e8..c08e2cc96584c 100644
>>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>> @@ -2621,11 +2621,13 @@ static u32 *dw_hdmi_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge,
>>>   		output_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30;
>>>   	}
>>>   -	if (info->color_formats & DRM_COLOR_FORMAT_YCBCR422)
>>> -		output_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16;
>>> +	if (max_bpc >= 8 && info->bpc >= 8) {
>>> +		if (info->color_formats & DRM_COLOR_FORMAT_YCBCR422)
>>> +			output_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16;
>>>   -	if (info->color_formats & DRM_COLOR_FORMAT_YCBCR444)
>>> -		output_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24;
>>> +		if (info->color_formats & DRM_COLOR_FORMAT_YCBCR444)
>>> +			output_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24;
>>> +	}
>>
>> It should not select YUV here if it's not possible, so something is wrong.
>>
>> Can you check if https://lore.kernel.org/r/20220119123656.1456355-2-narmstrong@baylibre.com fixes this issue instead ?
> 
> Well, I had to manually fix it to be appliable to drm-misc/drm-misc-next
> and specifically:
> 
> c03d0b52ff71 ("drm/connector: Fix typo in output format")
> 
> My resulting patch is attached.
> 
> Unfortunately it did not work.
> 
> I added a printk for hdmi->sink_is_hdmi. This returns 1. Which IMHO is to be expected
> since I am using a HDMI connector and panel... So your patch will still add the UYVY formats.
> 
> Either the synposys module inside the jz4780 or the panel does not understand them.

By selecting the UYVY formats, the driver will enable the colorspace converters in the dw-hdmi IP,
I don't see why it doesn't work here...

There is a bit called `Support Color Space Converter` in config0_id:
bit	|	Name	|	R/W	|	Desc
2 	|	csc	| 	R 	|	Indicates if Color Space Conversion block is present

Could you dump all the config0 bits:

=======================><=============================
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index 54d8fdad395f..547731482da8 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -3431,6 +3431,7 @@ struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev,
         pdevinfo.id = PLATFORM_DEVID_AUTO;

         config0 = hdmi_readb(hdmi, HDMI_CONFIG0_ID);
+       dev_info(dev, "config0: %x\n", config0);
         config3 = hdmi_readb(hdmi, HDMI_CONFIG3_ID);

         if (iores && config3 & HDMI_CONFIG3_AHBAUDDMA) {
=======================><=============================

If this bit is missing, this would explain the black screen.

Neil

> 
> Here is the EDID. Unfortunately it does not pretty print the extended descriptors for UYVY etc.
> so that I don't know the exact capabilities of the panel. And what I am not sure is if the
> jz4780 SoC can convert to UYVY or how it can.
> 
> root@letux:~# parse-edid </sys/devices/platform/13050000.lcdc0/drm/card0/card0-HDMI-A-1/edid
> Checksum Correct
> 
> Section "Monitor"
>          Identifier "LEN L1950wD"
>          ModelName "LEN L1950wD"
>          VendorName "LEN"
>          # Monitor Manufactured week 34 of 2011
>          # EDID version 1.3
>          # Digital Display
>          DisplaySize 410 260
>          Gamma 2.20
>          Option "DPMS" "true"
>          Horizsync 30-81
>          VertRefresh 50-76
>          # Maximum pixel clock is 140MHz
>          #Not giving standard mode: 1152x864, 75Hz
>          #Not giving standard mode: 1280x720, 60Hz
>          #Not giving standard mode: 1280x1024, 60Hz
>          #Not giving standard mode: 1280x1024, 60Hz
>          #Not giving standard mode: 1280x1024, 60Hz
>          #Not giving standard mode: 1440x900, 60Hz
>          #Not giving standard mode: 1440x900, 75Hz
>          #Not giving standard mode: 1920x1080, 60Hz
> 
>          #Extension block found. Parsing...
>          Modeline        "Mode 15" -hsync -vsync
>          Modeline        "Mode 0" -hsync +vsync
>          Modeline        "Mode 1" 27.027 1440 1478 1602 1716 480 484 487 525 -hsync -vsync interlace
>          Modeline        "Mode 2" 27.027 1440 1478 1602 1716 480 484 487 525 -hsync -vsync interlace
>          Modeline        "Mode 3" 27.027 720 736 798 858 480 489 495 525 -hsync -vsync
>          Modeline        "Mode 4" 27.027 720 736 798 858 480 489 495 525 -hsync -vsync
>          Modeline        "Mode 5" 27.000 1440 1464 1590 1728 576 578 581 625 -hsync -vsync interlace
>          Modeline        "Mode 6" 27.000 1440 1464 1590 1728 576 578 581 625 -hsync -vsync interlace
>          Modeline        "Mode 7" 27.000 720 732 796 864 576 581 586 625 -hsync -vsync
>          Modeline        "Mode 8" 27.000 720 732 796 864 576 581 586 625 -hsync -vsync
>          Modeline        "Mode 9" 74.250 1280 1720 1760 1980 720 725 730 750 +hsync +vsync
>          Modeline        "Mode 10" 74.250 1280 1390 1420 1650 720 725 730 750 +hsync +vsync
>          Modeline        "Mode 11" 74.250 1920 2448 2492 2640 1080 1082 1089 1125 +hsync +vsync interlace
>          Modeline        "Mode 12" 74.250 1920 2008 2052 2200 1080 1082 1087 1125 +hsync +vsync interlace
>          Modeline        "Mode 13" 148.500 1920 2448 2492 2640 1080 1084 1089 1125 +hsync +vsync
>          Modeline        "Mode 14" 148.500 1920 2008 2052 2200 1080 1084 1089 1125 +hsync +vsync
>          Modeline        "Mode 16" +hsync +vsync interlace
>          Modeline        "Mode 17" +hsync +vsync interlace
>          Modeline        "Mode 18" +hsync +vsync
>          Option "PreferredMode" "Mode 15"
> EndSection
> root@letux:~# xxd /sys/devices/platform/13050000.lcdc0/drm/card0/card0-HDMI-A-1/
> 00000000: 00ff ffff ffff ff00 30ae 8610 0101 0101  ........0.......
> 00000010: 2215 0103 8029 1a78 eee5 b5a3 5549 9927  "....).x....UI.'
> 00000020: 1350 54af ef00 714f 81c0 8180 8180 8180  .PT...qO........
> 00000030: 9500 950f d1c0 2413 0020 4158 1620 050d  ......$.. AX. ..
> 00000040: 2300 ffff 0000 001c 0000 00fc 004c 454e  #............LEN
> 00000050: 204c 3139 3530 7744 0a20 0000 00fd 0032   L1950wD. .....2
> 00000060: 4c1e 510e 000a 2020 2020 2020 0000 00ff  L.Q...      ....
> 00000070: 0042 3334 3332 3834 350a 2020 2020 0101  .B3432845.    ..
> 00000080: 0203 2171 4e06 0702 0315 9611 1213 0414  ..!qN...........
> 00000090: 051f 9023 0907 0783 0100 0065 030c 0010  ...#.......e....
> 000000a0: 008c 0ad0 9020 4031 200c 4055 00b9 8821  ..... @1 .@U...!
> 000000b0: 0000 1801 1d80 1871 1c16 2058 2c25 00b9  .......q.. X,%..
> 000000c0: 8821 0000 9e01 1d80 d072 1c16 2010 2c25  .!.......r.. .,%
> 000000d0: 80b9 8821 0000 9e01 1d00 bc52 d01e 20b8  ...!.......R.. .
> 000000e0: 2855 40b9 8821 0000 1e02 3a80 d072 382d  (U@..!....:..r8-
> 000000f0: 4010 2c45 80b9 8821 0000 1e00 0000 00d0  @.,E...!........
> root@letux:~# root@letux:~# dmesg|grep dw.hdmi
> [    9.622138] dw-hdmi-ingenic 10180000.hdmi: Detected HDMI TX controller v1.31a with HDCP (DWC HDMI 3D TX PHY)
> [    9.727840] dw-hdmi-ingenic 10180000.hdmi: registered DesignWare HDMI I2C bus driver
> [   10.103864] dw_hdmi_bridge_atomic_get_output_bus_fmts: hdmi->sink_is_hdmi=1
> 
> So please let me know which parameters I should try to printk()...
> 
> BR and thanks,
> Nikolaus
> 
> 
> ------
> 
>  From c84a3c4a500684e57b1243fe5386696c48fa1e1b Mon Sep 17 00:00:00 2001
> From: Neil Armstrong <narmstrong@baylibre.com>
> Date: Wed, 19 Jan 2022 13:36:56 +0100
> Subject: [PATCH] drm/bridge: dw-hdmi: filter out YUV output formats when DVI
> 
> When the display is not an HDMI sink, only the RGB output format is
> valid. Thus stop returning YUV output formats when sink is not HDMI.
> 
> Fixes: 6c3c719936da ("drm/bridge: synopsys: dw-hdmi: add bus format negociation")
> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>   drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 17 +++++++++--------
>   1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index 43e375da131e8..0ec0cbe448e05 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -2538,6 +2538,7 @@ static u32 *dw_hdmi_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge,
>          struct drm_connector *conn = conn_state->connector;
>          struct drm_display_info *info = &conn->display_info;
>          struct drm_display_mode *mode = &crtc_state->mode;
> +       struct dw_hdmi *hdmi = bridge->driver_private;
>          u8 max_bpc = conn_state->max_requested_bpc;
>          bool is_hdmi2_sink = info->hdmi.scdc.supported ||
>                               (info->color_formats & DRM_COLOR_FORMAT_YCBCR420);
> @@ -2564,7 +2565,7 @@ static u32 *dw_hdmi_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge,
>           * If the current mode enforces 4:2:0, force the output but format
>           * to 4:2:0 and do not add the YUV422/444/RGB formats
>           */
> -       if (conn->ycbcr_420_allowed &&
> +       if (hdmi->sink_is_hdmi && conn->ycbcr_420_allowed &&
>              (drm_mode_is_420_only(info, mode) ||
>               (is_hdmi2_sink && drm_mode_is_420_also(info, mode)))) {
>   
> @@ -2595,36 +2596,36 @@ static u32 *dw_hdmi_bridge_atomic_get_output_bus_fmts(struct drm_bridge *bridge,
>           */
>   
>          if (max_bpc >= 16 && info->bpc == 16) {
> -               if (info->color_formats & DRM_COLOR_FORMAT_YCBCR444)
> +               if (hdmi->sink_is_hdmi && info->color_formats & DRM_COLOR_FORMAT_YCBCR444)
>                          output_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48;
>   
>                  output_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48;
>          }
>   
>          if (max_bpc >= 12 && info->bpc >= 12) {
> -               if (info->color_formats & DRM_COLOR_FORMAT_YCBCR422)
> +               if (hdmi->sink_is_hdmi && info->color_formats & DRM_COLOR_FORMAT_YCBCR422)
>                          output_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24;
>   
> -               if (info->color_formats & DRM_COLOR_FORMAT_YCBCR444)
> +               if (hdmi->sink_is_hdmi && info->color_formats & DRM_COLOR_FORMAT_YCBCR444)
>                          output_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36;
>   
>                  output_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36;
>          }
>   
>          if (max_bpc >= 10 && info->bpc >= 10) {
> -               if (info->color_formats & DRM_COLOR_FORMAT_YCBCR422)
> +               if (hdmi->sink_is_hdmi && info->color_formats & DRM_COLOR_FORMAT_YCBCR422)
>                          output_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20;
>   
> -               if (info->color_formats & DRM_COLOR_FORMAT_YCBCR444)
> +               if (hdmi->sink_is_hdmi && info->color_formats & DRM_COLOR_FORMAT_YCBCR444)
>                          output_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30;
>   
>                  output_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30;
>          }
>   
> -       if (info->color_formats & DRM_COLOR_FORMAT_YCBCR422)
> +       if (hdmi->sink_is_hdmi && info->color_formats & DRM_COLOR_FORMAT_YCBCR422)
>                  output_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16;
>   
> -       if (info->color_formats & DRM_COLOR_FORMAT_YCBCR444)
> +       if (hdmi->sink_is_hdmi && info->color_formats & DRM_COLOR_FORMAT_YCBCR444)
>                  output_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24;
>   
>          /* Default 8bit RGB fallback */

Re: [PATCH v16 4/4] drm/bridge: dw-hdmi: fix bus formats negotiation for 8 bit modes

[H. Nikolaus Schaller]

Hi Neil,

> Am 02.03.2022 um 11:25 schrieb Neil Armstrong <narmstrong@baylibre.com>:
> 
>> I added a printk for hdmi->sink_is_hdmi. This returns 1. Which IMHO is to be expected
>> since I am using a HDMI connector and panel... So your patch will still add the UYVY formats.
>> Either the synposys module inside the jz4780 or the panel does not understand them.
> 
> By selecting the UYVY formats, the driver will enable the colorspace converters in the dw-hdmi IP,
> I don't see why it doesn't work here...
> 
> There is a bit called `Support Color Space Converter` in config0_id:
> bit	|	Name	|	R/W	|	Desc
> 2 	|	csc	| 	R 	|	Indicates if Color Space Conversion block is present
> 
> Could you dump all the config0 bits:
> 
> =======================><=============================
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index 54d8fdad395f..547731482da8 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -3431,6 +3431,7 @@ struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev,
>        pdevinfo.id = PLATFORM_DEVID_AUTO;
> 
>        config0 = hdmi_readb(hdmi, HDMI_CONFIG0_ID);
> +       dev_info(dev, "config0: %x\n", config0);
>        config3 = hdmi_readb(hdmi, HDMI_CONFIG3_ID);
> 
>        if (iores && config3 & HDMI_CONFIG3_AHBAUDDMA) {
> =======================><=============================
> 
> If this bit is missing, this would explain the black screen.

[    9.291011] dw-hdmi-ingenic 10180000.hdmi: config0: bf

Hm. Or is the color-space conversion of the sw-hdmi module inside the jz4780 broken
or not configured properly?

(cross-checked: RGB mode still works if I force hdmi->sink_is_hdmi = false)

BR and thanks,
Nikolaus

Re: [PATCH v16 4/4] drm/bridge: dw-hdmi: fix bus formats negotiation for 8 bit modes

[Neil Armstrong]

Hi,

On 02/03/2022 12:15, H. Nikolaus Schaller wrote:
> Hi Neil,
> 
>> Am 02.03.2022 um 11:25 schrieb Neil Armstrong <narmstrong@baylibre.com>:
>>
>>> I added a printk for hdmi->sink_is_hdmi. This returns 1. Which IMHO is to be expected
>>> since I am using a HDMI connector and panel... So your patch will still add the UYVY formats.
>>> Either the synposys module inside the jz4780 or the panel does not understand them.
>>
>> By selecting the UYVY formats, the driver will enable the colorspace converters in the dw-hdmi IP,
>> I don't see why it doesn't work here...
>>
>> There is a bit called `Support Color Space Converter` in config0_id:
>> bit	|	Name	|	R/W	|	Desc
>> 2 	|	csc	| 	R 	|	Indicates if Color Space Conversion block is present
>>
>> Could you dump all the config0 bits:
>>
>> =======================><=============================
>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> index 54d8fdad395f..547731482da8 100644
>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> @@ -3431,6 +3431,7 @@ struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev,
>>         pdevinfo.id = PLATFORM_DEVID_AUTO;
>>
>>         config0 = hdmi_readb(hdmi, HDMI_CONFIG0_ID);
>> +       dev_info(dev, "config0: %x\n", config0);
>>         config3 = hdmi_readb(hdmi, HDMI_CONFIG3_ID);
>>
>>         if (iores && config3 & HDMI_CONFIG3_AHBAUDDMA) {
>> =======================><=============================
>>
>> If this bit is missing, this would explain the black screen.
> 
> [    9.291011] dw-hdmi-ingenic 10180000.hdmi: config0: bf
> 
> Hm. Or is the color-space conversion of the sw-hdmi module inside the jz4780 broken
> or not configured properly?
> 
> (cross-checked: RGB mode still works if I force hdmi->sink_is_hdmi = false)

I don't understand what's wrong, can you try to make the logic select MEDIA_BUS_FMT_YUV8_1X24 instead of DRM_COLOR_FORMAT_YCBCR422 ?

If your CSC is broken, we'll need to disable it on your platform.

Thanks,
Neil

> 
> BR and thanks,
> Nikolaus
> 

Re: [PATCH v16 4/4] drm/bridge: dw-hdmi: fix bus formats negotiation for 8 bit modes

[H. Nikolaus Schaller]

Hi Neil,

> Am 02.03.2022 um 15:34 schrieb Neil Armstrong <narmstrong@baylibre.com>:
> 
> Hi,
> 
>> (cross-checked: RGB mode still works if I force hdmi->sink_is_hdmi = false)
> 
> I don't understand what's wrong, can you try to make the logic select MEDIA_BUS_FMT_YUV8_1X24 instead of DRM_COLOR_FORMAT_YCBCR422 ?

I have forced hdmi->sink_is_hdmi = false and replaced

 	/* Default 8bit RGB fallback */
-	output_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24;
+	output_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24;

And then screen remains black. MEDIA_BUS_FMT_RGB888_1X24 works.
(MEDIA_BUS_FMT_VUY8_1X24 doesn't work either).

So this indicates that YUV conversion is not working properly. Maybe missing some special
setup.

What I have to test if it works on a different monitor. Not that this specific panel
(a 7 inch waveshare touch with HDMIinput) is buggy and reports YUV capabilities
but does not handle them...

On the other hand this panel works on RasPi and OMAP5 (where I admit I do not know in
which mode).

> If your CSC is broken, we'll need to disable it on your platform.

Indeed.

So it seems as if we need a mechanism to overwrite dw_hdmi_bridge_atomic_get_output_bus_fmts()
in our ingenic-dw-hdmi platform specialization [1] to always return MEDIA_BUS_FMT_RGB888_1X24.

Or alternatively set sink_is_hdmi = false there (unfortunately there is no direct access to
struct dw_hdmi in a specialization drivers).

Is this already possible or how can it be done?

BR and thanks,
Nikolaus

[1]: https://lore.kernel.org/all/24a27226a22adf5f5573f013e5d7d89b0ec73664.1645895582.git.hns@goldelico.com/

Re: [PATCH v16 4/4] drm/bridge: dw-hdmi: fix bus formats negotiation for 8 bit modes

[Neil Armstrong]

Hi,

On 02/03/2022 23:24, H. Nikolaus Schaller wrote:
> Hi Neil,
> 
>> Am 02.03.2022 um 15:34 schrieb Neil Armstrong <narmstrong@baylibre.com>:
>>
>> Hi,
>>
>>> (cross-checked: RGB mode still works if I force hdmi->sink_is_hdmi = false)
>>
>> I don't understand what's wrong, can you try to make the logic select MEDIA_BUS_FMT_YUV8_1X24 instead of DRM_COLOR_FORMAT_YCBCR422 ?
> 
> I have forced hdmi->sink_is_hdmi = false and replaced
> 
>   	/* Default 8bit RGB fallback */
> -	output_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24;
> +	output_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24;
> 
> And then screen remains black. MEDIA_BUS_FMT_RGB888_1X24 works.
> (MEDIA_BUS_FMT_VUY8_1X24 doesn't work either).
> 
> So this indicates that YUV conversion is not working properly. Maybe missing some special
> setup.
> 
> What I have to test if it works on a different monitor. Not that this specific panel
> (a 7 inch waveshare touch with HDMIinput) is buggy and reports YUV capabilities
> but does not handle them...
> 
> On the other hand this panel works on RasPi and OMAP5 (where I admit I do not know in
> which mode).

Pretty sure they don't support YUV HDMI output.

If you can try on a certified HDMI devices like a TV, it would here figuring out where comes the issue.

> 
>> If your CSC is broken, we'll need to disable it on your platform.
> 
> Indeed.
> 
> So it seems as if we need a mechanism to overwrite dw_hdmi_bridge_atomic_get_output_bus_fmts()
> in our ingenic-dw-hdmi platform specialization [1] to always return MEDIA_BUS_FMT_RGB888_1X24.
> 
> Or alternatively set sink_is_hdmi = false there (unfortunately there is no direct access to
> struct dw_hdmi in a specialization drivers).
> 
> Is this already possible or how can it be done?

It's not handled yet, but we may add the logic to handle the lack of CSC config bit and
add a glue config bit to override this like we already did for CEC.

I wrote an initial support to disable CSC (only compile-tested), could you try on your platform with setting disable_csc = 1 in your dw-hdmi glue code ?

================><=======================================
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index 54d8fdad395f..d345166a69aa 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -158,6 +158,8 @@ struct dw_hdmi {
  	struct hdmi_data_info hdmi_data;
  	const struct dw_hdmi_plat_data *plat_data;

+	bool csc_available;		/* indicates if the CSC engine is usable */
+
  	int vic;

  	u8 edid[HDMI_EDID_LEN];
@@ -1009,9 +1011,10 @@ static int is_color_space_interpolation(struct dw_hdmi *hdmi)

  static bool is_csc_needed(struct dw_hdmi *hdmi)
  {
-	return is_color_space_conversion(hdmi) ||
-	       is_color_space_decimation(hdmi) ||
-	       is_color_space_interpolation(hdmi);
+	return hdmi->csc_available &&
+	       (is_color_space_conversion(hdmi) ||
+	        is_color_space_decimation(hdmi) ||
+	        is_color_space_interpolation(hdmi));
  }

  static void dw_hdmi_update_csc_coeffs(struct dw_hdmi *hdmi)
@@ -1064,6 +1067,9 @@ static void hdmi_video_csc(struct dw_hdmi *hdmi)
  	int interpolation = HDMI_CSC_CFG_INTMODE_DISABLE;
  	int decimation = 0;

+	if (!hdmi->csc_available)
+		return;
+
  	/* YCC422 interpolation to 444 mode */
  	if (is_color_space_interpolation(hdmi))
  		interpolation = HDMI_CSC_CFG_INTMODE_CHROMA_INT_FORMULA1;
@@ -2663,6 +2669,7 @@ static u32 *dw_hdmi_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
  					u32 output_fmt,
  					unsigned int *num_input_fmts)
  {
+	struct dw_hdmi *hdmi = bridge->driver_private;
  	u32 *input_fmts;
  	unsigned int i = 0;

@@ -2681,62 +2688,81 @@ static u32 *dw_hdmi_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
  	/* 8bit */
  	case MEDIA_BUS_FMT_RGB888_1X24:
  		input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24;
-		input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24;
-		input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16;
+		if (hdmi->csc_available) {
+			input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24;
+			input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16;
+		}
  		break;
  	case MEDIA_BUS_FMT_YUV8_1X24:
  		input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24;
-		input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16;
-		input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24;
+		if (hdmi->csc_available) {
+			input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16;
+			input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24;
+		}
  		break;
  	case MEDIA_BUS_FMT_UYVY8_1X16:
  		input_fmts[i++] = MEDIA_BUS_FMT_UYVY8_1X16;
-		input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24;
-		input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24;
+		if (hdmi->csc_available) {
+			input_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24;
+			input_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24;
+		}
  		break;

  	/* 10bit */
  	case MEDIA_BUS_FMT_RGB101010_1X30:
  		input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30;
-		input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30;
-		input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20;
+		if (hdmi->csc_available) {
+			input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30;
+			input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20;
+		}
  		break;
  	case MEDIA_BUS_FMT_YUV10_1X30:
  		input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30;
-		input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20;
-		input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30;
+		if (hdmi->csc_available) {
+			input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20;
+			input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30;
+		}
  		break;
  	case MEDIA_BUS_FMT_UYVY10_1X20:
  		input_fmts[i++] = MEDIA_BUS_FMT_UYVY10_1X20;
-		input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30;
-		input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30;
+		if (hdmi->csc_available) {
+			input_fmts[i++] = MEDIA_BUS_FMT_YUV10_1X30;
+			input_fmts[i++] = MEDIA_BUS_FMT_RGB101010_1X30;
+		}
  		break;

  	/* 12bit */
  	case MEDIA_BUS_FMT_RGB121212_1X36:
  		input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36;
-		input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36;
-		input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24;
+		if (hdmi->csc_available) {
+			input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36;
+			input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24;
+		}
  		break;
  	case MEDIA_BUS_FMT_YUV12_1X36:
  		input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36;
-		input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24;
-		input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36;
+		if (hdmi->csc_available) {
+			input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24;
+			input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36;
+		}
  		break;
  	case MEDIA_BUS_FMT_UYVY12_1X24:
  		input_fmts[i++] = MEDIA_BUS_FMT_UYVY12_1X24;
-		input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36;
-		input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36;
+		if (hdmi->csc_available) {
+			input_fmts[i++] = MEDIA_BUS_FMT_YUV12_1X36;
+			input_fmts[i++] = MEDIA_BUS_FMT_RGB121212_1X36;
+		}
  		break;

  	/* 16bit */
  	case MEDIA_BUS_FMT_RGB161616_1X48:
  		input_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48;
-		input_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48;
+		if (hdmi->csc_available)
+			input_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48;
  		break;
  	case MEDIA_BUS_FMT_YUV16_1X48:
-		input_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48;
-		input_fmts[i++] = MEDIA_BUS_FMT_RGB161616_1X48;
+		if (hdmi->csc_available)
+			input_fmts[i++] = MEDIA_BUS_FMT_YUV16_1X48;
  		break;

  	/*YUV 4:2:0 */
@@ -2765,15 +2791,24 @@ static int dw_hdmi_bridge_atomic_check(struct drm_bridge *bridge,
  {
  	struct dw_hdmi *hdmi = bridge->driver_private;

-	hdmi->hdmi_data.enc_out_bus_format =
-			bridge_state->output_bus_cfg.format;
+	if (!hdmi->csc_available &&
+	    bridge_state->output_bus_cfg.format != bridge_state->input_bus_cfg.format) {
+		dev_warn(hdmi->dev, "different input format 0x%04x & output format 0x%04x while CSC isn't usable, fallback to safe format\n",
+			 bridge_state->input_bus_cfg.format,
+			 bridge_state->output_bus_cfg.format);
+		hdmi->hdmi_data.enc_out_bus_format = MEDIA_BUS_FMT_FIXED;
+		hdmi->hdmi_data.enc_in_bus_format = MEDIA_BUS_FMT_FIXED;
+	} else {
+		hdmi->hdmi_data.enc_out_bus_format =
+				bridge_state->output_bus_cfg.format;

-	hdmi->hdmi_data.enc_in_bus_format =
-			bridge_state->input_bus_cfg.format;
+		hdmi->hdmi_data.enc_in_bus_format =
+				bridge_state->input_bus_cfg.format;

-	dev_dbg(hdmi->dev, "input format 0x%04x, output format 0x%04x\n",
-		bridge_state->input_bus_cfg.format,
-		bridge_state->output_bus_cfg.format);
+		dev_dbg(hdmi->dev, "input format 0x%04x, output format 0x%04x\n",
+			bridge_state->input_bus_cfg.format,
+			bridge_state->output_bus_cfg.format);
+	}

  	return 0;
  }
@@ -3479,6 +3514,9 @@ struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev,
  		hdmi->cec = platform_device_register_full(&pdevinfo);
  	}

+	/* Get CSC useability from config0 register and permit override for platforms */
+	hdmi->csc_available = !plat_data->disable_csc || (config0 & HDMI_CONFIG0_CSC);
+
  	drm_bridge_add(&hdmi->bridge);

  	return hdmi;
diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
index 1999db05bc3b..279722e4d189 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.h
@@ -541,6 +541,7 @@ enum {

  /* CONFIG0_ID field values */
  	HDMI_CONFIG0_I2S = 0x10,
+	HDMI_CONFIG0_CSC = 0x04,
  	HDMI_CONFIG0_CEC = 0x02,

  /* CONFIG1_ID field values */
diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
index 2a1f85f9a8a3..b2f689cbe864 100644
--- a/include/drm/bridge/dw_hdmi.h
+++ b/include/drm/bridge/dw_hdmi.h
@@ -157,6 +157,7 @@ struct dw_hdmi_plat_data {
  			     unsigned long mpixelclock);

  	unsigned int disable_cec : 1;
+	unsigned int disable_csc : 1;
  };

  struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev,
=================><==============================================================

Neil

> 
> BR and thanks,
> Nikolaus
> 
> [1]: https://lore.kernel.org/all/24a27226a22adf5f5573f013e5d7d89b0ec73664.1645895582.git.hns@goldelico.com/

Re: [PATCH v16 4/4] drm/bridge: dw-hdmi: fix bus formats negotiation for 8 bit modes

[H. Nikolaus Schaller]

Hi Neil,

> Am 03.03.2022 um 09:35 schrieb Neil Armstrong <narmstrong@baylibre.com>:
> 
> Hi,
> 
> On 02/03/2022 23:24, H. Nikolaus Schaller wrote:
>> Hi Neil,
>>> Am 02.03.2022 um 15:34 schrieb Neil Armstrong <narmstrong@baylibre.com>:
>>> 
>>> Hi,
>>> 
>>>> (cross-checked: RGB mode still works if I force hdmi->sink_is_hdmi = false)
>>> 
>>> I don't understand what's wrong, can you try to make the logic select MEDIA_BUS_FMT_YUV8_1X24 instead of DRM_COLOR_FORMAT_YCBCR422 ?
>> I have forced hdmi->sink_is_hdmi = false and replaced
>>  	/* Default 8bit RGB fallback */
>> -	output_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24;
>> +	output_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24;
>> And then screen remains black. MEDIA_BUS_FMT_RGB888_1X24 works.
>> (MEDIA_BUS_FMT_VUY8_1X24 doesn't work either).
>> So this indicates that YUV conversion is not working properly. Maybe missing some special
>> setup.
>> What I have to test if it works on a different monitor.

Same effect on a Xiaomi monitor (user manual just telling HDMI1,4 compatible), an
older Acer video projector and a Sharp TV set.

The Xiaomi monitor does not say "No signal" but shows a black screen. The others
do not even report any HDMI signals. All work well with MEDIA_BUS_FMT_RGB888_1X24.

This means the transcoding to YUV does not work properly on the jz4780 SoC setup.

So it looks as if we have to disable it (at least unless someone finds a fix).

>> Not that this specific panel
>> (a 7 inch waveshare touch with HDMIinput) is buggy and reports YUV capabilities
>> but does not handle them...
>> On the other hand this panel works on RasPi and OMAP5 (where I admit I do not know in
>> which mode).
> 
> Pretty sure they don't support YUV HDMI output.
> 
> If you can try on a certified HDMI devices like a TV, it would here figuring out where comes the issue.

I am not sure if the Sharp TV is fully certified but would assume...

> 
>>> If your CSC is broken, we'll need to disable it on your platform.
>> Indeed.
>> So it seems as if we need a mechanism to overwrite dw_hdmi_bridge_atomic_get_output_bus_fmts()
>> in our ingenic-dw-hdmi platform specialization [1] to always return MEDIA_BUS_FMT_RGB888_1X24.
>> Or alternatively set sink_is_hdmi = false there (unfortunately there is no direct access to
>> struct dw_hdmi in a specialization drivers).
>> Is this already possible or how can it be done?
> 
> It's not handled yet, but we may add the logic to handle the lack of CSC config bit and
> add a glue config bit to override this like we already did for CEC.
> 
> I wrote an initial support to disable CSC (only compile-tested), could you try on your platform with setting disable_csc = 1 in your dw-hdmi glue code ?

This works!

So how can we get that merged? IMHO your proposal should be before we add ingenic-dw-hdmi.
If you have a version with proper commit message I can add it to the beginning of my
seried and include it in a v17. Or if you get yours merged to drm-misc/drm-misc-next I
can build on top.

BR and thanks,
Nikolaus

Re: [PATCH v16 4/4] drm/bridge: dw-hdmi: fix bus formats negotiation for 8 bit modes

[Neil Armstrong]

On 03/03/2022 11:40, H. Nikolaus Schaller wrote:
> Hi Neil,
> 
>> Am 03.03.2022 um 09:35 schrieb Neil Armstrong <narmstrong@baylibre.com>:
>>
>> Hi,
>>
>> On 02/03/2022 23:24, H. Nikolaus Schaller wrote:
>>> Hi Neil,
>>>> Am 02.03.2022 um 15:34 schrieb Neil Armstrong <narmstrong@baylibre.com>:
>>>>
>>>> Hi,
>>>>
>>>>> (cross-checked: RGB mode still works if I force hdmi->sink_is_hdmi = false)
>>>>
>>>> I don't understand what's wrong, can you try to make the logic select MEDIA_BUS_FMT_YUV8_1X24 instead of DRM_COLOR_FORMAT_YCBCR422 ?
>>> I have forced hdmi->sink_is_hdmi = false and replaced
>>>   	/* Default 8bit RGB fallback */
>>> -	output_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24;
>>> +	output_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24;
>>> And then screen remains black. MEDIA_BUS_FMT_RGB888_1X24 works.
>>> (MEDIA_BUS_FMT_VUY8_1X24 doesn't work either).
>>> So this indicates that YUV conversion is not working properly. Maybe missing some special
>>> setup.
>>> What I have to test if it works on a different monitor.
> 
> Same effect on a Xiaomi monitor (user manual just telling HDMI1,4 compatible), an
> older Acer video projector and a Sharp TV set.
> 
> The Xiaomi monitor does not say "No signal" but shows a black screen. The others
> do not even report any HDMI signals. All work well with MEDIA_BUS_FMT_RGB888_1X24.
> 
> This means the transcoding to YUV does not work properly on the jz4780 SoC setup.
> 
> So it looks as if we have to disable it (at least unless someone finds a fix).
> 
>>> Not that this specific panel
>>> (a 7 inch waveshare touch with HDMIinput) is buggy and reports YUV capabilities
>>> but does not handle them...
>>> On the other hand this panel works on RasPi and OMAP5 (where I admit I do not know in
>>> which mode).
>>
>> Pretty sure they don't support YUV HDMI output.
>>
>> If you can try on a certified HDMI devices like a TV, it would here figuring out where comes the issue.
> 
> I am not sure if the Sharp TV is fully certified but would assume...
> 
>>
>>>> If your CSC is broken, we'll need to disable it on your platform.
>>> Indeed.
>>> So it seems as if we need a mechanism to overwrite dw_hdmi_bridge_atomic_get_output_bus_fmts()
>>> in our ingenic-dw-hdmi platform specialization [1] to always return MEDIA_BUS_FMT_RGB888_1X24.
>>> Or alternatively set sink_is_hdmi = false there (unfortunately there is no direct access to
>>> struct dw_hdmi in a specialization drivers).
>>> Is this already possible or how can it be done?
>>
>> It's not handled yet, but we may add the logic to handle the lack of CSC config bit and
>> add a glue config bit to override this like we already did for CEC.
>>
>> I wrote an initial support to disable CSC (only compile-tested), could you try on your platform with setting disable_csc = 1 in your dw-hdmi glue code ?
> 
> This works!
> 
> So how can we get that merged? IMHO your proposal should be before we add ingenic-dw-hdmi.
> If you have a version with proper commit message I can add it to the beginning of my
> seried and include it in a v17. Or if you get yours merged to drm-misc/drm-misc-next I
> can build on top.

You can add it in your v17 patchset with my authorship and my Signed-off-by tag + yours.

As commit message something like :
====================
drm/bridge: dw-hdmi: handle unusable or non-configured CSC module

The dw-hdmi integrates an optional Color Space Conversion feature used
to handle color-space conversions.

On some platforms, the CSC isn't built-in or non-functional.

This adds the necessary code to disable the CSC functionality
and limit the bus format negotiation to force using the same
input bus format as the output bus format.
====================

Thanks,
Neil

> 
> BR and thanks,
> Nikolaus
> 

Re: [PATCH v16 4/4] drm/bridge: dw-hdmi: fix bus formats negotiation for 8 bit modes

[H. Nikolaus Schaller]

Hi Neil,

> Am 03.03.2022 um 12:42 schrieb Neil Armstrong <narmstrong@baylibre.com>:
> 
>> So how can we get that merged? IMHO your proposal should be before we add ingenic-dw-hdmi.
>> If you have a version with proper commit message I can add it to the beginning of my
>> seried and include it in a v17. Or if you get yours merged to drm-misc/drm-misc-next I
>> can build on top.
> 
> You can add it in your v17 patchset with my authorship and my Signed-off-by tag + yours.
> 
> As commit message something like :
> ====================
> drm/bridge: dw-hdmi: handle unusable or non-configured CSC module
> 
> The dw-hdmi integrates an optional Color Space Conversion feature used
> to handle color-space conversions.
> 
> On some platforms, the CSC isn't built-in or non-functional.
> 
> This adds the necessary code to disable the CSC functionality
> and limit the bus format negotiation to force using the same
> input bus format as the output bus format.
> ====================

Fine! Will do.

BR and thanks,
Nikolaus

Re: [PATCH v16 4/4] drm/bridge: dw-hdmi: fix bus formats negotiation for 8 bit modes

[Paul Cercueil]

Hi Neil,

Any feedback on the other patches?

They look fine to me, but I still need an ack to merge them in 
drm-misc-next.

Cheers,
-Paul


Le jeu., mars 3 2022 at 12:42:02 +0100, Neil Armstrong 
<narmstrong@baylibre.com> a écrit :
> On 03/03/2022 11:40, H. Nikolaus Schaller wrote:
>> Hi Neil,
>> 
>>> Am 03.03.2022 um 09:35 schrieb Neil Armstrong 
>>> <narmstrong@baylibre.com>:
>>> 
>>> Hi,
>>> 
>>> On 02/03/2022 23:24, H. Nikolaus Schaller wrote:
>>>> Hi Neil,
>>>>> Am 02.03.2022 um 15:34 schrieb Neil Armstrong 
>>>>> <narmstrong@baylibre.com>:
>>>>> 
>>>>> Hi,
>>>>> 
>>>>>> (cross-checked: RGB mode still works if I force 
>>>>>> hdmi->sink_is_hdmi = false)
>>>>> 
>>>>> I don't understand what's wrong, can you try to make the logic 
>>>>> select MEDIA_BUS_FMT_YUV8_1X24 instead of 
>>>>> DRM_COLOR_FORMAT_YCBCR422 ?
>>>> I have forced hdmi->sink_is_hdmi = false and replaced
>>>>   	/* Default 8bit RGB fallback */
>>>> -	output_fmts[i++] = MEDIA_BUS_FMT_RGB888_1X24;
>>>> +	output_fmts[i++] = MEDIA_BUS_FMT_YUV8_1X24;
>>>> And then screen remains black. MEDIA_BUS_FMT_RGB888_1X24 works.
>>>> (MEDIA_BUS_FMT_VUY8_1X24 doesn't work either).
>>>> So this indicates that YUV conversion is not working properly. 
>>>> Maybe missing some special
>>>> setup.
>>>> What I have to test if it works on a different monitor.
>> 
>> Same effect on a Xiaomi monitor (user manual just telling HDMI1,4 
>> compatible), an
>> older Acer video projector and a Sharp TV set.
>> 
>> The Xiaomi monitor does not say "No signal" but shows a black 
>> screen. The others
>> do not even report any HDMI signals. All work well with 
>> MEDIA_BUS_FMT_RGB888_1X24.
>> 
>> This means the transcoding to YUV does not work properly on the 
>> jz4780 SoC setup.
>> 
>> So it looks as if we have to disable it (at least unless someone 
>> finds a fix).
>> 
>>>> Not that this specific panel
>>>> (a 7 inch waveshare touch with HDMIinput) is buggy and reports YUV 
>>>> capabilities
>>>> but does not handle them...
>>>> On the other hand this panel works on RasPi and OMAP5 (where I 
>>>> admit I do not know in
>>>> which mode).
>>> 
>>> Pretty sure they don't support YUV HDMI output.
>>> 
>>> If you can try on a certified HDMI devices like a TV, it would here 
>>> figuring out where comes the issue.
>> 
>> I am not sure if the Sharp TV is fully certified but would assume...
>> 
>>> 
>>>>> If your CSC is broken, we'll need to disable it on your platform.
>>>> Indeed.
>>>> So it seems as if we need a mechanism to overwrite 
>>>> dw_hdmi_bridge_atomic_get_output_bus_fmts()
>>>> in our ingenic-dw-hdmi platform specialization [1] to always 
>>>> return MEDIA_BUS_FMT_RGB888_1X24.
>>>> Or alternatively set sink_is_hdmi = false there (unfortunately 
>>>> there is no direct access to
>>>> struct dw_hdmi in a specialization drivers).
>>>> Is this already possible or how can it be done?
>>> 
>>> It's not handled yet, but we may add the logic to handle the lack 
>>> of CSC config bit and
>>> add a glue config bit to override this like we already did for CEC.
>>> 
>>> I wrote an initial support to disable CSC (only compile-tested), 
>>> could you try on your platform with setting disable_csc = 1 in your 
>>> dw-hdmi glue code ?
>> 
>> This works!
>> 
>> So how can we get that merged? IMHO your proposal should be before 
>> we add ingenic-dw-hdmi.
>> If you have a version with proper commit message I can add it to the 
>> beginning of my
>> seried and include it in a v17. Or if you get yours merged to 
>> drm-misc/drm-misc-next I
>> can build on top.
> 
> You can add it in your v17 patchset with my authorship and my 
> Signed-off-by tag + yours.
> 
> As commit message something like :
> ====================
> drm/bridge: dw-hdmi: handle unusable or non-configured CSC module
> 
> The dw-hdmi integrates an optional Color Space Conversion feature used
> to handle color-space conversions.
> 
> On some platforms, the CSC isn't built-in or non-functional.
> 
> This adds the necessary code to disable the CSC functionality
> and limit the bus format negotiation to force using the same
> input bus format as the output bus format.
> ====================
> 
> Thanks,
> Neil
> 
>> 
>> BR and thanks,
>> Nikolaus
>> 
> 

Re: [PATCH v16 4/4] drm/bridge: dw-hdmi: fix bus formats negotiation for 8 bit modes

[H. Nikolaus Schaller]

Hi Neil,

> Am 03.03.2022 um 12:45 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
> 
> Hi Neil,
> 
>> Am 03.03.2022 um 12:42 schrieb Neil Armstrong <narmstrong@baylibre.com>:
>> 
>>> So how can we get that merged? IMHO your proposal should be before we add ingenic-dw-hdmi.
>>> If you have a version with proper commit message I can add it to the beginning of my
>>> seried and include it in a v17. Or if you get yours merged to drm-misc/drm-misc-next I
>>> can build on top.
>> 
>> You can add it in your v17 patchset with my authorship and my Signed-off-by tag + yours.
>> 
>> As commit message something like :
>> ====================
>> drm/bridge: dw-hdmi: handle unusable or non-configured CSC module
>> 
>> The dw-hdmi integrates an optional Color Space Conversion feature used
>> to handle color-space conversions.
>> 
>> On some platforms, the CSC isn't built-in or non-functional.
>> 
>> This adds the necessary code to disable the CSC functionality
>> and limit the bus format negotiation to force using the same
>> input bus format as the output bus format.
>> ====================
> 
> Fine! Will do.

I was a little too early.

While preparing the patches I found that I still had the hack to force
sink_is_hdmi = false in my test branch. Sorry for that.

Removing this made the panel go black again, even with your latest
proposal.

So I looked deeper into your patch and it seems to influence the
input formats only in dw_hdmi_bridge_atomic_get_input_bus_fmts()?

While the problem I see is with output formats and we had worked on
modifying dw_hdmi_bridge_atomic_get_output_bus_fmts().

BR and thanks,
Nikolaus

Re: [PATCH v16 4/4] drm/bridge: dw-hdmi: fix bus formats negotiation for 8 bit modes

[Neil Armstrong]

Hi,

On 03/03/2022 16:37, H. Nikolaus Schaller wrote:
> Hi Neil,
> 
>> Am 03.03.2022 um 12:45 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
>>
>> Hi Neil,
>>
>>> Am 03.03.2022 um 12:42 schrieb Neil Armstrong <narmstrong@baylibre.com>:
>>>
>>>> So how can we get that merged? IMHO your proposal should be before we add ingenic-dw-hdmi.
>>>> If you have a version with proper commit message I can add it to the beginning of my
>>>> seried and include it in a v17. Or if you get yours merged to drm-misc/drm-misc-next I
>>>> can build on top.
>>>
>>> You can add it in your v17 patchset with my authorship and my Signed-off-by tag + yours.
>>>
>>> As commit message something like :
>>> ====================
>>> drm/bridge: dw-hdmi: handle unusable or non-configured CSC module
>>>
>>> The dw-hdmi integrates an optional Color Space Conversion feature used
>>> to handle color-space conversions.
>>>
>>> On some platforms, the CSC isn't built-in or non-functional.
>>>
>>> This adds the necessary code to disable the CSC functionality
>>> and limit the bus format negotiation to force using the same
>>> input bus format as the output bus format.
>>> ====================
>>
>> Fine! Will do.
> 
> I was a little too early.
> 
> While preparing the patches I found that I still had the hack to force
> sink_is_hdmi = false in my test branch. Sorry for that.
> 
> Removing this made the panel go black again, even with your latest
> proposal.
> 
> So I looked deeper into your patch and it seems to influence the
> input formats only in dw_hdmi_bridge_atomic_get_input_bus_fmts()?
> 
> While the problem I see is with output formats and we had worked on
> modifying dw_hdmi_bridge_atomic_get_output_bus_fmts().

I just looked and the ingenic drm driver first bridge uses drm_atomic_helper_bridge_propagate_bus_fmt()
which is why this last patch doesn't work, and perhaps would be the main issue here.

Indeed, the core will loop on all the possible output formats for your HDMI monitor :
- MEDIA_BUS_FMT_UYVY8_1X16
- MEDIA_BUS_FMT_YUV8_1X24
- MEDIA_BUS_FMT_RGB888_1X24

For each of these, the dw-hdmi dw_hdmi_bridge_atomic_get_input_bus_fmts() will
return the same format + the possible CSC transformations, for example
for MEDIA_BUS_FMT_UYVY8_1X16 will return as possible inputs:
- MEDIA_BUS_FMT_UYVY8_1X16
- MEDIA_BUS_FMT_YUV8_1X24
- MEDIA_BUS_FMT_RGB888_1X24

The the core will call for each of the those the .atomic_get_input_bus_fmts of
the Ingenic DRM driver, but by using drm_atomic_helper_bridge_propagate_bus_fmt()
it basically sets a pass-through and accepts any format.

This is why MEDIA_BUS_FMT_UYVY8_1X16 is selected, but in this case the ingenic
ingenic_drm_bridge_atomic_check() would fail in the switch.

The Ingenic should implement a proper .atomic_get_input_bus_fmts returning
only the possible supported formats.

Can you check if you hit the default case in ingenic_drm_bridge_atomic_check() ?

Neil

> 
> BR and thanks,
> Nikolaus
> 

Re: [PATCH v16 1/4] drm/bridge: dw-hdmi: introduce dw_hdmi_enable_poll()

[Neil Armstrong]

Hi,

On 26/02/2022 18:12, H. Nikolaus Schaller wrote:
> so that specialization drivers like ingenic-dw-hdmi can enable polling.
> 
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
>   drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 9 +++++++++
>   include/drm/bridge/dw_hdmi.h              | 1 +
>   2 files changed, 10 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index 4befc104d2200..43e375da131e8 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -3217,6 +3217,15 @@ static int dw_hdmi_parse_dt(struct dw_hdmi *hdmi)
>   	return 0;
>   }
>   
> +void dw_hdmi_enable_poll(struct dw_hdmi *hdmi, bool enable)
> +{
> +	if (hdmi->bridge.dev)
> +		hdmi->bridge.dev->mode_config.poll_enabled = enable;
> +	else
> +		dev_warn(hdmi->dev, "no hdmi->bridge.dev");
> +}
> +EXPORT_SYMBOL_GPL(dw_hdmi_enable_poll);
> +
>   struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev,
>   			      const struct dw_hdmi_plat_data *plat_data)
>   {
> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
> index 2a1f85f9a8a3f..963960794b40e 100644
> --- a/include/drm/bridge/dw_hdmi.h
> +++ b/include/drm/bridge/dw_hdmi.h
> @@ -196,5 +196,6 @@ enum drm_connector_status dw_hdmi_phy_read_hpd(struct dw_hdmi *hdmi,
>   void dw_hdmi_phy_update_hpd(struct dw_hdmi *hdmi, void *data,
>   			    bool force, bool disabled, bool rxsense);
>   void dw_hdmi_phy_setup_hpd(struct dw_hdmi *hdmi, void *data);
> +void dw_hdmi_enable_poll(struct dw_hdmi *hdmi, bool enable);
>   
>   #endif /* __IMX_HDMI_H__ */

As I understand, this is because the IRQ line of the dw-hdmi IP isn't connected right ? and you use the display-connector ddc gpio instead ?

In this case I think the Ingenic DRM core should call drm_kms_helper_poll_init(drm) instead.

Neil

Re: [PATCH v16 1/4] drm/bridge: dw-hdmi: introduce dw_hdmi_enable_poll()

[H. Nikolaus Schaller]

Hi Neil,

> Am 03.03.2022 um 17:23 schrieb Neil Armstrong <narmstrong@baylibre.com>:
> 
> Hi,
> 
> On 26/02/2022 18:12, H. Nikolaus Schaller wrote:
>> so that specialization drivers like ingenic-dw-hdmi can enable polling.
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> ---
>>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 9 +++++++++
>>  include/drm/bridge/dw_hdmi.h              | 1 +
>>  2 files changed, 10 insertions(+)
>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> index 4befc104d2200..43e375da131e8 100644
>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> @@ -3217,6 +3217,15 @@ static int dw_hdmi_parse_dt(struct dw_hdmi *hdmi)
>>  	return 0;
>>  }
>>  +void dw_hdmi_enable_poll(struct dw_hdmi *hdmi, bool enable)
>> +{
>> +	if (hdmi->bridge.dev)
>> +		hdmi->bridge.dev->mode_config.poll_enabled = enable;
>> +	else
>> +		dev_warn(hdmi->dev, "no hdmi->bridge.dev");
>> +}
>> +EXPORT_SYMBOL_GPL(dw_hdmi_enable_poll);
>> +
>>  struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev,
>>  			      const struct dw_hdmi_plat_data *plat_data)
>>  {
>> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
>> index 2a1f85f9a8a3f..963960794b40e 100644
>> --- a/include/drm/bridge/dw_hdmi.h
>> +++ b/include/drm/bridge/dw_hdmi.h
>> @@ -196,5 +196,6 @@ enum drm_connector_status dw_hdmi_phy_read_hpd(struct dw_hdmi *hdmi,
>>  void dw_hdmi_phy_update_hpd(struct dw_hdmi *hdmi, void *data,
>>  			    bool force, bool disabled, bool rxsense);
>>  void dw_hdmi_phy_setup_hpd(struct dw_hdmi *hdmi, void *data);
>> +void dw_hdmi_enable_poll(struct dw_hdmi *hdmi, bool enable);
>>    #endif /* __IMX_HDMI_H__ */
> 
> As I understand, this is because the IRQ line of the dw-hdmi IP isn't connected right ? and you use the display-connector ddc gpio instead ?

Yes. The IRQ line is not connected on all boards as far as I can see.

> 
> In this case I think the Ingenic DRM core should call drm_kms_helper_poll_init(drm) instead.

Ah, that is good. it seems to do "dw_hdmi_enable_poll()" in a more generic way.

Will test and rework for v17 asap.

BR and thanks,
Nikolaus

Re: [Letux-kernel] [PATCH v16 1/4] drm/bridge: dw-hdmi: introduce dw_hdmi_enable_poll()

[H. Nikolaus Schaller]

Hi Neil,

> Am 03.03.2022 um 17:30 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
> 
> Hi Neil,
> 
>> Am 03.03.2022 um 17:23 schrieb Neil Armstrong <narmstrong@baylibre.com>:
>> 
>> Hi,
>> 
>> On 26/02/2022 18:12, H. Nikolaus Schaller wrote:
>>> so that specialization drivers like ingenic-dw-hdmi can enable polling.
>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>>> ---
>>> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 9 +++++++++
>>> include/drm/bridge/dw_hdmi.h              | 1 +
>>> 2 files changed, 10 insertions(+)
>>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>> index 4befc104d2200..43e375da131e8 100644
>>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>> @@ -3217,6 +3217,15 @@ static int dw_hdmi_parse_dt(struct dw_hdmi *hdmi)
>>> 	return 0;
>>> }
>>> +void dw_hdmi_enable_poll(struct dw_hdmi *hdmi, bool enable)
>>> +{
>>> +	if (hdmi->bridge.dev)
>>> +		hdmi->bridge.dev->mode_config.poll_enabled = enable;
>>> +	else
>>> +		dev_warn(hdmi->dev, "no hdmi->bridge.dev");
>>> +}
>>> +EXPORT_SYMBOL_GPL(dw_hdmi_enable_poll);
>>> +
>>> struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev,
>>> 			      const struct dw_hdmi_plat_data *plat_data)
>>> {
>>> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
>>> index 2a1f85f9a8a3f..963960794b40e 100644
>>> --- a/include/drm/bridge/dw_hdmi.h
>>> +++ b/include/drm/bridge/dw_hdmi.h
>>> @@ -196,5 +196,6 @@ enum drm_connector_status dw_hdmi_phy_read_hpd(struct dw_hdmi *hdmi,
>>> void dw_hdmi_phy_update_hpd(struct dw_hdmi *hdmi, void *data,
>>> 			    bool force, bool disabled, bool rxsense);
>>> void dw_hdmi_phy_setup_hpd(struct dw_hdmi *hdmi, void *data);
>>> +void dw_hdmi_enable_poll(struct dw_hdmi *hdmi, bool enable);
>>>   #endif /* __IMX_HDMI_H__ */
>> 
>> As I understand, this is because the IRQ line of the dw-hdmi IP isn't connected right ? and you use the display-connector ddc gpio instead ?
> 
> Yes. The IRQ line is not connected on all boards as far as I can see.
> 
>> 
>> In this case I think the Ingenic DRM core should call drm_kms_helper_poll_init(drm) instead.
> 
> Ah, that is good. it seems to do "dw_hdmi_enable_poll()" in a more generic way.

Well, I looked through source code and it is defined as 

	void drm_kms_helper_poll_init(struct drm_device *dev)

But there is no direct pointer to some drm_device available.
Neither in dw-hdmi nor ingenic-dw-hdmi.

What should the parameter to drm_kms_helper_poll_init(drm) be?

From comparing code to be able to set mode_config.poll_enabled = enable it should be

	&hdmi->bridge.dev

but the struct dw_hdmi *hdmi is an opaque type for the ingenic-dw-hdmi driver.
So it can't access the hdmi-bridge directly.

What we can do is to make dw_hdmi_enable_poll() call drm_kms_helper_poll_init()
or drm_kms_helper_poll_fini().

BR and thanks,
Nikolaus

Re: [PATCH v16 1/4] drm/bridge: dw-hdmi: introduce dw_hdmi_enable_poll()

[Paul Cercueil]

Hi Neil,

Le jeu., mars 3 2022 at 17:23:02 +0100, Neil Armstrong 
<narmstrong@baylibre.com> a écrit :
> Hi,
> 
> On 26/02/2022 18:12, H. Nikolaus Schaller wrote:
>> so that specialization drivers like ingenic-dw-hdmi can enable 
>> polling.
>> 
>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>> ---
>>   drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 9 +++++++++
>>   include/drm/bridge/dw_hdmi.h              | 1 +
>>   2 files changed, 10 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c 
>> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> index 4befc104d2200..43e375da131e8 100644
>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>> @@ -3217,6 +3217,15 @@ static int dw_hdmi_parse_dt(struct dw_hdmi 
>> *hdmi)
>>   	return 0;
>>   }
>>   \x7f+void dw_hdmi_enable_poll(struct dw_hdmi *hdmi, bool enable)
>> +{
>> +	if (hdmi->bridge.dev)
>> +		hdmi->bridge.dev->mode_config.poll_enabled = enable;
>> +	else
>> +		dev_warn(hdmi->dev, "no hdmi->bridge.dev");
>> +}
>> +EXPORT_SYMBOL_GPL(dw_hdmi_enable_poll);
>> +
>>   struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev,
>>   			      const struct dw_hdmi_plat_data *plat_data)
>>   {
>> diff --git a/include/drm/bridge/dw_hdmi.h 
>> b/include/drm/bridge/dw_hdmi.h
>> index 2a1f85f9a8a3f..963960794b40e 100644
>> --- a/include/drm/bridge/dw_hdmi.h
>> +++ b/include/drm/bridge/dw_hdmi.h
>> @@ -196,5 +196,6 @@ enum drm_connector_status 
>> dw_hdmi_phy_read_hpd(struct dw_hdmi *hdmi,
>>   void dw_hdmi_phy_update_hpd(struct dw_hdmi *hdmi, void *data,
>>   			    bool force, bool disabled, bool rxsense);
>>   void dw_hdmi_phy_setup_hpd(struct dw_hdmi *hdmi, void *data);
>> +void dw_hdmi_enable_poll(struct dw_hdmi *hdmi, bool enable);
>>   \x7f  #endif /* __IMX_HDMI_H__ */
> 
> As I understand, this is because the IRQ line of the dw-hdmi IP isn't 
> connected right ? and you use the display-connector ddc gpio instead ?

According to the CI20 schematic, it is wired properly. So that's 
strange.

> 
> In this case I think the Ingenic DRM core should call 
> drm_kms_helper_poll_init(drm) instead.

Yes, the ingenic-drm driver does not poll for connectors because until 
now it never has been needed.

Cheers,
-Paul

Re: [Letux-kernel] [PATCH v16 1/4] drm/bridge: dw-hdmi: introduce dw_hdmi_enable_poll()

[Paul Cercueil]

Hi Nikolaus,

Le jeu., mars 3 2022 at 17:43:05 +0100, H. Nikolaus Schaller 
<hns@goldelico.com> a écrit :
> Hi Neil,
> 
>>  Am 03.03.2022 um 17:30 schrieb H. Nikolaus Schaller 
>> <hns@goldelico.com>:
>> 
>>  Hi Neil,
>> 
>>>  Am 03.03.2022 um 17:23 schrieb Neil Armstrong 
>>> <narmstrong@baylibre.com>:
>>> 
>>>  Hi,
>>> 
>>>  On 26/02/2022 18:12, H. Nikolaus Schaller wrote:
>>>>  so that specialization drivers like ingenic-dw-hdmi can enable 
>>>> polling.
>>>>  Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>>>>  ---
>>>>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 9 +++++++++
>>>>  include/drm/bridge/dw_hdmi.h              | 1 +
>>>>  2 files changed, 10 insertions(+)
>>>>  diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c 
>>>> b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>>>  index 4befc104d2200..43e375da131e8 100644
>>>>  --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>>>  +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>>>  @@ -3217,6 +3217,15 @@ static int dw_hdmi_parse_dt(struct dw_hdmi 
>>>> *hdmi)
>>>>  	return 0;
>>>>  }
>>>>  +void dw_hdmi_enable_poll(struct dw_hdmi *hdmi, bool enable)
>>>>  +{
>>>>  +	if (hdmi->bridge.dev)
>>>>  +		hdmi->bridge.dev->mode_config.poll_enabled = enable;
>>>>  +	else
>>>>  +		dev_warn(hdmi->dev, "no hdmi->bridge.dev");
>>>>  +}
>>>>  +EXPORT_SYMBOL_GPL(dw_hdmi_enable_poll);
>>>>  +
>>>>  struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev,
>>>>  			      const struct dw_hdmi_plat_data *plat_data)
>>>>  {
>>>>  diff --git a/include/drm/bridge/dw_hdmi.h 
>>>> b/include/drm/bridge/dw_hdmi.h
>>>>  index 2a1f85f9a8a3f..963960794b40e 100644
>>>>  --- a/include/drm/bridge/dw_hdmi.h
>>>>  +++ b/include/drm/bridge/dw_hdmi.h
>>>>  @@ -196,5 +196,6 @@ enum drm_connector_status 
>>>> dw_hdmi_phy_read_hpd(struct dw_hdmi *hdmi,
>>>>  void dw_hdmi_phy_update_hpd(struct dw_hdmi *hdmi, void *data,
>>>>  			    bool force, bool disabled, bool rxsense);
>>>>  void dw_hdmi_phy_setup_hpd(struct dw_hdmi *hdmi, void *data);
>>>>  +void dw_hdmi_enable_poll(struct dw_hdmi *hdmi, bool enable);
>>>>    #endif /* __IMX_HDMI_H__ */
>>> 
>>>  As I understand, this is because the IRQ line of the dw-hdmi IP 
>>> isn't connected right ? and you use the display-connector ddc gpio 
>>> instead ?
>> 
>>  Yes. The IRQ line is not connected on all boards as far as I can 
>> see.
>> 
>>> 
>>>  In this case I think the Ingenic DRM core should call 
>>> drm_kms_helper_poll_init(drm) instead.
>> 
>>  Ah, that is good. it seems to do "dw_hdmi_enable_poll()" in a more 
>> generic way.
> 
> Well, I looked through source code and it is defined as
> 
> 	void drm_kms_helper_poll_init(struct drm_device *dev)
> 
> But there is no direct pointer to some drm_device available.
> Neither in dw-hdmi nor ingenic-dw-hdmi.

Well he said "the Ingenic DRM core" aka ingenic-drm-drv.c. You do have 
access to the main drm_device in the ingenic_drm_bind() function, so 
you can add it there (with a cleanup function calling 
drm_kms_helper_poll_fini() registered with drmm_add_action_or_reset()).

Cheers,
-Paul

> What should the parameter to drm_kms_helper_poll_init(drm) be?
> 
> From comparing code to be able to set mode_config.poll_enabled = 
> enable it should be
> 
> 	&hdmi->bridge.dev
> 
> but the struct dw_hdmi *hdmi is an opaque type for the 
> ingenic-dw-hdmi driver.
> So it can't access the hdmi-bridge directly.
> 
> What we can do is to make dw_hdmi_enable_poll() call 
> drm_kms_helper_poll_init()
> or drm_kms_helper_poll_fini().
> 
> BR and thanks,
> Nikolaus
> 
> 

Re: [PATCH v16 1/4] drm/bridge: dw-hdmi: introduce dw_hdmi_enable_poll()

[H. Nikolaus Schaller]

Hi Paul and Neil,

> Am 03.03.2022 um 17:46 schrieb Paul Cercueil <paul@crapouillou.net>:
> 
> Hi Neil,
> 
> Le jeu., mars 3 2022 at 17:23:02 +0100, Neil Armstrong <narmstrong@baylibre.com> a écrit :
>> Hi,
>> On 26/02/2022 18:12, H. Nikolaus Schaller wrote:
>>> so that specialization drivers like ingenic-dw-hdmi can enable polling.
>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>>> ---
>>>  drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 9 +++++++++
>>>  include/drm/bridge/dw_hdmi.h              | 1 +
>>>  2 files changed, 10 insertions(+)
>>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>> index 4befc104d2200..43e375da131e8 100644
>>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>> @@ -3217,6 +3217,15 @@ static int dw_hdmi_parse_dt(struct dw_hdmi *hdmi)
>>>  	return 0;
>>>  }
>>>  \x7f+void dw_hdmi_enable_poll(struct dw_hdmi *hdmi, bool enable)
>>> +{
>>> +	if (hdmi->bridge.dev)
>>> +		hdmi->bridge.dev->mode_config.poll_enabled = enable;
>>> +	else
>>> +		dev_warn(hdmi->dev, "no hdmi->bridge.dev");
>>> +}
>>> +EXPORT_SYMBOL_GPL(dw_hdmi_enable_poll);
>>> +
>>>  struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev,
>>>  			      const struct dw_hdmi_plat_data *plat_data)
>>>  {
>>> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
>>> index 2a1f85f9a8a3f..963960794b40e 100644
>>> --- a/include/drm/bridge/dw_hdmi.h
>>> +++ b/include/drm/bridge/dw_hdmi.h
>>> @@ -196,5 +196,6 @@ enum drm_connector_status dw_hdmi_phy_read_hpd(struct dw_hdmi *hdmi,
>>>  void dw_hdmi_phy_update_hpd(struct dw_hdmi *hdmi, void *data,
>>>  			    bool force, bool disabled, bool rxsense);
>>>  void dw_hdmi_phy_setup_hpd(struct dw_hdmi *hdmi, void *data);
>>> +void dw_hdmi_enable_poll(struct dw_hdmi *hdmi, bool enable);
>>>  \x7f  #endif /* __IMX_HDMI_H__ */
>> As I understand, this is because the IRQ line of the dw-hdmi IP isn't connected right ? and you use the display-connector ddc gpio instead ?

Ah, I should finish work for today since I am no longer reading every word properly...

No, we do NOT use the display connector for HPD. We use HPD of the dw-hdmi.
Either IRQ is not enabled properly or not working in IRQ mode.
But it works if polling is enabled.

> 
> According to the CI20 schematic, it is wired properly. So that's strange.

Yes, HTPLG input goes through an 1kΩ + 1µF low-pass filter to debounce HDMI_HTPLG.
This goes to the HPD (BGA ball N19).
There is an optional Q14 driving HDMI_DETE_N.
This could become the hpd-gpios property of the connector in the device tree.
But it is optional.

So we have to use dw-hdmi HPD and make it work (in combination with a chained hdmi-connector).

> 
>> In this case I think the Ingenic DRM core should call drm_kms_helper_poll_init(drm) instead.
> 
> Yes, the ingenic-drm driver does not poll for connectors because until now it never has been needed.

Well, if we go back a while we only needed it after introducing the hdmi-connectors
and making dw-hdmi a bridge.

Originally the dw-hdmi driver did properly take care of everything (by registering its own connector).

BR and thanks,
Nikolaus

Re: [Letux-kernel] [PATCH v16 1/4] drm/bridge: dw-hdmi: introduce dw_hdmi_enable_poll()

[H. Nikolaus Schaller]

Hi Paul,

> Am 03.03.2022 um 17:51 schrieb Paul Cercueil <paul@crapouillou.net>:
> 
> Hi Nikolaus,
> 
> Le jeu., mars 3 2022 at 17:43:05 +0100, H. Nikolaus Schaller <hns@goldelico.com> a écrit :
>> Hi Neil,
>>> Am 03.03.2022 um 17:30 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
>>> Hi Neil,
>>>> Am 03.03.2022 um 17:23 schrieb Neil Armstrong <narmstrong@baylibre.com>:
>>>> Hi,
>>>> On 26/02/2022 18:12, H. Nikolaus Schaller wrote:
>>>>> so that specialization drivers like ingenic-dw-hdmi can enable polling.
>>>>> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
>>>>> ---
>>>>> drivers/gpu/drm/bridge/synopsys/dw-hdmi.c | 9 +++++++++
>>>>> include/drm/bridge/dw_hdmi.h              | 1 +
>>>>> 2 files changed, 10 insertions(+)
>>>>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>>>> index 4befc104d2200..43e375da131e8 100644
>>>>> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>>>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
>>>>> @@ -3217,6 +3217,15 @@ static int dw_hdmi_parse_dt(struct dw_hdmi *hdmi)
>>>>> 	return 0;
>>>>> }
>>>>> +void dw_hdmi_enable_poll(struct dw_hdmi *hdmi, bool enable)
>>>>> +{
>>>>> +	if (hdmi->bridge.dev)
>>>>> +		hdmi->bridge.dev->mode_config.poll_enabled = enable;
>>>>> +	else
>>>>> +		dev_warn(hdmi->dev, "no hdmi->bridge.dev");
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(dw_hdmi_enable_poll);
>>>>> +
>>>>> struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev,
>>>>> 			      const struct dw_hdmi_plat_data *plat_data)
>>>>> {
>>>>> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
>>>>> index 2a1f85f9a8a3f..963960794b40e 100644
>>>>> --- a/include/drm/bridge/dw_hdmi.h
>>>>> +++ b/include/drm/bridge/dw_hdmi.h
>>>>> @@ -196,5 +196,6 @@ enum drm_connector_status dw_hdmi_phy_read_hpd(struct dw_hdmi *hdmi,
>>>>> void dw_hdmi_phy_update_hpd(struct dw_hdmi *hdmi, void *data,
>>>>> 			    bool force, bool disabled, bool rxsense);
>>>>> void dw_hdmi_phy_setup_hpd(struct dw_hdmi *hdmi, void *data);
>>>>> +void dw_hdmi_enable_poll(struct dw_hdmi *hdmi, bool enable);
>>>>>   #endif /* __IMX_HDMI_H__ */
>>>> As I understand, this is because the IRQ line of the dw-hdmi IP isn't connected right ? and you use the display-connector ddc gpio instead ?
>>> Yes. The IRQ line is not connected on all boards as far as I can see.
>>>> In this case I think the Ingenic DRM core should call drm_kms_helper_poll_init(drm) instead.
>>> Ah, that is good. it seems to do "dw_hdmi_enable_poll()" in a more generic way.
>> Well, I looked through source code and it is defined as
>> 	void drm_kms_helper_poll_init(struct drm_device *dev)
>> But there is no direct pointer to some drm_device available.
>> Neither in dw-hdmi nor ingenic-dw-hdmi.
> 
> Well he said "the Ingenic DRM core" aka ingenic-drm-drv.c. You do have access to the main drm_device in the ingenic_drm_bind() function, so you can add it there (with a cleanup function calling drm_kms_helper_poll_fini() registered with drmm_add_action_or_reset()).

Well, do you really want to mix HPD detection between connector, Synopsys bridge and Ingenic DRM core? These are independent...
Or should be accessed only through the bridge chain pointers.

IMHO we should keep separate functions separate.

And maybe this should also be conditional? Maybe not depend on compatible = jz4780 but compatible = ci20?

Looks to me to be a quick fix in the wrong place.

Let's fix the CSC issue first.

BR,
Nikolaus

Re: [Letux-kernel] [PATCH v16 1/4] drm/bridge: dw-hdmi: introduce dw_hdmi_enable_poll()

[Paul Cercueil]

Hi Nikolaus,

[snip]

>>  Well he said "the Ingenic DRM core" aka ingenic-drm-drv.c. You do 
>> have access to the main drm_device in the ingenic_drm_bind() 
>> function, so you can add it there (with a cleanup function calling 
>> drm_kms_helper_poll_fini() registered with 
>> drmm_add_action_or_reset()).
> 
> Well, do you really want to mix HPD detection between connector, 
> Synopsys bridge and Ingenic DRM core? These are independent...
> Or should be accessed only through the bridge chain pointers.
> 
> IMHO we should keep separate functions separate.

The drm_kms_helper_poll_init() just says "this DRM device may have 
connectors that need to be polled" so it very well fits inside the main 
driver, IMHO.

-Paul

> 
> And maybe this should also be conditional? Maybe not depend on 
> compatible = jz4780 but compatible = ci20?
> 
> Looks to me to be a quick fix in the wrong place.
> 
> Let's fix the CSC issue first.
> 
> BR,
> Nikolaus
> 

Re: [Letux-kernel] [PATCH v16 1/4] drm/bridge: dw-hdmi: introduce dw_hdmi_enable_poll()

[H. Nikolaus Schaller]

Hi Paul, Neil,

> Am 03.03.2022 um 18:20 schrieb Paul Cercueil <paul@crapouillou.net>:
> 
> Hi Nikolaus,
> 
> [snip]
> 
>>> Well he said "the Ingenic DRM core" aka ingenic-drm-drv.c. You do have access to the main drm_device in the ingenic_drm_bind() function, so you can add it there (with a cleanup function calling drm_kms_helper_poll_fini() registered with drmm_add_action_or_reset()).
>> Well, do you really want to mix HPD detection between connector, Synopsys bridge and Ingenic DRM core? These are independent...
>> Or should be accessed only through the bridge chain pointers.
>> IMHO we should keep separate functions separate.
> 
> The drm_kms_helper_poll_init() just says "this DRM device may have connectors that need to be polled" so it very well fits inside the main driver, IMHO.

As far as I understand, it has the side-effect to always set dev->mode_config.poll_enabled and
schedule_delayed_work() for all devices.
I am not sure if this is intended for arbitrary ingenic-drm devices. But you know better than me.


Hm. But wait, I think I now finally remember why I have proposed it the way it is!
It is always better to go back to requirements and find the least invasive solution.

- HPD IRQ works and calls dw_hdmi_irq() [as can be shown by adding printk()]
- it is just that the udevd is only notified if poll_enabled = true (but no polling takes place!).

An earlier version (v4) to fix this proposed to add an explicit call to drm_kms_helper_hotplug_event()
in dw_hdmi_irq() but that was rejected a while ago because drm_helper_hpd_irq_event() will already call it:

	https://www.spinics.net/lists/dri-devel/msg316846.html

Since this did not take into account that dev->mode_config.poll_enabled must be set true, I then proposed the
enable_poll() mechanism just to set this bit for the ingenic-dw-hdmi specialization.

So a HPD event is delivered to the dw-hdmi driver as dw_hdmi_irq() and that calls drm_helper_hpd_irq_event()
but not drm_kms_helper_hotplug_event() and user-space is not getting aware.

It is all a hack because we mix the dw-hdmi driver which originally did register its own connector
with an explicit connector...

In summary I now thing that the v4 patch is the simplest and least invasive solution.

We neither have to introduce a dw_hdmi_enable_poll() function or call drm_kms_helper_poll_init() anywhere.

It is just a single line to add to dw-hdmi. And neither touches ingenic-dw-hdmi nor ingenic-drm-drv.

So let's go back to v4 version (just modify commit message to better describe why we have to call
drm_kms_helper_hotplug_event() explicitly) and forget about alternatives.

BR,
Nikolaus

Re: [Letux-kernel] [PATCH v16 1/4] drm/bridge: dw-hdmi: introduce dw_hdmi_enable_poll()

[Neil Armstrong]

Hi,

On 03/03/2022 18:59, H. Nikolaus Schaller wrote:
> Hi Paul, Neil,
> 
>> Am 03.03.2022 um 18:20 schrieb Paul Cercueil <paul@crapouillou.net>:
>>
>> Hi Nikolaus,
>>
>> [snip]
>>
>>>> Well he said "the Ingenic DRM core" aka ingenic-drm-drv.c. You do have access to the main drm_device in the ingenic_drm_bind() function, so you can add it there (with a cleanup function calling drm_kms_helper_poll_fini() registered with drmm_add_action_or_reset()).
>>> Well, do you really want to mix HPD detection between connector, Synopsys bridge and Ingenic DRM core? These are independent...
>>> Or should be accessed only through the bridge chain pointers.
>>> IMHO we should keep separate functions separate.
>>
>> The drm_kms_helper_poll_init() just says "this DRM device may have connectors that need to be polled" so it very well fits inside the main driver, IMHO.
> 
> As far as I understand, it has the side-effect to always set dev->mode_config.poll_enabled and
> schedule_delayed_work() for all devices.
> I am not sure if this is intended for arbitrary ingenic-drm devices. But you know better than me.
> 
> 
> Hm. But wait, I think I now finally remember why I have proposed it the way it is!
> It is always better to go back to requirements and find the least invasive solution.
> 
> - HPD IRQ works and calls dw_hdmi_irq() [as can be shown by adding printk()]
> - it is just that the udevd is only notified if poll_enabled = true (but no polling takes place!).
> 
> An earlier version (v4) to fix this proposed to add an explicit call to drm_kms_helper_hotplug_event()
> in dw_hdmi_irq() but that was rejected a while ago because drm_helper_hpd_irq_event() will already call it:
> 
> 	https://www.spinics.net/lists/dri-devel/msg316846.html
> 
> Since this did not take into account that dev->mode_config.poll_enabled must be set true, I then proposed the
> enable_poll() mechanism just to set this bit for the ingenic-dw-hdmi specialization.
> 
> So a HPD event is delivered to the dw-hdmi driver as dw_hdmi_irq() and that calls drm_helper_hpd_irq_event()
> but not drm_kms_helper_hotplug_event() and user-space is not getting aware.
> 
> It is all a hack because we mix the dw-hdmi driver which originally did register its own connector
> with an explicit connector...
> 
> In summary I now thing that the v4 patch is the simplest and least invasive solution.
> 
> We neither have to introduce a dw_hdmi_enable_poll() function or call drm_kms_helper_poll_init() anywhere.
> 
> It is just a single line to add to dw-hdmi. And neither touches ingenic-dw-hdmi nor ingenic-drm-drv.
> 
> So let's go back to v4 version (just modify commit message to better describe why we have to call
> drm_kms_helper_hotplug_event() explicitly) and forget about alternatives.

Please don't and add drm_kms_helper_poll_init() from the ingenic-drm-drv.c like every other DRM driver.

Adding drm_kms_helper_hotplug_event() in dw-hdmi will impact other drivers using dw-hdmi but correctly
calling drm_kms_helper_poll_init().

Neil

> 
> BR,
> Nikolaus

Re: [Letux-kernel] [PATCH v16 1/4] drm/bridge: dw-hdmi: introduce dw_hdmi_enable_poll()

[Paul Cercueil]

Hi Neil,

Le ven., mars 4 2022 at 14:30:46 +0100, Neil Armstrong 
<narmstrong@baylibre.com> a écrit :
> Hi,
> 
> On 03/03/2022 18:59, H. Nikolaus Schaller wrote:
>> Hi Paul, Neil,
>> 
>>> Am 03.03.2022 um 18:20 schrieb Paul Cercueil <paul@crapouillou.net>:
>>> 
>>> Hi Nikolaus,
>>> 
>>> [snip]
>>> 
>>>>> Well he said "the Ingenic DRM core" aka ingenic-drm-drv.c. You do 
>>>>> have access to the main drm_device in the ingenic_drm_bind() 
>>>>> function, so you can add it there (with a cleanup function 
>>>>> calling drm_kms_helper_poll_fini() registered with 
>>>>> drmm_add_action_or_reset()).
>>>> Well, do you really want to mix HPD detection between connector, 
>>>> Synopsys bridge and Ingenic DRM core? These are independent...
>>>> Or should be accessed only through the bridge chain pointers.
>>>> IMHO we should keep separate functions separate.
>>> 
>>> The drm_kms_helper_poll_init() just says "this DRM device may have 
>>> connectors that need to be polled" so it very well fits inside the 
>>> main driver, IMHO.
>> 
>> As far as I understand, it has the side-effect to always set 
>> dev->mode_config.poll_enabled and
>> schedule_delayed_work() for all devices.
>> I am not sure if this is intended for arbitrary ingenic-drm devices. 
>> But you know better than me.
>> 
>> 
>> Hm. But wait, I think I now finally remember why I have proposed it 
>> the way it is!
>> It is always better to go back to requirements and find the least 
>> invasive solution.
>> 
>> - HPD IRQ works and calls dw_hdmi_irq() [as can be shown by adding 
>> printk()]
>> - it is just that the udevd is only notified if poll_enabled = true 
>> (but no polling takes place!).
>> 
>> An earlier version (v4) to fix this proposed to add an explicit call 
>> to drm_kms_helper_hotplug_event()
>> in dw_hdmi_irq() but that was rejected a while ago because 
>> drm_helper_hpd_irq_event() will already call it:
>> 
>> 	https://www.spinics.net/lists/dri-devel/msg316846.html
>> 
>> Since this did not take into account that 
>> dev->mode_config.poll_enabled must be set true, I then proposed the
>> enable_poll() mechanism just to set this bit for the ingenic-dw-hdmi 
>> specialization.
>> 
>> So a HPD event is delivered to the dw-hdmi driver as dw_hdmi_irq() 
>> and that calls drm_helper_hpd_irq_event()
>> but not drm_kms_helper_hotplug_event() and user-space is not getting 
>> aware.
>> 
>> It is all a hack because we mix the dw-hdmi driver which originally 
>> did register its own connector
>> with an explicit connector...
>> 
>> In summary I now thing that the v4 patch is the simplest and least 
>> invasive solution.
>> 
>> We neither have to introduce a dw_hdmi_enable_poll() function or 
>> call drm_kms_helper_poll_init() anywhere.
>> 
>> It is just a single line to add to dw-hdmi. And neither touches 
>> ingenic-dw-hdmi nor ingenic-drm-drv.
>> 
>> So let's go back to v4 version (just modify commit message to better 
>> describe why we have to call
>> drm_kms_helper_hotplug_event() explicitly) and forget about 
>> alternatives.
> 
> Please don't and add drm_kms_helper_poll_init() from the 
> ingenic-drm-drv.c like every other DRM driver.
> 
> Adding drm_kms_helper_hotplug_event() in dw-hdmi will impact other 
> drivers using dw-hdmi but correctly
> calling drm_kms_helper_poll_init().

 From what I understood in Nikolaus' last message, HDMI hotplug is 
actually correctly detected, so there's no need for polling. What is 
missing is the call to drm_kms_helper_hotplug_event *somewhere*, so 
that the information is correctly relayed to userspace.

I think this issue can be fixed by calling 
drm_bridge_connector_enable_hpd() on the connector in ingenic-drm-drv.c.

Cheers,
-Paul

Re: [Letux-kernel] [PATCH v16 1/4] drm/bridge: dw-hdmi: introduce dw_hdmi_enable_poll()

[H. Nikolaus Schaller]

Hi Paul, Neil,

> Am 04.03.2022 um 17:47 schrieb Paul Cercueil <paul@crapouillou.net>:
> 
> From what I understood in Nikolaus' last message, HDMI hotplug is actually correctly detected, so there's no need for polling. What is missing is the call to drm_kms_helper_hotplug_event *somewhere*, so that the information is correctly relayed to userspace.

Exactly.

As Maxime pointed out it should already be called by drm_helper_hpd_irq_event() in dw_hdmi_irq() but isn't
because mode_config.poll_enabled isn't enabled.

So we can either
a) enable mode_config.poll_enabled so that it is called by drm_helper_hpd_irq_event() or

b) make drm_kms_helper_hotplug_event() being called explicitly in dw_hdmi_irq().
   We could guard that by mode_config.poll_enabled to avoid drm_kms_helper_hotplug_event()
   being called twice (but I think the "changed" mechanism will take care of).

> I think this issue can be fixed by calling drm_bridge_connector_enable_hpd() on the connector in ingenic-drm-drv.c.

I don't see yet how this would solve it, but it may work.

Anyways, this all is a missing feature (sometimes called "bug") of the *dw-hdmi driver* and IMHO
neither of the connector nor the ingenic-drm-drv.

So I think it should not be solved outside dw-hdmi.

BR and thanks,
Nikolaus

Re: [Letux-kernel] [PATCH v16 1/4] drm/bridge: dw-hdmi: introduce dw_hdmi_enable_poll()

[Paul Cercueil]

Le ven., mars 4 2022 at 18:51:14 +0100, H. Nikolaus Schaller 
<hns@goldelico.com> a écrit :
> Hi Paul, Neil,
> 
>>  Am 04.03.2022 um 17:47 schrieb Paul Cercueil <paul@crapouillou.net>:
>> 
>>  From what I understood in Nikolaus' last message, HDMI hotplug is 
>> actually correctly detected, so there's no need for polling. What is 
>> missing is the call to drm_kms_helper_hotplug_event *somewhere*, so 
>> that the information is correctly relayed to userspace.
> 
> Exactly.
> 
> As Maxime pointed out it should already be called by 
> drm_helper_hpd_irq_event() in dw_hdmi_irq() but isn't
> because mode_config.poll_enabled isn't enabled.
> 
> So we can either
> a) enable mode_config.poll_enabled so that it is called by 
> drm_helper_hpd_irq_event() or
> 
> b) make drm_kms_helper_hotplug_event() being called explicitly in 
> dw_hdmi_irq().
>    We could guard that by mode_config.poll_enabled to avoid 
> drm_kms_helper_hotplug_event()
>    being called twice (but I think the "changed" mechanism will take 
> care of).
> 
>>  I think this issue can be fixed by calling 
>> drm_bridge_connector_enable_hpd() on the connector in 
>> ingenic-drm-drv.c.
> 
> I don't see yet how this would solve it, but it may work.

dw_hdmi_irq() calls drm_bridge_hpd_notify(), which would call 
bridge->hpd_cb() if it was non-NULL.

Calling drm_bridge_connector_enable_hpd() will set the bridge->hpd_cb() 
callback to point to drm_bridge_connector_hpd_cb(), which itself will 
call drm_kms_helper_hotplug_event(). Therefore, all that is missing is 
one call to drm_bridge_connector_enable_hpd().

Cheers,
-Paul

> 
> Anyways, this all is a missing feature (sometimes called "bug") of 
> the *dw-hdmi driver* and IMHO
> neither of the connector nor the ingenic-drm-drv.
> 
> So I think it should not be solved outside dw-hdmi.
> 
> BR and thanks,
> Nikolaus
> 

Re: [Letux-kernel] [PATCH v16 1/4] drm/bridge: dw-hdmi: introduce dw_hdmi_enable_poll()

[H. Nikolaus Schaller]

Hi Paul,

> Am 04.03.2022 um 19:04 schrieb Paul Cercueil <paul@crapouillou.net>:
> 
> 
> 
> Le ven., mars 4 2022 at 18:51:14 +0100, H. Nikolaus Schaller <hns@goldelico.com> a écrit :
>> Hi Paul, Neil,
>>> Am 04.03.2022 um 17:47 schrieb Paul Cercueil <paul@crapouillou.net>:
>>> From what I understood in Nikolaus' last message, HDMI hotplug is actually correctly detected, so there's no need for polling. What is missing is the call to drm_kms_helper_hotplug_event *somewhere*, so that the information is correctly relayed to userspace.
>> Exactly.
>> As Maxime pointed out it should already be called by drm_helper_hpd_irq_event() in dw_hdmi_irq() but isn't
>> because mode_config.poll_enabled isn't enabled.
>> So we can either
>> a) enable mode_config.poll_enabled so that it is called by drm_helper_hpd_irq_event() or
>> b) make drm_kms_helper_hotplug_event() being called explicitly in dw_hdmi_irq().
>>   We could guard that by mode_config.poll_enabled to avoid drm_kms_helper_hotplug_event()
>>   being called twice (but I think the "changed" mechanism will take care of).
>>> I think this issue can be fixed by calling drm_bridge_connector_enable_hpd() on the connector in ingenic-drm-drv.c.
>> I don't see yet how this would solve it, but it may work.
> 
> dw_hdmi_irq() calls drm_bridge_hpd_notify(), which would call bridge->hpd_cb() if it was non-NULL.

Ok, this is a case c).

I vaguely remember having tried to analyse what bridge->hpd_cb is but stopped since it is NULL...

> 
> Calling drm_bridge_connector_enable_hpd() will set the bridge->hpd_cb() callback to point to drm_bridge_connector_hpd_cb(), which itself will call drm_kms_helper_hotplug_event(). Therefore, all that is missing is one call to drm_bridge_connector_enable_hpd().

Ah, ok, I see.

>> Anyways, this all is a missing feature (sometimes called "bug") of the *dw-hdmi driver* and IMHO
>> neither of the connector nor the ingenic-drm-drv.

Well, a little more analysis shows that drm_bridge_connector_enable_hpd is called
in the *-drv.c for some other plaforms:

https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/imx/dcss/dcss-dev.c#L292
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/imx/dcss/dcss-kms.c#L145
https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/omapdrm/omap_drv.c#L393
https://elixir.bootlin.com/linux/v5.17-rc6/source/drivers/gpu/drm/msm/hdmi/hdmi.c#L317

>> So I think it should not be solved outside dw-hdmi.

Hm. Can we call drm_bridge_connector_enable_hpd() from inside dw-hdmi?

Or would this be the solution if merged? (I currently can't try code).

https://lore.kernel.org/lkml/a7d0b013-6114-07b3-0a7b-0d17db8a3982@cogentembedded.com/T/

BR and thanks,
Nikolaus

Re: [Letux-kernel] [PATCH v16 1/4] drm/bridge: dw-hdmi: introduce dw_hdmi_enable_poll()

[Paul Cercueil]

Le ven., mars 4 2022 at 19:15:13 +0100, H. Nikolaus Schaller 
<hns@goldelico.com> a écrit :
> Hi Paul,
> 
>>  Am 04.03.2022 um 19:04 schrieb Paul Cercueil <paul@crapouillou.net>:
>> 
>> 
>> 
>>  Le ven., mars 4 2022 at 18:51:14 +0100, H. Nikolaus Schaller 
>> <hns@goldelico.com> a écrit :
>>>  Hi Paul, Neil,
>>>>  Am 04.03.2022 um 17:47 schrieb Paul Cercueil 
>>>> <paul@crapouillou.net>:
>>>>  From what I understood in Nikolaus' last message, HDMI hotplug is 
>>>> actually correctly detected, so there's no need for polling. What 
>>>> is missing is the call to drm_kms_helper_hotplug_event 
>>>> *somewhere*, so that the information is correctly relayed to 
>>>> userspace.
>>>  Exactly.
>>>  As Maxime pointed out it should already be called by 
>>> drm_helper_hpd_irq_event() in dw_hdmi_irq() but isn't
>>>  because mode_config.poll_enabled isn't enabled.
>>>  So we can either
>>>  a) enable mode_config.poll_enabled so that it is called by 
>>> drm_helper_hpd_irq_event() or
>>>  b) make drm_kms_helper_hotplug_event() being called explicitly in 
>>> dw_hdmi_irq().
>>>    We could guard that by mode_config.poll_enabled to avoid 
>>> drm_kms_helper_hotplug_event()
>>>    being called twice (but I think the "changed" mechanism will 
>>> take care of).
>>>>  I think this issue can be fixed by calling 
>>>> drm_bridge_connector_enable_hpd() on the connector in 
>>>> ingenic-drm-drv.c.
>>>  I don't see yet how this would solve it, but it may work.
>> 
>>  dw_hdmi_irq() calls drm_bridge_hpd_notify(), which would call 
>> bridge->hpd_cb() if it was non-NULL.
> 
> Ok, this is a case c).
> 
> I vaguely remember having tried to analyse what bridge->hpd_cb is but 
> stopped since it is NULL...
> 
>> 
>>  Calling drm_bridge_connector_enable_hpd() will set the 
>> bridge->hpd_cb() callback to point to drm_bridge_connector_hpd_cb(), 
>> which itself will call drm_kms_helper_hotplug_event(). Therefore, 
>> all that is missing is one call to drm_bridge_connector_enable_hpd().
> 
> Ah, ok, I see.
> 
>>>  Anyways, this all is a missing feature (sometimes called "bug") of 
>>> the *dw-hdmi driver* and IMHO
>>>  neither of the connector nor the ingenic-drm-drv.
> 
> Well, a little more analysis shows that 
> drm_bridge_connector_enable_hpd is called
> in the *-drv.c for some other plaforms:
> 
> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/imx/dcss/dcss-dev.c#L292
> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/imx/dcss/dcss-kms.c#L145
> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/omapdrm/omap_drv.c#L393
> https://elixir.bootlin.com/linux/v5.17-rc6/source/drivers/gpu/drm/msm/hdmi/hdmi.c#L317
> 
>>>  So I think it should not be solved outside dw-hdmi.
> 
> Hm. Can we call drm_bridge_connector_enable_hpd() from inside dw-hdmi?
> 
> Or would this be the solution if merged? (I currently can't try code).
> 
> https://lore.kernel.org/lkml/a7d0b013-6114-07b3-0a7b-0d17db8a3982@cogentembedded.com/T/

Looks correct to me. It has been reviewed by two people so I believe it 
will be merged very soon.

Cheers,
-Paul

Re: [Letux-kernel] [PATCH v16 1/4] drm/bridge: dw-hdmi: introduce dw_hdmi_enable_poll()

[H. Nikolaus Schaller]

> Am 04.03.2022 um 19:33 schrieb Paul Cercueil <paul@crapouillou.net>:
> 
> 
> 
> Le ven., mars 4 2022 at 19:15:13 +0100, H. Nikolaus Schaller <hns@goldelico.com> a écrit :
>> Hi Paul,
>>> Am 04.03.2022 um 19:04 schrieb Paul Cercueil <paul@crapouillou.net>:
>>> Le ven., mars 4 2022 at 18:51:14 +0100, H. Nikolaus Schaller <hns@goldelico.com> a écrit :
>>>> Hi Paul, Neil,
>>>>> Am 04.03.2022 um 17:47 schrieb Paul Cercueil <paul@crapouillou.net>:
>>>>> From what I understood in Nikolaus' last message, HDMI hotplug is actually correctly detected, so there's no need for polling. What is missing is the call to drm_kms_helper_hotplug_event *somewhere*, so that the information is correctly relayed to userspace.
>>>> Exactly.
>>>> As Maxime pointed out it should already be called by drm_helper_hpd_irq_event() in dw_hdmi_irq() but isn't
>>>> because mode_config.poll_enabled isn't enabled.
>>>> So we can either
>>>> a) enable mode_config.poll_enabled so that it is called by drm_helper_hpd_irq_event() or
>>>> b) make drm_kms_helper_hotplug_event() being called explicitly in dw_hdmi_irq().
>>>>   We could guard that by mode_config.poll_enabled to avoid drm_kms_helper_hotplug_event()
>>>>   being called twice (but I think the "changed" mechanism will take care of).
>>>>> I think this issue can be fixed by calling drm_bridge_connector_enable_hpd() on the connector in ingenic-drm-drv.c.
>>>> I don't see yet how this would solve it, but it may work.
>>> dw_hdmi_irq() calls drm_bridge_hpd_notify(), which would call bridge->hpd_cb() if it was non-NULL.
>> Ok, this is a case c).
>> I vaguely remember having tried to analyse what bridge->hpd_cb is but stopped since it is NULL...
>>> Calling drm_bridge_connector_enable_hpd() will set the bridge->hpd_cb() callback to point to drm_bridge_connector_hpd_cb(), which itself will call drm_kms_helper_hotplug_event(). Therefore, all that is missing is one call to drm_bridge_connector_enable_hpd().
>> Ah, ok, I see.
>>>> Anyways, this all is a missing feature (sometimes called "bug") of the *dw-hdmi driver* and IMHO
>>>> neither of the connector nor the ingenic-drm-drv.
>> Well, a little more analysis shows that drm_bridge_connector_enable_hpd is called
>> in the *-drv.c for some other plaforms:
>> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/imx/dcss/dcss-dev.c#L292
>> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/imx/dcss/dcss-kms.c#L145
>> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/omapdrm/omap_drv.c#L393
>> https://elixir.bootlin.com/linux/v5.17-rc6/source/drivers/gpu/drm/msm/hdmi/hdmi.c#L317
>>>> So I think it should not be solved outside dw-hdmi.
>> Hm. Can we call drm_bridge_connector_enable_hpd() from inside dw-hdmi?
>> Or would this be the solution if merged? (I currently can't try code).
>> https://lore.kernel.org/lkml/a7d0b013-6114-07b3-0a7b-0d17db8a3982@cogentembedded.com/T/
> 
> Looks correct to me. It has been reviewed by two people so I believe it will be merged very soon.

Great. I will try asap. If it works we can drop all our private ideas...

And focus on the last missing piece for jz4780 HDMI: the output format negotiation (which still is not working properly - but I have to analyse why).

BR and thanks,
Nikolaus

Re: [Letux-kernel] [PATCH v16 1/4] drm/bridge: dw-hdmi: introduce dw_hdmi_enable_poll()

[H. Nikolaus Schaller]

Hi Paul,

> Am 04.03.2022 um 19:41 schrieb H. Nikolaus Schaller <hns@goldelico.com>:
> 
> 
> 
>> Am 04.03.2022 um 19:33 schrieb Paul Cercueil <paul@crapouillou.net>:
>> 
>> 
>> 
>> Le ven., mars 4 2022 at 19:15:13 +0100, H. Nikolaus Schaller <hns@goldelico.com> a écrit :
>>> Hi Paul,
>>>> Am 04.03.2022 um 19:04 schrieb Paul Cercueil <paul@crapouillou.net>:
>>>> Le ven., mars 4 2022 at 18:51:14 +0100, H. Nikolaus Schaller <hns@goldelico.com> a écrit :
>>>>> Hi Paul, Neil,
>>>>>> Am 04.03.2022 um 17:47 schrieb Paul Cercueil <paul@crapouillou.net>:
>>>>>> From what I understood in Nikolaus' last message, HDMI hotplug is actually correctly detected, so there's no need for polling. What is missing is the call to drm_kms_helper_hotplug_event *somewhere*, so that the information is correctly relayed to userspace.
>>>>> Exactly.
>>>>> As Maxime pointed out it should already be called by drm_helper_hpd_irq_event() in dw_hdmi_irq() but isn't
>>>>> because mode_config.poll_enabled isn't enabled.
>>>>> So we can either
>>>>> a) enable mode_config.poll_enabled so that it is called by drm_helper_hpd_irq_event() or
>>>>> b) make drm_kms_helper_hotplug_event() being called explicitly in dw_hdmi_irq().
>>>>>  We could guard that by mode_config.poll_enabled to avoid drm_kms_helper_hotplug_event()
>>>>>  being called twice (but I think the "changed" mechanism will take care of).
>>>>>> I think this issue can be fixed by calling drm_bridge_connector_enable_hpd() on the connector in ingenic-drm-drv.c.
>>>>> I don't see yet how this would solve it, but it may work.
>>>> dw_hdmi_irq() calls drm_bridge_hpd_notify(), which would call bridge->hpd_cb() if it was non-NULL.
>>> Ok, this is a case c).
>>> I vaguely remember having tried to analyse what bridge->hpd_cb is but stopped since it is NULL...
>>>> Calling drm_bridge_connector_enable_hpd() will set the bridge->hpd_cb() callback to point to drm_bridge_connector_hpd_cb(), which itself will call drm_kms_helper_hotplug_event(). Therefore, all that is missing is one call to drm_bridge_connector_enable_hpd().
>>> Ah, ok, I see.
>>>>> Anyways, this all is a missing feature (sometimes called "bug") of the *dw-hdmi driver* and IMHO
>>>>> neither of the connector nor the ingenic-drm-drv.
>>> Well, a little more analysis shows that drm_bridge_connector_enable_hpd is called
>>> in the *-drv.c for some other plaforms:
>>> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/imx/dcss/dcss-dev.c#L292
>>> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/imx/dcss/dcss-kms.c#L145
>>> https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/omapdrm/omap_drv.c#L393
>>> https://elixir.bootlin.com/linux/v5.17-rc6/source/drivers/gpu/drm/msm/hdmi/hdmi.c#L317
>>>>> So I think it should not be solved outside dw-hdmi.
>>> Hm. Can we call drm_bridge_connector_enable_hpd() from inside dw-hdmi?
>>> Or would this be the solution if merged? (I currently can't try code).
>>> https://lore.kernel.org/lkml/a7d0b013-6114-07b3-0a7b-0d17db8a3982@cogentembedded.com/T/
>> 
>> Looks correct to me. It has been reviewed by two people so I believe it will be merged very soon.
> 
> Great. I will try asap. If it works we can drop all our private ideas...
> 
> And focus on the last missing piece for jz4780 HDMI: the output format negotiation (which still is not working properly - but I have to analyse why).

Yes, it works. And I see you have merged it to drm-misc-next so that I can build on it.

So there is only the bus format negotiation to be understood and finally solved for v17.

Great and thanks,
Nikolaus