| Message ID | 20210128112314.1304160-7-hsinyi@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/mediatek: add support for mediatek SOC MT8183 | expand |
Hi, Hsin-Yi: On Thu, 2021-01-28 at 19:23 +0800, Hsin-Yi Wang wrote: > From: Yongqiang Niu <yongqiang.niu@mediatek.com> > > for 5 or 6 bpc panel, we need enable dither function > to improve the display quality > > Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com> > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org> > --- > 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 > > - 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)
On Fri, Jan 29, 2021 at 9:33 AM CK Hu <ck.hu@mediatek.com> wrote: > > Hi, Hsin-Yi: > > On Thu, 2021-01-28 at 19:23 +0800, Hsin-Yi Wang wrote: > > From: Yongqiang Niu <yongqiang.niu@mediatek.com> > > > > for 5 or 6 bpc panel, we need enable dither function > > to improve the display quality > > > > Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com> > > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org> > > --- > > 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 > > > > - 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) >
On Fri, 2021-01-29 at 14:24 +0800, Hsin-Yi Wang wrote: > On Fri, Jan 29, 2021 at 9:33 AM CK Hu <ck.hu@mediatek.com> wrote: > > > > Hi, Hsin-Yi: > > > > On Thu, 2021-01-28 at 19:23 +0800, Hsin-Yi Wang wrote: > > > From: Yongqiang Niu <yongqiang.niu@mediatek.com> > > > > > > for 5 or 6 bpc panel, we need enable dither function > > > to improve the display quality > > > > > > Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com> > > > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org> > > > --- > > > 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 > > > > > > - 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) > >
On Fri, Jan 29, 2021 at 2:30 PM Yongqiang Niu <yongqiang.niu@mediatek.com> wrote: > > On Fri, 2021-01-29 at 14:24 +0800, Hsin-Yi Wang wrote: > > On Fri, Jan 29, 2021 at 9:33 AM CK Hu <ck.hu@mediatek.com> wrote: > > > > > > Hi, Hsin-Yi: > > > > > > On Thu, 2021-01-28 at 19:23 +0800, Hsin-Yi Wang wrote: > > > > From: Yongqiang Niu <yongqiang.niu@mediatek.com> > > > > > > > > for 5 or 6 bpc panel, we need enable dither function > > > > to improve the display quality > > > > > > > > Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com> > > > > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org> > > > > --- > > > > 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? 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? > > > > > > > > > - 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) > > > >
On Fri, 2021-01-29 at 14:46 +0800, Hsin-Yi Wang wrote: > On Fri, Jan 29, 2021 at 2:30 PM Yongqiang Niu > <yongqiang.niu@mediatek.com> wrote: > > > > On Fri, 2021-01-29 at 14:24 +0800, Hsin-Yi Wang wrote: > > > On Fri, Jan 29, 2021 at 9:33 AM CK Hu <ck.hu@mediatek.com> wrote: > > > > > > > > Hi, Hsin-Yi: > > > > > > > > On Thu, 2021-01-28 at 19:23 +0800, Hsin-Yi Wang wrote: > > > > > From: Yongqiang Niu <yongqiang.niu@mediatek.com> > > > > > > > > > > for 5 or 6 bpc panel, we need enable dither function > > > > > to improve the display quality > > > > > > > > > > Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com> > > > > > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org> > > > > > --- > > > > > 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; } > > > > > > > > > > > > - 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) > > > > > >
On Fri, Jan 29, 2021 at 3:42 PM Yongqiang Niu <yongqiang.niu@mediatek.com> wrote: > > On Fri, 2021-01-29 at 14:46 +0800, Hsin-Yi Wang wrote: > > On Fri, Jan 29, 2021 at 2:30 PM Yongqiang Niu > > <yongqiang.niu@mediatek.com> wrote: > > > > > > On Fri, 2021-01-29 at 14:24 +0800, Hsin-Yi Wang wrote: > > > > On Fri, Jan 29, 2021 at 9:33 AM CK Hu <ck.hu@mediatek.com> wrote: > > > > > > > > > > Hi, Hsin-Yi: > > > > > > > > > > On Thu, 2021-01-28 at 19:23 +0800, Hsin-Yi Wang wrote: > > > > > > From: Yongqiang Niu <yongqiang.niu@mediatek.com> > > > > > > > > > > > > for 5 or 6 bpc panel, we need enable dither function > > > > > > to improve the display quality > > > > > > > > > > > > Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com> > > > > > > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org> > > > > > > --- > > > > > > 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. > Only bpc 4, 6, 8 case will call mtk_dither_set_common, other bpc will be considered invalid and stays in relay mode. diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c index ac2cb25620357..5761dd15eedf2 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,17 @@ 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); - - 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); + bool valid_bpc = (bpc == 4 || bpc == 6 || bpc == 8); + + mtk_ddp_write(cmdq_pkt, h << 16 | w, &priv->cmdq_reg, priv->regs, + DISP_DITHER_SIZE); + if (valid_bpc) + 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); > > > > > > - 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) > > > > > > > > >
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); - 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)