[PATCH v12,0/3] Add dpi output format control for MT8186

[PATCH v12,0/3] Add dpi output format control for MT8186

[xinlei.lee]

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

Base on the branch of linus/master v6.1 rc1.

Change since v11:
1. Rebase on v6.1-rc1. Change nothing.

Change since v10:
1. Modify patch title and add review tag.

Change since v9:
1. Modify the location of the mmsys_dev member variable.

Change since v8:
1. Modified the title and some description information.

Changes since v7:
1. This series is based on the following patch:
   [1] soc: mediatek: Add mmsys func to adapt to dpi output for MT8186
   https://patchwork.kernel.org/project/linux-mediatek/patch/1663161662-1598-2-git-send-email-xinlei.lee@mediatek.com/
2. Modify the DPI_FORMAT_MASK macro definition to GENMASK(1, 0);
3. Add all settings to mtk_mmsys_ddp_dpi_fmt_config;
4. Modify the commit title to Add mt8186 dpi compatibles and platform
data.

Changes since v6:
1. Different from other ICs, when mt8186 DPI changes the output format,
the mmsys_base+400 register needs to be set to be valid at the same
time.
   In this series, all the situations that mmsys need to be set up are
perfected (not necessarily used in practice).
2. Put the value that controls the mmsys function in mtk-mmsys.h.
3. Encountered the sink ic switched between dual edge and single edge,
perfected setting and clearing mmsys bit operations in mtk_dpi.c.

Changes since v5:
1. Separate the patch that adds edge_cfg_in_mmsys from the patch that
adds mt8186 dpi support.
2. Move the mmsys register definition to mmsys driver.
 
Changes since v4:
1. This series of cancellations is based on the following patches:
   [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/
2. Added mtk_mmsys_update_bits function in mtk-mmsys.c;
3. MMSYS 0x400 register is modified to MT8186_MMSYS_DPI_OUTPUT_FORMAT;
4. Fix formatting issues.

Changes since v3:
1. Fix formatting issues;
2. Modify the edge output control name & description;
3. Fix the threading problem.

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 (3):
  soc: mediatek: Add all settings to mtk_mmsys_ddp_dpi_fmt_config func
  drm: mediatek: Set dpi format in mmsys
  drm: mediatek: Add mt8186 dpi compatibles and platform data

 drivers/gpu/drm/mediatek/mtk_dpi.c     | 32 ++++++++++++++++++++++++++
 drivers/gpu/drm/mediatek/mtk_drm_drv.c |  2 ++
 drivers/soc/mediatek/mt8186-mmsys.h    |  8 ++++---
 drivers/soc/mediatek/mtk-mmsys.c       | 27 +++++++++++++++++-----
 include/linux/soc/mediatek/mtk-mmsys.h |  7 ++++++
 5 files changed, 67 insertions(+), 9 deletions(-)

-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v12,1/3] soc: mediatek: Add all settings to mtk_mmsys_ddp_dpi_fmt_config func

[xinlei.lee]

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

The difference between MT8186 and other ICs is that when modifying the
output format, we need to modify the mmsys_base+0x400 register to take
effect.
So when setting the dpi output format, we need to call mmsys_func to set
it to MT8186 synchronously.
Adding mmsys all the settings that need to be modified with dpi are for
mt8186.

Fixes: a071e52f75d1 ("soc: mediatek: Add mmsys func to adapt to dpi
output for MT8186")

Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Reviewed-by: CK Hu <ck.hu@mediatek.com>
---
 drivers/soc/mediatek/mt8186-mmsys.h    |  8 +++++---
 drivers/soc/mediatek/mtk-mmsys.c       | 27 ++++++++++++++++++++------
 include/linux/soc/mediatek/mtk-mmsys.h |  7 +++++++
 3 files changed, 33 insertions(+), 9 deletions(-)

diff --git a/drivers/soc/mediatek/mt8186-mmsys.h b/drivers/soc/mediatek/mt8186-mmsys.h
index 09b1ccbc0093..035aec1eb616 100644
--- a/drivers/soc/mediatek/mt8186-mmsys.h
+++ b/drivers/soc/mediatek/mt8186-mmsys.h
@@ -5,9 +5,11 @@
 
 /* Values for DPI configuration in MMSYS address space */
 #define MT8186_MMSYS_DPI_OUTPUT_FORMAT		0x400
-#define DPI_FORMAT_MASK					0x1
-#define DPI_RGB888_DDR_CON				BIT(0)
-#define DPI_RGB565_SDR_CON				BIT(1)
+#define DPI_FORMAT_MASK					GENMASK(1, 0)
+#define DPI_RGB888_SDR_CON				0
+#define DPI_RGB888_DDR_CON				1
+#define DPI_RGB565_SDR_CON				2
+#define DPI_RGB565_DDR_CON				3
 
 #define MT8186_MMSYS_OVL_CON			0xF04
 #define MT8186_MMSYS_OVL0_CON_MASK			0x3
diff --git a/drivers/soc/mediatek/mtk-mmsys.c b/drivers/soc/mediatek/mtk-mmsys.c
index d2c7a87aab87..205f6de45851 100644
--- a/drivers/soc/mediatek/mtk-mmsys.c
+++ b/drivers/soc/mediatek/mtk-mmsys.c
@@ -238,12 +238,27 @@ static void mtk_mmsys_update_bits(struct mtk_mmsys *mmsys, u32 offset, u32 mask,
 
 void mtk_mmsys_ddp_dpi_fmt_config(struct device *dev, u32 val)
 {
-	if (val)
-		mtk_mmsys_update_bits(dev_get_drvdata(dev), MT8186_MMSYS_DPI_OUTPUT_FORMAT,
-				      DPI_RGB888_DDR_CON, DPI_FORMAT_MASK);
-	else
-		mtk_mmsys_update_bits(dev_get_drvdata(dev), MT8186_MMSYS_DPI_OUTPUT_FORMAT,
-				      DPI_RGB565_SDR_CON, DPI_FORMAT_MASK);
+	struct mtk_mmsys *mmsys = dev_get_drvdata(dev);
+
+	switch (val) {
+	case MTK_DPI_RGB888_SDR_CON:
+		mtk_mmsys_update_bits(mmsys, MT8186_MMSYS_DPI_OUTPUT_FORMAT,
+				      DPI_FORMAT_MASK, DPI_RGB888_SDR_CON);
+		break;
+	case MTK_DPI_RGB565_SDR_CON:
+		mtk_mmsys_update_bits(mmsys, MT8186_MMSYS_DPI_OUTPUT_FORMAT,
+				      DPI_FORMAT_MASK, DPI_RGB565_SDR_CON);
+		break;
+	case MTK_DPI_RGB565_DDR_CON:
+		mtk_mmsys_update_bits(mmsys, MT8186_MMSYS_DPI_OUTPUT_FORMAT,
+				      DPI_FORMAT_MASK, DPI_RGB565_DDR_CON);
+		break;
+	case MTK_DPI_RGB888_DDR_CON:
+	default:
+		mtk_mmsys_update_bits(mmsys, MT8186_MMSYS_DPI_OUTPUT_FORMAT,
+				      DPI_FORMAT_MASK, DPI_RGB888_DDR_CON);
+		break;
+	}
 }
 EXPORT_SYMBOL_GPL(mtk_mmsys_ddp_dpi_fmt_config);
 
diff --git a/include/linux/soc/mediatek/mtk-mmsys.h b/include/linux/soc/mediatek/mtk-mmsys.h
index d2b02bb43768..b85f66db33e1 100644
--- a/include/linux/soc/mediatek/mtk-mmsys.h
+++ b/include/linux/soc/mediatek/mtk-mmsys.h
@@ -9,6 +9,13 @@
 enum mtk_ddp_comp_id;
 struct device;
 
+enum mtk_dpi_out_format_con {
+	MTK_DPI_RGB888_SDR_CON,
+	MTK_DPI_RGB888_DDR_CON,
+	MTK_DPI_RGB565_SDR_CON,
+	MTK_DPI_RGB565_DDR_CON
+};
+
 enum mtk_ddp_comp_id {
 	DDP_COMPONENT_AAL0,
 	DDP_COMPONENT_AAL1,
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v12,2/3] drm: mediatek: Set dpi format in mmsys

[xinlei.lee]

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

Dpi output needs to adjust the output format to dual edge for MT8186.

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>
Reviewed-by: CK Hu <ck.hu@mediatek.com>
---
 drivers/gpu/drm/mediatek/mtk_dpi.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c b/drivers/gpu/drm/mediatek/mtk_dpi.c
index 508a6d994e83..b9d740efdb6a 100644
--- a/drivers/gpu/drm/mediatek/mtk_dpi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dpi.c
@@ -14,6 +14,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>
@@ -29,6 +30,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,
@@ -66,6 +68,7 @@ struct mtk_dpi {
 	struct drm_connector *connector;
 	void __iomem *regs;
 	struct device *dev;
+	struct device *mmsys_dev;
 	struct clk *engine_clk;
 	struct clk *pixel_clk;
 	struct clk *tvd_clk;
@@ -134,6 +137,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.
+ * @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);
@@ -152,6 +156,7 @@ struct mtk_dpi_conf {
 	u32 yuv422_en_bit;
 	u32 csc_enable_bit;
 	u32 pixels_per_iter;
+	bool edge_cfg_in_mmsys;
 };
 
 static void mtk_dpi_mask(struct mtk_dpi *dpi, u32 offset, u32 val, u32 mask)
@@ -448,8 +453,12 @@ 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->edge_cfg_in_mmsys)
+			mtk_mmsys_ddp_dpi_fmt_config(dpi->mmsys_dev, MTK_DPI_RGB888_DDR_CON);
 	} else {
 		mtk_dpi_mask(dpi, DPI_DDR_SETTING, DDR_EN | DDR_4PHASE, 0);
+		if (dpi->conf->edge_cfg_in_mmsys)
+			mtk_mmsys_ddp_dpi_fmt_config(dpi->mmsys_dev, MTK_DPI_RGB888_SDR_CON);
 	}
 }
 
@@ -777,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) {
-- 
2.18.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v12,3/3] drm: mediatek: Add mt8186 dpi compatibles and platform data

[xinlei.lee]

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

Add the compatible because use edge_cfg_in_mmsys in mt8186.

Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
Reviewed-by: CK Hu <ck.hu@mediatek.com>
Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/gpu/drm/mediatek/mtk_dpi.c     | 21 +++++++++++++++++++++
 drivers/gpu/drm/mediatek/mtk_drm_drv.c |  2 ++
 2 files changed, 23 insertions(+)

diff --git a/drivers/gpu/drm/mediatek/mtk_dpi.c b/drivers/gpu/drm/mediatek/mtk_dpi.c
index b9d740efdb6a..fa39e37caeb3 100644
--- a/drivers/gpu/drm/mediatek/mtk_dpi.c
+++ b/drivers/gpu/drm/mediatek/mtk_dpi.c
@@ -940,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),
+	.edge_cfg_in_mmsys = 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,
@@ -1090,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_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
index 91f58db5915f..a5a1a91429ec 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
@@ -631,6 +631,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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v12,2/3] drm: mediatek: Set dpi format in mmsys

[AngeloGioacchino Del Regno]

Il 19/10/22 04:52, xinlei.lee@mediatek.com ha scritto:
> From: Xinlei Lee <xinlei.lee@mediatek.com>
> 
> Dpi output needs to adjust the output format to dual edge for MT8186.
> 
> 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>
> Reviewed-by: CK Hu <ck.hu@mediatek.com>

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



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v12,1/3] soc: mediatek: Add all settings to mtk_mmsys_ddp_dpi_fmt_config func

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

Hi,

On Wed, Oct 19, 2022 at 10:52:14AM +0800, xinlei.lee@mediatek.com wrote:
> From: Xinlei Lee <xinlei.lee@mediatek.com>
> 
> The difference between MT8186 and other ICs is that when modifying the
> output format, we need to modify the mmsys_base+0x400 register to take
> effect.
> So when setting the dpi output format, we need to call mmsys_func to set

mmsys_func isn't something that exists in the code. Instead mention the actual
function name: mtk_mmsys_ddp_dpi_fmt_config.

> it to MT8186 synchronously.


Here, before saying that the commit adds all the settings for dpi, you could
have mentioned that the previous commit lacked those, to make it clearer:

Commit a071e52f75d1 ("soc: mediatek: Add mmsys func to adapt to dpi output for MT8186")
lacked some of the possible output formats and also had a wrong bitmask.


> Adding mmsys all the settings that need to be modified with dpi are for
> mt8186.

This sentence I would change to the following one:

Add the missing output formats and fix the bitmask.


Finally, you're also making the function more HW-agnostic (although in my
opinion this could've been a future separate commit), so it's worth mentioning
it here:

While at it, also update mtk_mmsys_ddp_dpi_fmt_config() to use generic formats,
so that it is slightly easier to extend for other platforms.

> 
> Fixes: a071e52f75d1 ("soc: mediatek: Add mmsys func to adapt to dpi
> output for MT8186")

The fixes tag should be kept in a single line, without wrapping.

> 
> Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> Reviewed-by: CK Hu <ck.hu@mediatek.com>
> ---
>  drivers/soc/mediatek/mt8186-mmsys.h    |  8 +++++---
>  drivers/soc/mediatek/mtk-mmsys.c       | 27 ++++++++++++++++++++------
>  include/linux/soc/mediatek/mtk-mmsys.h |  7 +++++++
>  3 files changed, 33 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/soc/mediatek/mt8186-mmsys.h b/drivers/soc/mediatek/mt8186-mmsys.h
> index 09b1ccbc0093..035aec1eb616 100644
> --- a/drivers/soc/mediatek/mt8186-mmsys.h
> +++ b/drivers/soc/mediatek/mt8186-mmsys.h
> @@ -5,9 +5,11 @@
>  
>  /* Values for DPI configuration in MMSYS address space */
>  #define MT8186_MMSYS_DPI_OUTPUT_FORMAT		0x400
> -#define DPI_FORMAT_MASK					0x1
> -#define DPI_RGB888_DDR_CON				BIT(0)
> -#define DPI_RGB565_SDR_CON				BIT(1)
> +#define DPI_FORMAT_MASK					GENMASK(1, 0)
> +#define DPI_RGB888_SDR_CON				0
> +#define DPI_RGB888_DDR_CON				1
> +#define DPI_RGB565_SDR_CON				2
> +#define DPI_RGB565_DDR_CON				3

These defines should all have a MT8186_ prefix. This will avoid confusions now
that mtk_mmsys_ddp_dpi_fmt_config() is being made more platform-agnostic.

>  
>  #define MT8186_MMSYS_OVL_CON			0xF04
>  #define MT8186_MMSYS_OVL0_CON_MASK			0x3
> diff --git a/drivers/soc/mediatek/mtk-mmsys.c b/drivers/soc/mediatek/mtk-mmsys.c
> index d2c7a87aab87..205f6de45851 100644
> --- a/drivers/soc/mediatek/mtk-mmsys.c
> +++ b/drivers/soc/mediatek/mtk-mmsys.c
> @@ -238,12 +238,27 @@ static void mtk_mmsys_update_bits(struct mtk_mmsys *mmsys, u32 offset, u32 mask,
>  
>  void mtk_mmsys_ddp_dpi_fmt_config(struct device *dev, u32 val)
>  {
> -	if (val)
> -		mtk_mmsys_update_bits(dev_get_drvdata(dev), MT8186_MMSYS_DPI_OUTPUT_FORMAT,
> -				      DPI_RGB888_DDR_CON, DPI_FORMAT_MASK);
> -	else
> -		mtk_mmsys_update_bits(dev_get_drvdata(dev), MT8186_MMSYS_DPI_OUTPUT_FORMAT,
> -				      DPI_RGB565_SDR_CON, DPI_FORMAT_MASK);
> +	struct mtk_mmsys *mmsys = dev_get_drvdata(dev);
> +
> +	switch (val) {
> +	case MTK_DPI_RGB888_SDR_CON:
> +		mtk_mmsys_update_bits(mmsys, MT8186_MMSYS_DPI_OUTPUT_FORMAT,
> +				      DPI_FORMAT_MASK, DPI_RGB888_SDR_CON);
> +		break;
> +	case MTK_DPI_RGB565_SDR_CON:
> +		mtk_mmsys_update_bits(mmsys, MT8186_MMSYS_DPI_OUTPUT_FORMAT,
> +				      DPI_FORMAT_MASK, DPI_RGB565_SDR_CON);
> +		break;
> +	case MTK_DPI_RGB565_DDR_CON:
> +		mtk_mmsys_update_bits(mmsys, MT8186_MMSYS_DPI_OUTPUT_FORMAT,
> +				      DPI_FORMAT_MASK, DPI_RGB565_DDR_CON);
> +		break;
> +	case MTK_DPI_RGB888_DDR_CON:
> +	default:
> +		mtk_mmsys_update_bits(mmsys, MT8186_MMSYS_DPI_OUTPUT_FORMAT,
> +				      DPI_FORMAT_MASK, DPI_RGB888_DDR_CON);
> +		break;
> +	}

To be honest I don't really see the point of making the function slightly more
platform-agnostic like this. With a single platform making use of it it's just
an unneeded extra abstraction, and it could easily be done when a second
platform starts requiring this as well...

In any case,

Reviewed-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>

Thanks,
Nícolas

>  }
[..]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v12,2/3] drm: mediatek: Set dpi format in mmsys

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

On Wed, Oct 19, 2022 at 10:52:15AM +0800, xinlei.lee@mediatek.com wrote:
[..]
> @@ -134,6 +137,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.
> + * @edge_cfg_in_mmsys: If the edge configuration for DPI's output needs to be set in MMSYS.

As Angelo suggested previously, this could be written slightly shorter as 

  * @edge_cfg_in_mmsys: Edge configuration for DPI output has to be set in MMSYS.

>   */
[..]
> @@ -448,8 +453,12 @@ 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->edge_cfg_in_mmsys)
> +			mtk_mmsys_ddp_dpi_fmt_config(dpi->mmsys_dev, MTK_DPI_RGB888_DDR_CON);
>  	} else {
>  		mtk_dpi_mask(dpi, DPI_DDR_SETTING, DDR_EN | DDR_4PHASE, 0);
> +		if (dpi->conf->edge_cfg_in_mmsys)
> +			mtk_mmsys_ddp_dpi_fmt_config(dpi->mmsys_dev, MTK_DPI_RGB888_SDR_CON);

I know this isn't one of the formats supported by MT8186, but since we're using
platform-agnostic formats now... This else branch in theory could also run for a
format like MEDIA_BUS_FMT_YUYV8_1X16. Would it make sense to set
MTK_DPI_RGB888_SDR_CON in that case?

Thanks,
Nícolas

>  	}
[..]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v12,3/3] drm: mediatek: Add mt8186 dpi compatibles and platform data

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

On Wed, Oct 19, 2022 at 10:52:16AM +0800, xinlei.lee@mediatek.com wrote:
> From: Xinlei Lee <xinlei.lee@mediatek.com>
> 
> Add the compatible because use edge_cfg_in_mmsys in mt8186.

I think the commit message could be improved:

Add support for mt8186. It needs its own mtk_dpi_conf data since
edge_cfg_in_mmsys is set to true for this platform.

> 
> Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> Reviewed-by: CK Hu <ck.hu@mediatek.com>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Reviewed-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>

Thanks,
Nícolas

> ---
[..]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v12,1/3] soc: mediatek: Add all settings to mtk_mmsys_ddp_dpi_fmt_config func

[xinlei.lee]

On Thu, 2022-10-20 at 12:33 -0400, Nícolas F. R. A. Prado wrote:
> Hi,
> 
> On Wed, Oct 19, 2022 at 10:52:14AM +0800, xinlei.lee@mediatek.com
> wrote:
> > From: Xinlei Lee <xinlei.lee@mediatek.com>
> > 
> > The difference between MT8186 and other ICs is that when modifying
> > the
> > output format, we need to modify the mmsys_base+0x400 register to
> > take
> > effect.
> > So when setting the dpi output format, we need to call mmsys_func
> > to set
> 
> mmsys_func isn't something that exists in the code. Instead mention
> the actual
> function name: mtk_mmsys_ddp_dpi_fmt_config.
> 
> > it to MT8186 synchronously.
> 
> 
> Here, before saying that the commit adds all the settings for dpi,
> you could
> have mentioned that the previous commit lacked those, to make it
> clearer:
> 
> Commit a071e52f75d1 ("soc: mediatek: Add mmsys func to adapt to dpi
> output for MT8186")
> lacked some of the possible output formats and also had a wrong
> bitmask.
> 
> 
> > Adding mmsys all the settings that need to be modified with dpi are
> > for
> > mt8186.
> 
> This sentence I would change to the following one:
> 
> Add the missing output formats and fix the bitmask.
> 
> 
> Finally, you're also making the function more HW-agnostic (although
> in my
> opinion this could've been a future separate commit), so it's worth
> mentioning
> it here:
> 
> While at it, also update mtk_mmsys_ddp_dpi_fmt_config() to use
> generic formats,
> so that it is slightly easier to extend for other platforms.
> 
> > 
> > Fixes: a071e52f75d1 ("soc: mediatek: Add mmsys func to adapt to dpi
> > output for MT8186")
> 
> The fixes tag should be kept in a single line, without wrapping.
> 
> > 
> > Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> > Reviewed-by: AngeloGioacchino Del Regno <
> > angelogioacchino.delregno@collabora.com>
> > Reviewed-by: CK Hu <ck.hu@mediatek.com>
> > ---
> >  drivers/soc/mediatek/mt8186-mmsys.h    |  8 +++++---
> >  drivers/soc/mediatek/mtk-mmsys.c       | 27 ++++++++++++++++++++
> > ------
> >  include/linux/soc/mediatek/mtk-mmsys.h |  7 +++++++
> >  3 files changed, 33 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/soc/mediatek/mt8186-mmsys.h
> > b/drivers/soc/mediatek/mt8186-mmsys.h
> > index 09b1ccbc0093..035aec1eb616 100644
> > --- a/drivers/soc/mediatek/mt8186-mmsys.h
> > +++ b/drivers/soc/mediatek/mt8186-mmsys.h
> > @@ -5,9 +5,11 @@
> >  
> >  /* Values for DPI configuration in MMSYS address space */
> >  #define MT8186_MMSYS_DPI_OUTPUT_FORMAT		0x400
> > -#define DPI_FORMAT_MASK					0x1
> > -#define DPI_RGB888_DDR_CON				BIT(0)
> > -#define DPI_RGB565_SDR_CON				BIT(1)
> > +#define DPI_FORMAT_MASK					GENMASK
> > (1, 0)
> > +#define DPI_RGB888_SDR_CON				0
> > +#define DPI_RGB888_DDR_CON				1
> > +#define DPI_RGB565_SDR_CON				2
> > +#define DPI_RGB565_DDR_CON				3
> 
> These defines should all have a MT8186_ prefix. This will avoid
> confusions now
> that mtk_mmsys_ddp_dpi_fmt_config() is being made more platform-
> agnostic.
> 
> >  
> >  #define MT8186_MMSYS_OVL_CON			0xF04
> >  #define MT8186_MMSYS_OVL0_CON_MASK			0x3
> > diff --git a/drivers/soc/mediatek/mtk-mmsys.c
> > b/drivers/soc/mediatek/mtk-mmsys.c
> > index d2c7a87aab87..205f6de45851 100644
> > --- a/drivers/soc/mediatek/mtk-mmsys.c
> > +++ b/drivers/soc/mediatek/mtk-mmsys.c
> > @@ -238,12 +238,27 @@ static void mtk_mmsys_update_bits(struct
> > mtk_mmsys *mmsys, u32 offset, u32 mask,
> >  
> >  void mtk_mmsys_ddp_dpi_fmt_config(struct device *dev, u32 val)
> >  {
> > -	if (val)
> > -		mtk_mmsys_update_bits(dev_get_drvdata(dev),
> > MT8186_MMSYS_DPI_OUTPUT_FORMAT,
> > -				      DPI_RGB888_DDR_CON,
> > DPI_FORMAT_MASK);
> > -	else
> > -		mtk_mmsys_update_bits(dev_get_drvdata(dev),
> > MT8186_MMSYS_DPI_OUTPUT_FORMAT,
> > -				      DPI_RGB565_SDR_CON,
> > DPI_FORMAT_MASK);
> > +	struct mtk_mmsys *mmsys = dev_get_drvdata(dev);
> > +
> > +	switch (val) {
> > +	case MTK_DPI_RGB888_SDR_CON:
> > +		mtk_mmsys_update_bits(mmsys,
> > MT8186_MMSYS_DPI_OUTPUT_FORMAT,
> > +				      DPI_FORMAT_MASK,
> > DPI_RGB888_SDR_CON);
> > +		break;
> > +	case MTK_DPI_RGB565_SDR_CON:
> > +		mtk_mmsys_update_bits(mmsys,
> > MT8186_MMSYS_DPI_OUTPUT_FORMAT,
> > +				      DPI_FORMAT_MASK,
> > DPI_RGB565_SDR_CON);
> > +		break;
> > +	case MTK_DPI_RGB565_DDR_CON:
> > +		mtk_mmsys_update_bits(mmsys,
> > MT8186_MMSYS_DPI_OUTPUT_FORMAT,
> > +				      DPI_FORMAT_MASK,
> > DPI_RGB565_DDR_CON);
> > +		break;
> > +	case MTK_DPI_RGB888_DDR_CON:
> > +	default:
> > +		mtk_mmsys_update_bits(mmsys,
> > MT8186_MMSYS_DPI_OUTPUT_FORMAT,
> > +				      DPI_FORMAT_MASK,
> > DPI_RGB888_DDR_CON);
> > +		break;
> > +	}
> 
> To be honest I don't really see the point of making the function
> slightly more
> platform-agnostic like this. With a single platform making use of it
> it's just
> an unneeded extra abstraction, and it could easily be done when a
> second
> platform starts requiring this as well...
> 
> In any case,
> 
> Reviewed-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> 
> Thanks,
> Nícolas
> 
> >  }
> 
> [..]

Hi Nícolas:

Thanks for your detailed reply and correction.
Before sending out the next edition, I have two questions I would like 
to confirm with you in response to your responses:
1.While at it, also update mtk_mmsys_ddp_dpi_fmt_config() to use 
generic formats, so that it is slightly easier to extend for other 
platforms.
=> This is to make this mtk_mmsys_ddp_dpi_fmt_config() func more 
general? 
This function may only be used by MT8186, because only MT8186
has 
corresponding modifications on HW, and enables the registers reserved 
in mmsys for dpi use to control the output format. Because this 
register is not defined for other ic, I added control to this function 
call in mtk_dpi.c. If you think there are other ways to make it look 
more generic, where should I correct it?

2. These definitions should all have a MT8186_ prefix. This will avoid 
confusion as mtk_mmsys_ddp_dpi_fmt_config() becomes more platform 
independent.

Honestly, I don't really see the point of making the feature platform-
agnostic like this. Using it on a single platform is just an extra 
abstraction that isn't needed, when a second platform starts needing 
it too, it can be done easily...

=> My understanding here is that prefixing variables with labels is 
more conducive to making functions generic, and can be reused if there 
is such a situation in the future. I understand the importance of 
keeping the function platform agnostic, but as mentioned, it may only 
be used by the MT8186 if there are special cases where other ICs may 
rely on mtk_mmsys_update_bits to create new functions.

The above content is only my understanding. If you have any questions 
or suggestions, we will communicate again.

Looking forward to your reply.

Best Regards!
xinlei


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v12,2/3] drm: mediatek: Set dpi format in mmsys

[xinlei.lee]

On Thu, 2022-10-20 at 12:40 -0400, Nícolas F. R. A. Prado wrote:
> On Wed, Oct 19, 2022 at 10:52:15AM +0800, xinlei.lee@mediatek.com
> wrote:
> [..]
> > @@ -134,6 +137,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.
> > + * @edge_cfg_in_mmsys: If the edge configuration for DPI's output
> > needs to be set in MMSYS.
> 
> As Angelo suggested previously, this could be written slightly
> shorter as 
> 
>   * @edge_cfg_in_mmsys: Edge configuration for DPI output has to be
> set in MMSYS.
> 
> >   */
> 
> [..]
> > @@ -448,8 +453,12 @@ 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->edge_cfg_in_mmsys)
> > +			mtk_mmsys_ddp_dpi_fmt_config(dpi->mmsys_dev,
> > MTK_DPI_RGB888_DDR_CON);
> >  	} else {
> >  		mtk_dpi_mask(dpi, DPI_DDR_SETTING, DDR_EN | DDR_4PHASE,
> > 0);
> > +		if (dpi->conf->edge_cfg_in_mmsys)
> > +			mtk_mmsys_ddp_dpi_fmt_config(dpi->mmsys_dev,
> > MTK_DPI_RGB888_SDR_CON);
> 
> I know this isn't one of the formats supported by MT8186, but since
> we're using
> platform-agnostic formats now... This else branch in theory could
> also run for a
> format like MEDIA_BUS_FMT_YUYV8_1X16. Would it make sense to set
> MTK_DPI_RGB888_SDR_CON in that case?
> 
> Thanks,
> Nícolas
> 
> >  	}
> 
> [..]

Hi Nícolas:

Thanks for your review!
 
You are right, I understand you think this MTK_DPI_RGB888_SDR_CON 
format seems useless as it will not be set, I confirmed with the 
designer how the setting in mmsys affects the output format of the 
MT8186, this mmsys setting will not be used by other ICs.

As mentioned earlier, the mmsys setting will make the MT8186dpi have 
four output formats, even though the MT8186 dpi may not use them all.

So what needs to change here?

Best Regards!
xinlei


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v12,1/3] soc: mediatek: Add all settings to mtk_mmsys_ddp_dpi_fmt_config func

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

On Fri, Oct 21, 2022 at 07:59:02PM +0800, xinlei.lee wrote:
> On Thu, 2022-10-20 at 12:33 -0400, Nícolas F. R. A. Prado wrote:
> > Hi,
> > 
> > On Wed, Oct 19, 2022 at 10:52:14AM +0800, xinlei.lee@mediatek.com
> > wrote:
> > > From: Xinlei Lee <xinlei.lee@mediatek.com>
> > > 
> > > The difference between MT8186 and other ICs is that when modifying
> > > the
> > > output format, we need to modify the mmsys_base+0x400 register to
> > > take
> > > effect.
> > > So when setting the dpi output format, we need to call mmsys_func
> > > to set
> > 
> > mmsys_func isn't something that exists in the code. Instead mention
> > the actual
> > function name: mtk_mmsys_ddp_dpi_fmt_config.
> > 
> > > it to MT8186 synchronously.
> > 
> > 
> > Here, before saying that the commit adds all the settings for dpi,
> > you could
> > have mentioned that the previous commit lacked those, to make it
> > clearer:
> > 
> > Commit a071e52f75d1 ("soc: mediatek: Add mmsys func to adapt to dpi
> > output for MT8186")
> > lacked some of the possible output formats and also had a wrong
> > bitmask.
> > 
> > 
> > > Adding mmsys all the settings that need to be modified with dpi are
> > > for
> > > mt8186.
> > 
> > This sentence I would change to the following one:
> > 
> > Add the missing output formats and fix the bitmask.
> > 
> > 
> > Finally, you're also making the function more HW-agnostic (although
> > in my
> > opinion this could've been a future separate commit), so it's worth
> > mentioning
> > it here:
> > 
> > While at it, also update mtk_mmsys_ddp_dpi_fmt_config() to use
> > generic formats,
> > so that it is slightly easier to extend for other platforms.
> > 
> > > 
> > > Fixes: a071e52f75d1 ("soc: mediatek: Add mmsys func to adapt to dpi
> > > output for MT8186")
> > 
> > The fixes tag should be kept in a single line, without wrapping.
> > 
> > > 
> > > Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> > > Reviewed-by: AngeloGioacchino Del Regno <
> > > angelogioacchino.delregno@collabora.com>
> > > Reviewed-by: CK Hu <ck.hu@mediatek.com>
> > > ---
> > >  drivers/soc/mediatek/mt8186-mmsys.h    |  8 +++++---
> > >  drivers/soc/mediatek/mtk-mmsys.c       | 27 ++++++++++++++++++++
> > > ------
> > >  include/linux/soc/mediatek/mtk-mmsys.h |  7 +++++++
> > >  3 files changed, 33 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/soc/mediatek/mt8186-mmsys.h
> > > b/drivers/soc/mediatek/mt8186-mmsys.h
> > > index 09b1ccbc0093..035aec1eb616 100644
> > > --- a/drivers/soc/mediatek/mt8186-mmsys.h
> > > +++ b/drivers/soc/mediatek/mt8186-mmsys.h
> > > @@ -5,9 +5,11 @@
> > >  
> > >  /* Values for DPI configuration in MMSYS address space */
> > >  #define MT8186_MMSYS_DPI_OUTPUT_FORMAT		0x400
> > > -#define DPI_FORMAT_MASK					0x1
> > > -#define DPI_RGB888_DDR_CON				BIT(0)
> > > -#define DPI_RGB565_SDR_CON				BIT(1)
> > > +#define DPI_FORMAT_MASK					GENMASK
> > > (1, 0)
> > > +#define DPI_RGB888_SDR_CON				0
> > > +#define DPI_RGB888_DDR_CON				1
> > > +#define DPI_RGB565_SDR_CON				2
> > > +#define DPI_RGB565_DDR_CON				3
> > 
> > These defines should all have a MT8186_ prefix. This will avoid
> > confusions now
> > that mtk_mmsys_ddp_dpi_fmt_config() is being made more platform-
> > agnostic.
> > 
> > >  
> > >  #define MT8186_MMSYS_OVL_CON			0xF04
> > >  #define MT8186_MMSYS_OVL0_CON_MASK			0x3
> > > diff --git a/drivers/soc/mediatek/mtk-mmsys.c
> > > b/drivers/soc/mediatek/mtk-mmsys.c
> > > index d2c7a87aab87..205f6de45851 100644
> > > --- a/drivers/soc/mediatek/mtk-mmsys.c
> > > +++ b/drivers/soc/mediatek/mtk-mmsys.c
> > > @@ -238,12 +238,27 @@ static void mtk_mmsys_update_bits(struct
> > > mtk_mmsys *mmsys, u32 offset, u32 mask,
> > >  
> > >  void mtk_mmsys_ddp_dpi_fmt_config(struct device *dev, u32 val)
> > >  {
> > > -	if (val)
> > > -		mtk_mmsys_update_bits(dev_get_drvdata(dev),
> > > MT8186_MMSYS_DPI_OUTPUT_FORMAT,
> > > -				      DPI_RGB888_DDR_CON,
> > > DPI_FORMAT_MASK);
> > > -	else
> > > -		mtk_mmsys_update_bits(dev_get_drvdata(dev),
> > > MT8186_MMSYS_DPI_OUTPUT_FORMAT,
> > > -				      DPI_RGB565_SDR_CON,
> > > DPI_FORMAT_MASK);
> > > +	struct mtk_mmsys *mmsys = dev_get_drvdata(dev);
> > > +
> > > +	switch (val) {
> > > +	case MTK_DPI_RGB888_SDR_CON:
> > > +		mtk_mmsys_update_bits(mmsys,
> > > MT8186_MMSYS_DPI_OUTPUT_FORMAT,
> > > +				      DPI_FORMAT_MASK,
> > > DPI_RGB888_SDR_CON);
> > > +		break;
> > > +	case MTK_DPI_RGB565_SDR_CON:
> > > +		mtk_mmsys_update_bits(mmsys,
> > > MT8186_MMSYS_DPI_OUTPUT_FORMAT,
> > > +				      DPI_FORMAT_MASK,
> > > DPI_RGB565_SDR_CON);
> > > +		break;
> > > +	case MTK_DPI_RGB565_DDR_CON:
> > > +		mtk_mmsys_update_bits(mmsys,
> > > MT8186_MMSYS_DPI_OUTPUT_FORMAT,
> > > +				      DPI_FORMAT_MASK,
> > > DPI_RGB565_DDR_CON);
> > > +		break;
> > > +	case MTK_DPI_RGB888_DDR_CON:
> > > +	default:
> > > +		mtk_mmsys_update_bits(mmsys,
> > > MT8186_MMSYS_DPI_OUTPUT_FORMAT,
> > > +				      DPI_FORMAT_MASK,
> > > DPI_RGB888_DDR_CON);
> > > +		break;
> > > +	}
> > 
> > To be honest I don't really see the point of making the function
> > slightly more
> > platform-agnostic like this. With a single platform making use of it
> > it's just
> > an unneeded extra abstraction, and it could easily be done when a
> > second
> > platform starts requiring this as well...
> > 
> > In any case,
> > 
> > Reviewed-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> > 
> > Thanks,
> > Nícolas
> > 
> > >  }
> > 
> > [..]
> 
> Hi Nícolas:
> 
> Thanks for your detailed reply and correction.
> Before sending out the next edition, I have two questions I would like 
> to confirm with you in response to your responses:
> 1.While at it, also update mtk_mmsys_ddp_dpi_fmt_config() to use 
> generic formats, so that it is slightly easier to extend for other 
> platforms.
> => This is to make this mtk_mmsys_ddp_dpi_fmt_config() func more 
> general? 
> This function may only be used by MT8186, because only MT8186
> has 
> corresponding modifications on HW, and enables the registers reserved 
> in mmsys for dpi use to control the output format. Because this 
> register is not defined for other ic, I added control to this function 
> call in mtk_dpi.c. If you think there are other ways to make it look 
> more generic, where should I correct it?

You already made the mtk_mmsys_ddp_dpi_fmt_config() more generic by making it's
format parameter decoupled from its register representation on MT8186, that is,
MTK_DPI_RGB888_SDR_CON instead of DPI_RGB888_SDR_CON.

I wasn't asking for any code modification on that comment, I was suggesting you
add this sentence in the commit message, so it reflects the changes you're
already doing.

To be extra clear, I was suggesting you update the commit message to the
following:

  The difference between MT8186 and other ICs is that when modifying the output
  format, we need to modify the mmsys_base+0x400 register to take effect. So when
  setting the dpi output format, we need to call mtk_mmsys_ddp_dpi_fmt_config to
  set it to MT8186 synchronously.
  
  Commit a071e52f75d1 ("soc: mediatek: Add mmsys func to adapt to dpi output for
  MT8186") lacked some of the possible output formats and also had a wrong
  bitmask.
  
  Add the missing output formats and fix the bitmask.
  
  While at it, also update mtk_mmsys_ddp_dpi_fmt_config() to use generic formats,
  so that it is slightly easier to extend for other platforms.
  
  Fixes: a071e52f75d1 ("soc: mediatek: Add mmsys func to adapt to dpi output for MT8186")

> 
> 2. These definitions should all have a MT8186_ prefix. This will avoid 
> confusion as mtk_mmsys_ddp_dpi_fmt_config() becomes more platform 
> independent.
> 
> Honestly, I don't really see the point of making the feature platform-
> agnostic like this. Using it on a single platform is just an extra 
> abstraction that isn't needed, when a second platform starts needing 
> it too, it can be done easily...
> 
> => My understanding here is that prefixing variables with labels is 
> more conducive to making functions generic, and can be reused if there 
> is such a situation in the future. I understand the importance of 
> keeping the function platform agnostic, but as mentioned, it may only 
> be used by the MT8186 if there are special cases where other ICs may 
> rely on mtk_mmsys_update_bits to create new functions.

What I'm saying is that, even though you've made the function receive a generic
format as a parameter, like MTK_DPI_RGB888_SDR_CON, at this point in time
MT8186 is the only SoC that has a register in mmsys for it, so the values

DPI_FORMAT_MASK
DPI_RGB888_SDR_CON
DPI_RGB888_DDR_CON
DPI_RGB565_SDR_CON
DPI_RGB565_DDR_CON

are really all MT8186-specific, at least at this point. Leaving them without the
MT8186_ can give the false impression that they're already used elsewhere. Also
it's really easy to mistake them for the generic ones (like
MTK_DPI_RGB888_SDR_CON). MT8186_MMSYS_DPI_OUTPUT_FORMAT already has the MT8186_
prefix, so I'm really just saying that the other ones should have as well.

If/when the same address, mask or values for this register start being used on a
different SoC, then you can remove the prefix and move it to the mtk-mmsys.h
generic header.

But for now adding the prefixes will avoid confusion and make it clear this is
MT8186 specific.

Thanks,
Nícolas

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v12,2/3] drm: mediatek: Set dpi format in mmsys

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

On Fri, Oct 21, 2022 at 08:18:25PM +0800, xinlei.lee wrote:
> On Thu, 2022-10-20 at 12:40 -0400, Nícolas F. R. A. Prado wrote:
> > On Wed, Oct 19, 2022 at 10:52:15AM +0800, xinlei.lee@mediatek.com
> > wrote:
[..]
> > > @@ -448,8 +453,12 @@ 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->edge_cfg_in_mmsys)
> > > +			mtk_mmsys_ddp_dpi_fmt_config(dpi->mmsys_dev,
> > > MTK_DPI_RGB888_DDR_CON);
> > >  	} else {
> > >  		mtk_dpi_mask(dpi, DPI_DDR_SETTING, DDR_EN | DDR_4PHASE,
> > > 0);
> > > +		if (dpi->conf->edge_cfg_in_mmsys)
> > > +			mtk_mmsys_ddp_dpi_fmt_config(dpi->mmsys_dev,
> > > MTK_DPI_RGB888_SDR_CON);
> > 
> > I know this isn't one of the formats supported by MT8186, but since
> > we're using
> > platform-agnostic formats now... This else branch in theory could
> > also run for a
> > format like MEDIA_BUS_FMT_YUYV8_1X16. Would it make sense to set
> > MTK_DPI_RGB888_SDR_CON in that case?
> > 
> > Thanks,
> > Nícolas
> > 
> > >  	}
> > 
> > [..]
> 
> Hi Nícolas:
> 
> Thanks for your review!
>  
> You are right, I understand you think this MTK_DPI_RGB888_SDR_CON 
> format seems useless as it will not be set, I confirmed with the 
> designer how the setting in mmsys affects the output format of the 
> MT8186, this mmsys setting will not be used by other ICs.
> 
> As mentioned earlier, the mmsys setting will make the MT8186dpi have 
> four output formats, even though the MT8186 dpi may not use them all.
> 
> So what needs to change here?

We could check that the format in the else path is a single edge RGB888 format
like MEDIA_BUS_FMT_RGB888_1X24 before setting the mmsys config, but there are
also other formats possible, and I actually don't think it's worth it to
complicate the logic further to protect from an edge-case that can't be hit
yet...

So just leave it as it is. We can worry about it when/if a non-RGB888 single
edge format needs to be setup on mmsys.

So,

Reviewed-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>

Thanks,
Nícolas

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v12,1/3] soc: mediatek: Add all settings to mtk_mmsys_ddp_dpi_fmt_config func

[xinlei.lee]

On Fri, 2022-10-21 at 11:14 -0400, Nícolas F. R. A. Prado wrote:
> On Fri, Oct 21, 2022 at 07:59:02PM +0800, xinlei.lee wrote:
> > On Thu, 2022-10-20 at 12:33 -0400, Nícolas F. R. A. Prado wrote:
> > > Hi,
> > > 
> > > On Wed, Oct 19, 2022 at 10:52:14AM +0800, xinlei.lee@mediatek.com
> > > wrote:
> > > > From: Xinlei Lee <xinlei.lee@mediatek.com>
> > > > 
> > > > The difference between MT8186 and other ICs is that when
> > > > modifying
> > > > the
> > > > output format, we need to modify the mmsys_base+0x400 register
> > > > to
> > > > take
> > > > effect.
> > > > So when setting the dpi output format, we need to call
> > > > mmsys_func
> > > > to set
> > > 
> > > mmsys_func isn't something that exists in the code. Instead
> > > mention
> > > the actual
> > > function name: mtk_mmsys_ddp_dpi_fmt_config.
> > > 
> > > > it to MT8186 synchronously.
> > > 
> > > 
> > > Here, before saying that the commit adds all the settings for
> > > dpi,
> > > you could
> > > have mentioned that the previous commit lacked those, to make it
> > > clearer:
> > > 
> > > Commit a071e52f75d1 ("soc: mediatek: Add mmsys func to adapt to
> > > dpi
> > > output for MT8186")
> > > lacked some of the possible output formats and also had a wrong
> > > bitmask.
> > > 
> > > 
> > > > Adding mmsys all the settings that need to be modified with dpi
> > > > are
> > > > for
> > > > mt8186.
> > > 
> > > This sentence I would change to the following one:
> > > 
> > > Add the missing output formats and fix the bitmask.
> > > 
> > > 
> > > Finally, you're also making the function more HW-agnostic
> > > (although
> > > in my
> > > opinion this could've been a future separate commit), so it's
> > > worth
> > > mentioning
> > > it here:
> > > 
> > > While at it, also update mtk_mmsys_ddp_dpi_fmt_config() to use
> > > generic formats,
> > > so that it is slightly easier to extend for other platforms.
> > > 
> > > > 
> > > > Fixes: a071e52f75d1 ("soc: mediatek: Add mmsys func to adapt to
> > > > dpi
> > > > output for MT8186")
> > > 
> > > The fixes tag should be kept in a single line, without wrapping.
> > > 
> > > > 
> > > > Signed-off-by: Xinlei Lee <xinlei.lee@mediatek.com>
> > > > Reviewed-by: AngeloGioacchino Del Regno <
> > > > angelogioacchino.delregno@collabora.com>
> > > > Reviewed-by: CK Hu <ck.hu@mediatek.com>
> > > > ---
> > > >  drivers/soc/mediatek/mt8186-mmsys.h    |  8 +++++---
> > > >  drivers/soc/mediatek/mtk-mmsys.c       | 27
> > > > ++++++++++++++++++++
> > > > ------
> > > >  include/linux/soc/mediatek/mtk-mmsys.h |  7 +++++++
> > > >  3 files changed, 33 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/drivers/soc/mediatek/mt8186-mmsys.h
> > > > b/drivers/soc/mediatek/mt8186-mmsys.h
> > > > index 09b1ccbc0093..035aec1eb616 100644
> > > > --- a/drivers/soc/mediatek/mt8186-mmsys.h
> > > > +++ b/drivers/soc/mediatek/mt8186-mmsys.h
> > > > @@ -5,9 +5,11 @@
> > > >  
> > > >  /* Values for DPI configuration in MMSYS address space */
> > > >  #define MT8186_MMSYS_DPI_OUTPUT_FORMAT		0x400
> > > > -#define DPI_FORMAT_MASK					
> > > > 0x1
> > > > -#define DPI_RGB888_DDR_CON				BIT(0)
> > > > -#define DPI_RGB565_SDR_CON				BIT(1)
> > > > +#define DPI_FORMAT_MASK					
> > > > GENMASK
> > > > (1, 0)
> > > > +#define DPI_RGB888_SDR_CON				0
> > > > +#define DPI_RGB888_DDR_CON				1
> > > > +#define DPI_RGB565_SDR_CON				2
> > > > +#define DPI_RGB565_DDR_CON				3
> > > 
> > > These defines should all have a MT8186_ prefix. This will avoid
> > > confusions now
> > > that mtk_mmsys_ddp_dpi_fmt_config() is being made more platform-
> > > agnostic.
> > > 
> > > >  
> > > >  #define MT8186_MMSYS_OVL_CON			0xF04
> > > >  #define MT8186_MMSYS_OVL0_CON_MASK			0x3
> > > > diff --git a/drivers/soc/mediatek/mtk-mmsys.c
> > > > b/drivers/soc/mediatek/mtk-mmsys.c
> > > > index d2c7a87aab87..205f6de45851 100644
> > > > --- a/drivers/soc/mediatek/mtk-mmsys.c
> > > > +++ b/drivers/soc/mediatek/mtk-mmsys.c
> > > > @@ -238,12 +238,27 @@ static void mtk_mmsys_update_bits(struct
> > > > mtk_mmsys *mmsys, u32 offset, u32 mask,
> > > >  
> > > >  void mtk_mmsys_ddp_dpi_fmt_config(struct device *dev, u32 val)
> > > >  {
> > > > -	if (val)
> > > > -		mtk_mmsys_update_bits(dev_get_drvdata(dev),
> > > > MT8186_MMSYS_DPI_OUTPUT_FORMAT,
> > > > -				      DPI_RGB888_DDR_CON,
> > > > DPI_FORMAT_MASK);
> > > > -	else
> > > > -		mtk_mmsys_update_bits(dev_get_drvdata(dev),
> > > > MT8186_MMSYS_DPI_OUTPUT_FORMAT,
> > > > -				      DPI_RGB565_SDR_CON,
> > > > DPI_FORMAT_MASK);
> > > > +	struct mtk_mmsys *mmsys = dev_get_drvdata(dev);
> > > > +
> > > > +	switch (val) {
> > > > +	case MTK_DPI_RGB888_SDR_CON:
> > > > +		mtk_mmsys_update_bits(mmsys,
> > > > MT8186_MMSYS_DPI_OUTPUT_FORMAT,
> > > > +				      DPI_FORMAT_MASK,
> > > > DPI_RGB888_SDR_CON);
> > > > +		break;
> > > > +	case MTK_DPI_RGB565_SDR_CON:
> > > > +		mtk_mmsys_update_bits(mmsys,
> > > > MT8186_MMSYS_DPI_OUTPUT_FORMAT,
> > > > +				      DPI_FORMAT_MASK,
> > > > DPI_RGB565_SDR_CON);
> > > > +		break;
> > > > +	case MTK_DPI_RGB565_DDR_CON:
> > > > +		mtk_mmsys_update_bits(mmsys,
> > > > MT8186_MMSYS_DPI_OUTPUT_FORMAT,
> > > > +				      DPI_FORMAT_MASK,
> > > > DPI_RGB565_DDR_CON);
> > > > +		break;
> > > > +	case MTK_DPI_RGB888_DDR_CON:
> > > > +	default:
> > > > +		mtk_mmsys_update_bits(mmsys,
> > > > MT8186_MMSYS_DPI_OUTPUT_FORMAT,
> > > > +				      DPI_FORMAT_MASK,
> > > > DPI_RGB888_DDR_CON);
> > > > +		break;
> > > > +	}
> > > 
> > > To be honest I don't really see the point of making the function
> > > slightly more
> > > platform-agnostic like this. With a single platform making use of
> > > it
> > > it's just
> > > an unneeded extra abstraction, and it could easily be done when a
> > > second
> > > platform starts requiring this as well...
> > > 
> > > In any case,
> > > 
> > > Reviewed-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> > > 
> > > Thanks,
> > > Nícolas
> > > 
> > > >  }
> > > 
> > > [..]
> > 
> > Hi Nícolas:
> > 
> > Thanks for your detailed reply and correction.
> > Before sending out the next edition, I have two questions I would
> > like 
> > to confirm with you in response to your responses:
> > 1.While at it, also update mtk_mmsys_ddp_dpi_fmt_config() to use 
> > generic formats, so that it is slightly easier to extend for other 
> > platforms.
> > => This is to make this mtk_mmsys_ddp_dpi_fmt_config() func more 
> > general? 
> > This function may only be used by MT8186, because only MT8186
> > has 
> > corresponding modifications on HW, and enables the registers
> > reserved 
> > in mmsys for dpi use to control the output format. Because this 
> > register is not defined for other ic, I added control to this
> > function 
> > call in mtk_dpi.c. If you think there are other ways to make it
> > look 
> > more generic, where should I correct it?
> 
> You already made the mtk_mmsys_ddp_dpi_fmt_config() more generic by
> making it's
> format parameter decoupled from its register representation on
> MT8186, that is,
> MTK_DPI_RGB888_SDR_CON instead of DPI_RGB888_SDR_CON.
> 
> I wasn't asking for any code modification on that comment, I was
> suggesting you
> add this sentence in the commit message, so it reflects the changes
> you're
> already doing.
> 
> To be extra clear, I was suggesting you update the commit message to
> the
> following:
> 
>   The difference between MT8186 and other ICs is that when modifying
> the output
>   format, we need to modify the mmsys_base+0x400 register to take
> effect. So when
>   setting the dpi output format, we need to call
> mtk_mmsys_ddp_dpi_fmt_config to
>   set it to MT8186 synchronously.
>   
>   Commit a071e52f75d1 ("soc: mediatek: Add mmsys func to adapt to dpi
> output for
>   MT8186") lacked some of the possible output formats and also had a
> wrong
>   bitmask.
>   
>   Add the missing output formats and fix the bitmask.
>   
>   While at it, also update mtk_mmsys_ddp_dpi_fmt_config() to use
> generic formats,
>   so that it is slightly easier to extend for other platforms.
>   
>   Fixes: a071e52f75d1 ("soc: mediatek: Add mmsys func to adapt to dpi
> output for MT8186")
> 
> > 
> > 2. These definitions should all have a MT8186_ prefix. This will
> > avoid 
> > confusion as mtk_mmsys_ddp_dpi_fmt_config() becomes more platform 
> > independent.
> > 
> > Honestly, I don't really see the point of making the feature
> > platform-
> > agnostic like this. Using it on a single platform is just an extra 
> > abstraction that isn't needed, when a second platform starts
> > needing 
> > it too, it can be done easily...
> > 
> > => My understanding here is that prefixing variables with labels
> > is 
> > more conducive to making functions generic, and can be reused if
> > there 
> > is such a situation in the future. I understand the importance of 
> > keeping the function platform agnostic, but as mentioned, it may
> > only 
> > be used by the MT8186 if there are special cases where other ICs
> > may 
> > rely on mtk_mmsys_update_bits to create new functions.
> 
> What I'm saying is that, even though you've made the function receive
> a generic
> format as a parameter, like MTK_DPI_RGB888_SDR_CON, at this point in
> time
> MT8186 is the only SoC that has a register in mmsys for it, so the
> values
> 
> DPI_FORMAT_MASK
> DPI_RGB888_SDR_CON
> DPI_RGB888_DDR_CON
> DPI_RGB565_SDR_CON
> DPI_RGB565_DDR_CON
> 
> are really all MT8186-specific, at least at this point. Leaving them
> without the
> MT8186_ can give the false impression that they're already used
> elsewhere. Also
> it's really easy to mistake them for the generic ones (like
> MTK_DPI_RGB888_SDR_CON). MT8186_MMSYS_DPI_OUTPUT_FORMAT already has
> the MT8186_
> prefix, so I'm really just saying that the other ones should have as
> well.
> 
> If/when the same address, mask or values for this register start
> being used on a
> different SoC, then you can remove the prefix and move it to the mtk-
> mmsys.h
> generic header.
> 
> But for now adding the prefixes will avoid confusion and make it
> clear this is
> MT8186 specific.
> 
> Thanks,
> Nícolas

Hi Nícolas:

Thanks for the detailed explanation and correction, I understand that 
these values in the mt8186-mmsys.h file should be given:
DPI_FORMAT_MASK
DPI_RGB888_SDR_CON
DPI_RGB888_DDR_CON
DPI_RGB565_SDR_CON
DPI_RGB565_DDR_CON

Add the prefix of MT8186_ to avoid confusion, it does look generic in 
the mtk_mmsys_update_bits() function.

Thanks again for your suggestion, I will revise it in the next edition.

Best Regards!
xinlei


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v12,2/3] drm: mediatek: Set dpi format in mmsys

[xinlei.lee]

On Fri, 2022-10-21 at 11:39 -0400, Nícolas F. R. A. Prado wrote:
> On Fri, Oct 21, 2022 at 08:18:25PM +0800, xinlei.lee wrote:
> > On Thu, 2022-10-20 at 12:40 -0400, Nícolas F. R. A. Prado wrote:
> > > On Wed, Oct 19, 2022 at 10:52:15AM +0800, xinlei.lee@mediatek.com
> > > wrote:
> 
> [..]
> > > > @@ -448,8 +453,12 @@ 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->edge_cfg_in_mmsys)
> > > > +			mtk_mmsys_ddp_dpi_fmt_config(dpi-
> > > > >mmsys_dev,
> > > > MTK_DPI_RGB888_DDR_CON);
> > > >  	} else {
> > > >  		mtk_dpi_mask(dpi, DPI_DDR_SETTING, DDR_EN |
> > > > DDR_4PHASE,
> > > > 0);
> > > > +		if (dpi->conf->edge_cfg_in_mmsys)
> > > > +			mtk_mmsys_ddp_dpi_fmt_config(dpi-
> > > > >mmsys_dev,
> > > > MTK_DPI_RGB888_SDR_CON);
> > > 
> > > I know this isn't one of the formats supported by MT8186, but
> > > since
> > > we're using
> > > platform-agnostic formats now... This else branch in theory could
> > > also run for a
> > > format like MEDIA_BUS_FMT_YUYV8_1X16. Would it make sense to set
> > > MTK_DPI_RGB888_SDR_CON in that case?
> > > 
> > > Thanks,
> > > Nícolas
> > > 
> > > >  	}
> > > 
> > > [..]
> > 
> > Hi Nícolas:
> > 
> > Thanks for your review!
> >  
> > You are right, I understand you think this MTK_DPI_RGB888_SDR_CON 
> > format seems useless as it will not be set, I confirmed with the 
> > designer how the setting in mmsys affects the output format of the 
> > MT8186, this mmsys setting will not be used by other ICs.
> > 
> > As mentioned earlier, the mmsys setting will make the MT8186dpi
> > have 
> > four output formats, even though the MT8186 dpi may not use them
> > all.
> > 
> > So what needs to change here?
> 
> We could check that the format in the else path is a single edge
> RGB888 format
> like MEDIA_BUS_FMT_RGB888_1X24 before setting the mmsys config, but
> there are
> also other formats possible, and I actually don't think it's worth it
> to
> complicate the logic further to protect from an edge-case that can't
> be hit
> yet...
> 
> So just leave it as it is. We can worry about it when/if a non-RGB888 
> single
> edge format needs to be setup on mmsys.
> 
> So,
> 
> Reviewed-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> 
> Thanks,
> Nícolas


Hi Nícolas:

Thanks for your review!

Best Regards!
xinlei


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel