On Fri, 2021-01-29 at 14:46 +0800, Hsin-Yi Wang wrote:

On Fri, Jan 29, 2021 at 2:30 PM Yongqiang Niu
[off-list ref] wrote:

quoted

On Fri, 2021-01-29 at 14:24 +0800, Hsin-Yi Wang wrote:

quoted

On Fri, Jan 29, 2021 at 9:33 AM CK Hu [off-list ref] wrote:

quoted

Hi, Hsin-Yi:

On Thu, 2021-01-28 at 19:23 +0800, Hsin-Yi Wang wrote:

quoted

From: Yongqiang Niu <redacted>

for 5 or 6 bpc panel, we need enable dither function
to improve the display quality

Signed-off-by: Yongqiang Niu <redacted>
Signed-off-by: Hsin-Yi Wang <redacted>
---
 drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
index ac2cb25620357..6c8f246380a74 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c

@@ -53,6 +53,7 @@
 #define DITHER_EN                            BIT(0)
 #define DISP_DITHER_CFG                              0x0020
 #define DITHER_RELAY_MODE                    BIT(0)
+#define DITHER_ENGINE_EN                     BIT(1)
 #define DISP_DITHER_SIZE                     0x0030

 #define LUT_10BIT_MASK                               0x03ff

@@ -314,9 +315,19 @@ static void mtk_dither_config(struct device *dev, unsigned int w,
                            unsigned int bpc, struct cmdq_pkt *cmdq_pkt)
 {
      struct mtk_ddp_comp_dev *priv = dev_get_drvdata(dev);
+     bool enable = (bpc == 5 || bpc == 6);

I strongly believe that dither function in dither is identical to the
one in gamma and od, and in mtk_dither_set_common(), 'bpc >=
MTK_MIN_BPC' is valid, so I believe we need not to limit bpc to 5 or 6.
But we should consider the case that bpc is invalid in
mtk_dither_set_common(). Invalid case in gamma and od use different way
to process. For gamma, dither is default relay mode, so invalid bpc
would do nothing in mtk_dither_set_common() and result in relay mode.
For od, it set to relay mode first, them invalid bpc would do nothing in
mtk_dither_set_common() and result in relay mode. I would like dither,
gamma and od to process invalid bpc in the same way. One solution is to
set relay mode in mtk_dither_set_common() for invalid bpc.

Regards,
CK

I modify the mtk_dither_config() to follow:

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c

b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
index ac2cb25620357..5b7fcedb9f9a8 100644

--- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c

@@ -53,6 +53,7 @@
 #define DITHER_EN                              BIT(0)
 #define DISP_DITHER_CFG                                0x0020
 #define DITHER_RELAY_MODE                      BIT(0)
+#define DITHER_ENGINE_EN                       BIT(1)
 #define DISP_DITHER_SIZE                       0x0030

 #define LUT_10BIT_MASK                         0x03ff

@@ -166,6 +167,8 @@ void mtk_dither_set_common(void __iomem *regs,

struct cmdq_client_reg *cmdq_reg,
                              DITHER_ADD_LSHIFT_G(MTK_MAX_BPC - bpc),
                              cmdq_reg, regs, DISP_DITHER_16);
                mtk_ddp_write(cmdq_pkt, dither_en, cmdq_reg, regs, cfg);
+       } else {
+               mtk_ddp_write(cmdq_pkt, DITHER_RELAY_MODE, cmdq_reg, regs, cfg);
        }
 }

@@ -315,8 +318,12 @@ static void mtk_dither_config(struct device *dev,

unsigned int w,
 {
        struct mtk_ddp_comp_dev *priv = dev_get_drvdata(dev);

-       mtk_ddp_write(cmdq_pkt, h << 16 | w, &priv->cmdq_reg,
priv->regs, DISP_DITHER_SIZE);
-       mtk_ddp_write(cmdq_pkt, DITHER_RELAY_MODE, &priv->cmdq_reg,
priv->regs, DISP_DITHER_CFG);
+       mtk_ddp_write(cmdq_pkt, h << 16 | w, &priv->cmdq_reg, priv->regs,
+                     DISP_DITHER_SIZE);
+       mtk_ddp_write(cmdq_pkt, DITHER_RELAY_MODE, &priv->cmdq_reg, priv->regs,
+                     DISP_DITHER_CFG);
+       mtk_dither_set_common(priv->regs, &priv->cmdq_reg, bpc, DISP_DITHER_CFG,
+                              DITHER_ENGINE_EN, cmdq_pkt);
 }

So now, not only bpc==5 or 6, but all valid bpc, dither config will
call mtk_dither_set_common() with the flag DITHER_ENGINE_EN(BIT(1)).
od config will call mtk_dither_set_common() with the flag
DISP_DITHERING(BIT(2)).
Additionally for 8173, gamma config will call mtk_dither_set_common()
with the flag DISP_DITHERING (BIT(2))

For invalid mode all of them will be DITHER_RELAY_MODE.

Just to make sure that this follows the spec? thanks

for mt8173 gamma, there is no relay mode, only dither enable or not(bit
2).
for mt8183 dither, there is dither enable bit 1, and relay mode bit 0

CK suggested to set relay mode for invalid cases. Or should I just set
invalid case in mtk_dither_config()? So that invalid case for gamma
and od would remain its default mode?

od and gamma has no relay mode
set relay mode in  mtk_dither_config is better

Besides that, the major difference of this patch and original version
is that not only bpc ==5 or 6 will set dither enable bit 1. Does this
looks good to you?

dither only support  bpc 4 6 8 , there is no bpc 5 use case,
please modify this error.

and drm only has these bpc
switch (edid->input & DRM_EDID_DIGITAL_DEPTH_MASK) {
	case DRM_EDID_DIGITAL_DEPTH_6:
		info->bpc = 6;
		break;
	case DRM_EDID_DIGITAL_DEPTH_8:
		info->bpc = 8;
		break;
	case DRM_EDID_DIGITAL_DEPTH_10:
		info->bpc = 10;
		break;
	case DRM_EDID_DIGITAL_DEPTH_12:
		info->bpc = 12;
		break;
	case DRM_EDID_DIGITAL_DEPTH_14:
		info->bpc = 14;
		break;
	case DRM_EDID_DIGITAL_DEPTH_16:
		info->bpc = 16;
		break;
	case DRM_EDID_DIGITAL_DEPTH_UNDEF:
	default:
		info->bpc = 0;
		break;
	}

quoted

quoted

quoted

quoted

-     mtk_ddp_write(cmdq_pkt, h << 16 | w, &priv->cmdq_reg, priv->regs, DISP_DITHER_SIZE);
-     mtk_ddp_write(cmdq_pkt, DITHER_RELAY_MODE, &priv->cmdq_reg, priv->regs, DISP_DITHER_CFG);
+     if (enable) {
+             mtk_dither_set_common(priv->regs, &priv->cmdq_reg, bpc,
+                                   DISP_DITHER_CFG, DITHER_ENGINE_EN,
+                                   cmdq_pkt);
+     } else {
+             mtk_ddp_write(cmdq_pkt, DITHER_RELAY_MODE, &priv->cmdq_reg,
+                           priv->regs, DISP_DITHER_CFG);
+     }
+
+     mtk_ddp_write(cmdq_pkt, h << 16 | w, &priv->cmdq_reg, priv->regs,
+                   DISP_DITHER_SIZE);
 }

 static void mtk_dither_start(struct device *dev)

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek