[PATCH v3,0/2] Add dpi output format control for MT8186

[PATCH v3,0/2] Add dpi output format control for MT8186

[xinlei.lee]

From: Xinlei Lee <xinlei.lee@mediatek.com>

Base on the branch of Linux-next/master.
This series are based on the following patch:
[1] Add MediaTek SoC(vdosys1) support for mt8195
https://patchwork.kernel.org/project/linux-mediatek/cover/20220711075245.10492-1-nancy.lin@mediatek.com/
[2] Add MediaTek SoC DRM (vdosys1) support for mt8195
https://patchwork.kernel.org/project/linux-mediatek/cover/20220804072827.22383-1-nancy.lin@mediatek.com/

Changes since v2:
1. Modify key nouns in the description;
2. Add the label of jitao to Co-developed-by;
3. Macro definition address lowercase problem and function naming;
4. Add missing a description of this property in the mtk_dpi_conf.

Change since v1:
1. Modify mt8186 compatiable location.
2. Modify MT8186_DPI_OUTPUT_FORMAT name.

When MT8186 outputs dpi signal, it is necessary to add dual edge output
format control in mmsys.

Xinlei Lee (2):
  FROMLIST: soc: mediatek: Add mmsys func to adapt to dpi output for
    MT8186
  FROMLIST: drm: mediatek: Adjust the dpi output format to MT8186

Xinlei Lee (2):
  FROMLIST: soc: mediatek: Add mmsys func to adapt to dpi output for
    MT8186
  FROMLIST: drm: mediatek: Adjust the dpi output format to MT8186

 drivers/gpu/drm/mediatek/mtk_dpi.c      | 31 +++++++++++++++++++++++++
 drivers/gpu/drm/mediatek/mtk_dpi_regs.h |  4 ++++
 drivers/gpu/drm/mediatek/mtk_drm_drv.c  |  2 ++
 drivers/soc/mediatek/mt8186-mmsys.h     |  1 +
 drivers/soc/mediatek/mtk-mmsys.c        |  8 +++++++
 include/linux/soc/mediatek/mtk-mmsys.h  |  3 +++
 6 files changed, 49 insertions(+)

-- 
2.18.0

[PATCH v3,0/2] Add dpi output format control for MT8186

[xinlei.lee]

From: Xinlei Lee <xinlei.lee@mediatek.com>

Base on the branch of Linux-next/master.
This series are based on the following patch:
[1] Add MediaTek SoC(vdosys1) support for mt8195
https://patchwork.kernel.org/project/linux-mediatek/cover/20220711075245.10492-1-nancy.lin@mediatek.com/
[2] Add MediaTek SoC DRM (vdosys1) support for mt8195
https://patchwork.kernel.org/project/linux-mediatek/cover/20220804072827.22383-1-nancy.lin@mediatek.com/

Changes since v2:
1. Modify key nouns in the description;
2. Add the label of jitao to Co-developed-by;
3. Macro definition address lowercase problem and function naming;
4. Add missing a description of this property in the mtk_dpi_conf.

Change since v1:
1. Modify mt8186 compatiable location.
2. Modify MT8186_DPI_OUTPUT_FORMAT name.

When MT8186 outputs dpi signal, it is necessary to add dual edge output
format control in mmsys.

Xinlei Lee (2):
  FROMLIST: soc: mediatek: Add mmsys func to adapt to dpi output for
    MT8186
  FROMLIST: drm: mediatek: Adjust the dpi output format to MT8186

Xinlei Lee (2):
  FROMLIST: soc: mediatek: Add mmsys func to adapt to dpi output for
    MT8186
  FROMLIST: drm: mediatek: Adjust the dpi output format to MT8186

 drivers/gpu/drm/mediatek/mtk_dpi.c      | 31 +++++++++++++++++++++++++
 drivers/gpu/drm/mediatek/mtk_dpi_regs.h |  4 ++++
 drivers/gpu/drm/mediatek/mtk_drm_drv.c  |  2 ++
 drivers/soc/mediatek/mt8186-mmsys.h     |  1 +
 drivers/soc/mediatek/mtk-mmsys.c        |  8 +++++++
 include/linux/soc/mediatek/mtk-mmsys.h  |  3 +++
 6 files changed, 49 insertions(+)

-- 
2.18.0

[PATCH v3,2/2] drm: mediatek: Adjust the dpi output format to MT8186

[xinlei.lee]

From: Xinlei Lee <xinlei.lee@mediatek.com>

Dpi output needs to adjust the output format to dual edge for MT8186.
Because MT8186 HW has been modified at that time, SW needs to cooperate.
And the register (MMSYS) reserved for dpi will be used for output
format control (dual_edge/single_edge).

Co-developed-by: Jitao Shi <jitao.shi@mediatek.com>
Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>

---
 drivers/gpu/drm/mediatek/mtk_dpi.c      | 31 +++++++++++++++++++++++++
 drivers/gpu/drm/mediatek/mtk_dpi_regs.h |  4 ++++
 drivers/gpu/drm/mediatek/mtk_drm_drv.c  |  2 ++
 3 files changed, 37 insertions(+)

diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c b/drivers/gpu/drm/mediatek/mtk_dpi.c
index 630a4e301ef6..b66423bd7765 100644
--- a/drivers/gpu/drm/mediatek/mtk_dpi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dpi.c
@@ -15,6 +15,7 @@
 #include <linux/of_graph.h>
 #include <linux/pinctrl/consumer.h>
 #include <linux/platform_device.h>
+#include <linux/soc/mediatek/mtk-mmsys.h>
 #include <linux/types.h>
 
 #include <video/videomode.h>
@@ -30,6 +31,7 @@
 #include "mtk_disp_drv.h"
 #include "mtk_dpi_regs.h"
 #include "mtk_drm_ddp_comp.h"
+#include "mtk_drm_drv.h"
 
 enum mtk_dpi_out_bit_num {
 	MTK_DPI_OUT_BIT_NUM_8BITS,
@@ -82,6 +84,7 @@ struct mtk_dpi {
 	struct pinctrl_state *pins_dpi;
 	u32 output_fmt;
 	int refcount;
+	struct device *mmsys_dev;
 };
 
 static inline struct mtk_dpi *bridge_to_dpi(struct drm_bridge *b)
@@ -135,6 +138,7 @@ struct mtk_dpi_yc_limit {
  * @yuv422_en_bit: Enable bit of yuv422.
  * @csc_enable_bit: Enable bit of CSC.
  * @pixels_per_iter: Quantity of transferred pixels per iteration.
+ * @rgb888_dual_enable: Control output format for mt8186.
  */
 struct mtk_dpi_conf {
 	unsigned int (*cal_factor)(int clock);
@@ -153,6 +157,7 @@ struct mtk_dpi_conf {
 	u32 yuv422_en_bit;
 	u32 csc_enable_bit;
 	u32 pixels_per_iter;
+	bool rgb888_dual_enable;
 };
 
 static void mtk_dpi_mask(struct mtk_dpi *dpi, u32 offset, u32 val, u32 mask)
@@ -449,6 +454,9 @@ static void mtk_dpi_dual_edge(struct mtk_dpi *dpi)
 		mtk_dpi_mask(dpi, DPI_OUTPUT_SETTING,
 			     dpi->output_fmt == MEDIA_BUS_FMT_RGB888_2X12_LE ?
 			     EDGE_SEL : 0, EDGE_SEL);
+	if (dpi->conf->rgb888_dual_enable)
+		mtk_mmsys_ddp_dpi_fmt_config(dpi->mmsys_dev, DPI_RGB888_DDR_CON,
+					     DPI_FORMAT_MASK, NULL);
 	} else {
 		mtk_dpi_mask(dpi, DPI_DDR_SETTING, DDR_EN | DDR_4PHASE, 0);
 	}
@@ -778,8 +786,10 @@ static int mtk_dpi_bind(struct device *dev, struct device *master, void *data)
 {
 	struct mtk_dpi *dpi = dev_get_drvdata(dev);
 	struct drm_device *drm_dev = data;
+	struct mtk_drm_private *priv = drm_dev->dev_private;
 	int ret;
 
+	dpi->mmsys_dev = priv->mmsys_dev;
 	ret = drm_simple_encoder_init(drm_dev, &dpi->encoder,
 				      DRM_MODE_ENCODER_TMDS);
 	if (ret) {
@@ -930,6 +940,24 @@ static const struct mtk_dpi_conf mt8183_conf = {
 	.csc_enable_bit = CSC_ENABLE,
 };
 
+static const struct mtk_dpi_conf mt8186_conf = {
+	.cal_factor = mt8183_calculate_factor,
+	.reg_h_fre_con = 0xe0,
+	.max_clock_khz = 150000,
+	.output_fmts = mt8183_output_fmts,
+	.num_output_fmts = ARRAY_SIZE(mt8183_output_fmts),
+	.rgb888_dual_enable = true,
+	.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,
+};
+
 static const struct mtk_dpi_conf mt8192_conf = {
 	.cal_factor = mt8183_calculate_factor,
 	.reg_h_fre_con = 0xe0,
@@ -1080,6 +1108,9 @@ static const struct of_device_id mtk_dpi_of_ids[] = {
 	{ .compatible = "mediatek,mt8183-dpi",
 	  .data = &mt8183_conf,
 	},
+	{ .compatible = "mediatek,mt8186-dpi",
+	  .data = &mt8186_conf,
+	},
 	{ .compatible = "mediatek,mt8192-dpi",
 	  .data = &mt8192_conf,
 	},
diff --git a/drivers/gpu/drm/mediatek/mtk_dpi_regs.h b/drivers/gpu/drm/mediatek/mtk_dpi_regs.h
index 62bd4931b344..e5d254a2097b 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_FORMAT_MASK			0x1
+#define DPI_RGB888_DDR_CON		BIT(0)
+#define DPI_RGB565_SDR_CON		BIT(1)
+
 #endif /* __MTK_DPI_REGS_H */
diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
index d72263c8a621..6cafbfa25d02 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
@@ -784,6 +784,8 @@ static const struct of_device_id mtk_ddp_comp_dt_ids[] = {
 	  .data = (void *)MTK_DPI },
 	{ .compatible = "mediatek,mt8183-dpi",
 	  .data = (void *)MTK_DPI },
+	{ .compatible = "mediatek,mt8186-dpi",
+	  .data = (void *)MTK_DPI },
 	{ .compatible = "mediatek,mt8192-dpi",
 	  .data = (void *)MTK_DPI },
 	{ .compatible = "mediatek,mt8195-dp-intf",
-- 
2.18.0

Re: [PATCH v3,2/2] drm: mediatek: Adjust the dpi output format to MT8186

[Nícolas F. R. A. Prado]

On Tue, Aug 23, 2022 at 02:18:37PM +0800, xinlei.lee@mediatek.com wrote:
> From: Xinlei Lee <xinlei.lee@mediatek.com>
> 
> Dpi output needs to adjust the output format to dual edge for MT8186.
> Because MT8186 HW has been modified at that time, SW needs to cooperate.
> And the register (MMSYS) reserved for dpi will be used for output
> format control (dual_edge/single_edge).
> 
> Co-developed-by: Jitao Shi <jitao.shi@mediatek.com>
> Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> 
> ---
[..]
> --- a/drivers/gpu/drm/mediatek/mtk_dpi.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c
[..]
>   * @yuv422_en_bit: Enable bit of yuv422.
>   * @csc_enable_bit: Enable bit of CSC.
>   * @pixels_per_iter: Quantity of transferred pixels per iteration.
> + * @rgb888_dual_enable: Control output format for mt8186.

Let's not mention mt8186 in the description to keep the property generic. Also,
this description should say what having 'rgb888_dual_enable = true' indicates
about the hardware (in this case mt8186) and it currently doesn't.

Let's take a step back. What does 'dual enable' mean in this context and how
does it relate to 'dual edge' and the dpi output format? By answering those
questions we can find a description (and maybe variable name) that makes more
sense.

>   */
[..]
> @@ -449,6 +454,9 @@ static void mtk_dpi_dual_edge(struct mtk_dpi *dpi)
>  		mtk_dpi_mask(dpi, DPI_OUTPUT_SETTING,
>  			     dpi->output_fmt == MEDIA_BUS_FMT_RGB888_2X12_LE ?
>  			     EDGE_SEL : 0, EDGE_SEL);
> +	if (dpi->conf->rgb888_dual_enable)
> +		mtk_mmsys_ddp_dpi_fmt_config(dpi->mmsys_dev, DPI_RGB888_DDR_CON,
> +					     DPI_FORMAT_MASK, NULL);

This if block should be further indented.

>  	} else {
>  		mtk_dpi_mask(dpi, DPI_DDR_SETTING, DDR_EN | DDR_4PHASE, 0);
>  	}
[..]
> --- 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_FORMAT_MASK			0x1
> +#define DPI_RGB888_DDR_CON		BIT(0)
> +#define DPI_RGB565_SDR_CON		BIT(1)

I'm not sure if it would make more sense to have these definitions in the mmsys
header since they're configurations of a register in mmsys' iospace... I think
we can keep them here but at least add a comment above:

/* Values for DPI configuration in MMSYS address space */

Thanks,
Nícolas

Re: [PATCH v3,2/2] drm: mediatek: Adjust the dpi output format to MT8186

[xinlei.lee]

On Tue, 2022-08-23 at 16:16 -0400, Nícolas F. R. A. Prado wrote:
> On Tue, Aug 23, 2022 at 02:18:37PM +0800, xinlei.lee@mediatek.com
> wrote:
> > From: Xinlei Lee <xinlei.lee@mediatek.com>
> > 
> > Dpi output needs to adjust the output format to dual edge for
> > MT8186.
> > Because MT8186 HW has been modified at that time, SW needs to
> > cooperate.
> > And the register (MMSYS) reserved for dpi will be used for output
> > format control (dual_edge/single_edge).
> > 
> > Co-developed-by: Jitao Shi <jitao.shi@mediatek.com>
> > Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> > Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> > 
> > ---
> 
> [..]
> > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c
> 
> [..]
> >   * @yuv422_en_bit: Enable bit of yuv422.
> >   * @csc_enable_bit: Enable bit of CSC.
> >   * @pixels_per_iter: Quantity of transferred pixels per iteration.
> > + * @rgb888_dual_enable: Control output format for mt8186.
> 
> Let's not mention mt8186 in the description to keep the property
> generic. Also,
> this description should say what having 'rgb888_dual_enable = true'
> indicates
> about the hardware (in this case mt8186) and it currently doesn't.
> 
> Let's take a step back. What does 'dual enable' mean in this context
> and how
> does it relate to 'dual edge' and the dpi output format? By answering
> those
> questions we can find a description (and maybe variable name) that
> makes more
> sense.
> 
> >   */
> 
> [..]
> > @@ -449,6 +454,9 @@ static void mtk_dpi_dual_edge(struct mtk_dpi
> > *dpi)
> >  		mtk_dpi_mask(dpi, DPI_OUTPUT_SETTING,
> >  			     dpi->output_fmt ==
> > MEDIA_BUS_FMT_RGB888_2X12_LE ?
> >  			     EDGE_SEL : 0, EDGE_SEL);
> > +	if (dpi->conf->rgb888_dual_enable)
> > +		mtk_mmsys_ddp_dpi_fmt_config(dpi->mmsys_dev,
> > DPI_RGB888_DDR_CON,
> > +					     DPI_FORMAT_MASK, NULL);
> 
> This if block should be further indented.
> 
> >  	} else {
> >  		mtk_dpi_mask(dpi, DPI_DDR_SETTING, DDR_EN | DDR_4PHASE,
> > 0);
> >  	}
> 
> [..]
> > --- 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_FORMAT_MASK			0x1
> > +#define DPI_RGB888_DDR_CON		BIT(0)
> > +#define DPI_RGB565_SDR_CON		BIT(1)
> 
> I'm not sure if it would make more sense to have these definitions in
> the mmsys
> header since they're configurations of a register in mmsys'
> iospace... I think
> we can keep them here but at least add a comment above:
> 
> /* Values for DPI configuration in MMSYS address space */
> 
> Thanks,
> Nícolas

Hi Nícolas:
Thanks for your careful review!
I will modify the description of this member variable and add the
hardware state corresponding to the software setting.
(eg. rgb888_dual_enable = true the hardware output rgb888_dual_edge
format data)

Your suggestion is very necessary, maybe my name is not accurate
enough, this flag is to enable RGB888_dual_edge format output. 
Would it be better for the variable to be called
RGB888_dual_edge_enable then?

Also I'll fix the code formatting issues
and add descriptions to the mmsys registers in the mtk_dpi_regs.h file.

Best Regards!
xinlei

Re: [PATCH v3,2/2] drm: mediatek: Adjust the dpi output format to MT8186

[Nícolas F. R. A. Prado]

On Wed, Aug 24, 2022 at 09:59:21AM +0800, xinlei.lee wrote:
> On Tue, 2022-08-23 at 16:16 -0400, Nícolas F. R. A. Prado wrote:
> > On Tue, Aug 23, 2022 at 02:18:37PM +0800, xinlei.lee@mediatek.com
> > wrote:
> > > From: Xinlei Lee <xinlei.lee@mediatek.com>
> > > 
> > > Dpi output needs to adjust the output format to dual edge for
> > > MT8186.
> > > Because MT8186 HW has been modified at that time, SW needs to
> > > cooperate.
> > > And the register (MMSYS) reserved for dpi will be used for output
> > > format control (dual_edge/single_edge).
> > > 
> > > Co-developed-by: Jitao Shi <jitao.shi@mediatek.com>
> > > Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> > > Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> > > 
> > > ---
> > 
> > [..]
> > > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c
> > > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c
> > 
> > [..]
> > >   * @yuv422_en_bit: Enable bit of yuv422.
> > >   * @csc_enable_bit: Enable bit of CSC.
> > >   * @pixels_per_iter: Quantity of transferred pixels per iteration.
> > > + * @rgb888_dual_enable: Control output format for mt8186.
> > 
> > Let's not mention mt8186 in the description to keep the property
> > generic. Also,
> > this description should say what having 'rgb888_dual_enable = true'
> > indicates
> > about the hardware (in this case mt8186) and it currently doesn't.
> > 
> > Let's take a step back. What does 'dual enable' mean in this context
> > and how
> > does it relate to 'dual edge' and the dpi output format? By answering
> > those
> > questions we can find a description (and maybe variable name) that
> > makes more
> > sense.
> > 
> > >   */
> > 
> > [..]
> > > @@ -449,6 +454,9 @@ static void mtk_dpi_dual_edge(struct mtk_dpi
> > > *dpi)
> > >  		mtk_dpi_mask(dpi, DPI_OUTPUT_SETTING,
> > >  			     dpi->output_fmt ==
> > > MEDIA_BUS_FMT_RGB888_2X12_LE ?
> > >  			     EDGE_SEL : 0, EDGE_SEL);
> > > +	if (dpi->conf->rgb888_dual_enable)
> > > +		mtk_mmsys_ddp_dpi_fmt_config(dpi->mmsys_dev,
> > > DPI_RGB888_DDR_CON,
> > > +					     DPI_FORMAT_MASK, NULL);
> > 
> > This if block should be further indented.
> > 
> > >  	} else {
> > >  		mtk_dpi_mask(dpi, DPI_DDR_SETTING, DDR_EN | DDR_4PHASE,
> > > 0);
> > >  	}
> > 
> > [..]
> > > --- 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_FORMAT_MASK			0x1
> > > +#define DPI_RGB888_DDR_CON		BIT(0)
> > > +#define DPI_RGB565_SDR_CON		BIT(1)
> > 
> > I'm not sure if it would make more sense to have these definitions in
> > the mmsys
> > header since they're configurations of a register in mmsys'
> > iospace... I think
> > we can keep them here but at least add a comment above:
> > 
> > /* Values for DPI configuration in MMSYS address space */
> > 
> > Thanks,
> > Nícolas
> 
> Hi Nícolas:
> Thanks for your careful review!
> I will modify the description of this member variable and add the
> hardware state corresponding to the software setting.
> (eg. rgb888_dual_enable = true the hardware output rgb888_dual_edge
> format data)
> 
> Your suggestion is very necessary, maybe my name is not accurate
> enough, this flag is to enable RGB888_dual_edge format output. 
> Would it be better for the variable to be called
> RGB888_dual_edge_enable then?

The thing is, we also output in rgb888 dual edge format on mt8183 and mt8192,
and therefore set DDR_EN in mtk_dpi_dual_edge(), right? But, as you said, we
don't need to enable this new rgb888_dual_enable variable on those platforms,
only on mt8186. So that's why I don't think the current name/description is
suitable. If the variable only needs to be set on mt8186, it should have a name
and description that shows what is different between mt8186 and the others. But
without containing the "mt8186" name, since this might happen on other SoCs
later on.

My understanding is that even though both mt8186 and mt8192 output in the rgb888
dual edge format, only mt8186 is able to configure the edge setting in MMSYS (so
on mt8192 it would be hardwired to dual edge and not possible to change). So
what I propose is

Name: edge_cfg_in_mmsys

Description: "If the edge configuration for DPI's output needs to be set in MMSYS"

But maybe since you know the hardware, you might be able to find an even better
name/description.

Thanks,
Nícolas

Re: [PATCH v3,2/2] drm: mediatek: Adjust the dpi output format to MT8186

[xinlei.lee]

On Wed, 2022-08-24 at 11:44 -0400, Nícolas F. R. A. Prado wrote:
> On Wed, Aug 24, 2022 at 09:59:21AM +0800, xinlei.lee wrote:
> > On Tue, 2022-08-23 at 16:16 -0400, Nícolas F. R. A. Prado wrote:
> > > On Tue, Aug 23, 2022 at 02:18:37PM +0800, xinlei.lee@mediatek.com
> > > wrote:
> > > > From: Xinlei Lee <xinlei.lee@mediatek.com>
> > > > 
> > > > Dpi output needs to adjust the output format to dual edge for
> > > > MT8186.
> > > > Because MT8186 HW has been modified at that time, SW needs to
> > > > cooperate.
> > > > And the register (MMSYS) reserved for dpi will be used for
> > > > output
> > > > format control (dual_edge/single_edge).
> > > > 
> > > > Co-developed-by: Jitao Shi <jitao.shi@mediatek.com>
> > > > Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> > > > Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> > > > 
> > > > ---
> > > 
> > > [..]
> > > > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c
> > > > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c
> > > 
> > > [..]
> > > >   * @yuv422_en_bit: Enable bit of yuv422.
> > > >   * @csc_enable_bit: Enable bit of CSC.
> > > >   * @pixels_per_iter: Quantity of transferred pixels per
> > > > iteration.
> > > > + * @rgb888_dual_enable: Control output format for mt8186.
> > > 
> > > Let's not mention mt8186 in the description to keep the property
> > > generic. Also,
> > > this description should say what having 'rgb888_dual_enable =
> > > true'
> > > indicates
> > > about the hardware (in this case mt8186) and it currently
> > > doesn't.
> > > 
> > > Let's take a step back. What does 'dual enable' mean in this
> > > context
> > > and how
> > > does it relate to 'dual edge' and the dpi output format? By
> > > answering
> > > those
> > > questions we can find a description (and maybe variable name)
> > > that
> > > makes more
> > > sense.
> > > 
> > > >   */
> > > 
> > > [..]
> > > > @@ -449,6 +454,9 @@ static void mtk_dpi_dual_edge(struct
> > > > mtk_dpi
> > > > *dpi)
> > > >  		mtk_dpi_mask(dpi, DPI_OUTPUT_SETTING,
> > > >  			     dpi->output_fmt ==
> > > > MEDIA_BUS_FMT_RGB888_2X12_LE ?
> > > >  			     EDGE_SEL : 0, EDGE_SEL);
> > > > +	if (dpi->conf->rgb888_dual_enable)
> > > > +		mtk_mmsys_ddp_dpi_fmt_config(dpi->mmsys_dev,
> > > > DPI_RGB888_DDR_CON,
> > > > +					     DPI_FORMAT_MASK,
> > > > NULL);
> > > 
> > > This if block should be further indented.
> > > 
> > > >  	} else {
> > > >  		mtk_dpi_mask(dpi, DPI_DDR_SETTING, DDR_EN |
> > > > DDR_4PHASE,
> > > > 0);
> > > >  	}
> > > 
> > > [..]
> > > > --- 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_FORMAT_MASK			0x1
> > > > +#define DPI_RGB888_DDR_CON		BIT(0)
> > > > +#define DPI_RGB565_SDR_CON		BIT(1)
> > > 
> > > I'm not sure if it would make more sense to have these
> > > definitions in
> > > the mmsys
> > > header since they're configurations of a register in mmsys'
> > > iospace... I think
> > > we can keep them here but at least add a comment above:
> > > 
> > > /* Values for DPI configuration in MMSYS address space */
> > > 
> > > Thanks,
> > > Nícolas
> > 
> > Hi Nícolas:
> > Thanks for your careful review!
> > I will modify the description of this member variable and add the
> > hardware state corresponding to the software setting.
> > (eg. rgb888_dual_enable = true the hardware output rgb888_dual_edge
> > format data)
> > 
> > Your suggestion is very necessary, maybe my name is not accurate
> > enough, this flag is to enable RGB888_dual_edge format output. 
> > Would it be better for the variable to be called
> > RGB888_dual_edge_enable then?
> 
> The thing is, we also output in rgb888 dual edge format on mt8183 and
> mt8192,
> and therefore set DDR_EN in mtk_dpi_dual_edge(), right? But, as you
> said, we
> don't need to enable this new rgb888_dual_enable variable on those
> platforms,
> only on mt8186. So that's why I don't think the current
> name/description is
> suitable. If the variable only needs to be set on mt8186, it should
> have a name
> and description that shows what is different between mt8186 and the
> others. But
> without containing the "mt8186" name, since this might happen on
> other SoCs
> later on.
> 
> My understanding is that even though both mt8186 and mt8192 output in
> the rgb888
> dual edge format, only mt8186 is able to configure the edge setting
> in MMSYS (so
> on mt8192 it would be hardwired to dual edge and not possible to
> change). So
> what I propose is
> 
> Name: edge_cfg_in_mmsys
> 
> Description: "If the edge configuration for DPI's output needs to be
> set in MMSYS"
> 
> But maybe since you know the hardware, you might be able to find an
> even better
> name/description.
> 
> Thanks,
> Nícolas
Hi Nícolas:

Thanks for your suggestion.

At present, it is true that only 8186 needs to set this flag when
outputting dual_edge format. 
If other ICs need to modify the output format, they only need to modify
the DPI register. 
On the 8186, DPI MUX (0x400) is required for synchronous modification.
A more detailed explanation of this DPI MUX register is 
bit[0]: dual_edge enable, bit[1]: rgb565_en.
And the priority of bit[1] is higher, the following is the format of
different combinations:
00: SDR enable
01: DDR enable
10: RGB565
11: RGB565
 
The hardware characteristics can be ignored. Based on this situation,
what is your opinion if it is changed to "edge_cfg_in_mmsys"?
Best Regards!
Xinlei

Re: [PATCH v3,2/2] drm: mediatek: Adjust the dpi output format to MT8186

[Nícolas F. R. A. Prado]

On Fri, Aug 26, 2022 at 11:58:29AM +0800, xinlei.lee wrote:
> On Wed, 2022-08-24 at 11:44 -0400, Nícolas F. R. A. Prado wrote:
> > On Wed, Aug 24, 2022 at 09:59:21AM +0800, xinlei.lee wrote:
> > > On Tue, 2022-08-23 at 16:16 -0400, Nícolas F. R. A. Prado wrote:
> > > > On Tue, Aug 23, 2022 at 02:18:37PM +0800, xinlei.lee@mediatek.com
> > > > wrote:
> > > > > From: Xinlei Lee <xinlei.lee@mediatek.com>
> > > > > 
> > > > > Dpi output needs to adjust the output format to dual edge for
> > > > > MT8186.
> > > > > Because MT8186 HW has been modified at that time, SW needs to
> > > > > cooperate.
> > > > > And the register (MMSYS) reserved for dpi will be used for
> > > > > output
> > > > > format control (dual_edge/single_edge).
> > > > > 
> > > > > Co-developed-by: Jitao Shi <jitao.shi@mediatek.com>
> > > > > Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> > > > > Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> > > > > 
> > > > > ---
> > > > 
> > > > [..]
> > > > > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c
> > > > > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c
> > > > 
> > > > [..]
> > > > >   * @yuv422_en_bit: Enable bit of yuv422.
> > > > >   * @csc_enable_bit: Enable bit of CSC.
> > > > >   * @pixels_per_iter: Quantity of transferred pixels per
> > > > > iteration.
> > > > > + * @rgb888_dual_enable: Control output format for mt8186.
> > > > 
> > > > Let's not mention mt8186 in the description to keep the property
> > > > generic. Also,
> > > > this description should say what having 'rgb888_dual_enable =
> > > > true'
> > > > indicates
> > > > about the hardware (in this case mt8186) and it currently
> > > > doesn't.
> > > > 
> > > > Let's take a step back. What does 'dual enable' mean in this
> > > > context
> > > > and how
> > > > does it relate to 'dual edge' and the dpi output format? By
> > > > answering
> > > > those
> > > > questions we can find a description (and maybe variable name)
> > > > that
> > > > makes more
> > > > sense.
> > > > 
> > > > >   */
> > > > 
> > > > [..]
> > > > > @@ -449,6 +454,9 @@ static void mtk_dpi_dual_edge(struct
> > > > > mtk_dpi
> > > > > *dpi)
> > > > >  		mtk_dpi_mask(dpi, DPI_OUTPUT_SETTING,
> > > > >  			     dpi->output_fmt ==
> > > > > MEDIA_BUS_FMT_RGB888_2X12_LE ?
> > > > >  			     EDGE_SEL : 0, EDGE_SEL);
> > > > > +	if (dpi->conf->rgb888_dual_enable)
> > > > > +		mtk_mmsys_ddp_dpi_fmt_config(dpi->mmsys_dev,
> > > > > DPI_RGB888_DDR_CON,
> > > > > +					     DPI_FORMAT_MASK,
> > > > > NULL);
> > > > 
> > > > This if block should be further indented.
> > > > 
> > > > >  	} else {
> > > > >  		mtk_dpi_mask(dpi, DPI_DDR_SETTING, DDR_EN |
> > > > > DDR_4PHASE,
> > > > > 0);
> > > > >  	}
> > > > 
> > > > [..]
> > > > > --- 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_FORMAT_MASK			0x1
> > > > > +#define DPI_RGB888_DDR_CON		BIT(0)
> > > > > +#define DPI_RGB565_SDR_CON		BIT(1)
> > > > 
> > > > I'm not sure if it would make more sense to have these
> > > > definitions in
> > > > the mmsys
> > > > header since they're configurations of a register in mmsys'
> > > > iospace... I think
> > > > we can keep them here but at least add a comment above:
> > > > 
> > > > /* Values for DPI configuration in MMSYS address space */
> > > > 
> > > > Thanks,
> > > > Nícolas
> > > 
> > > Hi Nícolas:
> > > Thanks for your careful review!
> > > I will modify the description of this member variable and add the
> > > hardware state corresponding to the software setting.
> > > (eg. rgb888_dual_enable = true the hardware output rgb888_dual_edge
> > > format data)
> > > 
> > > Your suggestion is very necessary, maybe my name is not accurate
> > > enough, this flag is to enable RGB888_dual_edge format output. 
> > > Would it be better for the variable to be called
> > > RGB888_dual_edge_enable then?
> > 
> > The thing is, we also output in rgb888 dual edge format on mt8183 and
> > mt8192,
> > and therefore set DDR_EN in mtk_dpi_dual_edge(), right? But, as you
> > said, we
> > don't need to enable this new rgb888_dual_enable variable on those
> > platforms,
> > only on mt8186. So that's why I don't think the current
> > name/description is
> > suitable. If the variable only needs to be set on mt8186, it should
> > have a name
> > and description that shows what is different between mt8186 and the
> > others. But
> > without containing the "mt8186" name, since this might happen on
> > other SoCs
> > later on.
> > 
> > My understanding is that even though both mt8186 and mt8192 output in
> > the rgb888
> > dual edge format, only mt8186 is able to configure the edge setting
> > in MMSYS (so
> > on mt8192 it would be hardwired to dual edge and not possible to
> > change). So
> > what I propose is
> > 
> > Name: edge_cfg_in_mmsys
> > 
> > Description: "If the edge configuration for DPI's output needs to be
> > set in MMSYS"
> > 
> > But maybe since you know the hardware, you might be able to find an
> > even better
> > name/description.
> > 
> > Thanks,
> > Nícolas
> Hi Nícolas:
> 
> Thanks for your suggestion.
> 
> At present, it is true that only 8186 needs to set this flag when
> outputting dual_edge format. 
> If other ICs need to modify the output format, they only need to modify
> the DPI register. 
> On the 8186, DPI MUX (0x400) is required for synchronous modification.
> A more detailed explanation of this DPI MUX register is 
> bit[0]: dual_edge enable, bit[1]: rgb565_en.
> And the priority of bit[1] is higher, the following is the format of
> different combinations:
> 00: SDR enable
> 01: DDR enable
> 10: RGB565
> 11: RGB565
>  
> The hardware characteristics can be ignored. Based on this situation,
> what is your opinion if it is changed to "edge_cfg_in_mmsys"?

Hi Xinlei,

thank you for the detailed explanation of the situation. Based on this, I still
think the name I suggested, "edge_cfg_in_mmsys", and its description, make
sense. If you're also happy with this name and description, then let's go with
that.

Thanks,
Nícolas

Re: [PATCH v3,2/2] drm: mediatek: Adjust the dpi output format to MT8186

[xinlei.lee]

On Fri, 2022-08-26 at 10:13 -0400, Nícolas F. R. A. Prado wrote:
> On Fri, Aug 26, 2022 at 11:58:29AM +0800, xinlei.lee wrote:
> > On Wed, 2022-08-24 at 11:44 -0400, Nícolas F. R. A. Prado wrote:
> > > On Wed, Aug 24, 2022 at 09:59:21AM +0800, xinlei.lee wrote:
> > > > On Tue, 2022-08-23 at 16:16 -0400, Nícolas F. R. A. Prado
> > > > wrote:
> > > > > On Tue, Aug 23, 2022 at 02:18:37PM +0800, 
> > > > > xinlei.lee@mediatek.com
> > > > > wrote:
> > > > > > From: Xinlei Lee <xinlei.lee@mediatek.com>
> > > > > > 
> > > > > > Dpi output needs to adjust the output format to dual edge
> > > > > > for
> > > > > > MT8186.
> > > > > > Because MT8186 HW has been modified at that time, SW needs
> > > > > > to
> > > > > > cooperate.
> > > > > > And the register (MMSYS) reserved for dpi will be used for
> > > > > > output
> > > > > > format control (dual_edge/single_edge).
> > > > > > 
> > > > > > Co-developed-by: Jitao Shi <jitao.shi@mediatek.com>
> > > > > > Signed-off-by: Jitao Shi <jitao.shi@mediatek.com>
> > > > > > Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> > > > > > 
> > > > > > ---
> > > > > 
> > > > > [..]
> > > > > > --- a/drivers/gpu/drm/mediatek/mtk_dpi.c
> > > > > > +++ b/drivers/gpu/drm/mediatek/mtk_dpi.c
> > > > > 
> > > > > [..]
> > > > > >   * @yuv422_en_bit: Enable bit of yuv422.
> > > > > >   * @csc_enable_bit: Enable bit of CSC.
> > > > > >   * @pixels_per_iter: Quantity of transferred pixels per
> > > > > > iteration.
> > > > > > + * @rgb888_dual_enable: Control output format for mt8186.
> > > > > 
> > > > > Let's not mention mt8186 in the description to keep the
> > > > > property
> > > > > generic. Also,
> > > > > this description should say what having 'rgb888_dual_enable =
> > > > > true'
> > > > > indicates
> > > > > about the hardware (in this case mt8186) and it currently
> > > > > doesn't.
> > > > > 
> > > > > Let's take a step back. What does 'dual enable' mean in this
> > > > > context
> > > > > and how
> > > > > does it relate to 'dual edge' and the dpi output format? By
> > > > > answering
> > > > > those
> > > > > questions we can find a description (and maybe variable name)
> > > > > that
> > > > > makes more
> > > > > sense.
> > > > > 
> > > > > >   */
> > > > > 
> > > > > [..]
> > > > > > @@ -449,6 +454,9 @@ static void mtk_dpi_dual_edge(struct
> > > > > > mtk_dpi
> > > > > > *dpi)
> > > > > >  		mtk_dpi_mask(dpi, DPI_OUTPUT_SETTING,
> > > > > >  			     dpi->output_fmt ==
> > > > > > MEDIA_BUS_FMT_RGB888_2X12_LE ?
> > > > > >  			     EDGE_SEL : 0, EDGE_SEL);
> > > > > > +	if (dpi->conf->rgb888_dual_enable)
> > > > > > +		mtk_mmsys_ddp_dpi_fmt_config(dpi->mmsys_dev,
> > > > > > DPI_RGB888_DDR_CON,
> > > > > > +					     DPI_FORMAT_MASK,
> > > > > > NULL);
> > > > > 
> > > > > This if block should be further indented.
> > > > > 
> > > > > >  	} else {
> > > > > >  		mtk_dpi_mask(dpi, DPI_DDR_SETTING, DDR_EN |
> > > > > > DDR_4PHASE,
> > > > > > 0);
> > > > > >  	}
> > > > > 
> > > > > [..]
> > > > > > --- 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_FORMAT_MASK			0x1
> > > > > > +#define DPI_RGB888_DDR_CON		BIT(0)
> > > > > > +#define DPI_RGB565_SDR_CON		BIT(1)
> > > > > 
> > > > > I'm not sure if it would make more sense to have these
> > > > > definitions in
> > > > > the mmsys
> > > > > header since they're configurations of a register in mmsys'
> > > > > iospace... I think
> > > > > we can keep them here but at least add a comment above:
> > > > > 
> > > > > /* Values for DPI configuration in MMSYS address space */
> > > > > 
> > > > > Thanks,
> > > > > Nícolas
> > > > 
> > > > Hi Nícolas:
> > > > Thanks for your careful review!
> > > > I will modify the description of this member variable and add
> > > > the
> > > > hardware state corresponding to the software setting.
> > > > (eg. rgb888_dual_enable = true the hardware output
> > > > rgb888_dual_edge
> > > > format data)
> > > > 
> > > > Your suggestion is very necessary, maybe my name is not
> > > > accurate
> > > > enough, this flag is to enable RGB888_dual_edge format output. 
> > > > Would it be better for the variable to be called
> > > > RGB888_dual_edge_enable then?
> > > 
> > > The thing is, we also output in rgb888 dual edge format on mt8183
> > > and
> > > mt8192,
> > > and therefore set DDR_EN in mtk_dpi_dual_edge(), right? But, as
> > > you
> > > said, we
> > > don't need to enable this new rgb888_dual_enable variable on
> > > those
> > > platforms,
> > > only on mt8186. So that's why I don't think the current
> > > name/description is
> > > suitable. If the variable only needs to be set on mt8186, it
> > > should
> > > have a name
> > > and description that shows what is different between mt8186 and
> > > the
> > > others. But
> > > without containing the "mt8186" name, since this might happen on
> > > other SoCs
> > > later on.
> > > 
> > > My understanding is that even though both mt8186 and mt8192
> > > output in
> > > the rgb888
> > > dual edge format, only mt8186 is able to configure the edge
> > > setting
> > > in MMSYS (so
> > > on mt8192 it would be hardwired to dual edge and not possible to
> > > change). So
> > > what I propose is
> > > 
> > > Name: edge_cfg_in_mmsys
> > > 
> > > Description: "If the edge configuration for DPI's output needs to
> > > be
> > > set in MMSYS"
> > > 
> > > But maybe since you know the hardware, you might be able to find
> > > an
> > > even better
> > > name/description.
> > > 
> > > Thanks,
> > > Nícolas
> > 
> > Hi Nícolas:
> > 
> > Thanks for your suggestion.
> > 
> > At present, it is true that only 8186 needs to set this flag when
> > outputting dual_edge format. 
> > If other ICs need to modify the output format, they only need to
> > modify
> > the DPI register. 
> > On the 8186, DPI MUX (0x400) is required for synchronous
> > modification.
> > A more detailed explanation of this DPI MUX register is 
> > bit[0]: dual_edge enable, bit[1]: rgb565_en.
> > And the priority of bit[1] is higher, the following is the format
> > of
> > different combinations:
> > 00: SDR enable
> > 01: DDR enable
> > 10: RGB565
> > 11: RGB565
> >  
> > The hardware characteristics can be ignored. Based on this
> > situation,
> > what is your opinion if it is changed to "edge_cfg_in_mmsys"?
> 
> Hi Xinlei,
> 
> thank you for the detailed explanation of the situation. Based on
> this, I still
> think the name I suggested, "edge_cfg_in_mmsys", and its description,
> make
> sense. If you're also happy with this name and description, then
> let's go with
> that.
> 
> Thanks,
> Nícolas

Hi Nícolas:

Okay, I'll send out the next edition based on what we communicated.

Best Regards!
Xinlei