[PATCH v1 0/6] drm/mediatek: dpi: Add support for MT8195/8188 and Pattern Generator

[PATCH v1 0/6] drm/mediatek: dpi: Add support for MT8195/8188 and Pattern Generator

[AngeloGioacchino Del Regno]

Add support for the direct connection DPI found in MT8195 and MT8188 SoCs.

Bonus in this series is the addition of support for the Pattern Generator
found in the DPI HW: since I needed this for debugging during development,
I had to code in the actual support bits and it looked like a waste of
time to just remove it.
I instead decided to clean it up and upstream it, as this will anyway come
handy for multiple things, of which the most important (imo) are:
 - Adding support for new SoCs in the future will be less time consuming
   as this driver already has the pattern generator in;
 - CI Testing might be able to make use of this to validate that the
   data that comes out is not garbled (so, to help testing display
   support in an automated manner).

AngeloGioacchino Del Regno (6):
  dt-bindings: display: mediatek: dpi: Add MT8195 and MT8188 compat
  drm/mediatek: mtk_dpi: Add support for Pattern Generator in debugfs
  drm/mediatek: mtk_dpi: Use an array for pixclk factor calculation
  drm/mediatek: mtk_dpi: Move pixel clock setting flow to function
  drm/mediatek: mtk_dpi: Add checks for reg_h_fre_con existence
  drm/mediatek: Add support for MT8195 Digital Parallel Interface

 .../display/mediatek/mediatek,dpi.yaml        |   5 +
 drivers/gpu/drm/mediatek/mtk_dpi.c            | 311 +++++++++++++-----
 drivers/gpu/drm/mediatek/mtk_dpi_regs.h       |  10 +
 drivers/gpu/drm/mediatek/mtk_drm_drv.c        |   2 +
 4 files changed, 252 insertions(+), 76 deletions(-)

-- 
2.47.0

[PATCH v1 1/6] dt-bindings: display: mediatek: dpi: Add MT8195 and MT8188 compat

[AngeloGioacchino Del Regno]

Add compatibles for the Digital Parallel Interface (DPI) block
found in the MT8195 and MT8188 SoCs: inside of the chip, this one
is directly connected to the HDMI block.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 .../devicetree/bindings/display/mediatek/mediatek,dpi.yaml   | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yaml
index b567e3d58aa1..ef180a63ee72 100644
--- a/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yaml
+++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yaml
@@ -27,6 +27,7 @@ properties:
           - mediatek,mt8188-dp-intf
           - mediatek,mt8192-dpi
           - mediatek,mt8195-dp-intf
+          - mediatek,mt8195-dpi
       - items:
           - enum:
               - mediatek,mt6795-dpi
@@ -35,6 +36,10 @@ properties:
           - enum:
               - mediatek,mt8365-dpi
           - const: mediatek,mt8192-dpi
+      - items:
+          - enum:
+              - mediatek,mt8188-dpi
+          - const: mediatek,mt8195-dpi
 
   reg:
     maxItems: 1
-- 
2.47.0

Re: [PATCH v1 1/6] dt-bindings: display: mediatek: dpi: Add MT8195 and MT8188 compat

[Krzysztof Kozlowski]

On Wed, Nov 20, 2024 at 01:44:15PM +0100, AngeloGioacchino Del Regno wrote:
> Add compatibles for the Digital Parallel Interface (DPI) block
> found in the MT8195 and MT8188 SoCs: inside of the chip, this one
> is directly connected to the HDMI block.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  .../devicetree/bindings/display/mediatek/mediatek,dpi.yaml   | 5 +++++
>  1 file changed, 5 insertions(+)

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v1 1/6] dt-bindings: display: mediatek: dpi: Add MT8195 and MT8188 compat

[CK Hu (胡俊光)]

Hi, Angelo:

On Wed, 2024-11-20 at 13:44 +0100, AngeloGioacchino Del Regno wrote:
> External email : Please do not click links or open attachments until you have verified the sender or the content.
> 
> 
> Add compatibles for the Digital Parallel Interface (DPI) block
> found in the MT8195 and MT8188 SoCs: inside of the chip, this one
> is directly connected to the HDMI block.

Reviewed-by: CK Hu <ck.hu@mediatek.com>

> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  .../devicetree/bindings/display/mediatek/mediatek,dpi.yaml   | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yaml
> index b567e3d58aa1..ef180a63ee72 100644
> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yaml
> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.yaml
> @@ -27,6 +27,7 @@ properties:
>            - mediatek,mt8188-dp-intf
>            - mediatek,mt8192-dpi
>            - mediatek,mt8195-dp-intf
> +          - mediatek,mt8195-dpi
>        - items:
>            - enum:
>                - mediatek,mt6795-dpi
> @@ -35,6 +36,10 @@ properties:
>            - enum:
>                - mediatek,mt8365-dpi
>            - const: mediatek,mt8192-dpi
> +      - items:
> +          - enum:
> +              - mediatek,mt8188-dpi
> +          - const: mediatek,mt8195-dpi
> 
>    reg:
>      maxItems: 1
> --
> 2.47.0
> 

[PATCH v1 2/6] drm/mediatek: mtk_dpi: Add support for Pattern Generator in debugfs

[AngeloGioacchino Del Regno]

This IP includes a Pattern Generator which is useful for debugging
and testing purposes: add the relevant register and bits to the
mtk_dpi_regs.h header, and implement support for it in mtk_dpi.

Adding this required to introduce a .debugfs_init() callback for
the DPI bridge, which creates a "dpi_test_pattern" file in the
directory of the appropriate connector.

The pattern generator can generate various internal patterns and
this submission includes support for:
 - 256 or 1024 shades of gray in a Vertical or Horizontal Pattern
 - Vertical Color Bars
 - Frame border
 - Dot Moire

This generator also supports filling the entire screen with one
custom color, but support for that is not included in this commit.

Enabling and disabling this generator can be done by sending a
string to the dpi_test_pattern debugfs file; the pattern is
expected to be formatted as follows:

         <enable (1) or disable (0)> <pattern number>

where the pattern number can be a number from 0 to 7, excluding 5.

Of course 5 is excluded because that activates custom color fill
which, again, is not supported in this commit.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/gpu/drm/mediatek/mtk_dpi.c      | 107 ++++++++++++++++++++++++
 drivers/gpu/drm/mediatek/mtk_dpi_regs.h |   4 +
 2 files changed, 111 insertions(+)

diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c b/drivers/gpu/drm/mediatek/mtk_dpi.c
index 20a9d589fd75..c7143184e5de 100644
--- a/drivers/gpu/drm/mediatek/mtk_dpi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dpi.c
@@ -6,6 +6,7 @@
 
 #include <linux/clk.h>
 #include <linux/component.h>
+#include <linux/debugfs.h>
 #include <linux/interrupt.h>
 #include <linux/kernel.h>
 #include <linux/media-bus-format.h>
@@ -166,6 +167,18 @@ static void mtk_dpi_mask(struct mtk_dpi *dpi, u32 offset, u32 val, u32 mask)
 	writel(tmp, dpi->regs + offset);
 }
 
+static void mtk_dpi_test_pattern_en(struct mtk_dpi *dpi, u8 type, bool enable)
+{
+	u32 val;
+
+	if (enable)
+		val = FIELD_PREP(DPI_PAT_SEL, type) | DPI_PAT_EN;
+	else
+		val = 0;
+
+	mtk_dpi_mask(dpi, DPI_PATTERN0, val, DPI_PAT_SEL | DPI_PAT_EN);
+}
+
 static void mtk_dpi_sw_reset(struct mtk_dpi *dpi, bool reset)
 {
 	mtk_dpi_mask(dpi, DPI_RET, reset ? RST : 0, RST);
@@ -767,6 +780,99 @@ mtk_dpi_bridge_mode_valid(struct drm_bridge *bridge,
 	return MODE_OK;
 }
 
+static int mtk_dpi_debug_tp_show(struct seq_file *m, void *arg)
+{
+	struct mtk_dpi *dpi = m->private;
+	bool en;
+	u32 val;
+
+	if (!dpi)
+		return -EINVAL;
+
+	val = readl(dpi->regs + DPI_PATTERN0);
+	en = val & DPI_PAT_EN;
+	val = FIELD_GET(DPI_PAT_SEL, val);
+
+	seq_printf(m, "DPI Test Pattern: %s\n", en ? "Enabled" : "Disabled");
+
+	if (en) {
+		seq_printf(m, "Internal pattern %d: ", val);
+		switch (val) {
+		case 0:
+			seq_puts(m, "256 Vertical Gray\n");
+			break;
+		case 1:
+			seq_puts(m, "1024 Vertical Gray\n");
+			break;
+		case 2:
+			seq_puts(m, "256 Horizontal Gray\n");
+			break;
+		case 3:
+			seq_puts(m, "1024 Horizontal Gray\n");
+			break;
+		case 4:
+			seq_puts(m, "Vertical Color bars\n");
+			break;
+		case 6:
+			seq_puts(m, "Frame border\n");
+			break;
+		case 7:
+			seq_puts(m, "Dot moire\n");
+			break;
+		default:
+			seq_puts(m, "Invalid selection\n");
+			break;
+		}
+	}
+
+	return 0;
+}
+
+static ssize_t mtk_dpi_debug_tp_write(struct file *file, const char __user *ubuf,
+				      size_t len, loff_t *offp)
+{
+	struct seq_file *m = file->private_data;
+	u32 en, type;
+	char buf[6];
+
+	if (!m || !m->private || *offp || len > sizeof(buf) - 1)
+		return -EINVAL;
+
+	memset(buf, 0, sizeof(buf));
+	if (copy_from_user(buf, ubuf, len))
+		return -EFAULT;
+
+	if (sscanf(buf, "%u %u", &en, &type) != 2)
+		return -EINVAL;
+
+	if (en < 0 || en > 1 || type < 0 || type > 7)
+		return -EINVAL;
+
+	mtk_dpi_test_pattern_en((struct mtk_dpi *)m->private, type, en);
+	return len;
+}
+
+static int mtk_dpi_debug_tp_open(struct inode *inode, struct file *file)
+{
+	return single_open(file, mtk_dpi_debug_tp_show, inode->i_private);
+}
+
+static const struct file_operations mtk_dpi_debug_tp_fops = {
+	.owner = THIS_MODULE,
+	.open = mtk_dpi_debug_tp_open,
+	.read = seq_read,
+	.write = mtk_dpi_debug_tp_write,
+	.llseek = seq_lseek,
+	.release = single_release,
+};
+
+static void mtk_dpi_debugfs_init(struct drm_bridge *bridge, struct dentry *root)
+{
+	struct mtk_dpi *dpi = bridge_to_dpi(bridge);
+
+	debugfs_create_file("dpi_test_pattern", 0640, root, dpi, &mtk_dpi_debug_tp_fops);
+}
+
 static const struct drm_bridge_funcs mtk_dpi_bridge_funcs = {
 	.attach = mtk_dpi_bridge_attach,
 	.mode_set = mtk_dpi_bridge_mode_set,
@@ -779,6 +885,7 @@ static const struct drm_bridge_funcs mtk_dpi_bridge_funcs = {
 	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
 	.atomic_reset = drm_atomic_helper_bridge_reset,
+	.debugfs_init = mtk_dpi_debugfs_init,
 };
 
 void mtk_dpi_start(struct device *dev)
diff --git a/drivers/gpu/drm/mediatek/mtk_dpi_regs.h b/drivers/gpu/drm/mediatek/mtk_dpi_regs.h
index 62bd4931b344..a0b1d18bbbf7 100644
--- a/drivers/gpu/drm/mediatek/mtk_dpi_regs.h
+++ b/drivers/gpu/drm/mediatek/mtk_dpi_regs.h
@@ -235,4 +235,8 @@
 #define MATRIX_SEL_RGB_TO_JPEG		0
 #define MATRIX_SEL_RGB_TO_BT601		2
 
+#define DPI_PATTERN0		0xf00
+#define DPI_PAT_EN			BIT(0)
+#define DPI_PAT_SEL			GENMASK(6, 4)
+
 #endif /* __MTK_DPI_REGS_H */
-- 
2.47.0

Re: [PATCH v1 2/6] drm/mediatek: mtk_dpi: Add support for Pattern Generator in debugfs

[CK Hu (胡俊光)]

Hi, Angelo:

On Wed, 2024-11-20 at 13:44 +0100, AngeloGioacchino Del Regno wrote:
> External email : Please do not click links or open attachments until you have verified the sender or the content.
> 
> 
> This IP includes a Pattern Generator which is useful for debugging
> and testing purposes: add the relevant register and bits to the
> mtk_dpi_regs.h header, and implement support for it in mtk_dpi.
> 
> Adding this required to introduce a .debugfs_init() callback for
> the DPI bridge, which creates a "dpi_test_pattern" file in the
> directory of the appropriate connector.
> 
> The pattern generator can generate various internal patterns and
> this submission includes support for:
>  - 256 or 1024 shades of gray in a Vertical or Horizontal Pattern
>  - Vertical Color Bars
>  - Frame border
>  - Dot Moire
> 
> This generator also supports filling the entire screen with one
> custom color, but support for that is not included in this commit.
> 
> Enabling and disabling this generator can be done by sending a
> string to the dpi_test_pattern debugfs file; the pattern is
> expected to be formatted as follows:
> 
>          <enable (1) or disable (0)> <pattern number>
> 
> where the pattern number can be a number from 0 to 7, excluding 5.
> 
> Of course 5 is excluded because that activates custom color fill
> which, again, is not supported in this commit.

Reviewed-by: CK Hu <ck.hu@mediatek.com>

> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_dpi.c      | 107 ++++++++++++++++++++++++
>  drivers/gpu/drm/mediatek/mtk_dpi_regs.h |   4 +
>  2 files changed, 111 insertions(+)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c b/drivers/gpu/drm/mediatek/mtk_dpi.c
> index 20a9d589fd75..c7143184e5de 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dpi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c
> @@ -6,6 +6,7 @@
> 
>  #include <linux/clk.h>
>  #include <linux/component.h>
> +#include <linux/debugfs.h>
>  #include <linux/interrupt.h>
>  #include <linux/kernel.h>
>  #include <linux/media-bus-format.h>
> @@ -166,6 +167,18 @@ static void mtk_dpi_mask(struct mtk_dpi *dpi, u32 offset, u32 val, u32 mask)
>         writel(tmp, dpi->regs + offset);
>  }
> 
> +static void mtk_dpi_test_pattern_en(struct mtk_dpi *dpi, u8 type, bool enable)
> +{
> +       u32 val;
> +
> +       if (enable)
> +               val = FIELD_PREP(DPI_PAT_SEL, type) | DPI_PAT_EN;
> +       else
> +               val = 0;
> +
> +       mtk_dpi_mask(dpi, DPI_PATTERN0, val, DPI_PAT_SEL | DPI_PAT_EN);
> +}
> +
>  static void mtk_dpi_sw_reset(struct mtk_dpi *dpi, bool reset)
>  {
>         mtk_dpi_mask(dpi, DPI_RET, reset ? RST : 0, RST);
> @@ -767,6 +780,99 @@ mtk_dpi_bridge_mode_valid(struct drm_bridge *bridge,
>         return MODE_OK;
>  }
> 
> +static int mtk_dpi_debug_tp_show(struct seq_file *m, void *arg)
> +{
> +       struct mtk_dpi *dpi = m->private;
> +       bool en;
> +       u32 val;
> +
> +       if (!dpi)
> +               return -EINVAL;
> +
> +       val = readl(dpi->regs + DPI_PATTERN0);
> +       en = val & DPI_PAT_EN;
> +       val = FIELD_GET(DPI_PAT_SEL, val);
> +
> +       seq_printf(m, "DPI Test Pattern: %s\n", en ? "Enabled" : "Disabled");
> +
> +       if (en) {
> +               seq_printf(m, "Internal pattern %d: ", val);
> +               switch (val) {
> +               case 0:
> +                       seq_puts(m, "256 Vertical Gray\n");
> +                       break;
> +               case 1:
> +                       seq_puts(m, "1024 Vertical Gray\n");
> +                       break;
> +               case 2:
> +                       seq_puts(m, "256 Horizontal Gray\n");
> +                       break;
> +               case 3:
> +                       seq_puts(m, "1024 Horizontal Gray\n");
> +                       break;
> +               case 4:
> +                       seq_puts(m, "Vertical Color bars\n");
> +                       break;
> +               case 6:
> +                       seq_puts(m, "Frame border\n");
> +                       break;
> +               case 7:
> +                       seq_puts(m, "Dot moire\n");
> +                       break;
> +               default:
> +                       seq_puts(m, "Invalid selection\n");
> +                       break;
> +               }
> +       }
> +
> +       return 0;
> +}
> +
> +static ssize_t mtk_dpi_debug_tp_write(struct file *file, const char __user *ubuf,
> +                                     size_t len, loff_t *offp)
> +{
> +       struct seq_file *m = file->private_data;
> +       u32 en, type;
> +       char buf[6];
> +
> +       if (!m || !m->private || *offp || len > sizeof(buf) - 1)
> +               return -EINVAL;
> +
> +       memset(buf, 0, sizeof(buf));
> +       if (copy_from_user(buf, ubuf, len))
> +               return -EFAULT;
> +
> +       if (sscanf(buf, "%u %u", &en, &type) != 2)
> +               return -EINVAL;
> +
> +       if (en < 0 || en > 1 || type < 0 || type > 7)
> +               return -EINVAL;
> +
> +       mtk_dpi_test_pattern_en((struct mtk_dpi *)m->private, type, en);
> +       return len;
> +}
> +
> +static int mtk_dpi_debug_tp_open(struct inode *inode, struct file *file)
> +{
> +       return single_open(file, mtk_dpi_debug_tp_show, inode->i_private);
> +}
> +
> +static const struct file_operations mtk_dpi_debug_tp_fops = {
> +       .owner = THIS_MODULE,
> +       .open = mtk_dpi_debug_tp_open,
> +       .read = seq_read,
> +       .write = mtk_dpi_debug_tp_write,
> +       .llseek = seq_lseek,
> +       .release = single_release,
> +};
> +
> +static void mtk_dpi_debugfs_init(struct drm_bridge *bridge, struct dentry *root)
> +{
> +       struct mtk_dpi *dpi = bridge_to_dpi(bridge);
> +
> +       debugfs_create_file("dpi_test_pattern", 0640, root, dpi, &mtk_dpi_debug_tp_fops);
> +}
> +
>  static const struct drm_bridge_funcs mtk_dpi_bridge_funcs = {
>         .attach = mtk_dpi_bridge_attach,
>         .mode_set = mtk_dpi_bridge_mode_set,
> @@ -779,6 +885,7 @@ static const struct drm_bridge_funcs mtk_dpi_bridge_funcs = {
>         .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
>         .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
>         .atomic_reset = drm_atomic_helper_bridge_reset,
> +       .debugfs_init = mtk_dpi_debugfs_init,
>  };
> 
>  void mtk_dpi_start(struct device *dev)
> diff --git a/drivers/gpu/drm/mediatek/mtk_dpi_regs.h b/drivers/gpu/drm/mediatek/mtk_dpi_regs.h
> index 62bd4931b344..a0b1d18bbbf7 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dpi_regs.h
> +++ b/drivers/gpu/drm/mediatek/mtk_dpi_regs.h
> @@ -235,4 +235,8 @@
>  #define MATRIX_SEL_RGB_TO_JPEG         0
>  #define MATRIX_SEL_RGB_TO_BT601                2
> 
> +#define DPI_PATTERN0           0xf00
> +#define DPI_PAT_EN                     BIT(0)
> +#define DPI_PAT_SEL                    GENMASK(6, 4)
> +
>  #endif /* __MTK_DPI_REGS_H */
> --
> 2.47.0
> 

[PATCH v1 3/6] drm/mediatek: mtk_dpi: Use an array for pixclk factor calculation

[AngeloGioacchino Del Regno]

Setting the TVD PLL clock requires to multiply the target pixel
clock by a specific constant factor to achieve the target PLL
frequency, and this is done to reduce jitter to acceptable levels.

On all MediaTek SoCs, the factor is not retrieved by any real kind
of calculation but rather by checking if the target pixel clock
is less than a specified frequency, hence assigning a function
pointer for just a bunch of if branches does enlarge the code
size for little reason.

Remove all SoC-specific functions, add a structure `mtk_dpi_factor`
that holds a clock frequency and corresponding PLL factor, and
declare the constraints for each SoC in form of an array of said
structure.
Instead of function pointers, this structure (and its size) is then
assigned to each SoC's platform data.

The "calculation" is then performed with a new static function
mtk_dpi_calculate_factor(dpi, mode_clk) that iterates through all
of the entries of the aforementioned array and returns the right
factor.

If no factor is found, the lowest possible factor is returned,
mimicking the same flow as all of the old per-SoC calculation
functions.

This commit brings no functional change.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/gpu/drm/mediatek/mtk_dpi.c | 103 +++++++++++++++--------------
 1 file changed, 52 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c b/drivers/gpu/drm/mediatek/mtk_dpi.c
index c7143184e5de..9f59ee679ce1 100644
--- a/drivers/gpu/drm/mediatek/mtk_dpi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dpi.c
@@ -117,9 +117,15 @@ struct mtk_dpi_yc_limit {
 	u16 c_bottom;
 };
 
+struct mtk_dpi_factor {
+	u32 clock;
+	u8 factor;
+};
+
 /**
  * struct mtk_dpi_conf - Configuration of mediatek dpi.
- * @cal_factor: Callback function to calculate factor value.
+ * @dpi_factor: SoC-specific pixel clock PLL factor values.
+ * @num_dpi_factor: Number of pixel clock PLL factor values.
  * @reg_h_fre_con: Register address of frequency control.
  * @max_clock_khz: Max clock frequency supported for this SoCs in khz units.
  * @edge_sel_en: Enable of edge selection.
@@ -140,7 +146,8 @@ struct mtk_dpi_yc_limit {
  * @edge_cfg_in_mmsys: If the edge configuration for DPI's output needs to be set in MMSYS.
  */
 struct mtk_dpi_conf {
-	unsigned int (*cal_factor)(int clock);
+	const struct mtk_dpi_factor *dpi_factor;
+	const u8 num_dpi_factor;
 	u32 reg_h_fre_con;
 	u32 max_clock_khz;
 	bool edge_sel_en;
@@ -515,6 +522,20 @@ static int mtk_dpi_power_on(struct mtk_dpi *dpi)
 	return ret;
 }
 
+static unsigned int mtk_dpi_calculate_factor(struct mtk_dpi *dpi, int mode_clk)
+{
+	const struct mtk_dpi_factor *dpi_factor = dpi->conf->dpi_factor;
+	int i;
+
+	for (i = 0; i < dpi->conf->num_dpi_factor; i++) {
+		if (mode_clk <= dpi_factor[i].clock)
+			return dpi_factor[i].factor;
+	}
+
+	/* If no match try the lowest possible factor */
+	return dpi_factor[dpi->conf->num_dpi_factor - 1].factor;
+}
+
 static int mtk_dpi_set_display_mode(struct mtk_dpi *dpi,
 				    struct drm_display_mode *mode)
 {
@@ -529,7 +550,7 @@ static int mtk_dpi_set_display_mode(struct mtk_dpi *dpi,
 	unsigned int factor;
 
 	/* let pll_rate can fix the valid range of tvdpll (1G~2GHz) */
-	factor = dpi->conf->cal_factor(mode->clock);
+	factor = mtk_dpi_calculate_factor(dpi, mode_clk);
 	drm_display_mode_to_videomode(mode, &vm);
 	pll_rate = vm.pixelclock * factor;
 
@@ -964,48 +985,6 @@ static const struct component_ops mtk_dpi_component_ops = {
 	.unbind = mtk_dpi_unbind,
 };
 
-static unsigned int mt8173_calculate_factor(int clock)
-{
-	if (clock <= 27000)
-		return 3 << 4;
-	else if (clock <= 84000)
-		return 3 << 3;
-	else if (clock <= 167000)
-		return 3 << 2;
-	else
-		return 3 << 1;
-}
-
-static unsigned int mt2701_calculate_factor(int clock)
-{
-	if (clock <= 64000)
-		return 4;
-	else if (clock <= 128000)
-		return 2;
-	else
-		return 1;
-}
-
-static unsigned int mt8183_calculate_factor(int clock)
-{
-	if (clock <= 27000)
-		return 8;
-	else if (clock <= 167000)
-		return 4;
-	else
-		return 2;
-}
-
-static unsigned int mt8195_dpintf_calculate_factor(int clock)
-{
-	if (clock < 70000)
-		return 4;
-	else if (clock < 200000)
-		return 2;
-	else
-		return 1;
-}
-
 static const u32 mt8173_output_fmts[] = {
 	MEDIA_BUS_FMT_RGB888_1X24,
 };
@@ -1020,8 +999,25 @@ static const u32 mt8195_output_fmts[] = {
 	MEDIA_BUS_FMT_YUYV8_1X16,
 };
 
+static const struct mtk_dpi_factor dpi_factor_mt2701[] = {
+	{ 64000, 4 }, { 128000, 2 }, { U32_MAX, 1 }
+};
+
+static const struct mtk_dpi_factor dpi_factor_mt8173[] = {
+	{ 27000, 48 }, { 84000, 24 }, { 167000, 12 }, { U32_MAX, 6 }
+};
+
+static const struct mtk_dpi_factor dpi_factor_mt8183[] = {
+	{ 27000, 8 }, { 167000, 4 }, { U32_MAX, 2 }
+};
+
+static const struct mtk_dpi_factor dpi_factor_mt8195_dp_intf[] = {
+	{ 70000 - 1, 4 }, { 200000 - 1, 2 }, { U32_MAX, 1 }
+};
+
 static const struct mtk_dpi_conf mt8173_conf = {
-	.cal_factor = mt8173_calculate_factor,
+	.dpi_factor = dpi_factor_mt8173,
+	.num_dpi_factor = ARRAY_SIZE(dpi_factor_mt8173),
 	.reg_h_fre_con = 0xe0,
 	.max_clock_khz = 300000,
 	.output_fmts = mt8173_output_fmts,
@@ -1038,7 +1034,8 @@ static const struct mtk_dpi_conf mt8173_conf = {
 };
 
 static const struct mtk_dpi_conf mt2701_conf = {
-	.cal_factor = mt2701_calculate_factor,
+	.dpi_factor = dpi_factor_mt2701,
+	.num_dpi_factor = ARRAY_SIZE(dpi_factor_mt2701),
 	.reg_h_fre_con = 0xb0,
 	.edge_sel_en = true,
 	.max_clock_khz = 150000,
@@ -1056,7 +1053,8 @@ static const struct mtk_dpi_conf mt2701_conf = {
 };
 
 static const struct mtk_dpi_conf mt8183_conf = {
-	.cal_factor = mt8183_calculate_factor,
+	.dpi_factor = dpi_factor_mt8183,
+	.num_dpi_factor = ARRAY_SIZE(dpi_factor_mt8183),
 	.reg_h_fre_con = 0xe0,
 	.max_clock_khz = 100000,
 	.output_fmts = mt8183_output_fmts,
@@ -1073,7 +1071,8 @@ static const struct mtk_dpi_conf mt8183_conf = {
 };
 
 static const struct mtk_dpi_conf mt8186_conf = {
-	.cal_factor = mt8183_calculate_factor,
+	.dpi_factor = dpi_factor_mt8183,
+	.num_dpi_factor = ARRAY_SIZE(dpi_factor_mt8183),
 	.reg_h_fre_con = 0xe0,
 	.max_clock_khz = 150000,
 	.output_fmts = mt8183_output_fmts,
@@ -1091,7 +1090,8 @@ static const struct mtk_dpi_conf mt8186_conf = {
 };
 
 static const struct mtk_dpi_conf mt8192_conf = {
-	.cal_factor = mt8183_calculate_factor,
+	.dpi_factor = dpi_factor_mt8183,
+	.num_dpi_factor = ARRAY_SIZE(dpi_factor_mt8183),
 	.reg_h_fre_con = 0xe0,
 	.max_clock_khz = 150000,
 	.output_fmts = mt8183_output_fmts,
@@ -1108,7 +1108,8 @@ static const struct mtk_dpi_conf mt8192_conf = {
 };
 
 static const struct mtk_dpi_conf mt8195_dpintf_conf = {
-	.cal_factor = mt8195_dpintf_calculate_factor,
+	.dpi_factor = dpi_factor_mt8195_dp_intf,
+	.num_dpi_factor = ARRAY_SIZE(dpi_factor_mt8195_dp_intf),
 	.max_clock_khz = 600000,
 	.output_fmts = mt8195_output_fmts,
 	.num_output_fmts = ARRAY_SIZE(mt8195_output_fmts),
-- 
2.47.0

Re: [PATCH v1 3/6] drm/mediatek: mtk_dpi: Use an array for pixclk factor calculation

[CK Hu (胡俊光)]

Hi, Angelo:

On Wed, 2024-11-20 at 13:44 +0100, AngeloGioacchino Del Regno wrote:
> External email : Please do not click links or open attachments until you have verified the sender or the content.
> 
> 
> Setting the TVD PLL clock requires to multiply the target pixel
> clock by a specific constant factor to achieve the target PLL
> frequency, and this is done to reduce jitter to acceptable levels.
> 
> On all MediaTek SoCs, the factor is not retrieved by any real kind
> of calculation but rather by checking if the target pixel clock
> is less than a specified frequency, hence assigning a function
> pointer for just a bunch of if branches does enlarge the code
> size for little reason.
> 
> Remove all SoC-specific functions, add a structure `mtk_dpi_factor`
> that holds a clock frequency and corresponding PLL factor, and
> declare the constraints for each SoC in form of an array of said
> structure.
> Instead of function pointers, this structure (and its size) is then
> assigned to each SoC's platform data.
> 
> The "calculation" is then performed with a new static function
> mtk_dpi_calculate_factor(dpi, mode_clk) that iterates through all
> of the entries of the aforementioned array and returns the right
> factor.
> 
> If no factor is found, the lowest possible factor is returned,
> mimicking the same flow as all of the old per-SoC calculation
> functions.
> 
> This commit brings no functional change.

Reviewed-by: CK Hu <ck.hu@mediatek.com>

> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_dpi.c | 103 +++++++++++++++--------------
>  1 file changed, 52 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c b/drivers/gpu/drm/mediatek/mtk_dpi.c
> index c7143184e5de..9f59ee679ce1 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dpi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c
> @@ -117,9 +117,15 @@ struct mtk_dpi_yc_limit {
>         u16 c_bottom;
>  };
> 
> +struct mtk_dpi_factor {
> +       u32 clock;
> +       u8 factor;
> +};
> +
>  /**
>   * struct mtk_dpi_conf - Configuration of mediatek dpi.
> - * @cal_factor: Callback function to calculate factor value.
> + * @dpi_factor: SoC-specific pixel clock PLL factor values.
> + * @num_dpi_factor: Number of pixel clock PLL factor values.
>   * @reg_h_fre_con: Register address of frequency control.
>   * @max_clock_khz: Max clock frequency supported for this SoCs in khz units.
>   * @edge_sel_en: Enable of edge selection.
> @@ -140,7 +146,8 @@ struct mtk_dpi_yc_limit {
>   * @edge_cfg_in_mmsys: If the edge configuration for DPI's output needs to be set in MMSYS.
>   */
>  struct mtk_dpi_conf {
> -       unsigned int (*cal_factor)(int clock);
> +       const struct mtk_dpi_factor *dpi_factor;
> +       const u8 num_dpi_factor;
>         u32 reg_h_fre_con;
>         u32 max_clock_khz;
>         bool edge_sel_en;
> @@ -515,6 +522,20 @@ static int mtk_dpi_power_on(struct mtk_dpi *dpi)
>         return ret;
>  }
> 
> +static unsigned int mtk_dpi_calculate_factor(struct mtk_dpi *dpi, int mode_clk)
> +{
> +       const struct mtk_dpi_factor *dpi_factor = dpi->conf->dpi_factor;
> +       int i;
> +
> +       for (i = 0; i < dpi->conf->num_dpi_factor; i++) {
> +               if (mode_clk <= dpi_factor[i].clock)
> +                       return dpi_factor[i].factor;
> +       }
> +
> +       /* If no match try the lowest possible factor */
> +       return dpi_factor[dpi->conf->num_dpi_factor - 1].factor;
> +}
> +
>  static int mtk_dpi_set_display_mode(struct mtk_dpi *dpi,
>                                     struct drm_display_mode *mode)
>  {
> @@ -529,7 +550,7 @@ static int mtk_dpi_set_display_mode(struct mtk_dpi *dpi,
>         unsigned int factor;
> 
>         /* let pll_rate can fix the valid range of tvdpll (1G~2GHz) */
> -       factor = dpi->conf->cal_factor(mode->clock);
> +       factor = mtk_dpi_calculate_factor(dpi, mode_clk);
>         drm_display_mode_to_videomode(mode, &vm);
>         pll_rate = vm.pixelclock * factor;
> 
> @@ -964,48 +985,6 @@ static const struct component_ops mtk_dpi_component_ops = {
>         .unbind = mtk_dpi_unbind,
>  };
> 
> -static unsigned int mt8173_calculate_factor(int clock)
> -{
> -       if (clock <= 27000)
> -               return 3 << 4;
> -       else if (clock <= 84000)
> -               return 3 << 3;
> -       else if (clock <= 167000)
> -               return 3 << 2;
> -       else
> -               return 3 << 1;
> -}
> -
> -static unsigned int mt2701_calculate_factor(int clock)
> -{
> -       if (clock <= 64000)
> -               return 4;
> -       else if (clock <= 128000)
> -               return 2;
> -       else
> -               return 1;
> -}
> -
> -static unsigned int mt8183_calculate_factor(int clock)
> -{
> -       if (clock <= 27000)
> -               return 8;
> -       else if (clock <= 167000)
> -               return 4;
> -       else
> -               return 2;
> -}
> -
> -static unsigned int mt8195_dpintf_calculate_factor(int clock)
> -{
> -       if (clock < 70000)
> -               return 4;
> -       else if (clock < 200000)
> -               return 2;
> -       else
> -               return 1;
> -}
> -
>  static const u32 mt8173_output_fmts[] = {
>         MEDIA_BUS_FMT_RGB888_1X24,
>  };
> @@ -1020,8 +999,25 @@ static const u32 mt8195_output_fmts[] = {
>         MEDIA_BUS_FMT_YUYV8_1X16,
>  };
> 
> +static const struct mtk_dpi_factor dpi_factor_mt2701[] = {
> +       { 64000, 4 }, { 128000, 2 }, { U32_MAX, 1 }
> +};
> +
> +static const struct mtk_dpi_factor dpi_factor_mt8173[] = {
> +       { 27000, 48 }, { 84000, 24 }, { 167000, 12 }, { U32_MAX, 6 }
> +};
> +
> +static const struct mtk_dpi_factor dpi_factor_mt8183[] = {
> +       { 27000, 8 }, { 167000, 4 }, { U32_MAX, 2 }
> +};
> +
> +static const struct mtk_dpi_factor dpi_factor_mt8195_dp_intf[] = {
> +       { 70000 - 1, 4 }, { 200000 - 1, 2 }, { U32_MAX, 1 }
> +};
> +
>  static const struct mtk_dpi_conf mt8173_conf = {
> -       .cal_factor = mt8173_calculate_factor,
> +       .dpi_factor = dpi_factor_mt8173,
> +       .num_dpi_factor = ARRAY_SIZE(dpi_factor_mt8173),
>         .reg_h_fre_con = 0xe0,
>         .max_clock_khz = 300000,
>         .output_fmts = mt8173_output_fmts,
> @@ -1038,7 +1034,8 @@ static const struct mtk_dpi_conf mt8173_conf = {
>  };
> 
>  static const struct mtk_dpi_conf mt2701_conf = {
> -       .cal_factor = mt2701_calculate_factor,
> +       .dpi_factor = dpi_factor_mt2701,
> +       .num_dpi_factor = ARRAY_SIZE(dpi_factor_mt2701),
>         .reg_h_fre_con = 0xb0,
>         .edge_sel_en = true,
>         .max_clock_khz = 150000,
> @@ -1056,7 +1053,8 @@ static const struct mtk_dpi_conf mt2701_conf = {
>  };
> 
>  static const struct mtk_dpi_conf mt8183_conf = {
> -       .cal_factor = mt8183_calculate_factor,
> +       .dpi_factor = dpi_factor_mt8183,
> +       .num_dpi_factor = ARRAY_SIZE(dpi_factor_mt8183),
>         .reg_h_fre_con = 0xe0,
>         .max_clock_khz = 100000,
>         .output_fmts = mt8183_output_fmts,
> @@ -1073,7 +1071,8 @@ static const struct mtk_dpi_conf mt8183_conf = {
>  };
> 
>  static const struct mtk_dpi_conf mt8186_conf = {
> -       .cal_factor = mt8183_calculate_factor,
> +       .dpi_factor = dpi_factor_mt8183,
> +       .num_dpi_factor = ARRAY_SIZE(dpi_factor_mt8183),
>         .reg_h_fre_con = 0xe0,
>         .max_clock_khz = 150000,
>         .output_fmts = mt8183_output_fmts,
> @@ -1091,7 +1090,8 @@ static const struct mtk_dpi_conf mt8186_conf = {
>  };
> 
>  static const struct mtk_dpi_conf mt8192_conf = {
> -       .cal_factor = mt8183_calculate_factor,
> +       .dpi_factor = dpi_factor_mt8183,
> +       .num_dpi_factor = ARRAY_SIZE(dpi_factor_mt8183),
>         .reg_h_fre_con = 0xe0,
>         .max_clock_khz = 150000,
>         .output_fmts = mt8183_output_fmts,
> @@ -1108,7 +1108,8 @@ static const struct mtk_dpi_conf mt8192_conf = {
>  };
> 
>  static const struct mtk_dpi_conf mt8195_dpintf_conf = {
> -       .cal_factor = mt8195_dpintf_calculate_factor,
> +       .dpi_factor = dpi_factor_mt8195_dp_intf,
> +       .num_dpi_factor = ARRAY_SIZE(dpi_factor_mt8195_dp_intf),
>         .max_clock_khz = 600000,
>         .output_fmts = mt8195_output_fmts,
>         .num_output_fmts = ARRAY_SIZE(mt8195_output_fmts),
> --
> 2.47.0
> 

Re: [PATCH v1 3/6] drm/mediatek: mtk_dpi: Use an array for pixclk factor calculation

[CK Hu (胡俊光)]

Hi, Angelo:

On Fri, 2024-11-22 at 11:54 +0800, CK Hu wrote:
> Hi, Angelo:
> 
> On Wed, 2024-11-20 at 13:44 +0100, AngeloGioacchino Del Regno wrote:
> > External email : Please do not click links or open attachments until you have verified the sender or the content.
> > 
> > 
> > Setting the TVD PLL clock requires to multiply the target pixel
> > clock by a specific constant factor to achieve the target PLL
> > frequency, and this is done to reduce jitter to acceptable levels.
> > 
> > On all MediaTek SoCs, the factor is not retrieved by any real kind
> > of calculation but rather by checking if the target pixel clock
> > is less than a specified frequency, hence assigning a function
> > pointer for just a bunch of if branches does enlarge the code
> > size for little reason.
> > 
> > Remove all SoC-specific functions, add a structure `mtk_dpi_factor`
> > that holds a clock frequency and corresponding PLL factor, and
> > declare the constraints for each SoC in form of an array of said
> > structure.
> > Instead of function pointers, this structure (and its size) is then
> > assigned to each SoC's platform data.
> > 
> > The "calculation" is then performed with a new static function
> > mtk_dpi_calculate_factor(dpi, mode_clk) that iterates through all
> > of the entries of the aforementioned array and returns the right
> > factor.
> > 
> > If no factor is found, the lowest possible factor is returned,
> > mimicking the same flow as all of the old per-SoC calculation
> > functions.
> > 
> > This commit brings no functional change.
> 

[snip]

> >  static int mtk_dpi_set_display_mode(struct mtk_dpi *dpi,
> >                                     struct drm_display_mode *mode)
> >  {
> > @@ -529,7 +550,7 @@ static int mtk_dpi_set_display_mode(struct mtk_dpi *dpi,
> >         unsigned int factor;
> > 
> >         /* let pll_rate can fix the valid range of tvdpll (1G~2GHz) */
> > -       factor = dpi->conf->cal_factor(mode->clock);
> > +       factor = mtk_dpi_calculate_factor(dpi, mode_clk);

mode_clk is defined in next patch.
keep mode->clock in this patch to keep my reviewed-by tag.

Regards,
CK

> >         drm_display_mode_to_videomode(mode, &vm);
> >         pll_rate = vm.pixelclock * factor;
> > 
> > @@ -964,48 +985,6 @@ static const struct component_ops mtk_dpi_component_ops = {
> >         .unbind = mtk_dpi_unbind,
> >  };
> > 

Re: [PATCH v1 3/6] drm/mediatek: mtk_dpi: Use an array for pixclk factor calculation

[AngeloGioacchino Del Regno]

Il 22/11/24 07:23, CK Hu (胡俊光) ha scritto:
> Hi, Angelo:
> 
> On Fri, 2024-11-22 at 11:54 +0800, CK Hu wrote:
>> Hi, Angelo:
>>
>> On Wed, 2024-11-20 at 13:44 +0100, AngeloGioacchino Del Regno wrote:
>>> External email : Please do not click links or open attachments until you have verified the sender or the content.
>>>
>>>
>>> Setting the TVD PLL clock requires to multiply the target pixel
>>> clock by a specific constant factor to achieve the target PLL
>>> frequency, and this is done to reduce jitter to acceptable levels.
>>>
>>> On all MediaTek SoCs, the factor is not retrieved by any real kind
>>> of calculation but rather by checking if the target pixel clock
>>> is less than a specified frequency, hence assigning a function
>>> pointer for just a bunch of if branches does enlarge the code
>>> size for little reason.
>>>
>>> Remove all SoC-specific functions, add a structure `mtk_dpi_factor`
>>> that holds a clock frequency and corresponding PLL factor, and
>>> declare the constraints for each SoC in form of an array of said
>>> structure.
>>> Instead of function pointers, this structure (and its size) is then
>>> assigned to each SoC's platform data.
>>>
>>> The "calculation" is then performed with a new static function
>>> mtk_dpi_calculate_factor(dpi, mode_clk) that iterates through all
>>> of the entries of the aforementioned array and returns the right
>>> factor.
>>>
>>> If no factor is found, the lowest possible factor is returned,
>>> mimicking the same flow as all of the old per-SoC calculation
>>> functions.
>>>
>>> This commit brings no functional change.
>>
> 
> [snip]
> 
>>>   static int mtk_dpi_set_display_mode(struct mtk_dpi *dpi,
>>>                                      struct drm_display_mode *mode)
>>>   {
>>> @@ -529,7 +550,7 @@ static int mtk_dpi_set_display_mode(struct mtk_dpi *dpi,
>>>          unsigned int factor;
>>>
>>>          /* let pll_rate can fix the valid range of tvdpll (1G~2GHz) */
>>> -       factor = dpi->conf->cal_factor(mode->clock);
>>> +       factor = mtk_dpi_calculate_factor(dpi, mode_clk);
> 
> mode_clk is defined in next patch.
> keep mode->clock in this patch to keep my reviewed-by tag.
> 

Oh! Nice catch!
That happened during the final cleanup, heh :-)

Thanks btw, I will fix that in v2.

Cheers,
Angelo

> Regards,
> CK
> 
>>>          drm_display_mode_to_videomode(mode, &vm);
>>>          pll_rate = vm.pixelclock * factor;
>>>
>>> @@ -964,48 +985,6 @@ static const struct component_ops mtk_dpi_component_ops = {
>>>          .unbind = mtk_dpi_unbind,
>>>   };
>>>
> 
> 

[PATCH v1 4/6] drm/mediatek: mtk_dpi: Move pixel clock setting flow to function

[AngeloGioacchino Del Regno]

In preparation for adding support for the DPI IP found in MT8195
and in MT8188 used for HDMI, move the code flow for calculation
and setting of the DPI pixel clock to a separate function called
mtk_dpi_set_pixel_clk().

This was done because, on those platforms, the DPI instance that
is used for HDMI will get its pixel clock from the HDMI clock,
hence it is not necessary, nor desirable, to calculate or set
the pixel clock in DPI.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/gpu/drm/mediatek/mtk_dpi.c | 43 +++++++++++++++++-------------
 1 file changed, 24 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c b/drivers/gpu/drm/mediatek/mtk_dpi.c
index 9f59ee679ce1..378b49b6bdfb 100644
--- a/drivers/gpu/drm/mediatek/mtk_dpi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dpi.c
@@ -536,26 +536,17 @@ static unsigned int mtk_dpi_calculate_factor(struct mtk_dpi *dpi, int mode_clk)
 	return dpi_factor[dpi->conf->num_dpi_factor - 1].factor;
 }
 
-static int mtk_dpi_set_display_mode(struct mtk_dpi *dpi,
-				    struct drm_display_mode *mode)
+static void mtk_dpi_set_pixel_clk(struct mtk_dpi *dpi, struct videomode *vm, int mode_clk)
 {
-	struct mtk_dpi_polarities dpi_pol;
-	struct mtk_dpi_sync_param hsync;
-	struct mtk_dpi_sync_param vsync_lodd = { 0 };
-	struct mtk_dpi_sync_param vsync_leven = { 0 };
-	struct mtk_dpi_sync_param vsync_rodd = { 0 };
-	struct mtk_dpi_sync_param vsync_reven = { 0 };
-	struct videomode vm = { 0 };
 	unsigned long pll_rate;
 	unsigned int factor;
 
 	/* let pll_rate can fix the valid range of tvdpll (1G~2GHz) */
 	factor = mtk_dpi_calculate_factor(dpi, mode_clk);
-	drm_display_mode_to_videomode(mode, &vm);
-	pll_rate = vm.pixelclock * factor;
+	pll_rate = vm->pixelclock * factor;
 
 	dev_dbg(dpi->dev, "Want PLL %lu Hz, pixel clock %lu Hz\n",
-		pll_rate, vm.pixelclock);
+		pll_rate, vm->pixelclock);
 
 	clk_set_rate(dpi->tvd_clk, pll_rate);
 	pll_rate = clk_get_rate(dpi->tvd_clk);
@@ -565,20 +556,34 @@ static int mtk_dpi_set_display_mode(struct mtk_dpi *dpi,
 	 * pixels for each iteration: divide the clock by this number and
 	 * adjust the display porches accordingly.
 	 */
-	vm.pixelclock = pll_rate / factor;
-	vm.pixelclock /= dpi->conf->pixels_per_iter;
+	vm->pixelclock = pll_rate / factor;
+	vm->pixelclock /= dpi->conf->pixels_per_iter;
 
 	if ((dpi->output_fmt == MEDIA_BUS_FMT_RGB888_2X12_LE) ||
 	    (dpi->output_fmt == MEDIA_BUS_FMT_RGB888_2X12_BE))
-		clk_set_rate(dpi->pixel_clk, vm.pixelclock * 2);
+		clk_set_rate(dpi->pixel_clk, vm->pixelclock * 2);
 	else
-		clk_set_rate(dpi->pixel_clk, vm.pixelclock);
+		clk_set_rate(dpi->pixel_clk, vm->pixelclock);
 
-
-	vm.pixelclock = clk_get_rate(dpi->pixel_clk);
+	vm->pixelclock = clk_get_rate(dpi->pixel_clk);
 
 	dev_dbg(dpi->dev, "Got  PLL %lu Hz, pixel clock %lu Hz\n",
-		pll_rate, vm.pixelclock);
+		pll_rate, vm->pixelclock);
+}
+
+static int mtk_dpi_set_display_mode(struct mtk_dpi *dpi,
+				    struct drm_display_mode *mode)
+{
+	struct mtk_dpi_polarities dpi_pol;
+	struct mtk_dpi_sync_param hsync;
+	struct mtk_dpi_sync_param vsync_lodd = { 0 };
+	struct mtk_dpi_sync_param vsync_leven = { 0 };
+	struct mtk_dpi_sync_param vsync_rodd = { 0 };
+	struct mtk_dpi_sync_param vsync_reven = { 0 };
+	struct videomode vm = { 0 };
+
+	drm_display_mode_to_videomode(mode, &vm);
+	mtk_dpi_set_pixel_clk(dpi, &vm, mode->clock);
 
 	dpi_pol.ck_pol = MTK_DPI_POLARITY_FALLING;
 	dpi_pol.de_pol = MTK_DPI_POLARITY_RISING;
-- 
2.47.0

[PATCH v1 5/6] drm/mediatek: mtk_dpi: Add checks for reg_h_fre_con existence

[AngeloGioacchino Del Regno]

In preparation for adding support for newer DPI instances which
do support direct-pin but do not have any H_FRE_CON register,
like the one found in MT8195 and MT8188, add a branch to check
if the reg_h_fre_con variable was declared in the mtk_dpi_conf
structure for the probed SoC DPI version.

As a note, this is useful specifically only for cases in which
the support_direct_pin variable is true, so mt8195-dpintf is
not affected by any issue.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/gpu/drm/mediatek/mtk_dpi.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c b/drivers/gpu/drm/mediatek/mtk_dpi.c
index 378b49b6bdfb..79923d1bfbc9 100644
--- a/drivers/gpu/drm/mediatek/mtk_dpi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dpi.c
@@ -430,12 +430,13 @@ static void mtk_dpi_config_swap_input(struct mtk_dpi *dpi, bool enable)
 
 static void mtk_dpi_config_2n_h_fre(struct mtk_dpi *dpi)
 {
-	mtk_dpi_mask(dpi, dpi->conf->reg_h_fre_con, H_FRE_2N, H_FRE_2N);
+	if (dpi->conf->reg_h_fre_con)
+		mtk_dpi_mask(dpi, dpi->conf->reg_h_fre_con, H_FRE_2N, H_FRE_2N);
 }
 
 static void mtk_dpi_config_disable_edge(struct mtk_dpi *dpi)
 {
-	if (dpi->conf->edge_sel_en)
+	if (dpi->conf->edge_sel_en && dpi->conf->reg_h_fre_con)
 		mtk_dpi_mask(dpi, dpi->conf->reg_h_fre_con, 0, EDGE_SEL_EN);
 }
 
-- 
2.47.0

Re: [PATCH v1 5/6] drm/mediatek: mtk_dpi: Add checks for reg_h_fre_con existence

[CK Hu (胡俊光)]

Hi, Angelo:

On Wed, 2024-11-20 at 13:44 +0100, AngeloGioacchino Del Regno wrote:
> External email : Please do not click links or open attachments until you have verified the sender or the content.
> 
> 
> In preparation for adding support for newer DPI instances which
> do support direct-pin but do not have any H_FRE_CON register,
> like the one found in MT8195 and MT8188, add a branch to check
> if the reg_h_fre_con variable was declared in the mtk_dpi_conf
> structure for the probed SoC DPI version.
> 
> As a note, this is useful specifically only for cases in which
> the support_direct_pin variable is true, so mt8195-dpintf is
> not affected by any issue.

Reviewed-by: CK Hu <ck.hu@mediatek.com>

> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_dpi.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c b/drivers/gpu/drm/mediatek/mtk_dpi.c
> index 378b49b6bdfb..79923d1bfbc9 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dpi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c
> @@ -430,12 +430,13 @@ static void mtk_dpi_config_swap_input(struct mtk_dpi *dpi, bool enable)
> 
>  static void mtk_dpi_config_2n_h_fre(struct mtk_dpi *dpi)
>  {
> -       mtk_dpi_mask(dpi, dpi->conf->reg_h_fre_con, H_FRE_2N, H_FRE_2N);
> +       if (dpi->conf->reg_h_fre_con)
> +               mtk_dpi_mask(dpi, dpi->conf->reg_h_fre_con, H_FRE_2N, H_FRE_2N);
>  }
> 
>  static void mtk_dpi_config_disable_edge(struct mtk_dpi *dpi)
>  {
> -       if (dpi->conf->edge_sel_en)
> +       if (dpi->conf->edge_sel_en && dpi->conf->reg_h_fre_con)
>                 mtk_dpi_mask(dpi, dpi->conf->reg_h_fre_con, 0, EDGE_SEL_EN);
>  }
> 
> --
> 2.47.0
> 

[PATCH v1 6/6] drm/mediatek: Add support for MT8195 Digital Parallel Interface

[AngeloGioacchino Del Regno]

Add support for the DPI block found in the MT8195 and MT8188 SoCs.
Inside of the SoC, this block is directly connected to the HDMI IP.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/gpu/drm/mediatek/mtk_dpi.c      | 55 ++++++++++++++++++++++---
 drivers/gpu/drm/mediatek/mtk_dpi_regs.h |  6 +++
 drivers/gpu/drm/mediatek/mtk_drm_drv.c  |  2 +
 3 files changed, 58 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c b/drivers/gpu/drm/mediatek/mtk_dpi.c
index 79923d1bfbc9..f76d1c5b68bd 100644
--- a/drivers/gpu/drm/mediatek/mtk_dpi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dpi.c
@@ -144,6 +144,7 @@ struct mtk_dpi_factor {
  * @csc_enable_bit: Enable bit of CSC.
  * @pixels_per_iter: Quantity of transferred pixels per iteration.
  * @edge_cfg_in_mmsys: If the edge configuration for DPI's output needs to be set in MMSYS.
+ * @is_internal_hdmi: Specifies whether the DPI is internally connected to the HDMI block
  */
 struct mtk_dpi_conf {
 	const struct mtk_dpi_factor *dpi_factor;
@@ -164,6 +165,7 @@ struct mtk_dpi_conf {
 	u32 csc_enable_bit;
 	u32 pixels_per_iter;
 	bool edge_cfg_in_mmsys;
+	bool is_internal_hdmi;
 };
 
 static void mtk_dpi_mask(struct mtk_dpi *dpi, u32 offset, u32 val, u32 mask)
@@ -492,6 +494,7 @@ static void mtk_dpi_power_off(struct mtk_dpi *dpi)
 
 	mtk_dpi_disable(dpi);
 	clk_disable_unprepare(dpi->pixel_clk);
+	clk_disable_unprepare(dpi->tvd_clk);
 	clk_disable_unprepare(dpi->engine_clk);
 }
 
@@ -508,6 +511,12 @@ static int mtk_dpi_power_on(struct mtk_dpi *dpi)
 		goto err_refcount;
 	}
 
+	ret = clk_prepare_enable(dpi->tvd_clk);
+	if (ret) {
+		dev_err(dpi->dev, "Failed to enable tvd pll: %d\n", ret);
+		goto err_engine;
+	}
+
 	ret = clk_prepare_enable(dpi->pixel_clk);
 	if (ret) {
 		dev_err(dpi->dev, "Failed to enable pixel clock: %d\n", ret);
@@ -517,6 +526,8 @@ static int mtk_dpi_power_on(struct mtk_dpi *dpi)
 	return 0;
 
 err_pixel:
+	clk_disable_unprepare(dpi->tvd_clk);
+err_engine:
 	clk_disable_unprepare(dpi->engine_clk);
 err_refcount:
 	dpi->refcount--;
@@ -584,7 +595,9 @@ static int mtk_dpi_set_display_mode(struct mtk_dpi *dpi,
 	struct videomode vm = { 0 };
 
 	drm_display_mode_to_videomode(mode, &vm);
-	mtk_dpi_set_pixel_clk(dpi, &vm, mode->clock);
+
+	if (!dpi->conf->is_internal_hdmi)
+		mtk_dpi_set_pixel_clk(dpi, &vm, mode->clock);
 
 	dpi_pol.ck_pol = MTK_DPI_POLARITY_FALLING;
 	dpi_pol.de_pol = MTK_DPI_POLARITY_RISING;
@@ -647,10 +660,18 @@ static int mtk_dpi_set_display_mode(struct mtk_dpi *dpi,
 	if (dpi->conf->support_direct_pin) {
 		mtk_dpi_config_yc_map(dpi, dpi->yc_map);
 		mtk_dpi_config_2n_h_fre(dpi);
-		mtk_dpi_dual_edge(dpi);
+		/* DPI can connect to either an external bridge or the internal HDMI encoder */
+		if (dpi->conf->is_internal_hdmi) {
+			mtk_dpi_mask(dpi, DPI_CON, DPI_OUTPUT_1T1P_EN, DPI_OUTPUT_1T1P_EN);
+			mtk_dpi_mask(dpi, DPI_CON,
+				     dpi->conf->input_2pixel ? DPI_INPUT_2P_EN : 0,
+				     DPI_INPUT_2P_EN);
+		} else {
+			mtk_dpi_dual_edge(dpi);
+		}
 		mtk_dpi_config_disable_edge(dpi);
 	}
-	if (dpi->conf->input_2pixel) {
+	if (dpi->conf->input_2pixel && !dpi->conf->is_internal_hdmi) {
 		mtk_dpi_mask(dpi, DPI_CON, DPINTF_INPUT_2P_EN,
 			     DPINTF_INPUT_2P_EN);
 	}
@@ -919,14 +940,16 @@ void mtk_dpi_start(struct device *dev)
 {
 	struct mtk_dpi *dpi = dev_get_drvdata(dev);
 
-	mtk_dpi_power_on(dpi);
+	if (!dpi->conf->is_internal_hdmi)
+		mtk_dpi_power_on(dpi);
 }
 
 void mtk_dpi_stop(struct device *dev)
 {
 	struct mtk_dpi *dpi = dev_get_drvdata(dev);
 
-	mtk_dpi_power_off(dpi);
+	if (!dpi->conf->is_internal_hdmi)
+		mtk_dpi_power_off(dpi);
 }
 
 unsigned int mtk_dpi_encoder_index(struct device *dev)
@@ -1021,6 +1044,8 @@ static const struct mtk_dpi_factor dpi_factor_mt8195_dp_intf[] = {
 	{ 70000 - 1, 4 }, { 200000 - 1, 2 }, { U32_MAX, 1 }
 };
 
+static const struct mtk_dpi_factor dpi_factor_mt8195_dpi = { U32_MAX, 1 };
+
 static const struct mtk_dpi_conf mt8173_conf = {
 	.dpi_factor = dpi_factor_mt8173,
 	.num_dpi_factor = ARRAY_SIZE(dpi_factor_mt8173),
@@ -1113,6 +1138,25 @@ static const struct mtk_dpi_conf mt8192_conf = {
 	.csc_enable_bit = CSC_ENABLE,
 };
 
+static const struct mtk_dpi_conf mt8195_conf = {
+	.dpi_factor = &dpi_factor_mt8195_dpi,
+	.num_dpi_factor = 1,
+	.max_clock_khz = 594000,
+	.output_fmts = mt8183_output_fmts,
+	.num_output_fmts = ARRAY_SIZE(mt8183_output_fmts),
+	.pixels_per_iter = 1,
+	.is_ck_de_pol = true,
+	.swap_input_support = true,
+	.support_direct_pin = true,
+	.dimension_mask = HPW_MASK,
+	.hvsize_mask = HSIZE_MASK,
+	.channel_swap_shift = CH_SWAP,
+	.yuv422_en_bit = YUV422_EN,
+	.csc_enable_bit = CSC_ENABLE,
+	.is_internal_hdmi = true,
+	.input_2pixel = true,
+};
+
 static const struct mtk_dpi_conf mt8195_dpintf_conf = {
 	.dpi_factor = dpi_factor_mt8195_dp_intf,
 	.num_dpi_factor = ARRAY_SIZE(dpi_factor_mt8195_dp_intf),
@@ -1216,6 +1260,7 @@ static const struct of_device_id mtk_dpi_of_ids[] = {
 	{ .compatible = "mediatek,mt8188-dp-intf", .data = &mt8195_dpintf_conf },
 	{ .compatible = "mediatek,mt8192-dpi", .data = &mt8192_conf },
 	{ .compatible = "mediatek,mt8195-dp-intf", .data = &mt8195_dpintf_conf },
+	{ .compatible = "mediatek,mt8195-dpi", .data = &mt8195_conf },
 	{ /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, mtk_dpi_of_ids);
diff --git a/drivers/gpu/drm/mediatek/mtk_dpi_regs.h b/drivers/gpu/drm/mediatek/mtk_dpi_regs.h
index a0b1d18bbbf7..3c24d9e9f241 100644
--- a/drivers/gpu/drm/mediatek/mtk_dpi_regs.h
+++ b/drivers/gpu/drm/mediatek/mtk_dpi_regs.h
@@ -40,6 +40,12 @@
 #define FAKE_DE_LEVEN			BIT(21)
 #define FAKE_DE_RODD			BIT(22)
 #define FAKE_DE_REVEN			BIT(23)
+
+/* DPI_CON: DPI instances */
+#define DPI_OUTPUT_1T1P_EN		BIT(24)
+#define DPI_INPUT_2P_EN			BIT(25)
+
+/* DPI_CON: DPINTF instances */
 #define DPINTF_YUV422_EN		BIT(24)
 #define DPINTF_CSC_ENABLE		BIT(26)
 #define DPINTF_INPUT_2P_EN		BIT(29)
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
index 85be035a209a..a5974ce04359 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
@@ -808,6 +808,8 @@ static const struct of_device_id mtk_ddp_comp_dt_ids[] = {
 	  .data = (void *)MTK_DPI },
 	{ .compatible = "mediatek,mt8195-dp-intf",
 	  .data = (void *)MTK_DP_INTF },
+	{ .compatible = "mediatek,mt8195-dpi",
+	  .data = (void *)MTK_DPI },
 	{ .compatible = "mediatek,mt2701-dsi",
 	  .data = (void *)MTK_DSI },
 	{ .compatible = "mediatek,mt8173-dsi",
-- 
2.47.0

Re: [PATCH v1 6/6] drm/mediatek: Add support for MT8195 Digital Parallel Interface

[CK Hu (胡俊光)]

Hi, Angelo:

On Wed, 2024-11-20 at 13:44 +0100, AngeloGioacchino Del Regno wrote:
> External email : Please do not click links or open attachments until you have verified the sender or the content.
> 
> 
> Add support for the DPI block found in the MT8195 and MT8188 SoCs.
> Inside of the SoC, this block is directly connected to the HDMI IP.

In MT8173, DPI0 is directly connected to HDMI.
The first version of this driver is just for MT8173 DPI0.
Does MT8173 DPI0 need this modification?
Or this modification is just for MT8188 and MT8195, then the description should be more than 'directly connected'.

Regards,
CK

> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---

Re: [PATCH v1 6/6] drm/mediatek: Add support for MT8195 Digital Parallel Interface

[AngeloGioacchino Del Regno]

Il 22/11/24 08:23, CK Hu (胡俊光) ha scritto:
> Hi, Angelo:
> 
> On Wed, 2024-11-20 at 13:44 +0100, AngeloGioacchino Del Regno wrote:
>> External email : Please do not click links or open attachments until you have verified the sender or the content.
>>
>>
>> Add support for the DPI block found in the MT8195 and MT8188 SoCs.
>> Inside of the SoC, this block is directly connected to the HDMI IP.
> 
> In MT8173, DPI0 is directly connected to HDMI.
> The first version of this driver is just for MT8173 DPI0.
> Does MT8173 DPI0 need this modification?
> Or this modification is just for MT8188 and MT8195, then the description should be more than 'directly connected'.
> 

This is only for MT8188 and MT8195, and MT8173 does *not* need any modification.

Please, what would you like to see in the description of this commit?

Cheers,
Angelo

> Regards,
> CK
> 
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> ---

Re: [PATCH v1 6/6] drm/mediatek: Add support for MT8195 Digital Parallel Interface

[CK Hu (胡俊光)]

On Mon, 2024-11-25 at 17:55 +0100, AngeloGioacchino Del Regno wrote:
> External email : Please do not click links or open attachments until you have verified the sender or the content.
> 
> 
> Il 22/11/24 08:23, CK Hu (胡俊光) ha scritto:
> > Hi, Angelo:
> > 
> > On Wed, 2024-11-20 at 13:44 +0100, AngeloGioacchino Del Regno wrote:
> > > External email : Please do not click links or open attachments until you have verified the sender or the content.
> > > 
> > > 
> > > Add support for the DPI block found in the MT8195 and MT8188 SoCs.
> > > Inside of the SoC, this block is directly connected to the HDMI IP.
> > 
> > In MT8173, DPI0 is directly connected to HDMI.
> > The first version of this driver is just for MT8173 DPI0.
> > Does MT8173 DPI0 need this modification?
> > Or this modification is just for MT8188 and MT8195, then the description should be more than 'directly connected'.
> > 
> 
> This is only for MT8188 and MT8195, and MT8173 does *not* need any modification.
> 
> Please, what would you like to see in the description of this commit?

This patch does four jobs.

1. Enable/disable tvd_clk for MT8195/MT8188 DPI.
2. Do not set pixel clock for MT8195/MT8188 DPI.
3. New DPI_INPUT_XXX and DPI_OUTPUT_XXX control for MT8195/MT8188 DPI.
4. Do not power on/off for MT8195/MT8188 DPI.

Maybe you should break into 4 patches and each one has different reason.

For #1 and #2, I've not reviewed the HDMI driver. Is the clock control influenced by new HDMI driver.
If it is software reason, maybe we can modify the new HDMI driver and make DPI driver consistent with MT8173.
If it is hardware reason. just describe the hardware reason.

For #4, I don't know why DPI do not control power by its self?
Even though other driver may control the same power, power manager has reference count,
so each driver could control the same power by its self.

Regards,
CK


> 
> Cheers,
> Angelo
> 
> > Regards,
> > CK
> > 
> > > 
> > > Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> > > ---
> 
> 

Re: [PATCH v1 6/6] drm/mediatek: Add support for MT8195 Digital Parallel Interface

[AngeloGioacchino Del Regno]

Il 26/11/24 04:07, CK Hu (胡俊光) ha scritto:
> On Mon, 2024-11-25 at 17:55 +0100, AngeloGioacchino Del Regno wrote:
>> External email : Please do not click links or open attachments until you have verified the sender or the content.
>>
>>
>> Il 22/11/24 08:23, CK Hu (胡俊光) ha scritto:
>>> Hi, Angelo:
>>>
>>> On Wed, 2024-11-20 at 13:44 +0100, AngeloGioacchino Del Regno wrote:
>>>> External email : Please do not click links or open attachments until you have verified the sender or the content.
>>>>
>>>>
>>>> Add support for the DPI block found in the MT8195 and MT8188 SoCs.
>>>> Inside of the SoC, this block is directly connected to the HDMI IP.
>>>
>>> In MT8173, DPI0 is directly connected to HDMI.
>>> The first version of this driver is just for MT8173 DPI0.
>>> Does MT8173 DPI0 need this modification?
>>> Or this modification is just for MT8188 and MT8195, then the description should be more than 'directly connected'.
>>>
>>
>> This is only for MT8188 and MT8195, and MT8173 does *not* need any modification.
>>
>> Please, what would you like to see in the description of this commit?
> 
> This patch does four jobs.
> 
> 1. Enable/disable tvd_clk for MT8195/MT8188 DPI.
> 2. Do not set pixel clock for MT8195/MT8188 DPI.
> 3. New DPI_INPUT_XXX and DPI_OUTPUT_XXX control for MT8195/MT8188 DPI.
> 4. Do not power on/off for MT8195/MT8188 DPI.
> 
> Maybe you should break into 4 patches and each one has different reason.

Yeah I thought about that as well, but there's a fundamental issue with splitting
the thing in multiple patches...

For enabling the tvd_clk in a separate patch, there's no problem - however, for the
others....

1. We need to introduce support for MT8195/88 DPI-HDMI, or the other patches would
    not make sense (nor apply, anyway); then
2. We stop setting pixel clock with another patch; then
3. we don't power on/off, etc etc

The problem with doing it like so is that the patch #1 that I described would be
introducing *faulty code*, because the support for that really depends on all of
the others being present (otherwise the block won't work correctly).

So... if you want, I can easily split out the tvd_clk enable/disable, but splitting
the rest wouldn't be clean.

Besides, keep in mind that... actually... for anything else that is not MT8195/88
DPI0 (so, for other SoCs' DPI and for 95/88 DPINTF) the tvd_clk is already getting
enabled by its child.. so, for those ones, a call to enable tvd_clk does exactly
nothing apart from incrementing (enable) or decrementing (disable) the refcount for
this clock by 1.

This means that the enablement/disablement of tvd_clk is actually important only
for the MT8195/88 DPI and has literally no effect on anything else that is
currently supported by the mtk_dpi driver anyway.

Still - if you want me to split out the tvd_clk en/dis patch, just confirm and I
will split that one out...

> 
> For #1 and #2, I've not reviewed the HDMI driver. Is the clock control influenced by new HDMI driver.

It kinda is - the HDMI-TX block gets its clock from the HDMI PHY's clock gen,
but eventually it is the HDMI driver that tells to the PHY driver what clock it
actually wants.

For #1, clk_prepare_enable() is ungating the clock that would otherwise gate the
PHY's PLL output to the HDMI block.

> If it is software reason, maybe we can modify the new HDMI driver and make DPI driver consistent with MT8173.
> If it is hardware reason. just describe the hardware reason.

Alright - the hardware reason is that the HDMIPHY generates the clock for the HDMI
TX block, and that enabling the clock assigned to tvd_clk is necessary to ungate
the PHY's ckgen output to the HDMI-TX (and I think - but not sure as I haven't
analyzed that yet - that HDMI-RX should have the same gating technique, but that's
definitely out of scope for this submission).

> 
> For #4, I don't know why DPI do not control power by its self?
> Even though other driver may control the same power, power manager has reference count,
> so each driver could control the same power by its self.

#4 is there both for a SW and for a HW reason.

The HW reason is that the DPI shall be powered on in a specific sequence in regard
to HDMI-TX, due to the setup that is required by both (and ungating clocks before
full configuration happens would lock up the hw block).

The SW reason is that mtk_crtc.c calls mtk_crtc_ddp_hw_init()->mtk_ddp_comp_start()
in its .atomic_enable() callback, which happens in the wrong sequence in regard to
HDMI because of the "natural" components order in the DRM framework (for MT8195/88!
because for the others it either is the inverse or it does not matter - so for
performance it's okay for it to be like that both on older SoCs and on DPINTF for
95/88) and this means that we *must not* call dpi_power_on() at that time but
we must rather follow the atomic_enable()/bridge_enable() order imposed by DRM
*also* for the clock en/dis calls in DPI.

Cheers,
Angelo

> 
> Regards,
> CK
> 
> 
>>
>> Cheers,
>> Angelo
>>
>>> Regards,
>>> CK
>>>
>>>>
>>>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>>>> ---
>>
>>

Re: [PATCH v1 6/6] drm/mediatek: Add support for MT8195 Digital Parallel Interface

[CK Hu (胡俊光)]

On Tue, 2024-11-26 at 10:25 +0100, AngeloGioacchino Del Regno wrote:
> External email : Please do not click links or open attachments until you have verified the sender or the content.
> 
> 
> Il 26/11/24 04:07, CK Hu (胡俊光) ha scritto:
> > On Mon, 2024-11-25 at 17:55 +0100, AngeloGioacchino Del Regno wrote:
> > > External email : Please do not click links or open attachments until you have verified the sender or the content.
> > > 
> > > 
> > > Il 22/11/24 08:23, CK Hu (胡俊光) ha scritto:
> > > > Hi, Angelo:
> > > > 
> > > > On Wed, 2024-11-20 at 13:44 +0100, AngeloGioacchino Del Regno wrote:
> > > > > External email : Please do not click links or open attachments until you have verified the sender or the content.
> > > > > 
> > > > > 
> > > > > Add support for the DPI block found in the MT8195 and MT8188 SoCs.
> > > > > Inside of the SoC, this block is directly connected to the HDMI IP.
> > > > 
> > > > In MT8173, DPI0 is directly connected to HDMI.
> > > > The first version of this driver is just for MT8173 DPI0.
> > > > Does MT8173 DPI0 need this modification?
> > > > Or this modification is just for MT8188 and MT8195, then the description should be more than 'directly connected'.
> > > > 
> > > 
> > > This is only for MT8188 and MT8195, and MT8173 does *not* need any modification.
> > > 
> > > Please, what would you like to see in the description of this commit?
> > 
> > This patch does four jobs.
> > 
> > 1. Enable/disable tvd_clk for MT8195/MT8188 DPI.
> > 2. Do not set pixel clock for MT8195/MT8188 DPI.
> > 3. New DPI_INPUT_XXX and DPI_OUTPUT_XXX control for MT8195/MT8188 DPI.
> > 4. Do not power on/off for MT8195/MT8188 DPI.
> > 
> > Maybe you should break into 4 patches and each one has different reason.
> 
> Yeah I thought about that as well, but there's a fundamental issue with splitting
> the thing in multiple patches...
> 
> For enabling the tvd_clk in a separate patch, there's no problem - however, for the
> others....
> 
> 1. We need to introduce support for MT8195/88 DPI-HDMI, or the other patches would
>     not make sense (nor apply, anyway); then
> 2. We stop setting pixel clock with another patch; then
> 3. we don't power on/off, etc etc
> 
> The problem with doing it like so is that the patch #1 that I described would be
> introducing *faulty code*, because the support for that really depends on all of
> the others being present (otherwise the block won't work correctly).
> 
> So... if you want, I can easily split out the tvd_clk enable/disable, but splitting
> the rest wouldn't be clean.
> 
> Besides, keep in mind that... actually... for anything else that is not MT8195/88
> DPI0 (so, for other SoCs' DPI and for 95/88 DPINTF) the tvd_clk is already getting
> enabled by its child.. so, for those ones, a call to enable tvd_clk does exactly
> nothing apart from incrementing (enable) or decrementing (disable) the refcount for
> this clock by 1.
> 
> This means that the enablement/disablement of tvd_clk is actually important only
> for the MT8195/88 DPI and has literally no effect on anything else that is
> currently supported by the mtk_dpi driver anyway.
> 
> Still - if you want me to split out the tvd_clk en/dis patch, just confirm and I
> will split that one out...
> 
> > 
> > For #1 and #2, I've not reviewed the HDMI driver. Is the clock control influenced by new HDMI driver.
> 
> It kinda is - the HDMI-TX block gets its clock from the HDMI PHY's clock gen,
> but eventually it is the HDMI driver that tells to the PHY driver what clock it
> actually wants.
> 
> For #1, clk_prepare_enable() is ungating the clock that would otherwise gate the
> PHY's PLL output to the HDMI block.
> 
> > If it is software reason, maybe we can modify the new HDMI driver and make DPI driver consistent with MT8173.
> > If it is hardware reason. just describe the hardware reason.
> 
> Alright - the hardware reason is that the HDMIPHY generates the clock for the HDMI
> TX block, and that enabling the clock assigned to tvd_clk is necessary to ungate
> the PHY's ckgen output to the HDMI-TX (and I think - but not sure as I haven't
> analyzed that yet - that HDMI-RX should have the same gating technique, but that's
> definitely out of scope for this submission).

I think tvd_clk is the clock source of DPI, HDMI, and HDMI-PHY, so these hardware could work in the same frequency.
That means drivers of DPI, HDMI, and HDMI-PHY are equal to control tvd_clk.
In MT8173. software choose DPI driver to control tvd_clk.
In MT8195, software choose HDMI-PHY driver to control tvd_clk.

I would like to have the same control flow.
If "HDMI-PHY driver to control tvd_clk" is better, we could temporarily let MT8195 has different flow with MT8173.
So, is "HDMI-PHY driver to control tvd_clk" better?

> 
> > 
> > For #4, I don't know why DPI do not control power by its self?
> > Even though other driver may control the same power, power manager has reference count,
> > so each driver could control the same power by its self.
> 
> #4 is there both for a SW and for a HW reason.
> 
> The HW reason is that the DPI shall be powered on in a specific sequence in regard
> to HDMI-TX, due to the setup that is required by both (and ungating clocks before
> full configuration happens would lock up the hw block).
> 
> The SW reason is that mtk_crtc.c calls mtk_crtc_ddp_hw_init()->mtk_ddp_comp_start()
> in its .atomic_enable() callback, which happens in the wrong sequence in regard to
> HDMI because of the "natural" components order in the DRM framework (for MT8195/88!
> because for the others it either is the inverse or it does not matter - so for
> performance it's okay for it to be like that both on older SoCs and on DPINTF for
> 95/88) and this means that we *must not* call dpi_power_on() at that time but
> we must rather follow the atomic_enable()/bridge_enable() order imposed by DRM
> *also* for the clock en/dis calls in DPI.

It looks like the #4 could be a separate patch.
The commit message is what you describe here.
And

if (!dpi->conf->support_hdmi_power_sequence)
	mtk_dpi_power_on();

Regards,
CK

> 
> Cheers,
> Angelo
> 
> > 
> > Regards,
> > CK
> > 
> > 
> > > 
> > > Cheers,
> > > Angelo
> > > 
> > > > Regards,
> > > > CK
> > > > 
> > > > > 
> > > > > Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> > > > > ---
> > > 
> > > 
> 
> 
> 

Re: [PATCH v1 6/6] drm/mediatek: Add support for MT8195 Digital Parallel Interface

[AngeloGioacchino Del Regno]

Il 27/11/24 08:02, CK Hu (胡俊光) ha scritto:
> On Tue, 2024-11-26 at 10:25 +0100, AngeloGioacchino Del Regno wrote:
>> External email : Please do not click links or open attachments until you have verified the sender or the content.
>>
>>
>> Il 26/11/24 04:07, CK Hu (胡俊光) ha scritto:
>>> On Mon, 2024-11-25 at 17:55 +0100, AngeloGioacchino Del Regno wrote:
>>>> External email : Please do not click links or open attachments until you have verified the sender or the content.
>>>>
>>>>
>>>> Il 22/11/24 08:23, CK Hu (胡俊光) ha scritto:
>>>>> Hi, Angelo:
>>>>>
>>>>> On Wed, 2024-11-20 at 13:44 +0100, AngeloGioacchino Del Regno wrote:
>>>>>> External email : Please do not click links or open attachments until you have verified the sender or the content.
>>>>>>
>>>>>>
>>>>>> Add support for the DPI block found in the MT8195 and MT8188 SoCs.
>>>>>> Inside of the SoC, this block is directly connected to the HDMI IP.
>>>>>
>>>>> In MT8173, DPI0 is directly connected to HDMI.
>>>>> The first version of this driver is just for MT8173 DPI0.
>>>>> Does MT8173 DPI0 need this modification?
>>>>> Or this modification is just for MT8188 and MT8195, then the description should be more than 'directly connected'.
>>>>>
>>>>
>>>> This is only for MT8188 and MT8195, and MT8173 does *not* need any modification.
>>>>
>>>> Please, what would you like to see in the description of this commit?
>>>
>>> This patch does four jobs.
>>>
>>> 1. Enable/disable tvd_clk for MT8195/MT8188 DPI.
>>> 2. Do not set pixel clock for MT8195/MT8188 DPI.
>>> 3. New DPI_INPUT_XXX and DPI_OUTPUT_XXX control for MT8195/MT8188 DPI.
>>> 4. Do not power on/off for MT8195/MT8188 DPI.
>>>
>>> Maybe you should break into 4 patches and each one has different reason.
>>
>> Yeah I thought about that as well, but there's a fundamental issue with splitting
>> the thing in multiple patches...
>>
>> For enabling the tvd_clk in a separate patch, there's no problem - however, for the
>> others....
>>
>> 1. We need to introduce support for MT8195/88 DPI-HDMI, or the other patches would
>>      not make sense (nor apply, anyway); then
>> 2. We stop setting pixel clock with another patch; then
>> 3. we don't power on/off, etc etc
>>
>> The problem with doing it like so is that the patch #1 that I described would be
>> introducing *faulty code*, because the support for that really depends on all of
>> the others being present (otherwise the block won't work correctly).
>>
>> So... if you want, I can easily split out the tvd_clk enable/disable, but splitting
>> the rest wouldn't be clean.
>>
>> Besides, keep in mind that... actually... for anything else that is not MT8195/88
>> DPI0 (so, for other SoCs' DPI and for 95/88 DPINTF) the tvd_clk is already getting
>> enabled by its child.. so, for those ones, a call to enable tvd_clk does exactly
>> nothing apart from incrementing (enable) or decrementing (disable) the refcount for
>> this clock by 1.
>>
>> This means that the enablement/disablement of tvd_clk is actually important only
>> for the MT8195/88 DPI and has literally no effect on anything else that is
>> currently supported by the mtk_dpi driver anyway.
>>
>> Still - if you want me to split out the tvd_clk en/dis patch, just confirm and I
>> will split that one out...
>>
>>>
>>> For #1 and #2, I've not reviewed the HDMI driver. Is the clock control influenced by new HDMI driver.
>>
>> It kinda is - the HDMI-TX block gets its clock from the HDMI PHY's clock gen,
>> but eventually it is the HDMI driver that tells to the PHY driver what clock it
>> actually wants.
>>
>> For #1, clk_prepare_enable() is ungating the clock that would otherwise gate the
>> PHY's PLL output to the HDMI block.
>>
>>> If it is software reason, maybe we can modify the new HDMI driver and make DPI driver consistent with MT8173.
>>> If it is hardware reason. just describe the hardware reason.
>>
>> Alright - the hardware reason is that the HDMIPHY generates the clock for the HDMI
>> TX block, and that enabling the clock assigned to tvd_clk is necessary to ungate
>> the PHY's ckgen output to the HDMI-TX (and I think - but not sure as I haven't
>> analyzed that yet - that HDMI-RX should have the same gating technique, but that's
>> definitely out of scope for this submission).
> 
> I think tvd_clk is the clock source of DPI, HDMI, and HDMI-PHY, so these hardware could work in the same frequency.
> That means drivers of DPI, HDMI, and HDMI-PHY are equal to control tvd_clk.
> In MT8173. software choose DPI driver to control tvd_clk.
> In MT8195, software choose HDMI-PHY driver to control tvd_clk.

Yes, but in MT8195 the tvd is gated by a clock that is controller by the HDMI
driver only, and not by the PHY - so, PHY sets the frequency, mtk_hdmi_v2 ungates
that to the HDMITX block (with clk_prepare_enable(tvd_clk)).

> 
> I would like to have the same control flow.
> If "HDMI-PHY driver to control tvd_clk" is better, we could temporarily let MT8195 has different flow with MT8173.
> So, is "HDMI-PHY driver to control tvd_clk" better?
> 

I'm not sure I understand this last part, can you please rephrase?

>>
>>>
>>> For #4, I don't know why DPI do not control power by its self?
>>> Even though other driver may control the same power, power manager has reference count,
>>> so each driver could control the same power by its self.
>>
>> #4 is there both for a SW and for a HW reason.
>>
>> The HW reason is that the DPI shall be powered on in a specific sequence in regard
>> to HDMI-TX, due to the setup that is required by both (and ungating clocks before
>> full configuration happens would lock up the hw block).
>>
>> The SW reason is that mtk_crtc.c calls mtk_crtc_ddp_hw_init()->mtk_ddp_comp_start()
>> in its .atomic_enable() callback, which happens in the wrong sequence in regard to
>> HDMI because of the "natural" components order in the DRM framework (for MT8195/88!
>> because for the others it either is the inverse or it does not matter - so for
>> performance it's okay for it to be like that both on older SoCs and on DPINTF for
>> 95/88) and this means that we *must not* call dpi_power_on() at that time but
>> we must rather follow the atomic_enable()/bridge_enable() order imposed by DRM
>> *also* for the clock en/dis calls in DPI.
> 
> It looks like the #4 could be a separate patch.
> The commit message is what you describe here.
> And
> 
> if (!dpi->conf->support_hdmi_power_sequence)
> 	mtk_dpi_power_on();
> 

This means that I'd have to introduce the "hdmi power sequence" before actually
introducing the real support for MT8195 HDMI....
I honestly don't like that "too much", but it's fine, I don't have *too strong*
opinions about that, so I will separate #4 as you suggested for v2.

Cheers,
Angelo

> Regards,
> CK
> 
>>
>> Cheers,
>> Angelo
>>
>>>
>>> Regards,
>>> CK
>>>
>>>
>>>>
>>>> Cheers,
>>>> Angelo
>>>>
>>>>> Regards,
>>>>> CK
>>>>>
>>>>>>
>>>>>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>>>>>> ---
>>>>
>>>>
>>
>>
>>

Re: [PATCH v1 6/6] drm/mediatek: Add support for MT8195 Digital Parallel Interface

[CK Hu (胡俊光)]

On Wed, 2024-11-27 at 09:41 +0100, AngeloGioacchino Del Regno wrote:
> External email : Please do not click links or open attachments until you have verified the sender or the content.
> 
> 
> Il 27/11/24 08:02, CK Hu (胡俊光) ha scritto:
> > On Tue, 2024-11-26 at 10:25 +0100, AngeloGioacchino Del Regno wrote:
> > > External email : Please do not click links or open attachments until you have verified the sender or the content.
> > > 
> > > 
> > > Il 26/11/24 04:07, CK Hu (胡俊光) ha scritto:
> > > > On Mon, 2024-11-25 at 17:55 +0100, AngeloGioacchino Del Regno wrote:
> > > > > External email : Please do not click links or open attachments until you have verified the sender or the content.
> > > > > 
> > > > > 
> > > > > Il 22/11/24 08:23, CK Hu (胡俊光) ha scritto:
> > > > > > Hi, Angelo:
> > > > > > 
> > > > > > On Wed, 2024-11-20 at 13:44 +0100, AngeloGioacchino Del Regno wrote:
> > > > > > > External email : Please do not click links or open attachments until you have verified the sender or the content.
> > > > > > > 
> > > > > > > 
> > > > > > > Add support for the DPI block found in the MT8195 and MT8188 SoCs.
> > > > > > > Inside of the SoC, this block is directly connected to the HDMI IP.
> > > > > > 
> > > > > > In MT8173, DPI0 is directly connected to HDMI.
> > > > > > The first version of this driver is just for MT8173 DPI0.
> > > > > > Does MT8173 DPI0 need this modification?
> > > > > > Or this modification is just for MT8188 and MT8195, then the description should be more than 'directly connected'.
> > > > > > 
> > > > > 
> > > > > This is only for MT8188 and MT8195, and MT8173 does *not* need any modification.
> > > > > 
> > > > > Please, what would you like to see in the description of this commit?
> > > > 
> > > > This patch does four jobs.
> > > > 
> > > > 1. Enable/disable tvd_clk for MT8195/MT8188 DPI.
> > > > 2. Do not set pixel clock for MT8195/MT8188 DPI.
> > > > 3. New DPI_INPUT_XXX and DPI_OUTPUT_XXX control for MT8195/MT8188 DPI.
> > > > 4. Do not power on/off for MT8195/MT8188 DPI.
> > > > 
> > > > Maybe you should break into 4 patches and each one has different reason.
> > > 
> > > Yeah I thought about that as well, but there's a fundamental issue with splitting
> > > the thing in multiple patches...
> > > 
> > > For enabling the tvd_clk in a separate patch, there's no problem - however, for the
> > > others....
> > > 
> > > 1. We need to introduce support for MT8195/88 DPI-HDMI, or the other patches would
> > >      not make sense (nor apply, anyway); then
> > > 2. We stop setting pixel clock with another patch; then
> > > 3. we don't power on/off, etc etc
> > > 
> > > The problem with doing it like so is that the patch #1 that I described would be
> > > introducing *faulty code*, because the support for that really depends on all of
> > > the others being present (otherwise the block won't work correctly).
> > > 
> > > So... if you want, I can easily split out the tvd_clk enable/disable, but splitting
> > > the rest wouldn't be clean.
> > > 
> > > Besides, keep in mind that... actually... for anything else that is not MT8195/88
> > > DPI0 (so, for other SoCs' DPI and for 95/88 DPINTF) the tvd_clk is already getting
> > > enabled by its child.. so, for those ones, a call to enable tvd_clk does exactly
> > > nothing apart from incrementing (enable) or decrementing (disable) the refcount for
> > > this clock by 1.
> > > 
> > > This means that the enablement/disablement of tvd_clk is actually important only
> > > for the MT8195/88 DPI and has literally no effect on anything else that is
> > > currently supported by the mtk_dpi driver anyway.
> > > 
> > > Still - if you want me to split out the tvd_clk en/dis patch, just confirm and I
> > > will split that one out...
> > > 
> > > > 
> > > > For #1 and #2, I've not reviewed the HDMI driver. Is the clock control influenced by new HDMI driver.
> > > 
> > > It kinda is - the HDMI-TX block gets its clock from the HDMI PHY's clock gen,
> > > but eventually it is the HDMI driver that tells to the PHY driver what clock it
> > > actually wants.
> > > 
> > > For #1, clk_prepare_enable() is ungating the clock that would otherwise gate the
> > > PHY's PLL output to the HDMI block.
> > > 
> > > > If it is software reason, maybe we can modify the new HDMI driver and make DPI driver consistent with MT8173.
> > > > If it is hardware reason. just describe the hardware reason.
> > > 
> > > Alright - the hardware reason is that the HDMIPHY generates the clock for the HDMI
> > > TX block, and that enabling the clock assigned to tvd_clk is necessary to ungate
> > > the PHY's ckgen output to the HDMI-TX (and I think - but not sure as I haven't
> > > analyzed that yet - that HDMI-RX should have the same gating technique, but that's
> > > definitely out of scope for this submission).
> > 
> > I think tvd_clk is the clock source of DPI, HDMI, and HDMI-PHY, so these hardware could work in the same frequency.
> > That means drivers of DPI, HDMI, and HDMI-PHY are equal to control tvd_clk.
> > In MT8173. software choose DPI driver to control tvd_clk.
> > In MT8195, software choose HDMI-PHY driver to control tvd_clk.
> 
> Yes, but in MT8195 the tvd is gated by a clock that is controller by the HDMI
> driver only, and not by the PHY - so, PHY sets the frequency, mtk_hdmi_v2 ungates
> that to the HDMITX block (with clk_prepare_enable(tvd_clk)).
> 
> > 
> > I would like to have the same control flow.
> > If "HDMI-PHY driver to control tvd_clk" is better, we could temporarily let MT8195 has different flow with MT8173.
> > So, is "HDMI-PHY driver to control tvd_clk" better?
> > 
> 
> I'm not sure I understand this last part, can you please rephrase?

I would like MT8173 and MT8195 has the same control flow, so keep DPI driver to control tvd_clk in MT8195.
If it's better to control tvd_clk by HDMI-PHY driver, both MT8173 and MT8195 control tvd_clk by HDMI-PHY driver.
But we are not able to test MT8173. So MT8173 keep control tvd_clk by DPI driver.
So control tvd_clk by HDMI-PHY driver is better?

> 
> > > 
> > > > 
> > > > For #4, I don't know why DPI do not control power by its self?
> > > > Even though other driver may control the same power, power manager has reference count,
> > > > so each driver could control the same power by its self.
> > > 
> > > #4 is there both for a SW and for a HW reason.
> > > 
> > > The HW reason is that the DPI shall be powered on in a specific sequence in regard
> > > to HDMI-TX, due to the setup that is required by both (and ungating clocks before
> > > full configuration happens would lock up the hw block).
> > > 
> > > The SW reason is that mtk_crtc.c calls mtk_crtc_ddp_hw_init()->mtk_ddp_comp_start()
> > > in its .atomic_enable() callback, which happens in the wrong sequence in regard to
> > > HDMI because of the "natural" components order in the DRM framework (for MT8195/88!
> > > because for the others it either is the inverse or it does not matter - so for
> > > performance it's okay for it to be like that both on older SoCs and on DPINTF for
> > > 95/88) and this means that we *must not* call dpi_power_on() at that time but
> > > we must rather follow the atomic_enable()/bridge_enable() order imposed by DRM
> > > *also* for the clock en/dis calls in DPI.
> > 
> > It looks like the #4 could be a separate patch.
> > The commit message is what you describe here.
> > And
> > 
> > if (!dpi->conf->support_hdmi_power_sequence)
> >       mtk_dpi_power_on();
> > 
> 
> This means that I'd have to introduce the "hdmi power sequence" before actually
> introducing the real support for MT8195 HDMI....
> I honestly don't like that "too much", but it's fine, I don't have *too strong*
> opinions about that, so I will separate #4 as you suggested for v2.

This DPI series modification is all about HDMI.
Maybe merge this series with HDMI series and let the HDMI part in front of DPI part and it's more reasonable.

Regards,
CK

> 
> Cheers,
> Angelo
> 
> > Regards,
> > CK
> > 
> > > 
> > > Cheers,
> > > Angelo
> > > 
> > > > 
> > > > Regards,
> > > > CK
> > > > 
> > > > 
> > > > > 
> > > > > Cheers,
> > > > > Angelo
> > > > > 
> > > > > > Regards,
> > > > > > CK
> > > > > > 
> > > > > > > 
> > > > > > > Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> > > > > > > ---
> > > > > 
> > > > > 
> > > 
> > > 
> > > 
> 
> 
> 

Re: [PATCH v1 6/6] drm/mediatek: Add support for MT8195 Digital Parallel Interface

[AngeloGioacchino Del Regno]

Il 27/11/24 10:04, CK Hu (胡俊光) ha scritto:
> On Wed, 2024-11-27 at 09:41 +0100, AngeloGioacchino Del Regno wrote:
>> External email : Please do not click links or open attachments until you have verified the sender or the content.
>>
>>
>> Il 27/11/24 08:02, CK Hu (胡俊光) ha scritto:
>>> On Tue, 2024-11-26 at 10:25 +0100, AngeloGioacchino Del Regno wrote:
>>>> External email : Please do not click links or open attachments until you have verified the sender or the content.
>>>>
>>>>
>>>> Il 26/11/24 04:07, CK Hu (胡俊光) ha scritto:
>>>>> On Mon, 2024-11-25 at 17:55 +0100, AngeloGioacchino Del Regno wrote:
>>>>>> External email : Please do not click links or open attachments until you have verified the sender or the content.
>>>>>>
>>>>>>
>>>>>> Il 22/11/24 08:23, CK Hu (胡俊光) ha scritto:
>>>>>>> Hi, Angelo:
>>>>>>>
>>>>>>> On Wed, 2024-11-20 at 13:44 +0100, AngeloGioacchino Del Regno wrote:
>>>>>>>> External email : Please do not click links or open attachments until you have verified the sender or the content.
>>>>>>>>
>>>>>>>>
>>>>>>>> Add support for the DPI block found in the MT8195 and MT8188 SoCs.
>>>>>>>> Inside of the SoC, this block is directly connected to the HDMI IP.
>>>>>>>
>>>>>>> In MT8173, DPI0 is directly connected to HDMI.
>>>>>>> The first version of this driver is just for MT8173 DPI0.
>>>>>>> Does MT8173 DPI0 need this modification?
>>>>>>> Or this modification is just for MT8188 and MT8195, then the description should be more than 'directly connected'.
>>>>>>>
>>>>>>
>>>>>> This is only for MT8188 and MT8195, and MT8173 does *not* need any modification.
>>>>>>
>>>>>> Please, what would you like to see in the description of this commit?
>>>>>
>>>>> This patch does four jobs.
>>>>>
>>>>> 1. Enable/disable tvd_clk for MT8195/MT8188 DPI.
>>>>> 2. Do not set pixel clock for MT8195/MT8188 DPI.
>>>>> 3. New DPI_INPUT_XXX and DPI_OUTPUT_XXX control for MT8195/MT8188 DPI.
>>>>> 4. Do not power on/off for MT8195/MT8188 DPI.
>>>>>
>>>>> Maybe you should break into 4 patches and each one has different reason.
>>>>
>>>> Yeah I thought about that as well, but there's a fundamental issue with splitting
>>>> the thing in multiple patches...
>>>>
>>>> For enabling the tvd_clk in a separate patch, there's no problem - however, for the
>>>> others....
>>>>
>>>> 1. We need to introduce support for MT8195/88 DPI-HDMI, or the other patches would
>>>>       not make sense (nor apply, anyway); then
>>>> 2. We stop setting pixel clock with another patch; then
>>>> 3. we don't power on/off, etc etc
>>>>
>>>> The problem with doing it like so is that the patch #1 that I described would be
>>>> introducing *faulty code*, because the support for that really depends on all of
>>>> the others being present (otherwise the block won't work correctly).
>>>>
>>>> So... if you want, I can easily split out the tvd_clk enable/disable, but splitting
>>>> the rest wouldn't be clean.
>>>>
>>>> Besides, keep in mind that... actually... for anything else that is not MT8195/88
>>>> DPI0 (so, for other SoCs' DPI and for 95/88 DPINTF) the tvd_clk is already getting
>>>> enabled by its child.. so, for those ones, a call to enable tvd_clk does exactly
>>>> nothing apart from incrementing (enable) or decrementing (disable) the refcount for
>>>> this clock by 1.
>>>>
>>>> This means that the enablement/disablement of tvd_clk is actually important only
>>>> for the MT8195/88 DPI and has literally no effect on anything else that is
>>>> currently supported by the mtk_dpi driver anyway.
>>>>
>>>> Still - if you want me to split out the tvd_clk en/dis patch, just confirm and I
>>>> will split that one out...
>>>>
>>>>>
>>>>> For #1 and #2, I've not reviewed the HDMI driver. Is the clock control influenced by new HDMI driver.
>>>>
>>>> It kinda is - the HDMI-TX block gets its clock from the HDMI PHY's clock gen,
>>>> but eventually it is the HDMI driver that tells to the PHY driver what clock it
>>>> actually wants.
>>>>
>>>> For #1, clk_prepare_enable() is ungating the clock that would otherwise gate the
>>>> PHY's PLL output to the HDMI block.
>>>>
>>>>> If it is software reason, maybe we can modify the new HDMI driver and make DPI driver consistent with MT8173.
>>>>> If it is hardware reason. just describe the hardware reason.
>>>>
>>>> Alright - the hardware reason is that the HDMIPHY generates the clock for the HDMI
>>>> TX block, and that enabling the clock assigned to tvd_clk is necessary to ungate
>>>> the PHY's ckgen output to the HDMI-TX (and I think - but not sure as I haven't
>>>> analyzed that yet - that HDMI-RX should have the same gating technique, but that's
>>>> definitely out of scope for this submission).
>>>
>>> I think tvd_clk is the clock source of DPI, HDMI, and HDMI-PHY, so these hardware could work in the same frequency.
>>> That means drivers of DPI, HDMI, and HDMI-PHY are equal to control tvd_clk.
>>> In MT8173. software choose DPI driver to control tvd_clk.
>>> In MT8195, software choose HDMI-PHY driver to control tvd_clk.
>>
>> Yes, but in MT8195 the tvd is gated by a clock that is controller by the HDMI
>> driver only, and not by the PHY - so, PHY sets the frequency, mtk_hdmi_v2 ungates
>> that to the HDMITX block (with clk_prepare_enable(tvd_clk)).
>>
>>>
>>> I would like to have the same control flow.
>>> If "HDMI-PHY driver to control tvd_clk" is better, we could temporarily let MT8195 has different flow with MT8173.
>>> So, is "HDMI-PHY driver to control tvd_clk" better?
>>>
>>
>> I'm not sure I understand this last part, can you please rephrase?
> 
> I would like MT8173 and MT8195 has the same control flow, so keep DPI driver to control tvd_clk in MT8195.
> If it's better to control tvd_clk by HDMI-PHY driver, both MT8173 and MT8195 control tvd_clk by HDMI-PHY driver.
> But we are not able to test MT8173. So MT8173 keep control tvd_clk by DPI driver.
> So control tvd_clk by HDMI-PHY driver is better?
> 

Oh. Okay now I understand what you mean.

Unfortunately, we cannot control the tvd->hdmi gate from the PHY driver... this is
because we do really rely on a specific ungate sequence, and the DPI driver really
does need to control the gating of that CG on its own: when we want to bring up the
DPI+HDMI, we need to:
  - Start with *gated* clocks, so HW is OFF;
  - Call mtk_dpi_bridge_enable() (done by drm framework);
    - There, we now *ungate* the clocks
    - DPI HW is ON -> we reset and write config to DPI registers
      with function mtk_dpi_set_display_mode()
    - We enable the DPI output (set EN in DPI_EN register)

If we move the CG to HDMI PHY, then we have to phy_configure() and phy_enable()
inside of the DPI driver, which is also not really possible and can only be done
in the HDMI driver - and that's because the HDMI driver reads EDID from DDC,
which gives us the wanted pixel clock, and feeds it to the PHY.

In short, there's no way around that, the gating cannot be moved out of DPI driver.

>>
>>>>
>>>>>
>>>>> For #4, I don't know why DPI do not control power by its self?
>>>>> Even though other driver may control the same power, power manager has reference count,
>>>>> so each driver could control the same power by its self.
>>>>
>>>> #4 is there both for a SW and for a HW reason.
>>>>
>>>> The HW reason is that the DPI shall be powered on in a specific sequence in regard
>>>> to HDMI-TX, due to the setup that is required by both (and ungating clocks before
>>>> full configuration happens would lock up the hw block).
>>>>
>>>> The SW reason is that mtk_crtc.c calls mtk_crtc_ddp_hw_init()->mtk_ddp_comp_start()
>>>> in its .atomic_enable() callback, which happens in the wrong sequence in regard to
>>>> HDMI because of the "natural" components order in the DRM framework (for MT8195/88!
>>>> because for the others it either is the inverse or it does not matter - so for
>>>> performance it's okay for it to be like that both on older SoCs and on DPINTF for
>>>> 95/88) and this means that we *must not* call dpi_power_on() at that time but
>>>> we must rather follow the atomic_enable()/bridge_enable() order imposed by DRM
>>>> *also* for the clock en/dis calls in DPI.
>>>
>>> It looks like the #4 could be a separate patch.
>>> The commit message is what you describe here.
>>> And
>>>
>>> if (!dpi->conf->support_hdmi_power_sequence)
>>>        mtk_dpi_power_on();
>>>
>>
>> This means that I'd have to introduce the "hdmi power sequence" before actually
>> introducing the real support for MT8195 HDMI....
>> I honestly don't like that "too much", but it's fine, I don't have *too strong*
>> opinions about that, so I will separate #4 as you suggested for v2.
> 
> This DPI series modification is all about HDMI.
> Maybe merge this series with HDMI series and let the HDMI part in front of DPI part and it's more reasonable.
> 

I have sent the two separately only because I thought it'd be easier for you to
review them .. well, separately.
But .. yes, this series is 99% about HDMI - the only thing that's not related to
HDMI is patch [3/6] which is just a cleanup...

So if you want I can merge the two series into one, that's not a problem at all;
in that case, do you want me to keep the patches as they are, or do you want me
to still split #4 away from this patch?

> Regards,
> CK
> 
>>
>> Cheers,
>> Angelo
>>
>>> Regards,
>>> CK
>>>
>>>>
>>>> Cheers,
>>>> Angelo
>>>>
>>>>>
>>>>> Regards,
>>>>> CK
>>>>>
>>>>>
>>>>>>
>>>>>> Cheers,
>>>>>> Angelo
>>>>>>
>>>>>>> Regards,
>>>>>>> CK
>>>>>>>
>>>>>>>>
>>>>>>>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>>>>>>>> ---
>>>>>>
>>>>>>
>>>>
>>>>
>>>>
>>
>>
>>

Re: [PATCH v1 6/6] drm/mediatek: Add support for MT8195 Digital Parallel Interface

[CK Hu (胡俊光)]

On Wed, 2024-11-27 at 13:44 +0100, AngeloGioacchino Del Regno wrote:
> External email : Please do not click links or open attachments until you have verified the sender or the content.
> 
> 
> Il 27/11/24 10:04, CK Hu (胡俊光) ha scritto:
> > On Wed, 2024-11-27 at 09:41 +0100, AngeloGioacchino Del Regno wrote:
> > > External email : Please do not click links or open attachments until you have verified the sender or the content.
> > > 
> > > 
> > > Il 27/11/24 08:02, CK Hu (胡俊光) ha scritto:
> > > > On Tue, 2024-11-26 at 10:25 +0100, AngeloGioacchino Del Regno wrote:
> > > > > External email : Please do not click links or open attachments until you have verified the sender or the content.
> > > > > 
> > > > > 
> > > > > Il 26/11/24 04:07, CK Hu (胡俊光) ha scritto:
> > > > > > On Mon, 2024-11-25 at 17:55 +0100, AngeloGioacchino Del Regno wrote:
> > > > > > > External email : Please do not click links or open attachments until you have verified the sender or the content.
> > > > > > > 
> > > > > > > 
> > > > > > > Il 22/11/24 08:23, CK Hu (胡俊光) ha scritto:
> > > > > > > > Hi, Angelo:
> > > > > > > > 
> > > > > > > > On Wed, 2024-11-20 at 13:44 +0100, AngeloGioacchino Del Regno wrote:
> > > > > > > > > External email : Please do not click links or open attachments until you have verified the sender or the content.
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > Add support for the DPI block found in the MT8195 and MT8188 SoCs.
> > > > > > > > > Inside of the SoC, this block is directly connected to the HDMI IP.
> > > > > > > > 
> > > > > > > > In MT8173, DPI0 is directly connected to HDMI.
> > > > > > > > The first version of this driver is just for MT8173 DPI0.
> > > > > > > > Does MT8173 DPI0 need this modification?
> > > > > > > > Or this modification is just for MT8188 and MT8195, then the description should be more than 'directly connected'.
> > > > > > > > 
> > > > > > > 
> > > > > > > This is only for MT8188 and MT8195, and MT8173 does *not* need any modification.
> > > > > > > 
> > > > > > > Please, what would you like to see in the description of this commit?
> > > > > > 
> > > > > > This patch does four jobs.
> > > > > > 
> > > > > > 1. Enable/disable tvd_clk for MT8195/MT8188 DPI.
> > > > > > 2. Do not set pixel clock for MT8195/MT8188 DPI.
> > > > > > 3. New DPI_INPUT_XXX and DPI_OUTPUT_XXX control for MT8195/MT8188 DPI.
> > > > > > 4. Do not power on/off for MT8195/MT8188 DPI.
> > > > > > 
> > > > > > Maybe you should break into 4 patches and each one has different reason.
> > > > > 
> > > > > Yeah I thought about that as well, but there's a fundamental issue with splitting
> > > > > the thing in multiple patches...
> > > > > 
> > > > > For enabling the tvd_clk in a separate patch, there's no problem - however, for the
> > > > > others....
> > > > > 
> > > > > 1. We need to introduce support for MT8195/88 DPI-HDMI, or the other patches would
> > > > >       not make sense (nor apply, anyway); then
> > > > > 2. We stop setting pixel clock with another patch; then
> > > > > 3. we don't power on/off, etc etc
> > > > > 
> > > > > The problem with doing it like so is that the patch #1 that I described would be
> > > > > introducing *faulty code*, because the support for that really depends on all of
> > > > > the others being present (otherwise the block won't work correctly).
> > > > > 
> > > > > So... if you want, I can easily split out the tvd_clk enable/disable, but splitting
> > > > > the rest wouldn't be clean.
> > > > > 
> > > > > Besides, keep in mind that... actually... for anything else that is not MT8195/88
> > > > > DPI0 (so, for other SoCs' DPI and for 95/88 DPINTF) the tvd_clk is already getting
> > > > > enabled by its child.. so, for those ones, a call to enable tvd_clk does exactly
> > > > > nothing apart from incrementing (enable) or decrementing (disable) the refcount for
> > > > > this clock by 1.
> > > > > 
> > > > > This means that the enablement/disablement of tvd_clk is actually important only
> > > > > for the MT8195/88 DPI and has literally no effect on anything else that is
> > > > > currently supported by the mtk_dpi driver anyway.
> > > > > 
> > > > > Still - if you want me to split out the tvd_clk en/dis patch, just confirm and I
> > > > > will split that one out...
> > > > > 
> > > > > > 
> > > > > > For #1 and #2, I've not reviewed the HDMI driver. Is the clock control influenced by new HDMI driver.
> > > > > 
> > > > > It kinda is - the HDMI-TX block gets its clock from the HDMI PHY's clock gen,
> > > > > but eventually it is the HDMI driver that tells to the PHY driver what clock it
> > > > > actually wants.
> > > > > 
> > > > > For #1, clk_prepare_enable() is ungating the clock that would otherwise gate the
> > > > > PHY's PLL output to the HDMI block.
> > > > > 
> > > > > > If it is software reason, maybe we can modify the new HDMI driver and make DPI driver consistent with MT8173.
> > > > > > If it is hardware reason. just describe the hardware reason.
> > > > > 
> > > > > Alright - the hardware reason is that the HDMIPHY generates the clock for the HDMI
> > > > > TX block, and that enabling the clock assigned to tvd_clk is necessary to ungate
> > > > > the PHY's ckgen output to the HDMI-TX (and I think - but not sure as I haven't
> > > > > analyzed that yet - that HDMI-RX should have the same gating technique, but that's
> > > > > definitely out of scope for this submission).
> > > > 
> > > > I think tvd_clk is the clock source of DPI, HDMI, and HDMI-PHY, so these hardware could work in the same frequency.
> > > > That means drivers of DPI, HDMI, and HDMI-PHY are equal to control tvd_clk.
> > > > In MT8173. software choose DPI driver to control tvd_clk.
> > > > In MT8195, software choose HDMI-PHY driver to control tvd_clk.
> > > 
> > > Yes, but in MT8195 the tvd is gated by a clock that is controller by the HDMI
> > > driver only, and not by the PHY - so, PHY sets the frequency, mtk_hdmi_v2 ungates
> > > that to the HDMITX block (with clk_prepare_enable(tvd_clk)).
> > > 
> > > > 
> > > > I would like to have the same control flow.
> > > > If "HDMI-PHY driver to control tvd_clk" is better, we could temporarily let MT8195 has different flow with MT8173.
> > > > So, is "HDMI-PHY driver to control tvd_clk" better?
> > > > 
> > > 
> > > I'm not sure I understand this last part, can you please rephrase?
> > 
> > I would like MT8173 and MT8195 has the same control flow, so keep DPI driver to control tvd_clk in MT8195.
> > If it's better to control tvd_clk by HDMI-PHY driver, both MT8173 and MT8195 control tvd_clk by HDMI-PHY driver.
> > But we are not able to test MT8173. So MT8173 keep control tvd_clk by DPI driver.
> > So control tvd_clk by HDMI-PHY driver is better?
> > 
> 
> Oh. Okay now I understand what you mean.
> 
> Unfortunately, we cannot control the tvd->hdmi gate from the PHY driver... this is
> because we do really rely on a specific ungate sequence, and the DPI driver really
> does need to control the gating of that CG on its own: when we want to bring up the
> DPI+HDMI, we need to:
>   - Start with *gated* clocks, so HW is OFF;
>   - Call mtk_dpi_bridge_enable() (done by drm framework);
>     - There, we now *ungate* the clocks
>     - DPI HW is ON -> we reset and write config to DPI registers
>       with function mtk_dpi_set_display_mode()
>     - We enable the DPI output (set EN in DPI_EN register)
> 
> If we move the CG to HDMI PHY, then we have to phy_configure() and phy_enable()
> inside of the DPI driver, which is also not really possible and can only be done
> in the HDMI driver - and that's because the HDMI driver reads EDID from DDC,
> which gives us the wanted pixel clock, and feeds it to the PHY.
> 
> In short, there's no way around that, the gating cannot be moved out of DPI driver.
> 
> > > 
> > > > > 
> > > > > > 
> > > > > > For #4, I don't know why DPI do not control power by its self?
> > > > > > Even though other driver may control the same power, power manager has reference count,
> > > > > > so each driver could control the same power by its self.
> > > > > 
> > > > > #4 is there both for a SW and for a HW reason.
> > > > > 
> > > > > The HW reason is that the DPI shall be powered on in a specific sequence in regard
> > > > > to HDMI-TX, due to the setup that is required by both (and ungating clocks before
> > > > > full configuration happens would lock up the hw block).
> > > > > 
> > > > > The SW reason is that mtk_crtc.c calls mtk_crtc_ddp_hw_init()->mtk_ddp_comp_start()
> > > > > in its .atomic_enable() callback, which happens in the wrong sequence in regard to
> > > > > HDMI because of the "natural" components order in the DRM framework (for MT8195/88!
> > > > > because for the others it either is the inverse or it does not matter - so for
> > > > > performance it's okay for it to be like that both on older SoCs and on DPINTF for
> > > > > 95/88) and this means that we *must not* call dpi_power_on() at that time but
> > > > > we must rather follow the atomic_enable()/bridge_enable() order imposed by DRM
> > > > > *also* for the clock en/dis calls in DPI.
> > > > 
> > > > It looks like the #4 could be a separate patch.
> > > > The commit message is what you describe here.
> > > > And
> > > > 
> > > > if (!dpi->conf->support_hdmi_power_sequence)
> > > >        mtk_dpi_power_on();
> > > > 
> > > 
> > > This means that I'd have to introduce the "hdmi power sequence" before actually
> > > introducing the real support for MT8195 HDMI....
> > > I honestly don't like that "too much", but it's fine, I don't have *too strong*
> > > opinions about that, so I will separate #4 as you suggested for v2.
> > 
> > This DPI series modification is all about HDMI.
> > Maybe merge this series with HDMI series and let the HDMI part in front of DPI part and it's more reasonable.
> > 
> 
> I have sent the two separately only because I thought it'd be easier for you to
> review them .. well, separately.
> But .. yes, this series is 99% about HDMI - the only thing that's not related to
> HDMI is patch [3/6] which is just a cleanup...
> 
> So if you want I can merge the two series into one, that's not a problem at all;
> in that case, do you want me to keep the patches as they are, or do you want me
> to still split #4 away from this patch?

I think each part would use a different config variable to show which code is related to which part.
So #4 could keep in this patch.
The commit message should describe all these parts.

If breakdown, it would be:

Patch 1: Add config variable 1
Patch 2: Add config variable 2
...
Patch n: Add support for MT8195 DPI support (add MT8195 config table)

Both combine into one patch or breakdown to multiple patch is OK for me.

Regards,
CK

> 
> > Regards,
> > CK
> > 
> > > 
> > > Cheers,
> > > Angelo
> > > 
> > > > Regards,
> > > > CK
> > > > 
> > > > > 
> > > > > Cheers,
> > > > > Angelo
> > > > > 
> > > > > > 
> > > > > > Regards,
> > > > > > CK
> > > > > > 
> > > > > > 
> > > > > > > 
> > > > > > > Cheers,
> > > > > > > Angelo
> > > > > > > 
> > > > > > > > Regards,
> > > > > > > > CK
> > > > > > > > 
> > > > > > > > > 
> > > > > > > > > Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> > > > > > > > > ---
> > > > > > > 
> > > > > > > 
> > > > > 
> > > > > 
> > > > > 
> > > 
> > > 
> > > 
> 
> 

Re: [PATCH v1 6/6] drm/mediatek: Add support for MT8195 Digital Parallel Interface

[AngeloGioacchino Del Regno]

Il 28/11/24 04:08, CK Hu (胡俊光) ha scritto:
> On Wed, 2024-11-27 at 13:44 +0100, AngeloGioacchino Del Regno wrote:
>> External email : Please do not click links or open attachments until you have verified the sender or the content.
>>
>>
>> Il 27/11/24 10:04, CK Hu (胡俊光) ha scritto:
>>> On Wed, 2024-11-27 at 09:41 +0100, AngeloGioacchino Del Regno wrote:
>>>> External email : Please do not click links or open attachments until you have verified the sender or the content.
>>>>
>>>>
>>>> Il 27/11/24 08:02, CK Hu (胡俊光) ha scritto:
>>>>> On Tue, 2024-11-26 at 10:25 +0100, AngeloGioacchino Del Regno wrote:
>>>>>> External email : Please do not click links or open attachments until you have verified the sender or the content.
>>>>>>
>>>>>>
>>>>>> Il 26/11/24 04:07, CK Hu (胡俊光) ha scritto:
>>>>>>> On Mon, 2024-11-25 at 17:55 +0100, AngeloGioacchino Del Regno wrote:
>>>>>>>> External email : Please do not click links or open attachments until you have verified the sender or the content.
>>>>>>>>
>>>>>>>>
>>>>>>>> Il 22/11/24 08:23, CK Hu (胡俊光) ha scritto:
>>>>>>>>> Hi, Angelo:
>>>>>>>>>
>>>>>>>>> On Wed, 2024-11-20 at 13:44 +0100, AngeloGioacchino Del Regno wrote:
>>>>>>>>>> External email : Please do not click links or open attachments until you have verified the sender or the content.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Add support for the DPI block found in the MT8195 and MT8188 SoCs.
>>>>>>>>>> Inside of the SoC, this block is directly connected to the HDMI IP.
>>>>>>>>>
>>>>>>>>> In MT8173, DPI0 is directly connected to HDMI.
>>>>>>>>> The first version of this driver is just for MT8173 DPI0.
>>>>>>>>> Does MT8173 DPI0 need this modification?
>>>>>>>>> Or this modification is just for MT8188 and MT8195, then the description should be more than 'directly connected'.
>>>>>>>>>
>>>>>>>>
>>>>>>>> This is only for MT8188 and MT8195, and MT8173 does *not* need any modification.
>>>>>>>>
>>>>>>>> Please, what would you like to see in the description of this commit?
>>>>>>>
>>>>>>> This patch does four jobs.
>>>>>>>
>>>>>>> 1. Enable/disable tvd_clk for MT8195/MT8188 DPI.
>>>>>>> 2. Do not set pixel clock for MT8195/MT8188 DPI.
>>>>>>> 3. New DPI_INPUT_XXX and DPI_OUTPUT_XXX control for MT8195/MT8188 DPI.
>>>>>>> 4. Do not power on/off for MT8195/MT8188 DPI.
>>>>>>>
>>>>>>> Maybe you should break into 4 patches and each one has different reason.
>>>>>>
>>>>>> Yeah I thought about that as well, but there's a fundamental issue with splitting
>>>>>> the thing in multiple patches...
>>>>>>
>>>>>> For enabling the tvd_clk in a separate patch, there's no problem - however, for the
>>>>>> others....
>>>>>>
>>>>>> 1. We need to introduce support for MT8195/88 DPI-HDMI, or the other patches would
>>>>>>        not make sense (nor apply, anyway); then
>>>>>> 2. We stop setting pixel clock with another patch; then
>>>>>> 3. we don't power on/off, etc etc
>>>>>>
>>>>>> The problem with doing it like so is that the patch #1 that I described would be
>>>>>> introducing *faulty code*, because the support for that really depends on all of
>>>>>> the others being present (otherwise the block won't work correctly).
>>>>>>
>>>>>> So... if you want, I can easily split out the tvd_clk enable/disable, but splitting
>>>>>> the rest wouldn't be clean.
>>>>>>
>>>>>> Besides, keep in mind that... actually... for anything else that is not MT8195/88
>>>>>> DPI0 (so, for other SoCs' DPI and for 95/88 DPINTF) the tvd_clk is already getting
>>>>>> enabled by its child.. so, for those ones, a call to enable tvd_clk does exactly
>>>>>> nothing apart from incrementing (enable) or decrementing (disable) the refcount for
>>>>>> this clock by 1.
>>>>>>
>>>>>> This means that the enablement/disablement of tvd_clk is actually important only
>>>>>> for the MT8195/88 DPI and has literally no effect on anything else that is
>>>>>> currently supported by the mtk_dpi driver anyway.
>>>>>>
>>>>>> Still - if you want me to split out the tvd_clk en/dis patch, just confirm and I
>>>>>> will split that one out...
>>>>>>
>>>>>>>
>>>>>>> For #1 and #2, I've not reviewed the HDMI driver. Is the clock control influenced by new HDMI driver.
>>>>>>
>>>>>> It kinda is - the HDMI-TX block gets its clock from the HDMI PHY's clock gen,
>>>>>> but eventually it is the HDMI driver that tells to the PHY driver what clock it
>>>>>> actually wants.
>>>>>>
>>>>>> For #1, clk_prepare_enable() is ungating the clock that would otherwise gate the
>>>>>> PHY's PLL output to the HDMI block.
>>>>>>
>>>>>>> If it is software reason, maybe we can modify the new HDMI driver and make DPI driver consistent with MT8173.
>>>>>>> If it is hardware reason. just describe the hardware reason.
>>>>>>
>>>>>> Alright - the hardware reason is that the HDMIPHY generates the clock for the HDMI
>>>>>> TX block, and that enabling the clock assigned to tvd_clk is necessary to ungate
>>>>>> the PHY's ckgen output to the HDMI-TX (and I think - but not sure as I haven't
>>>>>> analyzed that yet - that HDMI-RX should have the same gating technique, but that's
>>>>>> definitely out of scope for this submission).
>>>>>
>>>>> I think tvd_clk is the clock source of DPI, HDMI, and HDMI-PHY, so these hardware could work in the same frequency.
>>>>> That means drivers of DPI, HDMI, and HDMI-PHY are equal to control tvd_clk.
>>>>> In MT8173. software choose DPI driver to control tvd_clk.
>>>>> In MT8195, software choose HDMI-PHY driver to control tvd_clk.
>>>>
>>>> Yes, but in MT8195 the tvd is gated by a clock that is controller by the HDMI
>>>> driver only, and not by the PHY - so, PHY sets the frequency, mtk_hdmi_v2 ungates
>>>> that to the HDMITX block (with clk_prepare_enable(tvd_clk)).
>>>>
>>>>>
>>>>> I would like to have the same control flow.
>>>>> If "HDMI-PHY driver to control tvd_clk" is better, we could temporarily let MT8195 has different flow with MT8173.
>>>>> So, is "HDMI-PHY driver to control tvd_clk" better?
>>>>>
>>>>
>>>> I'm not sure I understand this last part, can you please rephrase?
>>>
>>> I would like MT8173 and MT8195 has the same control flow, so keep DPI driver to control tvd_clk in MT8195.
>>> If it's better to control tvd_clk by HDMI-PHY driver, both MT8173 and MT8195 control tvd_clk by HDMI-PHY driver.
>>> But we are not able to test MT8173. So MT8173 keep control tvd_clk by DPI driver.
>>> So control tvd_clk by HDMI-PHY driver is better?
>>>
>>
>> Oh. Okay now I understand what you mean.
>>
>> Unfortunately, we cannot control the tvd->hdmi gate from the PHY driver... this is
>> because we do really rely on a specific ungate sequence, and the DPI driver really
>> does need to control the gating of that CG on its own: when we want to bring up the
>> DPI+HDMI, we need to:
>>    - Start with *gated* clocks, so HW is OFF;
>>    - Call mtk_dpi_bridge_enable() (done by drm framework);
>>      - There, we now *ungate* the clocks
>>      - DPI HW is ON -> we reset and write config to DPI registers
>>        with function mtk_dpi_set_display_mode()
>>      - We enable the DPI output (set EN in DPI_EN register)
>>
>> If we move the CG to HDMI PHY, then we have to phy_configure() and phy_enable()
>> inside of the DPI driver, which is also not really possible and can only be done
>> in the HDMI driver - and that's because the HDMI driver reads EDID from DDC,
>> which gives us the wanted pixel clock, and feeds it to the PHY.
>>
>> In short, there's no way around that, the gating cannot be moved out of DPI driver.
>>
>>>>
>>>>>>
>>>>>>>
>>>>>>> For #4, I don't know why DPI do not control power by its self?
>>>>>>> Even though other driver may control the same power, power manager has reference count,
>>>>>>> so each driver could control the same power by its self.
>>>>>>
>>>>>> #4 is there both for a SW and for a HW reason.
>>>>>>
>>>>>> The HW reason is that the DPI shall be powered on in a specific sequence in regard
>>>>>> to HDMI-TX, due to the setup that is required by both (and ungating clocks before
>>>>>> full configuration happens would lock up the hw block).
>>>>>>
>>>>>> The SW reason is that mtk_crtc.c calls mtk_crtc_ddp_hw_init()->mtk_ddp_comp_start()
>>>>>> in its .atomic_enable() callback, which happens in the wrong sequence in regard to
>>>>>> HDMI because of the "natural" components order in the DRM framework (for MT8195/88!
>>>>>> because for the others it either is the inverse or it does not matter - so for
>>>>>> performance it's okay for it to be like that both on older SoCs and on DPINTF for
>>>>>> 95/88) and this means that we *must not* call dpi_power_on() at that time but
>>>>>> we must rather follow the atomic_enable()/bridge_enable() order imposed by DRM
>>>>>> *also* for the clock en/dis calls in DPI.
>>>>>
>>>>> It looks like the #4 could be a separate patch.
>>>>> The commit message is what you describe here.
>>>>> And
>>>>>
>>>>> if (!dpi->conf->support_hdmi_power_sequence)
>>>>>         mtk_dpi_power_on();
>>>>>
>>>>
>>>> This means that I'd have to introduce the "hdmi power sequence" before actually
>>>> introducing the real support for MT8195 HDMI....
>>>> I honestly don't like that "too much", but it's fine, I don't have *too strong*
>>>> opinions about that, so I will separate #4 as you suggested for v2.
>>>
>>> This DPI series modification is all about HDMI.
>>> Maybe merge this series with HDMI series and let the HDMI part in front of DPI part and it's more reasonable.
>>>
>>
>> I have sent the two separately only because I thought it'd be easier for you to
>> review them .. well, separately.
>> But .. yes, this series is 99% about HDMI - the only thing that's not related to
>> HDMI is patch [3/6] which is just a cleanup...
>>
>> So if you want I can merge the two series into one, that's not a problem at all;
>> in that case, do you want me to keep the patches as they are, or do you want me
>> to still split #4 away from this patch?
> 
> I think each part would use a different config variable to show which code is related to which part.
> So #4 could keep in this patch.
> The commit message should describe all these parts.
> 
> If breakdown, it would be:
> 
> Patch 1: Add config variable 1
> Patch 2: Add config variable 2
> ...
> Patch n: Add support for MT8195 DPI support (add MT8195 config table)
> 
> Both combine into one patch or breakdown to multiple patch is OK for me.
> 

Okay, since both ways are ok for you, I will keep the commits layout as it is,
which is something that I do prefer because it simply makes more sense in my
opinion.

Thank you for the feedback

Cheers!

> Regards,
> CK
> 
>>
>>> Regards,
>>> CK
>>>
>>>>
>>>> Cheers,
>>>> Angelo
>>>>
>>>>> Regards,
>>>>> CK
>>>>>
>>>>>>
>>>>>> Cheers,
>>>>>> Angelo
>>>>>>
>>>>>>>
>>>>>>> Regards,
>>>>>>> CK
>>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> Cheers,
>>>>>>>> Angelo
>>>>>>>>
>>>>>>>>> Regards,
>>>>>>>>> CK
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>>>>>>>>>> ---
>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>
>>>>
>>>>
>>
>>