[PATCH v2 0/8] MIPS: jz4780 HDMI

[PATCH v2 0/8] MIPS: jz4780 HDMI

[H. Nikolaus Schaller]

This series adds HDMI support for JZ4780 and CI20 board

V2:
- 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)


H. Nikolaus Schaller (2):
  MIPS: CI20: defconfig: configure for DRM_DW_HDMI_JZ4780
  [RFC] drm/ingenic: convert to component framework for jz4780 hdmi

Paul Boddie (5):
  drm/bridge: synopsis: Add mode_fixup and bridge timings support
  drm/ingenic: Add jz4780 Synopsys HDMI driver
  drm/ingenic: Add support for JZ4780 and HDMI output
  MIPS: DTS: jz4780: account for Synopsys HDMI driver and LCD controller
  MIPS: DTS: CI20: add HDMI setup

Sam Ravnborg (1):
  dt-bindings: display: Add ingenic-jz4780-hdmi DT Schema

 .../bindings/display/ingenic-jz4780-hdmi.yaml |  82 +++++++++
 arch/mips/boot/dts/ingenic/ci20.dts           |  64 +++++++
 arch/mips/boot/dts/ingenic/jz4780.dtsi        |  45 +++++
 arch/mips/configs/ci20_defconfig              |   7 +
 drivers/gpu/drm/bridge/synopsys/dw-hdmi.c     |  16 ++
 drivers/gpu/drm/ingenic/Kconfig               |   9 +
 drivers/gpu/drm/ingenic/Makefile              |   1 +
 drivers/gpu/drm/ingenic/ingenic-drm-drv.c     | 163 ++++++++++++++++--
 drivers/gpu/drm/ingenic/ingenic-drm.h         |  52 ++++++
 drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c     | 156 +++++++++++++++++
 include/drm/bridge/dw_hdmi.h                  |   5 +
 11 files changed, 585 insertions(+), 15 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml
 create mode 100644 drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c

-- 
2.31.1

[PATCH v2 1/8] drm/bridge: synopsis: Add mode_fixup and bridge timings support

[H. Nikolaus Schaller]

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

The platform-specific configuration structure is augmented with
mode_fixup and timings members so that specialisations of the
Synopsys driver can introduce mode flags and bus flags.

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/bridge/synopsys/dw-hdmi.c | 16 ++++++++++++++++
 include/drm/bridge/dw_hdmi.h              |  5 +++++
 2 files changed, 21 insertions(+)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
index e7c7c9b9c646f..e8499eb11328c 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
@@ -2810,6 +2810,19 @@ dw_hdmi_bridge_mode_valid(struct drm_bridge *bridge,
 	return mode_status;
 }
 
+static bool
+dw_hdmi_bridge_mode_fixup(struct drm_bridge *bridge,
+			  const struct drm_display_mode *mode,
+			  struct drm_display_mode *adjusted_mode)
+{
+	struct dw_hdmi *hdmi = bridge->driver_private;
+
+	if (hdmi->plat_data->mode_fixup)
+		return hdmi->plat_data->mode_fixup(bridge, mode, adjusted_mode);
+
+	return true;
+}
+
 static void dw_hdmi_bridge_mode_set(struct drm_bridge *bridge,
 				    const struct drm_display_mode *orig_mode,
 				    const struct drm_display_mode *mode)
@@ -2883,6 +2896,7 @@ static const struct drm_bridge_funcs dw_hdmi_bridge_funcs = {
 	.atomic_disable = dw_hdmi_bridge_atomic_disable,
 	.mode_set = dw_hdmi_bridge_mode_set,
 	.mode_valid = dw_hdmi_bridge_mode_valid,
+	.mode_fixup = dw_hdmi_bridge_mode_fixup,
 	.detect = dw_hdmi_bridge_detect,
 	.get_edid = dw_hdmi_bridge_get_edid,
 };
@@ -3364,6 +3378,8 @@ struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev,
 #ifdef CONFIG_OF
 	hdmi->bridge.of_node = pdev->dev.of_node;
 #endif
+	if (plat_data->timings)
+		hdmi->bridge.timings = plat_data->timings;
 
 	memset(&pdevinfo, 0, sizeof(pdevinfo));
 	pdevinfo.parent = dev;
diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
index 6a5716655619b..677137445d534 100644
--- a/include/drm/bridge/dw_hdmi.h
+++ b/include/drm/bridge/dw_hdmi.h
@@ -8,6 +8,7 @@
 
 #include <sound/hdmi-codec.h>
 
+struct drm_bridge;
 struct drm_display_info;
 struct drm_display_mode;
 struct drm_encoder;
@@ -140,6 +141,10 @@ struct dw_hdmi_plat_data {
 	enum drm_mode_status (*mode_valid)(struct dw_hdmi *hdmi, void *data,
 					   const struct drm_display_info *info,
 					   const struct drm_display_mode *mode);
+	bool (*mode_fixup)(struct drm_bridge *bridge,
+			   const struct drm_display_mode *mode,
+			   struct drm_display_mode *adjusted_mode);
+	const struct drm_bridge_timings *timings;
 
 	/* Vendor PHY support */
 	const struct dw_hdmi_phy_ops *phy_ops;
-- 
2.31.1

Re: [PATCH v2 1/8] drm/bridge: synopsis: Add mode_fixup and bridge timings support

[Robert Foss]

Hey Nikolaus,

Thanks for submitting this series.

On Thu, 5 Aug 2021 at 16:08, H. Nikolaus Schaller <hns@goldelico.com> wrote:
>
> From: Paul Boddie <paul@boddie.org.uk>
>
> The platform-specific configuration structure is augmented with
> mode_fixup and timings members so that specialisations of the
> Synopsys driver can introduce mode flags and bus flags.
>
> 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/bridge/synopsys/dw-hdmi.c | 16 ++++++++++++++++
>  include/drm/bridge/dw_hdmi.h              |  5 +++++
>  2 files changed, 21 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> index e7c7c9b9c646f..e8499eb11328c 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi.c
> @@ -2810,6 +2810,19 @@ dw_hdmi_bridge_mode_valid(struct drm_bridge *bridge,
>         return mode_status;
>  }
>
> +static bool
> +dw_hdmi_bridge_mode_fixup(struct drm_bridge *bridge,
> +                         const struct drm_display_mode *mode,
> +                         struct drm_display_mode *adjusted_mode)
> +{
> +       struct dw_hdmi *hdmi = bridge->driver_private;
> +
> +       if (hdmi->plat_data->mode_fixup)
> +               return hdmi->plat_data->mode_fixup(bridge, mode, adjusted_mode);
> +
> +       return true;
> +}
> +
>  static void dw_hdmi_bridge_mode_set(struct drm_bridge *bridge,
>                                     const struct drm_display_mode *orig_mode,
>                                     const struct drm_display_mode *mode)
> @@ -2883,6 +2896,7 @@ static const struct drm_bridge_funcs dw_hdmi_bridge_funcs = {
>         .atomic_disable = dw_hdmi_bridge_atomic_disable,
>         .mode_set = dw_hdmi_bridge_mode_set,
>         .mode_valid = dw_hdmi_bridge_mode_valid,
> +       .mode_fixup = dw_hdmi_bridge_mode_fixup,

mode_fixup() has been deprecated[1] in favor of atomic_check(), care
has to be taken when switching to atomic_check() as it has access to
the full atomic commit.

Looking at this driver, it's using mode_set as well, which should be fixed.

[1] https://lore.kernel.org/dri-devel/20210722062246.2512666-8-sam@ravnborg.org/

>         .detect = dw_hdmi_bridge_detect,
>         .get_edid = dw_hdmi_bridge_get_edid,
>  };
> @@ -3364,6 +3378,8 @@ struct dw_hdmi *dw_hdmi_probe(struct platform_device *pdev,
>  #ifdef CONFIG_OF
>         hdmi->bridge.of_node = pdev->dev.of_node;
>  #endif
> +       if (plat_data->timings)
> +               hdmi->bridge.timings = plat_data->timings;
>
>         memset(&pdevinfo, 0, sizeof(pdevinfo));
>         pdevinfo.parent = dev;
> diff --git a/include/drm/bridge/dw_hdmi.h b/include/drm/bridge/dw_hdmi.h
> index 6a5716655619b..677137445d534 100644
> --- a/include/drm/bridge/dw_hdmi.h
> +++ b/include/drm/bridge/dw_hdmi.h
> @@ -8,6 +8,7 @@
>
>  #include <sound/hdmi-codec.h>
>
> +struct drm_bridge;
>  struct drm_display_info;
>  struct drm_display_mode;
>  struct drm_encoder;
> @@ -140,6 +141,10 @@ struct dw_hdmi_plat_data {
>         enum drm_mode_status (*mode_valid)(struct dw_hdmi *hdmi, void *data,
>                                            const struct drm_display_info *info,
>                                            const struct drm_display_mode *mode);
> +       bool (*mode_fixup)(struct drm_bridge *bridge,
> +                          const struct drm_display_mode *mode,
> +                          struct drm_display_mode *adjusted_mode);
> +       const struct drm_bridge_timings *timings;
>
>         /* Vendor PHY support */
>         const struct dw_hdmi_phy_ops *phy_ops;
> --
> 2.31.1
>

Re: [PATCH v2 1/8] drm/bridge: synopsis: Add mode_fixup and bridge timings support

[H. Nikolaus Schaller]

Hi Robert,

> Am 05.08.2021 um 16:32 schrieb Robert Foss <robert.foss@linaro.org>:
> 
> Hey Nikolaus,
> 
> Thanks for submitting this series.
> 
> On Thu, 5 Aug 2021 at 16:08, H. Nikolaus Schaller <hns@goldelico.com> wrote:
>> 
>> From: Paul Boddie <paul@boddie.org.uk>
>> 
>> +       .mode_fixup = dw_hdmi_bridge_mode_fixup,
> 
> mode_fixup() has been deprecated[1] in favor of atomic_check(), care
> has to be taken when switching to atomic_check() as it has access to
> the full atomic commit.
> 
> Looking at this driver, it's using mode_set as well, which should be fixed.
> 
> [1] https://lore.kernel.org/dri-devel/20210722062246.2512666-8-sam@ravnborg.org/

Thanks for this link!

I have found some patches which convert mode_fixup -> atomic_check (e.g. 3afb2a28fa2404)
and atomic_check was apparently introduced by b86d895524ab72

That should be sufficient information that we can modify it.

BR and thanks,
Nikolaus

Re: [PATCH v2 1/8] drm/bridge: synopsis: Add mode_fixup and bridge timings support

[H. Nikolaus Schaller]

Hi Robert,


> Am 05.08.2021 um 16:32 schrieb Robert Foss <robert.foss@linaro.org>:
> 
> Hey Nikolaus,
> 
> Thanks for submitting this series.
> 
> On Thu, 5 Aug 2021 at 16:08, H. Nikolaus Schaller <hns@goldelico.com> wrote:
>> 
>> From: Paul Boddie <paul@boddie.org.uk>
>> 
>> static void dw_hdmi_bridge_mode_set(struct drm_bridge *bridge,
>>                                    const struct drm_display_mode *orig_mode,
>>                                    const struct drm_display_mode *mode)
>> @@ -2883,6 +2896,7 @@ static const struct drm_bridge_funcs dw_hdmi_bridge_funcs = {
>>        .atomic_disable = dw_hdmi_bridge_atomic_disable,
>>        .mode_set = dw_hdmi_bridge_mode_set,
>>        .mode_valid = dw_hdmi_bridge_mode_valid,
>> +       .mode_fixup = dw_hdmi_bridge_mode_fixup,
> 
> mode_fixup() has been deprecated[1] in favor of atomic_check(), care
> has to be taken when switching to atomic_check() as it has access to
> the full atomic commit.
> 
> Looking at this driver, it's using mode_set as well, which should be fixed.
> 
> [1] https://lore.kernel.org/dri-devel/20210722062246.2512666-8-sam@ravnborg.org/

We have moved code from mode_fixup() to atomic_check().
Was not difficult.

v3 will come soon.

BR and thanks,
Nikolaus Schaller

[PATCH v2 2/8] drm/ingenic: Add jz4780 Synopsys HDMI driver

[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.

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 | 121 ++++++++++++++++++++++
 3 files changed, 131 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 3b57f8be007c4..4c7d311fbefff 100644
--- a/drivers/gpu/drm/ingenic/Kconfig
+++ b/drivers/gpu/drm/ingenic/Kconfig
@@ -25,4 +25,13 @@ config DRM_INGENIC_IPU
 
 	  The Image Processing Unit (IPU) will appear as a second primary plane.
 
+config DRM_INGENIC_DW_HDMI
+	bool "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..3db9888a6c046 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
+ingenic-drm-$(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..61e7a57d7cec1
--- /dev/null
+++ b/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c
@@ -0,0 +1,121 @@
+// 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.
+ * Probe and remove operations derived from rcar_dw_hdmi.c.
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#include <drm/bridge/dw_hdmi.h>
+#include <drm/drm_of.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;
+
+	return MODE_OK;
+}
+
+static bool
+ingenic_dw_hdmi_mode_fixup(struct drm_bridge *bridge,
+			   const struct drm_display_mode *mode,
+			   struct drm_display_mode *adjusted_mode)
+{
+	adjusted_mode->flags |= (DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
+	adjusted_mode->flags &= ~(DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC);
+
+	return true;
+}
+
+static const struct drm_bridge_timings ingenic_dw_hdmi_timings = {
+	.input_bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE,
+};
+
+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,
+	.mode_fixup = ingenic_dw_hdmi_mode_fixup,
+	.timings    = &ingenic_dw_hdmi_timings,
+};
+
+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 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);
+
+	platform_set_drvdata(pdev, hdmi);
+
+	return 0;
+}
+
+static int ingenic_dw_hdmi_remove(struct platform_device *pdev)
+{
+	struct dw_hdmi *hdmi = platform_get_drvdata(pdev);
+
+	dw_hdmi_remove(hdmi);
+
+	return 0;
+}
+
+static struct platform_driver ingenic_dw_hdmi_driver = {
+	.probe  = ingenic_dw_hdmi_probe,
+	.remove = ingenic_dw_hdmi_remove,
+	.driver = {
+		.name = "dw-hdmi-ingenic",
+		.of_match_table = ingenic_dw_hdmi_dt_ids,
+	},
+};
+
+struct platform_driver *ingenic_dw_hdmi_driver_ptr = &ingenic_dw_hdmi_driver;
-- 
2.31.1

[PATCH v2 3/8] drm/ingenic: Add support for JZ4780 and HDMI output

[H. Nikolaus Schaller]

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

Add support for the LCD controller present on JZ4780 SoCs.
This SoC uses 8-byte descriptors which extend the current
4-byte descriptors used for other Ingenic SoCs.

Also, add special handling for HDMI-A connectors.

For some reason, only the primary planes are working
properly. As soon as the overlay plane is enabled
things go south :P

Tested on MIPS Creator CI20 board.

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/ingenic-drm-drv.c | 163 ++++++++++++++++++++--
 drivers/gpu/drm/ingenic/ingenic-drm.h     |  52 +++++++
 2 files changed, 200 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
index 5244f47634777..a2d103ae46833 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
+++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
@@ -56,13 +56,27 @@ struct ingenic_dma_hwdescs {
 	u16 palette[256] __aligned(16);
 };
 
+struct ingenic_dma_hwdesc_ext {
+	struct ingenic_dma_hwdesc base;
+	u32 offsize;
+	u32 pagewidth;
+	u32 cpos;
+	u32 dessize;
+} __packed;
+
 struct jz_soc_info {
 	bool needs_dev_clk;
 	bool has_osd;
 	bool map_noncoherent;
+	bool has_alpha;
+	bool has_pcfg;
+	bool has_recover;
+	bool has_rgbc;
+	unsigned int hwdesc_size;
 	unsigned int max_width, max_height;
 	const u32 *formats_f0, *formats_f1;
 	unsigned int num_formats_f0, num_formats_f1;
+	unsigned int max_reg;
 };
 
 struct ingenic_drm {
@@ -118,12 +132,11 @@ static bool ingenic_drm_writeable_reg(struct device *dev, unsigned int reg)
 	}
 }
 
-static const struct regmap_config ingenic_drm_regmap_config = {
+static struct regmap_config ingenic_drm_regmap_config = {
 	.reg_bits = 32,
 	.val_bits = 32,
 	.reg_stride = 4,
 
-	.max_register = JZ_REG_LCD_SIZE1,
 	.writeable_reg = ingenic_drm_writeable_reg,
 };
 
@@ -582,7 +595,39 @@ static void ingenic_drm_plane_atomic_update(struct drm_plane *plane,
 			hwdesc = &priv->dma_hwdescs->hwdesc_f1;
 
 		hwdesc->addr = addr;
-		hwdesc->cmd = JZ_LCD_CMD_EOF_IRQ | (width * height * cpp / 4);
+		hwdesc->cmd = JZ_LCD_CMD_FRM_ENABLE | JZ_LCD_CMD_EOF_IRQ |
+			      (width * height * cpp / 4);
+
+		if (priv->soc_info->hwdesc_size == sizeof(struct ingenic_dma_hwdesc_ext)) {
+			struct ingenic_dma_hwdesc_ext *hwdesc_ext;
+
+			/* Extended 8-byte descriptor */
+			hwdesc_ext = (struct ingenic_dma_hwdesc_ext *) hwdesc;
+			hwdesc_ext->cpos = 0;
+			hwdesc_ext->offsize = 0;
+			hwdesc_ext->pagewidth = 0;
+
+			switch (newstate->fb->format->format) {
+			case DRM_FORMAT_XRGB1555:
+				hwdesc_ext->cpos |= JZ_LCD_CPOS_RGB555;
+				fallthrough;
+			case DRM_FORMAT_RGB565:
+				hwdesc_ext->cpos |= JZ_LCD_CPOS_BPP_15_16;
+				break;
+			case DRM_FORMAT_XRGB8888:
+				hwdesc_ext->cpos |= JZ_LCD_CPOS_BPP_18_24;
+				break;
+			}
+			hwdesc_ext->cpos |= JZ_LCD_CPOS_PREMULTIPLY_LCD |
+					    (3 << JZ_LCD_CPOS_COEFFICIENT_OFFSET);
+
+			hwdesc_ext->dessize =
+				(0xff << JZ_LCD_DESSIZE_ALPHA_OFFSET) |
+				(((height - 1) & JZ_LCD_DESSIZE_HEIGHT_MASK) <<
+						 JZ_LCD_DESSIZE_HEIGHT_OFFSET) |
+				(((width - 1) & JZ_LCD_DESSIZE_WIDTH_MASK) <<
+						JZ_LCD_DESSIZE_WIDTH_OFFSET);
+		}
 
 		if (drm_atomic_crtc_needs_modeset(crtc_state)) {
 			fourcc = newstate->fb->format->format;
@@ -612,8 +657,12 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder,
 	struct drm_display_mode *mode = &crtc_state->adjusted_mode;
 	struct drm_connector *conn = conn_state->connector;
 	struct drm_display_info *info = &conn->display_info;
+	u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;
 	unsigned int cfg, rgbcfg = 0;
 
+	if (info->num_bus_formats)
+		bus_format = info->bus_formats[0];
+
 	priv->panel_is_sharp = info->bus_flags & DRM_BUS_FLAG_SHARP_SIGNALS;
 
 	if (priv->panel_is_sharp) {
@@ -623,6 +672,13 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder,
 		    | JZ_LCD_CFG_SPL_DISABLE | JZ_LCD_CFG_REV_DISABLE;
 	}
 
+	if (priv->soc_info->has_recover)
+		cfg |= JZ_LCD_CFG_RECOVER_FIFO_UNDERRUN;
+
+	/* CI20: set use of the 8-word descriptor and OSD foreground usage. */
+	if (priv->soc_info->hwdesc_size == sizeof(struct ingenic_dma_hwdesc_ext))
+		cfg |= JZ_LCD_CFG_DESCRIPTOR_8;
+
 	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
 		cfg |= JZ_LCD_CFG_HSYNC_ACTIVE_LOW;
 	if (mode->flags & DRM_MODE_FLAG_NVSYNC)
@@ -639,7 +695,7 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder,
 			else
 				cfg |= JZ_LCD_CFG_MODE_TV_OUT_P;
 		} else {
-			switch (*info->bus_formats) {
+			switch (bus_format) {
 			case MEDIA_BUS_FMT_RGB565_1X16:
 				cfg |= JZ_LCD_CFG_MODE_GENERIC_16BIT;
 				break;
@@ -665,19 +721,23 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder,
 	regmap_write(priv->map, JZ_REG_LCD_RGBC, rgbcfg);
 }
 
-static int ingenic_drm_encoder_atomic_check(struct drm_encoder *encoder,
-					    struct drm_crtc_state *crtc_state,
-					    struct drm_connector_state *conn_state)
+static int
+ingenic_drm_encoder_atomic_check(struct drm_encoder *encoder,
+				 struct drm_crtc_state *crtc_state,
+				 struct drm_connector_state *conn_state)
 {
 	struct drm_display_info *info = &conn_state->connector->display_info;
 	struct drm_display_mode *mode = &crtc_state->adjusted_mode;
 
+	switch (conn_state->connector->connector_type) {
+	case DRM_MODE_CONNECTOR_TV:
+	case DRM_MODE_CONNECTOR_HDMIA:
+		return 0;
+	}
+
 	if (info->num_bus_formats != 1)
 		return -EINVAL;
 
-	if (conn_state->connector->connector_type == DRM_MODE_CONNECTOR_TV)
-		return 0;
-
 	switch (*info->bus_formats) {
 	case MEDIA_BUS_FMT_RGB888_3X8:
 	case MEDIA_BUS_FMT_RGB888_3X8_DELTA:
@@ -924,7 +984,7 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
 	drm->mode_config.min_width = 0;
 	drm->mode_config.min_height = 0;
 	drm->mode_config.max_width = soc_info->max_width;
-	drm->mode_config.max_height = 4095;
+	drm->mode_config.max_height = soc_info->max_height;
 	drm->mode_config.funcs = &ingenic_drm_mode_config_funcs;
 	drm->mode_config.helper_private = &ingenic_drm_mode_config_helpers;
 
@@ -934,6 +994,7 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
 		return PTR_ERR(base);
 	}
 
+	ingenic_drm_regmap_config.max_register = soc_info->max_reg;
 	priv->map = devm_regmap_init_mmio(dev, base,
 					  &ingenic_drm_regmap_config);
 	if (IS_ERR(priv->map)) {
@@ -966,7 +1027,6 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
 	if (!priv->dma_hwdescs)
 		return -ENOMEM;
 
-
 	/* Configure DMA hwdesc for foreground0 plane */
 	dma_hwdesc_phys_f0 = priv->dma_hwdescs_phys
 		+ offsetof(struct ingenic_dma_hwdescs, hwdesc_f0);
@@ -1147,7 +1207,26 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
 
 	/* Enable OSD if available */
 	if (soc_info->has_osd)
-		regmap_write(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_OSDEN);
+		regmap_set_bits(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_OSDEN);
+
+	if (soc_info->has_alpha)
+		regmap_set_bits(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_ALPHAEN);
+
+	/* Magic values from the vendor kernel for the priority thresholds. */
+	if (soc_info->has_pcfg)
+		regmap_write(priv->map, JZ_REG_LCD_PCFG,
+			     JZ_LCD_PCFG_PRI_MODE |
+			     JZ_LCD_PCFG_HP_BST_16 |
+			     (511 << JZ_LCD_PCFG_THRESHOLD2_OFFSET) |
+			     (400 << JZ_LCD_PCFG_THRESHOLD1_OFFSET) |
+			     (256 << JZ_LCD_PCFG_THRESHOLD0_OFFSET));
+
+	/* RGB output control may be superfluous. */
+	if (soc_info->has_rgbc)
+		regmap_write(priv->map, JZ_REG_LCD_RGBC,
+			     JZ_LCD_RGBC_RGB_FORMAT_ENABLE |
+			     JZ_LCD_RGBC_ODD_LINE_RGB |
+			     JZ_LCD_RGBC_EVEN_LINE_RGB);
 
 	mutex_init(&priv->clk_mutex);
 	priv->clock_nb.notifier_call = ingenic_drm_update_pixclk;
@@ -1296,41 +1375,75 @@ static const struct jz_soc_info jz4740_soc_info = {
 	.needs_dev_clk = true,
 	.has_osd = false,
 	.map_noncoherent = false,
+	.has_pcfg = false,
+	.has_recover = false,
+	.has_rgbc = false,
+	.hwdesc_size = sizeof(struct ingenic_dma_hwdesc),
 	.max_width = 800,
 	.max_height = 600,
 	.formats_f1 = jz4740_formats,
 	.num_formats_f1 = ARRAY_SIZE(jz4740_formats),
 	/* JZ4740 has only one plane */
+	.max_reg = JZ_REG_LCD_SIZE1,
 };
 
 static const struct jz_soc_info jz4725b_soc_info = {
 	.needs_dev_clk = false,
 	.has_osd = true,
 	.map_noncoherent = false,
+	.has_pcfg = false,
+	.has_recover = false,
+	.has_rgbc = false,
+	.hwdesc_size = sizeof(struct ingenic_dma_hwdesc),
 	.max_width = 800,
 	.max_height = 600,
 	.formats_f1 = jz4725b_formats_f1,
 	.num_formats_f1 = ARRAY_SIZE(jz4725b_formats_f1),
 	.formats_f0 = jz4725b_formats_f0,
 	.num_formats_f0 = ARRAY_SIZE(jz4725b_formats_f0),
+	.max_reg = JZ_REG_LCD_SIZE1,
 };
 
 static const struct jz_soc_info jz4770_soc_info = {
 	.needs_dev_clk = false,
 	.has_osd = true,
 	.map_noncoherent = true,
+	.has_pcfg = false,
+	.has_recover = false,
+	.has_rgbc = false,
+	.hwdesc_size = sizeof(struct ingenic_dma_hwdesc),
 	.max_width = 1280,
 	.max_height = 720,
 	.formats_f1 = jz4770_formats_f1,
 	.num_formats_f1 = ARRAY_SIZE(jz4770_formats_f1),
 	.formats_f0 = jz4770_formats_f0,
 	.num_formats_f0 = ARRAY_SIZE(jz4770_formats_f0),
+	.max_reg = JZ_REG_LCD_SIZE1,
+};
+
+static const struct jz_soc_info jz4780_soc_info = {
+	.needs_dev_clk = true,
+	.has_osd = true,
+	.has_alpha = true,
+	.has_pcfg = true,
+	.has_recover = true,
+	.has_rgbc = true,
+	.hwdesc_size = sizeof(struct ingenic_dma_hwdesc_ext),
+	.max_width = 4096,
+	.max_height = 4096,
+	/* REVISIT: do we support formats different from jz4770? */
+	.formats_f1 = jz4770_formats_f1,
+	.num_formats_f1 = ARRAY_SIZE(jz4770_formats_f1),
+	.formats_f0 = jz4770_formats_f0,
+	.num_formats_f0 = ARRAY_SIZE(jz4770_formats_f0),
+	.max_reg = JZ_REG_LCD_PCFG,
 };
 
 static const struct of_device_id ingenic_drm_of_match[] = {
 	{ .compatible = "ingenic,jz4740-lcd", .data = &jz4740_soc_info },
 	{ .compatible = "ingenic,jz4725b-lcd", .data = &jz4725b_soc_info },
 	{ .compatible = "ingenic,jz4770-lcd", .data = &jz4770_soc_info },
+	{ .compatible = "ingenic,jz4780-lcd", .data = &jz4780_soc_info },
 	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, ingenic_drm_of_match);
@@ -1349,13 +1462,31 @@ static int ingenic_drm_init(void)
 {
 	int err;
 
+	if (IS_ENABLED(CONFIG_DRM_INGENIC_DW_HDMI)) {
+		err = platform_driver_register(ingenic_dw_hdmi_driver_ptr);
+		if (err)
+			return err;
+	}
+
 	if (IS_ENABLED(CONFIG_DRM_INGENIC_IPU)) {
 		err = platform_driver_register(ingenic_ipu_driver_ptr);
 		if (err)
-			return err;
+			goto err_hdmi_unreg;
 	}
 
-	return platform_driver_register(&ingenic_drm_driver);
+	err = platform_driver_register(&ingenic_drm_driver);
+	if (err)
+		goto err_ipu_unreg;
+
+	return 0;
+
+err_ipu_unreg:
+	if (IS_ENABLED(CONFIG_DRM_INGENIC_IPU))
+		platform_driver_unregister(ingenic_ipu_driver_ptr);
+err_hdmi_unreg:
+	if (IS_ENABLED(CONFIG_DRM_INGENIC_DW_HDMI))
+		platform_driver_unregister(ingenic_dw_hdmi_driver_ptr);
+	return err;
 }
 module_init(ingenic_drm_init);
 
@@ -1363,6 +1494,8 @@ static void ingenic_drm_exit(void)
 {
 	platform_driver_unregister(&ingenic_drm_driver);
 
+	if (IS_ENABLED(CONFIG_DRM_INGENIC_DW_HDMI))
+		platform_driver_unregister(ingenic_dw_hdmi_driver_ptr);
 	if (IS_ENABLED(CONFIG_DRM_INGENIC_IPU))
 		platform_driver_unregister(ingenic_ipu_driver_ptr);
 }
diff --git a/drivers/gpu/drm/ingenic/ingenic-drm.h b/drivers/gpu/drm/ingenic/ingenic-drm.h
index 22654ac1dde1c..7e55a88243e28 100644
--- a/drivers/gpu/drm/ingenic/ingenic-drm.h
+++ b/drivers/gpu/drm/ingenic/ingenic-drm.h
@@ -44,8 +44,11 @@
 #define JZ_REG_LCD_XYP1				0x124
 #define JZ_REG_LCD_SIZE0			0x128
 #define JZ_REG_LCD_SIZE1			0x12c
+#define JZ_REG_LCD_PCFG				0x2c0
 
 #define JZ_LCD_CFG_SLCD				BIT(31)
+#define JZ_LCD_CFG_DESCRIPTOR_8			BIT(28)
+#define JZ_LCD_CFG_RECOVER_FIFO_UNDERRUN	BIT(25)
 #define JZ_LCD_CFG_PS_DISABLE			BIT(23)
 #define JZ_LCD_CFG_CLS_DISABLE			BIT(22)
 #define JZ_LCD_CFG_SPL_DISABLE			BIT(21)
@@ -63,6 +66,7 @@
 #define JZ_LCD_CFG_DE_ACTIVE_LOW		BIT(9)
 #define JZ_LCD_CFG_VSYNC_ACTIVE_LOW		BIT(8)
 #define JZ_LCD_CFG_18_BIT			BIT(7)
+#define JZ_LCD_CFG_24_BIT			BIT(6)
 #define JZ_LCD_CFG_PDW				(BIT(5) | BIT(4))
 
 #define JZ_LCD_CFG_MODE_GENERIC_16BIT		0
@@ -132,6 +136,7 @@
 #define JZ_LCD_CMD_SOF_IRQ			BIT(31)
 #define JZ_LCD_CMD_EOF_IRQ			BIT(30)
 #define JZ_LCD_CMD_ENABLE_PAL			BIT(28)
+#define JZ_LCD_CMD_FRM_ENABLE			BIT(26)
 
 #define JZ_LCD_SYNC_MASK			0x3ff
 
@@ -153,6 +158,7 @@
 #define JZ_LCD_RGBC_EVEN_BGR			(0x5 << 0)
 
 #define JZ_LCD_OSDC_OSDEN			BIT(0)
+#define JZ_LCD_OSDC_ALPHAEN			BIT(2)
 #define JZ_LCD_OSDC_F0EN			BIT(3)
 #define JZ_LCD_OSDC_F1EN			BIT(4)
 
@@ -176,6 +182,51 @@
 #define JZ_LCD_SIZE01_WIDTH_LSB			0
 #define JZ_LCD_SIZE01_HEIGHT_LSB		16
 
+#define JZ_LCD_DESSIZE_ALPHA_OFFSET		24
+#define JZ_LCD_DESSIZE_HEIGHT_OFFSET		12
+#define JZ_LCD_DESSIZE_WIDTH_OFFSET		0
+#define JZ_LCD_DESSIZE_HEIGHT_MASK		0xfff
+#define JZ_LCD_DESSIZE_WIDTH_MASK		0xfff
+
+/* TODO: 4,5 and 7 match the above BPP */
+#define JZ_LCD_CPOS_BPP_15_16			(4 << 27)
+#define JZ_LCD_CPOS_BPP_18_24			(5 << 27)
+#define JZ_LCD_CPOS_BPP_30			(7 << 27)
+#define JZ_LCD_CPOS_RGB555			BIT(30)
+#define JZ_LCD_CPOS_PREMULTIPLY_LCD		BIT(26)
+#define JZ_LCD_CPOS_COEFFICIENT_OFFSET		24
+
+#define JZ_LCD_RGBC_RGB_PADDING			BIT(15)
+#define JZ_LCD_RGBC_RGB_PADDING_FIRST		BIT(14)
+#define JZ_LCD_RGBC_422				BIT(8)
+#define JZ_LCD_RGBC_RGB_FORMAT_ENABLE		BIT(7)
+#define JZ_LCD_RGBC_ODD_LINE_MASK		(0x7 << 4)
+#define JZ_LCD_RGBC_ODD_LINE_RGB		(0 << 4)
+#define JZ_LCD_RGBC_ODD_LINE_RBG		(1 << 4)
+#define JZ_LCD_RGBC_ODD_LINE_GRB		(2 << 4)
+#define JZ_LCD_RGBC_ODD_LINE_GBR		(3 << 4)
+#define JZ_LCD_RGBC_ODD_LINE_BRG		(4 << 4)
+#define JZ_LCD_RGBC_ODD_LINE_BGR		(5 << 4)
+#define JZ_LCD_RGBC_EVEN_LINE_MASK		(0x7 << 0)
+#define JZ_LCD_RGBC_EVEN_LINE_RGB		0
+#define JZ_LCD_RGBC_EVEN_LINE_RBG		1
+#define JZ_LCD_RGBC_EVEN_LINE_GRB		2
+#define JZ_LCD_RGBC_EVEN_LINE_GBR		3
+#define JZ_LCD_RGBC_EVEN_LINE_BRG		4
+#define JZ_LCD_RGBC_EVEN_LINE_BGR		5
+
+#define JZ_LCD_PCFG_PRI_MODE			BIT(31)
+#define JZ_LCD_PCFG_HP_BST_4			(0 << 28)
+#define JZ_LCD_PCFG_HP_BST_8			(1 << 28)
+#define JZ_LCD_PCFG_HP_BST_16			(2 << 28)
+#define JZ_LCD_PCFG_HP_BST_32			(3 << 28)
+#define JZ_LCD_PCFG_HP_BST_64			(4 << 28)
+#define JZ_LCD_PCFG_HP_BST_16_CONT		(5 << 28)
+#define JZ_LCD_PCFG_HP_BST_DISABLE		(7 << 28)
+#define JZ_LCD_PCFG_THRESHOLD2_OFFSET		18
+#define JZ_LCD_PCFG_THRESHOLD1_OFFSET		9
+#define JZ_LCD_PCFG_THRESHOLD0_OFFSET		0
+
 struct device;
 struct drm_plane;
 struct drm_plane_state;
@@ -187,5 +238,6 @@ void ingenic_drm_plane_disable(struct device *dev, struct drm_plane *plane);
 bool ingenic_drm_map_noncoherent(const struct device *dev);
 
 extern struct platform_driver *ingenic_ipu_driver_ptr;
+extern struct platform_driver *ingenic_dw_hdmi_driver_ptr;
 
 #endif /* DRIVERS_GPU_DRM_INGENIC_INGENIC_DRM_H */
-- 
2.31.1

Re: [PATCH v2 3/8] drm/ingenic: Add support for JZ4780 and HDMI output

[Paul Cercueil]

Hi Nikolaus & Paul,

Le jeu., août 5 2021 at 16:07:52 +0200, H. Nikolaus Schaller 
<hns@goldelico.com> a écrit :
> From: Paul Boddie <paul@boddie.org.uk>
> 
> Add support for the LCD controller present on JZ4780 SoCs.
> This SoC uses 8-byte descriptors which extend the current
> 4-byte descriptors used for other Ingenic SoCs.
> 
> Also, add special handling for HDMI-A connectors.
> 
> For some reason, only the primary planes are working
> properly. As soon as the overlay plane is enabled
> things go south :P
> 
> Tested on MIPS Creator CI20 board.
> 
> 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/ingenic-drm-drv.c | 163 
> ++++++++++++++++++++--
>  drivers/gpu/drm/ingenic/ingenic-drm.h     |  52 +++++++
>  2 files changed, 200 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c 
> b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> index 5244f47634777..a2d103ae46833 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm-drv.c
> @@ -56,13 +56,27 @@ struct ingenic_dma_hwdescs {
>  	u16 palette[256] __aligned(16);
>  };
> 
> +struct ingenic_dma_hwdesc_ext {
> +	struct ingenic_dma_hwdesc base;
> +	u32 offsize;
> +	u32 pagewidth;
> +	u32 cpos;
> +	u32 dessize;
> +} __packed;
> +
>  struct jz_soc_info {
>  	bool needs_dev_clk;
>  	bool has_osd;
>  	bool map_noncoherent;
> +	bool has_alpha;
> +	bool has_pcfg;
> +	bool has_recover;
> +	bool has_rgbc;
> +	unsigned int hwdesc_size;
>  	unsigned int max_width, max_height;
>  	const u32 *formats_f0, *formats_f1;
>  	unsigned int num_formats_f0, num_formats_f1;
> +	unsigned int max_reg;
>  };
> 
>  struct ingenic_drm {
> @@ -118,12 +132,11 @@ static bool ingenic_drm_writeable_reg(struct 
> device *dev, unsigned int reg)
>  	}
>  }
> 
> -static const struct regmap_config ingenic_drm_regmap_config = {
> +static struct regmap_config ingenic_drm_regmap_config = {
>  	.reg_bits = 32,
>  	.val_bits = 32,
>  	.reg_stride = 4,
> 
> -	.max_register = JZ_REG_LCD_SIZE1,
>  	.writeable_reg = ingenic_drm_writeable_reg,
>  };
> 
> @@ -582,7 +595,39 @@ static void 
> ingenic_drm_plane_atomic_update(struct drm_plane *plane,
>  			hwdesc = &priv->dma_hwdescs->hwdesc_f1;
> 
>  		hwdesc->addr = addr;
> -		hwdesc->cmd = JZ_LCD_CMD_EOF_IRQ | (width * height * cpp / 4);
> +		hwdesc->cmd = JZ_LCD_CMD_FRM_ENABLE | JZ_LCD_CMD_EOF_IRQ |
> +			      (width * height * cpp / 4);
> +
> +		if (priv->soc_info->hwdesc_size == sizeof(struct 
> ingenic_dma_hwdesc_ext)) {

I'd prefer a boolean flag, e.g. "soc_info->use_extended_hwdesc"

> +			struct ingenic_dma_hwdesc_ext *hwdesc_ext;
> +
> +			/* Extended 8-byte descriptor */
> +			hwdesc_ext = (struct ingenic_dma_hwdesc_ext *) hwdesc;
> +			hwdesc_ext->cpos = 0;
> +			hwdesc_ext->offsize = 0;
> +			hwdesc_ext->pagewidth = 0;
> +
> +			switch (newstate->fb->format->format) {
> +			case DRM_FORMAT_XRGB1555:
> +				hwdesc_ext->cpos |= JZ_LCD_CPOS_RGB555;
> +				fallthrough;
> +			case DRM_FORMAT_RGB565:
> +				hwdesc_ext->cpos |= JZ_LCD_CPOS_BPP_15_16;
> +				break;
> +			case DRM_FORMAT_XRGB8888:
> +				hwdesc_ext->cpos |= JZ_LCD_CPOS_BPP_18_24;
> +				break;
> +			}
> +			hwdesc_ext->cpos |= JZ_LCD_CPOS_PREMULTIPLY_LCD |
> +					    (3 << JZ_LCD_CPOS_COEFFICIENT_OFFSET);

Where's that magic value '3' coming from?

> +
> +			hwdesc_ext->dessize =
> +				(0xff << JZ_LCD_DESSIZE_ALPHA_OFFSET) |
> +				(((height - 1) & JZ_LCD_DESSIZE_HEIGHT_MASK) <<
> +						 JZ_LCD_DESSIZE_HEIGHT_OFFSET) |
> +				(((width - 1) & JZ_LCD_DESSIZE_WIDTH_MASK) <<
> +						JZ_LCD_DESSIZE_WIDTH_OFFSET);

Use FIELD_PREP() from <linux/bitfield.h>.

> +		}
> 
>  		if (drm_atomic_crtc_needs_modeset(crtc_state)) {
>  			fourcc = newstate->fb->format->format;
> @@ -612,8 +657,12 @@ static void 
> ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder,
>  	struct drm_display_mode *mode = &crtc_state->adjusted_mode;
>  	struct drm_connector *conn = conn_state->connector;
>  	struct drm_display_info *info = &conn->display_info;
> +	u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;
>  	unsigned int cfg, rgbcfg = 0;
> 
> +	if (info->num_bus_formats)
> +		bus_format = info->bus_formats[0];
> +

That code is going to change really soon, as I have my own PR ready to 
convert the ingenic-drm driver to use a top-level bridge for bus format 
/ flags negociation.

The HDMI driver should therefore implement it as well; see for instance 
drivers/gpu/drm/bridge/ite-it66121.c for an example of how the bus 
format is negociated.

I'll be sure to Cc you as soon as I send it upstream - should be just 
in a couple of days.

>  	priv->panel_is_sharp = info->bus_flags & DRM_BUS_FLAG_SHARP_SIGNALS;
> 
>  	if (priv->panel_is_sharp) {
> @@ -623,6 +672,13 @@ static void 
> ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder,
>  		    | JZ_LCD_CFG_SPL_DISABLE | JZ_LCD_CFG_REV_DISABLE;
>  	}
> 
> +	if (priv->soc_info->has_recover)
> +		cfg |= JZ_LCD_CFG_RECOVER_FIFO_UNDERRUN;

Seems out of place. Does it not work without?

> +
> +	/* CI20: set use of the 8-word descriptor and OSD foreground usage. 
> */

Not really CI20-specific though.

> +	if (priv->soc_info->hwdesc_size == sizeof(struct 
> ingenic_dma_hwdesc_ext))
> +		cfg |= JZ_LCD_CFG_DESCRIPTOR_8;
> +
>  	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
>  		cfg |= JZ_LCD_CFG_HSYNC_ACTIVE_LOW;
>  	if (mode->flags & DRM_MODE_FLAG_NVSYNC)
> @@ -639,7 +695,7 @@ static void 
> ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder,
>  			else
>  				cfg |= JZ_LCD_CFG_MODE_TV_OUT_P;
>  		} else {
> -			switch (*info->bus_formats) {
> +			switch (bus_format) {
>  			case MEDIA_BUS_FMT_RGB565_1X16:
>  				cfg |= JZ_LCD_CFG_MODE_GENERIC_16BIT;
>  				break;
> @@ -665,19 +721,23 @@ static void 
> ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder,
>  	regmap_write(priv->map, JZ_REG_LCD_RGBC, rgbcfg);
>  }
> 
> -static int ingenic_drm_encoder_atomic_check(struct drm_encoder 
> *encoder,
> -					    struct drm_crtc_state *crtc_state,
> -					    struct drm_connector_state *conn_state)
> +static int
> +ingenic_drm_encoder_atomic_check(struct drm_encoder *encoder,
> +				 struct drm_crtc_state *crtc_state,
> +				 struct drm_connector_state *conn_state)
>  {
>  	struct drm_display_info *info = 
> &conn_state->connector->display_info;
>  	struct drm_display_mode *mode = &crtc_state->adjusted_mode;
> 
> +	switch (conn_state->connector->connector_type) {
> +	case DRM_MODE_CONNECTOR_TV:
> +	case DRM_MODE_CONNECTOR_HDMIA:
> +		return 0;
> +	}

This switch should move after the check on "num_bus_formats".
(I understand why you did it, but with proper bus format negociation 
this won't be needed).

> +
>  	if (info->num_bus_formats != 1)
>  		return -EINVAL;
> 
> -	if (conn_state->connector->connector_type == DRM_MODE_CONNECTOR_TV)
> -		return 0;
> -
>  	switch (*info->bus_formats) {
>  	case MEDIA_BUS_FMT_RGB888_3X8:
>  	case MEDIA_BUS_FMT_RGB888_3X8_DELTA:
> @@ -924,7 +984,7 @@ static int ingenic_drm_bind(struct device *dev, 
> bool has_components)
>  	drm->mode_config.min_width = 0;
>  	drm->mode_config.min_height = 0;
>  	drm->mode_config.max_width = soc_info->max_width;
> -	drm->mode_config.max_height = 4095;
> +	drm->mode_config.max_height = soc_info->max_height;

The drm->mode_config.max_height is different from soc_info->max_height; 
the former is the maximum framebuffer size, the latter is the maximum 
size that the SoC can display. The framebuffer can be bigger than what 
the SoC can display.

>  	drm->mode_config.funcs = &ingenic_drm_mode_config_funcs;
>  	drm->mode_config.helper_private = &ingenic_drm_mode_config_helpers;
> 
> @@ -934,6 +994,7 @@ static int ingenic_drm_bind(struct device *dev, 
> bool has_components)
>  		return PTR_ERR(base);
>  	}
> 
> +	ingenic_drm_regmap_config.max_register = soc_info->max_reg;

Avoid modifying a global variable; instead copy it to a local copy of 
ingenic_drm_regmap_config, and use this one in the regmap_init_mmio 
below.

>  	priv->map = devm_regmap_init_mmio(dev, base,
>  					  &ingenic_drm_regmap_config);
>  	if (IS_ERR(priv->map)) {
> @@ -966,7 +1027,6 @@ static int ingenic_drm_bind(struct device *dev, 
> bool has_components)
>  	if (!priv->dma_hwdescs)
>  		return -ENOMEM;
> 
> -

Cosmetic change - not needed.

>  	/* Configure DMA hwdesc for foreground0 plane */
>  	dma_hwdesc_phys_f0 = priv->dma_hwdescs_phys
>  		+ offsetof(struct ingenic_dma_hwdescs, hwdesc_f0);
> @@ -1147,7 +1207,26 @@ static int ingenic_drm_bind(struct device 
> *dev, bool has_components)
> 
>  	/* Enable OSD if available */
>  	if (soc_info->has_osd)
> -		regmap_write(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_OSDEN);
> +		regmap_set_bits(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_OSDEN);
> +
> +	if (soc_info->has_alpha)
> +		regmap_set_bits(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_ALPHAEN);

Do you need alpha blending support between planes, in this patch 
related to HDMI?

> +
> +	/* Magic values from the vendor kernel for the priority thresholds. 
> */
> +	if (soc_info->has_pcfg)
> +		regmap_write(priv->map, JZ_REG_LCD_PCFG,
> +			     JZ_LCD_PCFG_PRI_MODE |
> +			     JZ_LCD_PCFG_HP_BST_16 |
> +			     (511 << JZ_LCD_PCFG_THRESHOLD2_OFFSET) |
> +			     (400 << JZ_LCD_PCFG_THRESHOLD1_OFFSET) |
> +			     (256 << JZ_LCD_PCFG_THRESHOLD0_OFFSET));

And why is that needed in this patch? Doesn't HDMI work without it?

> +
> +	/* RGB output control may be superfluous. */
> +	if (soc_info->has_rgbc)
> +		regmap_write(priv->map, JZ_REG_LCD_RGBC,
> +			     JZ_LCD_RGBC_RGB_FORMAT_ENABLE |
> +			     JZ_LCD_RGBC_ODD_LINE_RGB |
> +			     JZ_LCD_RGBC_EVEN_LINE_RGB);

The last two macros set the subpixel ordering on the bus for serial 
(3x8) 24-bit panels - completely unrelated to HDMI.

> 
>  	mutex_init(&priv->clk_mutex);
>  	priv->clock_nb.notifier_call = ingenic_drm_update_pixclk;
> @@ -1296,41 +1375,75 @@ static const struct jz_soc_info 
> jz4740_soc_info = {
>  	.needs_dev_clk = true,
>  	.has_osd = false,
>  	.map_noncoherent = false,
> +	.has_pcfg = false,
> +	.has_recover = false,
> +	.has_rgbc = false,
> +	.hwdesc_size = sizeof(struct ingenic_dma_hwdesc),
>  	.max_width = 800,
>  	.max_height = 600,
>  	.formats_f1 = jz4740_formats,
>  	.num_formats_f1 = ARRAY_SIZE(jz4740_formats),
>  	/* JZ4740 has only one plane */
> +	.max_reg = JZ_REG_LCD_SIZE1,
>  };
> 
>  static const struct jz_soc_info jz4725b_soc_info = {
>  	.needs_dev_clk = false,
>  	.has_osd = true,
>  	.map_noncoherent = false,
> +	.has_pcfg = false,
> +	.has_recover = false,
> +	.has_rgbc = false,
> +	.hwdesc_size = sizeof(struct ingenic_dma_hwdesc),
>  	.max_width = 800,
>  	.max_height = 600,
>  	.formats_f1 = jz4725b_formats_f1,
>  	.num_formats_f1 = ARRAY_SIZE(jz4725b_formats_f1),
>  	.formats_f0 = jz4725b_formats_f0,
>  	.num_formats_f0 = ARRAY_SIZE(jz4725b_formats_f0),
> +	.max_reg = JZ_REG_LCD_SIZE1,
>  };
> 
>  static const struct jz_soc_info jz4770_soc_info = {
>  	.needs_dev_clk = false,
>  	.has_osd = true,
>  	.map_noncoherent = true,
> +	.has_pcfg = false,
> +	.has_recover = false,
> +	.has_rgbc = false,
> +	.hwdesc_size = sizeof(struct ingenic_dma_hwdesc),
>  	.max_width = 1280,
>  	.max_height = 720,
>  	.formats_f1 = jz4770_formats_f1,
>  	.num_formats_f1 = ARRAY_SIZE(jz4770_formats_f1),
>  	.formats_f0 = jz4770_formats_f0,
>  	.num_formats_f0 = ARRAY_SIZE(jz4770_formats_f0),
> +	.max_reg = JZ_REG_LCD_SIZE1,
> +};
> +
> +static const struct jz_soc_info jz4780_soc_info = {
> +	.needs_dev_clk = true,
> +	.has_osd = true,
> +	.has_alpha = true,
> +	.has_pcfg = true,
> +	.has_recover = true,
> +	.has_rgbc = true,
> +	.hwdesc_size = sizeof(struct ingenic_dma_hwdesc_ext),
> +	.max_width = 4096,
> +	.max_height = 4096,

The PM says that the display is up to 4096x2048-30Hz; so this is wrong.

> +	/* REVISIT: do we support formats different from jz4770? */
> +	.formats_f1 = jz4770_formats_f1,
> +	.num_formats_f1 = ARRAY_SIZE(jz4770_formats_f1),
> +	.formats_f0 = jz4770_formats_f0,
> +	.num_formats_f0 = ARRAY_SIZE(jz4770_formats_f0),
> +	.max_reg = JZ_REG_LCD_PCFG,
>  };
> 
>  static const struct of_device_id ingenic_drm_of_match[] = {
>  	{ .compatible = "ingenic,jz4740-lcd", .data = &jz4740_soc_info },
>  	{ .compatible = "ingenic,jz4725b-lcd", .data = &jz4725b_soc_info },
>  	{ .compatible = "ingenic,jz4770-lcd", .data = &jz4770_soc_info },
> +	{ .compatible = "ingenic,jz4780-lcd", .data = &jz4780_soc_info },
>  	{ /* sentinel */ },
>  };
>  MODULE_DEVICE_TABLE(of, ingenic_drm_of_match);
> @@ -1349,13 +1462,31 @@ static int ingenic_drm_init(void)
>  {
>  	int err;
> 
> +	if (IS_ENABLED(CONFIG_DRM_INGENIC_DW_HDMI)) {
> +		err = platform_driver_register(ingenic_dw_hdmi_driver_ptr);
> +		if (err)
> +			return err;
> +	}
> +
>  	if (IS_ENABLED(CONFIG_DRM_INGENIC_IPU)) {
>  		err = platform_driver_register(ingenic_ipu_driver_ptr);
>  		if (err)
> -			return err;
> +			goto err_hdmi_unreg;
>  	}
> 
> -	return platform_driver_register(&ingenic_drm_driver);
> +	err = platform_driver_register(&ingenic_drm_driver);
> +	if (err)
> +		goto err_ipu_unreg;

That's actually a change of behaviour - before it would return on error 
without calling platform_driver_unregister on the IPU.

It is not necesarily bad, but it belongs in a separate commit.

> +
> +	return 0;
> +
> +err_ipu_unreg:
> +	if (IS_ENABLED(CONFIG_DRM_INGENIC_IPU))
> +		platform_driver_unregister(ingenic_ipu_driver_ptr);
> +err_hdmi_unreg:
> +	if (IS_ENABLED(CONFIG_DRM_INGENIC_DW_HDMI))
> +		platform_driver_unregister(ingenic_dw_hdmi_driver_ptr);
> +	return err;
>  }
>  module_init(ingenic_drm_init);
> 
> @@ -1363,6 +1494,8 @@ static void ingenic_drm_exit(void)
>  {
>  	platform_driver_unregister(&ingenic_drm_driver);
> 
> +	if (IS_ENABLED(CONFIG_DRM_INGENIC_DW_HDMI))
> +		platform_driver_unregister(ingenic_dw_hdmi_driver_ptr);
>  	if (IS_ENABLED(CONFIG_DRM_INGENIC_IPU))
>  		platform_driver_unregister(ingenic_ipu_driver_ptr);
>  }
> diff --git a/drivers/gpu/drm/ingenic/ingenic-drm.h 
> b/drivers/gpu/drm/ingenic/ingenic-drm.h
> index 22654ac1dde1c..7e55a88243e28 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-drm.h
> +++ b/drivers/gpu/drm/ingenic/ingenic-drm.h
> @@ -44,8 +44,11 @@
>  #define JZ_REG_LCD_XYP1				0x124
>  #define JZ_REG_LCD_SIZE0			0x128
>  #define JZ_REG_LCD_SIZE1			0x12c
> +#define JZ_REG_LCD_PCFG				0x2c0
> 
>  #define JZ_LCD_CFG_SLCD				BIT(31)
> +#define JZ_LCD_CFG_DESCRIPTOR_8			BIT(28)
> +#define JZ_LCD_CFG_RECOVER_FIFO_UNDERRUN	BIT(25)
>  #define JZ_LCD_CFG_PS_DISABLE			BIT(23)
>  #define JZ_LCD_CFG_CLS_DISABLE			BIT(22)
>  #define JZ_LCD_CFG_SPL_DISABLE			BIT(21)
> @@ -63,6 +66,7 @@
>  #define JZ_LCD_CFG_DE_ACTIVE_LOW		BIT(9)
>  #define JZ_LCD_CFG_VSYNC_ACTIVE_LOW		BIT(8)
>  #define JZ_LCD_CFG_18_BIT			BIT(7)
> +#define JZ_LCD_CFG_24_BIT			BIT(6)
>  #define JZ_LCD_CFG_PDW				(BIT(5) | BIT(4))
> 
>  #define JZ_LCD_CFG_MODE_GENERIC_16BIT		0
> @@ -132,6 +136,7 @@
>  #define JZ_LCD_CMD_SOF_IRQ			BIT(31)
>  #define JZ_LCD_CMD_EOF_IRQ			BIT(30)
>  #define JZ_LCD_CMD_ENABLE_PAL			BIT(28)
> +#define JZ_LCD_CMD_FRM_ENABLE			BIT(26)
> 
>  #define JZ_LCD_SYNC_MASK			0x3ff
> 
> @@ -153,6 +158,7 @@
>  #define JZ_LCD_RGBC_EVEN_BGR			(0x5 << 0)
> 
>  #define JZ_LCD_OSDC_OSDEN			BIT(0)
> +#define JZ_LCD_OSDC_ALPHAEN			BIT(2)
>  #define JZ_LCD_OSDC_F0EN			BIT(3)
>  #define JZ_LCD_OSDC_F1EN			BIT(4)
> 
> @@ -176,6 +182,51 @@
>  #define JZ_LCD_SIZE01_WIDTH_LSB			0
>  #define JZ_LCD_SIZE01_HEIGHT_LSB		16
> 
> +#define JZ_LCD_DESSIZE_ALPHA_OFFSET		24
> +#define JZ_LCD_DESSIZE_HEIGHT_OFFSET		12
> +#define JZ_LCD_DESSIZE_WIDTH_OFFSET		0
> +#define JZ_LCD_DESSIZE_HEIGHT_MASK		0xfff
> +#define JZ_LCD_DESSIZE_WIDTH_MASK		0xfff
> +
> +/* TODO: 4,5 and 7 match the above BPP */
> +#define JZ_LCD_CPOS_BPP_15_16			(4 << 27)
> +#define JZ_LCD_CPOS_BPP_18_24			(5 << 27)
> +#define JZ_LCD_CPOS_BPP_30			(7 << 27)
> +#define JZ_LCD_CPOS_RGB555			BIT(30)
> +#define JZ_LCD_CPOS_PREMULTIPLY_LCD		BIT(26)
> +#define JZ_LCD_CPOS_COEFFICIENT_OFFSET		24
> +
> +#define JZ_LCD_RGBC_RGB_PADDING			BIT(15)
> +#define JZ_LCD_RGBC_RGB_PADDING_FIRST		BIT(14)
> +#define JZ_LCD_RGBC_422				BIT(8)
> +#define JZ_LCD_RGBC_RGB_FORMAT_ENABLE		BIT(7)
> +#define JZ_LCD_RGBC_ODD_LINE_MASK		(0x7 << 4)
> +#define JZ_LCD_RGBC_ODD_LINE_RGB		(0 << 4)
> +#define JZ_LCD_RGBC_ODD_LINE_RBG		(1 << 4)
> +#define JZ_LCD_RGBC_ODD_LINE_GRB		(2 << 4)
> +#define JZ_LCD_RGBC_ODD_LINE_GBR		(3 << 4)
> +#define JZ_LCD_RGBC_ODD_LINE_BRG		(4 << 4)
> +#define JZ_LCD_RGBC_ODD_LINE_BGR		(5 << 4)
> +#define JZ_LCD_RGBC_EVEN_LINE_MASK		(0x7 << 0)
> +#define JZ_LCD_RGBC_EVEN_LINE_RGB		0
> +#define JZ_LCD_RGBC_EVEN_LINE_RBG		1
> +#define JZ_LCD_RGBC_EVEN_LINE_GRB		2
> +#define JZ_LCD_RGBC_EVEN_LINE_GBR		3
> +#define JZ_LCD_RGBC_EVEN_LINE_BRG		4
> +#define JZ_LCD_RGBC_EVEN_LINE_BGR		5

We already have these in ingenic-drm.h...

Please only add the macros that you need and are missing.

Cheers,
-Paul

> +
> +#define JZ_LCD_PCFG_PRI_MODE			BIT(31)
> +#define JZ_LCD_PCFG_HP_BST_4			(0 << 28)
> +#define JZ_LCD_PCFG_HP_BST_8			(1 << 28)
> +#define JZ_LCD_PCFG_HP_BST_16			(2 << 28)
> +#define JZ_LCD_PCFG_HP_BST_32			(3 << 28)
> +#define JZ_LCD_PCFG_HP_BST_64			(4 << 28)
> +#define JZ_LCD_PCFG_HP_BST_16_CONT		(5 << 28)
> +#define JZ_LCD_PCFG_HP_BST_DISABLE		(7 << 28)
> +#define JZ_LCD_PCFG_THRESHOLD2_OFFSET		18
> +#define JZ_LCD_PCFG_THRESHOLD1_OFFSET		9
> +#define JZ_LCD_PCFG_THRESHOLD0_OFFSET		0
> +
>  struct device;
>  struct drm_plane;
>  struct drm_plane_state;
> @@ -187,5 +238,6 @@ void ingenic_drm_plane_disable(struct device 
> *dev, struct drm_plane *plane);
>  bool ingenic_drm_map_noncoherent(const struct device *dev);
> 
>  extern struct platform_driver *ingenic_ipu_driver_ptr;
> +extern struct platform_driver *ingenic_dw_hdmi_driver_ptr;
> 
>  #endif /* DRIVERS_GPU_DRM_INGENIC_INGENIC_DRM_H */
> --
> 2.31.1
> 

Re: [PATCH v2 3/8] drm/ingenic: Add support for JZ4780 and HDMI output

[H. Nikolaus Schaller]

Hi Paul,

> Am 05.08.2021 um 17:22 schrieb Paul Cercueil <paul@crapouillou.net>:
> 
> Hi Nikolaus & Paul,
> 
> Le jeu., août 5 2021 at 16:07:52 +0200, H. Nikolaus Schaller <hns@goldelico.com> a écrit :
>> From: Paul Boddie <paul@boddie.org.uk>
>> Add support for the LCD controller present on JZ4780 SoCs.
>> This SoC uses 8-byte descriptors which extend the current
>> 4-byte descriptors used for other Ingenic SoCs.
>> Also, add special handling for HDMI-A connectors.
>> For some reason, only the primary planes are working
>> properly. As soon as the overlay plane is enabled
>> things go south :P
>> Tested on MIPS Creator CI20 board.
>> 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/ingenic-drm-drv.c | 163 ++++++++++++++++++++--

... snip ...

> 
> We already have these in ingenic-drm.h...
> 
> Please only add the macros that you need and are missing.
> 
> Cheers,
> -Paul


all are valid comments. We'll add them for v3 or find answers
if some code fragments are needed (even if we don't know why)
or not.

BR and thanks,
Nikolaus

Re: [PATCH v2 3/8] drm/ingenic: Add support for JZ4780 and HDMI output

[H. Nikolaus Schaller]

Hi Paul,
we have v3 ready and I'll post soon.
Before, here are some feedback to your comments.

> Am 05.08.2021 um 17:22 schrieb Paul Cercueil <paul@crapouillou.net>:
> 
> Hi Nikolaus & Paul,
>> +
>> +		if (priv->soc_info->hwdesc_size == sizeof(struct ingenic_dma_hwdesc_ext)) {
> 
> I'd prefer a boolean flag, e.g. "soc_info->use_extended_hwdesc"

Done.

>> +			hwdesc_ext->cpos |= JZ_LCD_CPOS_PREMULTIPLY_LCD |
>> +					    (3 << JZ_LCD_CPOS_COEFFICIENT_OFFSET);
> 
> Where's that magic value '3' coming from?

We have defined a constant.

> 
>> +
>> +			hwdesc_ext->dessize =
>> +				(0xff << JZ_LCD_DESSIZE_ALPHA_OFFSET) |
>> +				(((height - 1) & JZ_LCD_DESSIZE_HEIGHT_MASK) <<
>> +						 JZ_LCD_DESSIZE_HEIGHT_OFFSET) |
>> +				(((width - 1) & JZ_LCD_DESSIZE_WIDTH_MASK) <<
>> +						JZ_LCD_DESSIZE_WIDTH_OFFSET);
> 
> Use FIELD_PREP() from <linux/bitfield.h>.

Changed.

> 
>> +		}
>> 		if (drm_atomic_crtc_needs_modeset(crtc_state)) {
>> 			fourcc = newstate->fb->format->format;
>> @@ -612,8 +657,12 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder,
>> 	struct drm_display_mode *mode = &crtc_state->adjusted_mode;
>> 	struct drm_connector *conn = conn_state->connector;
>> 	struct drm_display_info *info = &conn->display_info;
>> +	u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;
>> 	unsigned int cfg, rgbcfg = 0;
>> +	if (info->num_bus_formats)
>> +		bus_format = info->bus_formats[0];
>> +
> 
> That code is going to change really soon, as I have my own PR ready to convert the ingenic-drm driver to use a top-level bridge for bus format / flags negociation.
> 
> The HDMI driver should therefore implement it as well; see for instance drivers/gpu/drm/bridge/ite-it66121.c for an example of how the bus format is negociated.
> 
> I'll be sure to Cc you as soon as I send it upstream - should be just in a couple of days.

This one is still open.

> 
>> 	priv->panel_is_sharp = info->bus_flags & DRM_BUS_FLAG_SHARP_SIGNALS;
>> 	if (priv->panel_is_sharp) {
>> @@ -623,6 +672,13 @@ static void ingenic_drm_encoder_atomic_mode_set(struct drm_encoder *encoder,
>> 		    | JZ_LCD_CFG_SPL_DISABLE | JZ_LCD_CFG_REV_DISABLE;
>> 	}
>> +	if (priv->soc_info->has_recover)
>> +		cfg |= JZ_LCD_CFG_RECOVER_FIFO_UNDERRUN;
> 
> Seems out of place. Does it not work without?

Yes. We have moved it into a separate patch which enables additional jz4780 features.

> 
>> +
>> +	/* CI20: set use of the 8-word descriptor and OSD foreground usage. */
> 
> Not really CI20-specific though.

CI20 reference removed.

> 
>> +	switch (conn_state->connector->connector_type) {
>> +	case DRM_MODE_CONNECTOR_TV:
>> +	case DRM_MODE_CONNECTOR_HDMIA:
>> +		return 0;
>> +	}
> 
> This switch should move after the check on "num_bus_formats".
> (I understand why you did it, but with proper bus format negociation this won't be needed).

Not yet included since it breaks initialization. I think your proposed series will fix it.

> 
>> +
>> 	if (info->num_bus_formats != 1)
>> 		return -EINVAL;
>> -	drm->mode_config.max_height = 4095;
>> +	drm->mode_config.max_height = soc_info->max_height;
> 
> The drm->mode_config.max_height is different from soc_info->max_height; the former is the maximum framebuffer size, the latter is the maximum size that the SoC can display. The framebuffer can be bigger than what the SoC can display.

Change removed.

> 
>> 	drm->mode_config.funcs = &ingenic_drm_mode_config_funcs;
>> 	drm->mode_config.helper_private = &ingenic_drm_mode_config_helpers;
>> @@ -934,6 +994,7 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
>> 		return PTR_ERR(base);
>> 	}
>> +	ingenic_drm_regmap_config.max_register = soc_info->max_reg;
> 
> Avoid modifying a global variable; instead copy it to a local copy of ingenic_drm_regmap_config, and use this one in the regmap_init_mmio below.

modifies now a local copy on stack in v3.

> 
>> 	priv->map = devm_regmap_init_mmio(dev, base,
>> 					  &ingenic_drm_regmap_config);
>> 	if (IS_ERR(priv->map)) {
>> @@ -966,7 +1027,6 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
>> 	if (!priv->dma_hwdescs)
>> 		return -ENOMEM;
>> -
> 
> Cosmetic change - not needed.

removed.

> 
>> 	/* Configure DMA hwdesc for foreground0 plane */
>> 	dma_hwdesc_phys_f0 = priv->dma_hwdescs_phys
>> 		+ offsetof(struct ingenic_dma_hwdescs, hwdesc_f0);
>> @@ -1147,7 +1207,26 @@ static int ingenic_drm_bind(struct device *dev, bool has_components)
>> 	/* Enable OSD if available */
>> 	if (soc_info->has_osd)
>> -		regmap_write(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_OSDEN);
>> +		regmap_set_bits(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_OSDEN);
>> +
>> +	if (soc_info->has_alpha)
>> +		regmap_set_bits(priv->map, JZ_REG_LCD_OSDC, JZ_LCD_OSDC_ALPHAEN);
> 
> Do you need alpha blending support between planes, in this patch related to HDMI?

No. We have moved it into a separate patch which enables additional jz4780 features.

> 
>> +
>> +	/* Magic values from the vendor kernel for the priority thresholds. */
>> +	if (soc_info->has_pcfg)
>> +		regmap_write(priv->map, JZ_REG_LCD_PCFG,
>> +			     JZ_LCD_PCFG_PRI_MODE |
>> +			     JZ_LCD_PCFG_HP_BST_16 |
>> +			     (511 << JZ_LCD_PCFG_THRESHOLD2_OFFSET) |
>> +			     (400 << JZ_LCD_PCFG_THRESHOLD1_OFFSET) |
>> +			     (256 << JZ_LCD_PCFG_THRESHOLD0_OFFSET));
> 
> And why is that needed in this patch? Doesn't HDMI work without it?

Yes, works without. We have moved it into a separate patch which enables additional jz4780 features.

> 
>> +
>> +	/* RGB output control may be superfluous. */
>> +	if (soc_info->has_rgbc)
>> +		regmap_write(priv->map, JZ_REG_LCD_RGBC,
>> +			     JZ_LCD_RGBC_RGB_FORMAT_ENABLE |
>> +			     JZ_LCD_RGBC_ODD_LINE_RGB |
>> +			     JZ_LCD_RGBC_EVEN_LINE_RGB);
> 
> The last two macros set the subpixel ordering on the bus for serial (3x8) 24-bit panels - completely unrelated to HDMI.

Yes. We have moved it into a separate patch which enables additional jz4780 features.

> 
>> 	mutex_init(&priv->clk_mutex);
>> 	priv->clock_nb.notifier_call = ingenic_drm_update_pixclk;
>> @@ -1296,41 +1375,75 @@ static const struct jz_soc_info jz4740_soc_info = {
>> 	.needs_dev_clk = true,
>> 	.has_osd = false,
>> 	.map_noncoherent = false,
>> +	.has_pcfg = false,
>> +	.has_recover = false,
>> +	.has_rgbc = false,
>> +	.hwdesc_size = sizeof(struct ingenic_dma_hwdesc),

...

>> +	.has_alpha = true,
>> +	.has_pcfg = true,
>> +	.has_recover = true,
>> +	.has_rgbc = true,
>> +	.hwdesc_size = sizeof(struct ingenic_dma_hwdesc_ext),

We have moved it into a separate patch which enables additional jz4780 features.

>> +	.max_width = 4096,
>> +	.max_height = 4096,
> 
> The PM says that the display is up to 4096x2048-30Hz; so this is wrong.

Changed max_height to 2048.

>> -	return platform_driver_register(&ingenic_drm_driver);
>> +	err = platform_driver_register(&ingenic_drm_driver);
>> +	if (err)
>> +		goto err_ipu_unreg;
> 
> That's actually a change of behaviour - before it would return on error without calling platform_driver_unregister on the IPU.
> 
> It is not necesarily bad, but it belongs in a separate commit.

We have added a separate commit at the beginning of the v3 series to fix IPU unregister.
And then add the hdmi register/unregister.

>> +#define JZ_LCD_RGBC_ODD_LINE_MASK		(0x7 << 4)
>> +#define JZ_LCD_RGBC_ODD_LINE_RGB		(0 << 4)
>> +#define JZ_LCD_RGBC_ODD_LINE_RBG		(1 << 4)
>> +#define JZ_LCD_RGBC_ODD_LINE_GRB		(2 << 4)
>> +#define JZ_LCD_RGBC_ODD_LINE_GBR		(3 << 4)
>> +#define JZ_LCD_RGBC_ODD_LINE_BRG		(4 << 4)
>> +#define JZ_LCD_RGBC_ODD_LINE_BGR		(5 << 4)
>> +#define JZ_LCD_RGBC_EVEN_LINE_MASK		(0x7 << 0)
>> +#define JZ_LCD_RGBC_EVEN_LINE_RGB		0
>> +#define JZ_LCD_RGBC_EVEN_LINE_RBG		1
>> +#define JZ_LCD_RGBC_EVEN_LINE_GRB		2
>> +#define JZ_LCD_RGBC_EVEN_LINE_GBR		3
>> +#define JZ_LCD_RGBC_EVEN_LINE_BRG		4
>> +#define JZ_LCD_RGBC_EVEN_LINE_BGR		5
> 
> We already have these in ingenic-drm.h...
> 
> Please only add the macros that you need and are missing.

removed and made use of JZ_LCD_RGBC_EVEN_RGB etc.

> 
> Cheers,
> -Paul

BR and thanks,
Nikolaus

[PATCH v2 4/8] dt-bindings: display: Add ingenic-jz4780-hdmi DT Schema

[H. Nikolaus Schaller]

From: Sam Ravnborg <sam@ravnborg.org>

Add DT bindings for the hdmi driver for the Ingenic JZ4780 SoC.
Based on .txt binding from Zubair Lutfullah Kakakhel

Signed-off-by: Sam Ravnborg <sam@ravnborg.org>
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
Cc: Rob Herring <robh@kernel.org>
Cc: devicetree@vger.kernel.org
---
 .../bindings/display/ingenic-jz4780-hdmi.yaml | 82 +++++++++++++++++++
 1 file changed, 82 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml

diff --git a/Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml b/Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml
new file mode 100644
index 0000000000000..a545ff8704ebd
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/ingenic-jz4780-hdmi.yaml
@@ -0,0 +1,82 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/ingenic-jz4780-hdmi.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Bindings for Ingenic JZ4780 HDMI Transmitter
+
+maintainers:
+  - H. Nikolaus Schaller <hns@goldelico.com>
+
+description: |
+  The HDMI Transmitter in the Ingenic JZ4780 is a Synopsys DesignWare HDMI 1.4
+  TX controller IP with accompanying PHY IP.
+
+allOf:
+  - $ref: panel/panel-common.yaml#
+
+properties:
+  compatible:
+    items:
+      - const: ingenic,jz4780-hdmi
+
+  reg:
+    maxItems: 1
+    description: the address & size of the LCD controller registers
+
+  reg-io-width:
+    const: 4
+
+  interrupts:
+    maxItems: 1
+    description: Specifies the interrupt provided by parent
+
+  clocks:
+    maxItems: 2
+    description: Clock specifiers for isrf and iahb clocks
+
+  clock-names:
+    items:
+      - const: isfr
+      - const: iahb
+
+  ddc-i2c-bus: true
+  ports: true
+
+required:
+    - compatible
+    - clocks
+    - clock-names
+    - ports
+    - reg-io-width
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/jz4780-cgu.h>
+
+    hdmi: hdmi@10180000 {
+        compatible = "ingenic,jz4780-hdmi";
+        reg = <0x10180000 0x8000>;
+        reg-io-width = <4>;
+        ddc-i2c-bus = <&i2c4>;
+        interrupt-parent = <&intc>;
+        interrupts = <3>;
+        clocks = <&cgu JZ4780_CLK_HDMI>, <&cgu JZ4780_CLK_AHB0>;
+        clock-names = "isfr", "iahb";
+
+        ports {
+            hdmi_in: port {
+                #address-cells = <1>;
+                #size-cells = <0>;
+                hdmi_in_lcd: endpoint@0 {
+                    reg = <0>;
+                    remote-endpoint = <&jz4780_out_hdmi>;
+                };
+            };
+        };
+    };
+
+...
-- 
2.31.1

[PATCH v2 5/8] MIPS: DTS: jz4780: account for Synopsys HDMI driver and LCD controller

[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.

Signed-off-by: Paul Boddie <paul@boddie.org.uk>
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 arch/mips/boot/dts/ingenic/jz4780.dtsi | 45 ++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/arch/mips/boot/dts/ingenic/jz4780.dtsi b/arch/mips/boot/dts/ingenic/jz4780.dtsi
index 9e34f433b9b58..4cbc6a4db6cda 100644
--- a/arch/mips/boot/dts/ingenic/jz4780.dtsi
+++ b/arch/mips/boot/dts/ingenic/jz4780.dtsi
@@ -424,6 +424,51 @@ i2c4: i2c@10054000 {
 		status = "disabled";
 	};
 
+	hdmi: hdmi@10180000 {
+		compatible = "ingenic,jz4780-dw-hdmi";
+		reg = <0x10180000 0x8000>;
+		reg-io-width = <4>;
+
+		clocks = <&cgu JZ4780_CLK_HDMI>, <&cgu JZ4780_CLK_AHB0>;
+		clock-names = "isfr" , "iahb";
+
+		assigned-clocks = <&cgu JZ4780_CLK_HDMI>;
+		assigned-clock-rates = <27000000>;
+
+		interrupt-parent = <&intc>;
+		interrupts = <3>;
+
+		/* ddc-i2c-bus = <&i2c4>; */
+
+		status = "disabled";
+	};
+
+	lcdc0: lcdc0@13050000 {
+		compatible = "ingenic,jz4780-lcd";
+		reg = <0x13050000 0x1800>;
+
+		clocks = <&cgu JZ4780_CLK_TVE>, <&cgu JZ4780_CLK_LCD0PIXCLK>;
+		clock-names = "lcd", "lcd_pclk";
+
+		interrupt-parent = <&intc>;
+		interrupts = <31>;
+
+		status = "disabled";
+	};
+
+	lcdc1: lcdc1@130a0000 {
+		compatible = "ingenic,jz4780-lcd";
+		reg = <0x130a0000 0x1800>;
+
+		clocks = <&cgu JZ4780_CLK_TVE>, <&cgu JZ4780_CLK_LCD1PIXCLK>;
+		clock-names = "lcd", "lcd_pclk";
+
+		interrupt-parent = <&intc>;
+		interrupts = <31>;
+
+		status = "disabled";
+	};
+
 	nemc: nemc@13410000 {
 		compatible = "ingenic,jz4780-nemc", "simple-mfd";
 		reg = <0x13410000 0x10000>;
-- 
2.31.1

[PATCH v2 6/8] MIPS: DTS: CI20: add HDMI setup

[H. Nikolaus Schaller]

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

We need to hook up
* HDMI power regulator
* HDMI connector
* DDC pinmux
* HDMI and LCD endpoint connections

Signed-off-by: Paul Boddie <paul@boddie.org.uk>
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 arch/mips/boot/dts/ingenic/ci20.dts | 64 +++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

diff --git a/arch/mips/boot/dts/ingenic/ci20.dts b/arch/mips/boot/dts/ingenic/ci20.dts
index a688809beebca..9e87b1169dbdc 100644
--- a/arch/mips/boot/dts/ingenic/ci20.dts
+++ b/arch/mips/boot/dts/ingenic/ci20.dts
@@ -78,6 +78,28 @@ eth0_power: fixedregulator@0 {
 		enable-active-high;
 	};
 
+	hdmi_power: fixedregulator@2 {
+		compatible = "regulator-fixed";
+		regulator-name = "hdmi_power";
+		regulator-min-microvolt = <5000000>;
+		regulator-max-microvolt = <5000000>;
+		gpio = <&gpa 25 GPIO_ACTIVE_LOW>;
+		enable-active-high;
+		regulator-always-on;
+	};
+
+	hdmi_out: connector {
+		compatible = "hdmi-connector";
+		label = "HDMI OUT";
+		type = "a";
+
+		port {
+			hdmi_con: endpoint {
+				remote-endpoint = <&dw_hdmi_out>;
+			};
+		};
+	};
+
 	ir: ir {
 		compatible = "gpio-ir-receiver";
 		gpios = <&gpe 3 GPIO_ACTIVE_LOW>;
@@ -506,6 +528,12 @@ pins_i2c4: i2c4 {
 		bias-disable;
 	};
 
+	pins_hdmi_ddc: hdmi_ddc {
+		function = "hdmi-ddc";
+		groups = "hdmi-ddc";
+		bias-disable;
+	};
+
 	pins_nemc: nemc {
 		function = "nemc";
 		groups = "nemc-data", "nemc-cle-ale", "nemc-rd-we", "nemc-frd-fwe";
@@ -536,3 +564,39 @@ pins_mmc1: mmc1 {
 		bias-disable;
 	};
 };
+
+&hdmi {
+	status = "okay";
+
+	pinctrl-names = "default";
+	pinctrl-0 = <&pins_hdmi_ddc>;
+
+	ports {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		port@0 {
+			reg = <0>;
+			dw_hdmi_in: endpoint {
+				remote-endpoint = <&lcd_out>;
+			};
+		};
+
+		port@1 {
+			reg = <1>;
+			dw_hdmi_out: endpoint {
+				remote-endpoint = <&hdmi_con>;
+			};
+		};
+	};
+};
+
+&lcdc0 {
+	status = "okay";
+
+	port {
+		lcd_out: endpoint {
+			remote-endpoint = <&dw_hdmi_in>;
+		};
+	};
+};
-- 
2.31.1

[PATCH v2 7/8] MIPS: CI20: defconfig: configure for DRM_DW_HDMI_JZ4780

[H. Nikolaus Schaller]

Enable CONFIG options as modules.

Signed-off-by: Ezequiel Garcia <ezequiel@collabora.com>
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 arch/mips/configs/ci20_defconfig | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/mips/configs/ci20_defconfig b/arch/mips/configs/ci20_defconfig
index ab7ebb0668340..9d47163011ab6 100644
--- a/arch/mips/configs/ci20_defconfig
+++ b/arch/mips/configs/ci20_defconfig
@@ -98,7 +98,14 @@ CONFIG_RC_DEVICES=y
 CONFIG_IR_GPIO_CIR=m
 CONFIG_IR_GPIO_TX=m
 CONFIG_MEDIA_SUPPORT=m
+CONFIG_DRM=m
+CONFIG_DRM_INGENIC=m
+CONFIG_DRM_INGENIC_DW_HDMI=y
 # CONFIG_VGA_CONSOLE is not set
+CONFIG_FRAMEBUFFER_CONSOLE=y
+CONFIG_LOGO=y
+# CONFIG_LOGO_LINUX_MONO is not set
+# CONFIG_LOGO_LINUX_VGA16 is not set
 # CONFIG_HID is not set
 CONFIG_USB=y
 CONFIG_USB_STORAGE=y
-- 
2.31.1

[PATCH v2 8/8] [RFC] drm/ingenic: convert to component framework for jz4780 hdmi

[H. Nikolaus Schaller]

This patch attempts to convert the ingenic-dw-hdmi driver
into a version that uses the component framework.

Unfortunately the new version does not work.

Debugging shows that ingenic_dw_hdmi_bind() is never called.

Suggestions for reasons and fixes are welcome.

Signed-off-by: Paul Boddie <paul@boddie.org.uk>
Co-authored-by: Paul Boddie <paul@boddie.org.uk>
Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
---
 drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c | 57 ++++++++++++++++++-----
 1 file changed, 46 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c b/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c
index 61e7a57d7cec1..a5ba0b69baa8c 100644
--- a/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c
+++ b/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c
@@ -1,17 +1,24 @@
 // SPDX-License-Identifier: GPL-2.0
 /* Copyright (C) 2011-2013 Freescale Semiconductor, Inc.
- * Copyright (C) 2019, 2020 Paul Boddie <paul@boddie.org.uk>
+ * Copyright (C) 2019, 2020, 2021 Paul Boddie <paul@boddie.org.uk>
  *
  * Derived from dw_hdmi-imx.c with i.MX portions removed.
- * Probe and remove operations derived from rcar_dw_hdmi.c.
  */
 
+#include <linux/component.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 
 #include <drm/bridge/dw_hdmi.h>
 #include <drm/drm_of.h>
+#include <drm/drm_modeset_helper_vtables.h>
+#include <drm/drm_simple_kms_helper.h>
+
+struct ingenic_dw_hdmi_encoder {
+	struct drm_encoder encoder;
+	struct dw_hdmi *hdmi;
+};
 
 static const struct dw_hdmi_mpll_config ingenic_mpll_cfg[] = {
 	{ 45250000,  { { 0x01e0, 0x0000 }, { 0x21e1, 0x0000 }, { 0x41e2, 0x0000 } } },
@@ -87,24 +94,52 @@ static const struct of_device_id ingenic_dw_hdmi_dt_ids[] = {
 };
 MODULE_DEVICE_TABLE(of, ingenic_dw_hdmi_dt_ids);
 
-static int ingenic_dw_hdmi_probe(struct platform_device *pdev)
+static int ingenic_dw_hdmi_bind(struct device *dev, struct device *master,
+				void *data)
 {
-	struct dw_hdmi *hdmi;
+	struct platform_device *pdev = to_platform_device(dev);
+	struct drm_device *drm = data;
+	struct drm_encoder *enc;
+	struct ingenic_dw_hdmi_encoder *hdmi_encoder;
 
-	hdmi = dw_hdmi_probe(pdev, &ingenic_dw_hdmi_plat_data);
-	if (IS_ERR(hdmi))
-		return PTR_ERR(hdmi);
+	hdmi_encoder = drmm_simple_encoder_alloc(drm, struct ingenic_dw_hdmi_encoder,
+						 encoder, DRM_MODE_ENCODER_TMDS);
+	if (IS_ERR(hdmi_encoder))
+		return PTR_ERR(hdmi_encoder);
 
-	platform_set_drvdata(pdev, hdmi);
+	enc = &hdmi_encoder->encoder;
+	drm_encoder_helper_add(enc, NULL);
+	hdmi_encoder->hdmi = dw_hdmi_bind(pdev, enc, &ingenic_dw_hdmi_plat_data);
+
+	if (IS_ERR(hdmi_encoder->hdmi))
+		return PTR_ERR(hdmi_encoder->hdmi);
+
+	dev_set_drvdata(dev, hdmi_encoder->hdmi);
 
 	return 0;
 }
 
-static int ingenic_dw_hdmi_remove(struct platform_device *pdev)
+static void ingenic_dw_hdmi_unbind(struct device *dev, struct device *master,
+				   void *data)
+{
+	struct dw_hdmi *hdmi = dev_get_drvdata(dev);
+
+	dw_hdmi_unbind(hdmi);
+}
+
+static const struct component_ops ingenic_dw_hdmi_ops = {
+	.bind	= ingenic_dw_hdmi_bind,
+	.unbind	= ingenic_dw_hdmi_unbind,
+};
+
+static int ingenic_dw_hdmi_probe(struct platform_device *pdev)
 {
-	struct dw_hdmi *hdmi = platform_get_drvdata(pdev);
+	return component_add(&pdev->dev, &ingenic_dw_hdmi_ops);
+}
 
-	dw_hdmi_remove(hdmi);
+static int ingenic_dw_hdmi_remove(struct platform_device *pdev)
+{
+	component_del(&pdev->dev, &ingenic_dw_hdmi_ops);
 
 	return 0;
 }
-- 
2.31.1

Re: [PATCH v2 8/8] [RFC] drm/ingenic: convert to component framework for jz4780 hdmi

[Laurent Pinchart]

Hi Nikolaus,

Thank you for the patch.

On Thu, Aug 05, 2021 at 04:07:57PM +0200, H. Nikolaus Schaller wrote:
> This patch attempts to convert the ingenic-dw-hdmi driver
> into a version that uses the component framework.

Why ? What problem would this solve ?

> Unfortunately the new version does not work.
> 
> Debugging shows that ingenic_dw_hdmi_bind() is never called.
> 
> Suggestions for reasons and fixes are welcome.
> 
> Signed-off-by: Paul Boddie <paul@boddie.org.uk>
> Co-authored-by: Paul Boddie <paul@boddie.org.uk>
> Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com>
> ---
>  drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c | 57 ++++++++++++++++++-----
>  1 file changed, 46 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c b/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c
> index 61e7a57d7cec1..a5ba0b69baa8c 100644
> --- a/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c
> +++ b/drivers/gpu/drm/ingenic/ingenic-dw-hdmi.c
> @@ -1,17 +1,24 @@
>  // SPDX-License-Identifier: GPL-2.0
>  /* Copyright (C) 2011-2013 Freescale Semiconductor, Inc.
> - * Copyright (C) 2019, 2020 Paul Boddie <paul@boddie.org.uk>
> + * Copyright (C) 2019, 2020, 2021 Paul Boddie <paul@boddie.org.uk>
>   *
>   * Derived from dw_hdmi-imx.c with i.MX portions removed.
> - * Probe and remove operations derived from rcar_dw_hdmi.c.
>   */
>  
> +#include <linux/component.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>  #include <linux/regmap.h>
>  
>  #include <drm/bridge/dw_hdmi.h>
>  #include <drm/drm_of.h>
> +#include <drm/drm_modeset_helper_vtables.h>
> +#include <drm/drm_simple_kms_helper.h>
> +
> +struct ingenic_dw_hdmi_encoder {
> +	struct drm_encoder encoder;
> +	struct dw_hdmi *hdmi;
> +};
>  
>  static const struct dw_hdmi_mpll_config ingenic_mpll_cfg[] = {
>  	{ 45250000,  { { 0x01e0, 0x0000 }, { 0x21e1, 0x0000 }, { 0x41e2, 0x0000 } } },
> @@ -87,24 +94,52 @@ static const struct of_device_id ingenic_dw_hdmi_dt_ids[] = {
>  };
>  MODULE_DEVICE_TABLE(of, ingenic_dw_hdmi_dt_ids);
>  
> -static int ingenic_dw_hdmi_probe(struct platform_device *pdev)
> +static int ingenic_dw_hdmi_bind(struct device *dev, struct device *master,
> +				void *data)
>  {
> -	struct dw_hdmi *hdmi;
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct drm_device *drm = data;
> +	struct drm_encoder *enc;
> +	struct ingenic_dw_hdmi_encoder *hdmi_encoder;
>  
> -	hdmi = dw_hdmi_probe(pdev, &ingenic_dw_hdmi_plat_data);
> -	if (IS_ERR(hdmi))
> -		return PTR_ERR(hdmi);
> +	hdmi_encoder = drmm_simple_encoder_alloc(drm, struct ingenic_dw_hdmi_encoder,
> +						 encoder, DRM_MODE_ENCODER_TMDS);
> +	if (IS_ERR(hdmi_encoder))
> +		return PTR_ERR(hdmi_encoder);
>  
> -	platform_set_drvdata(pdev, hdmi);
> +	enc = &hdmi_encoder->encoder;
> +	drm_encoder_helper_add(enc, NULL);
> +	hdmi_encoder->hdmi = dw_hdmi_bind(pdev, enc, &ingenic_dw_hdmi_plat_data);
> +
> +	if (IS_ERR(hdmi_encoder->hdmi))
> +		return PTR_ERR(hdmi_encoder->hdmi);
> +
> +	dev_set_drvdata(dev, hdmi_encoder->hdmi);
>  
>  	return 0;
>  }
>  
> -static int ingenic_dw_hdmi_remove(struct platform_device *pdev)
> +static void ingenic_dw_hdmi_unbind(struct device *dev, struct device *master,
> +				   void *data)
> +{
> +	struct dw_hdmi *hdmi = dev_get_drvdata(dev);
> +
> +	dw_hdmi_unbind(hdmi);
> +}
> +
> +static const struct component_ops ingenic_dw_hdmi_ops = {
> +	.bind	= ingenic_dw_hdmi_bind,
> +	.unbind	= ingenic_dw_hdmi_unbind,
> +};
> +
> +static int ingenic_dw_hdmi_probe(struct platform_device *pdev)
>  {
> -	struct dw_hdmi *hdmi = platform_get_drvdata(pdev);
> +	return component_add(&pdev->dev, &ingenic_dw_hdmi_ops);
> +}
>  
> -	dw_hdmi_remove(hdmi);
> +static int ingenic_dw_hdmi_remove(struct platform_device *pdev)
> +{
> +	component_del(&pdev->dev, &ingenic_dw_hdmi_ops);
>  
>  	return 0;
>  }

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 8/8] [RFC] drm/ingenic: convert to component framework for jz4780 hdmi

[H. Nikolaus Schaller]

Hi Laurent,

> Am 05.08.2021 um 17:04 schrieb Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
> 
> Hi Nikolaus,
> 
> Thank you for the patch.
> 
> On Thu, Aug 05, 2021 at 04:07:57PM +0200, H. Nikolaus Schaller wrote:
>> This patch attempts to convert the ingenic-dw-hdmi driver
>> into a version that uses the component framework.
> 
> Why ? What problem would this solve ?

Well, it was suggested in a v1 we did post several months ago. I have not
looked up by whom and do not exactly remember the reasons.

We now simply thought that it is common style since dome dw-hdmi drivers
make use of it but some others don't. And we got it working without.

If it is not needed/requested by anyone, we can drop it from v3 (or add later).

BR and thanks,
Nikolaus

Re: [PATCH v2 8/8] [RFC] drm/ingenic: convert to component framework for jz4780 hdmi

[Paul Cercueil]

Hi Nikolaus and Laurent,

Le jeu., août 5 2021 at 18:07:20 +0200, H. Nikolaus Schaller 
<hns@goldelico.com> a écrit :
> Hi Laurent,
> 
>>  Am 05.08.2021 um 17:04 schrieb Laurent Pinchart 
>> <laurent.pinchart@ideasonboard.com>:
>> 
>>  Hi Nikolaus,
>> 
>>  Thank you for the patch.
>> 
>>  On Thu, Aug 05, 2021 at 04:07:57PM +0200, H. Nikolaus Schaller 
>> wrote:
>>>  This patch attempts to convert the ingenic-dw-hdmi driver
>>>  into a version that uses the component framework.
>> 
>>  Why ? What problem would this solve ?
> 
> Well, it was suggested in a v1 we did post several months ago. I have 
> not
> looked up by whom and do not exactly remember the reasons.
> 
> We now simply thought that it is common style since dome dw-hdmi 
> drivers
> make use of it but some others don't. And we got it working without.
> 
> If it is not needed/requested by anyone, we can drop it from v3 (or 
> add later).

I don't remember exactly TBH - the only reason to use a component is to 
have access to the main driver's "drm_device" structure. The IPU needs 
it for instance, to register planes; but I don't think this HDMI driver 
needs it as it registers a bridge.

Cheers,
-Paul

Re: [PATCH v2 8/8] [RFC] drm/ingenic: convert to component framework for jz4780 hdmi

[H. Nikolaus Schaller]

Hi Paul,

> Am 05.08.2021 um 18:17 schrieb Paul Cercueil <paul@crapouillou.net>:
> 
> Hi Nikolaus and Laurent,
> 
> Le jeu., août 5 2021 at 18:07:20 +0200, H. Nikolaus Schaller <hns@goldelico.com> a écrit :
>> Hi Laurent,
>>> Am 05.08.2021 um 17:04 schrieb Laurent Pinchart <laurent.pinchart@ideasonboard.com>:
>>> Hi Nikolaus,
>>> Thank you for the patch.
>>> On Thu, Aug 05, 2021 at 04:07:57PM +0200, H. Nikolaus Schaller wrote:
>>>> This patch attempts to convert the ingenic-dw-hdmi driver
>>>> into a version that uses the component framework.
>>> Why ? What problem would this solve ?
>> Well, it was suggested in a v1 we did post several months ago. I have not
>> looked up by whom and do not exactly remember the reasons.
>> We now simply thought that it is common style since dome dw-hdmi drivers
>> make use of it but some others don't. And we got it working without.
>> If it is not needed/requested by anyone, we can drop it from v3 (or add later).
> 
> I don't remember exactly TBH - the only reason to use a component is to have access to the main driver's "drm_device" structure. The IPU needs it for instance, to register planes; but I don't think this HDMI driver needs it as it registers a bridge.
> 
> Cheers,
> -Paul

Ok, fine! We'll drop it and don't waste time.

BR and thanks,
NIkolaus

Re: [PATCH v2 8/8] [RFC] drm/ingenic: convert to component framework for jz4780 hdmi

[Daniel Vetter]

On Thu, Aug 05, 2021 at 06:17:32PM +0200, Paul Cercueil wrote:
> Hi Nikolaus and Laurent,
> 
> Le jeu., août 5 2021 at 18:07:20 +0200, H. Nikolaus Schaller
> <hns@goldelico.com> a écrit :
> > Hi Laurent,
> > 
> > >  Am 05.08.2021 um 17:04 schrieb Laurent Pinchart
> > > <laurent.pinchart@ideasonboard.com>:
> > > 
> > >  Hi Nikolaus,
> > > 
> > >  Thank you for the patch.
> > > 
> > >  On Thu, Aug 05, 2021 at 04:07:57PM +0200, H. Nikolaus Schaller
> > > wrote:
> > > >  This patch attempts to convert the ingenic-dw-hdmi driver
> > > >  into a version that uses the component framework.
> > > 
> > >  Why ? What problem would this solve ?
> > 
> > Well, it was suggested in a v1 we did post several months ago. I have
> > not
> > looked up by whom and do not exactly remember the reasons.
> > 
> > We now simply thought that it is common style since dome dw-hdmi drivers
> > make use of it but some others don't. And we got it working without.
> > 
> > If it is not needed/requested by anyone, we can drop it from v3 (or add
> > later).
> 
> I don't remember exactly TBH - the only reason to use a component is to have
> access to the main driver's "drm_device" structure. The IPU needs it for
> instance, to register planes; but I don't think this HDMI driver needs it as
> it registers a bridge.

Imo for bridges/panels we really should move _away_ from component, not
towards it. If there's a gap in the bridge/panel api (I think there's some
patches floating around exactly to make this more a multi-step process for
reasons like the above) then we need to fix that.

Unfortunately the dw-hdmi and also dw-dsi drivers are very much built on
top of component.c and side-step the bridge stuff quite a lot. That
results in quite bad integration pains all around as we add more users of
these.

The other unfortunate thing is that there's not many people working in
this area, so fundamental improvements to the core design take a long time
to make and then even longer to roll out. It's a bit a tough spot, but
also, help very much welcome :-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch