Re: [PATCH 03/18] drm/mediatek: redefine mtk_ddp_sout_sel

Re: [PATCH 03/18] drm/mediatek: redefine mtk_ddp_sout_sel

[Nicolas Boichat]

On Mon, Dec 24, 2018 at 6:52 PM Yongqiang Niu
<yongqiang.niu@mediatek.com> wrote:
>
> This patch redefine mtk_ddp_sout_sel

Can you describe a bit more why you are making this change?

> Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_drm_ddp.c | 32 ++++++++++++++++++++------------
>  1 file changed, 20 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> index adb37e4..592f852 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> @@ -405,21 +405,27 @@ static unsigned int mtk_ddp_sel_in(enum mtk_ddp_comp_id cur,
>         return value;
>  }
>
> -static void mtk_ddp_sout_sel(void __iomem *config_regs,
> -                            enum mtk_ddp_comp_id cur,
> -                            enum mtk_ddp_comp_id next)
> +static unsigned int mtk_ddp_sout_sel(void __iomem *config_regs,

You don't use config_regs anymore, drop it.

> +                                    enum mtk_ddp_comp_id cur,
> +                                    enum mtk_ddp_comp_id next,
> +                                    unsigned int *addr)
>  {
> +       unsigned int value;
> +
>         if (cur == DDP_COMPONENT_BLS && next == DDP_COMPONENT_DSI0) {
> -               writel_relaxed(BLS_TO_DSI_RDMA1_TO_DPI1,
> -                              config_regs + DISP_REG_CONFIG_OUT_SEL);
> +               *addr = DISP_REG_CONFIG_OUT_SEL;
> +               value = BLS_TO_DSI_RDMA1_TO_DPI1;

You can directly return BLS_TO_DSI_RDMA1_TO_DPI1.

>         } else if (cur == DDP_COMPONENT_BLS && next == DDP_COMPONENT_DPI0) {
> -               writel_relaxed(BLS_TO_DPI_RDMA1_TO_DSI,
> -                              config_regs + DISP_REG_CONFIG_OUT_SEL);
> -               writel_relaxed(DSI_SEL_IN_RDMA,
> -                              config_regs + DISP_REG_CONFIG_DSI_SEL);
> -               writel_relaxed(DPI_SEL_IN_BLS,
> -                              config_regs + DISP_REG_CONFIG_DPI_SEL);
> +               *addr = DISP_REG_CONFIG_OUT_SEL;
> +               value = BLS_TO_DPI_RDMA1_TO_DSI;

I (kind of) understand the change above, as you still end up writing
BLS_TO_DSI_RDMA1_TO_DPI1 in DISP_REG_CONFIG_OUT_SEL.

This changes the behaviour, as now you only write
BLS_TO_DPI_RDMA1_TO_DSI to DISP_REG_CONFIG_OUT_SEL, but the previous
revision of the code would also write to DISP_REG_CONFIG_DSI_SEL and
DISP_REG_CONFIG_DPI_SEL. Why?

> +       } else if (cur == DDP_COMPONENT_RDMA1 && next == DDP_COMPONENT_DSI0) {
> +               *addr = DISP_REG_CONFIG_DSI_SEL;
> +               value = DSI_SEL_IN_RDMA;
> +       } else {
> +               value = 0;
>         }
> +
> +       return value;
>  }
>
>  void mtk_ddp_add_comp_to_path(void __iomem *config_regs,
> @@ -434,7 +440,9 @@ void mtk_ddp_add_comp_to_path(void __iomem *config_regs,
>                 writel_relaxed(reg, config_regs + addr);
>         }
>
> -       mtk_ddp_sout_sel(config_regs, cur, next);
> +       value = mtk_ddp_sout_sel(cur, next, &addr);
> +       if (value)
> +               writel_relaxed(value, config_regs + addr);

Why this change? I don't see mtk_ddp_sout_sel being used later in the
series, so I'm not sure why we don't directly write the value into the
register.



>         value = mtk_ddp_sel_in(cur, next, &addr);
>         if (value) {
> --
> 1.8.1.1.dirty
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 03/18] drm/mediatek: redefine mtk_ddp_sout_sel

[Yongqiang Niu]

On Tue, 2018-12-25 at 11:57 +0800, Nicolas Boichat wrote:
> On Mon, Dec 24, 2018 at 6:52 PM Yongqiang Niu
> <yongqiang.niu@mediatek.com> wrote:
> >
> > This patch redefine mtk_ddp_sout_sel
> 
> Can you describe a bit more why you are making this change?

the format of "mtk_ddp_sout_sel"was not flexible, after we add more
mediatek SOC support, that will be redundant

set this function format like mtk_ddp_mout_en and mtk_ddp_sel_in

> 
> > Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
> > ---
> >  drivers/gpu/drm/mediatek/mtk_drm_ddp.c | 32 ++++++++++++++++++++------------
> >  1 file changed, 20 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> > index adb37e4..592f852 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> > @@ -405,21 +405,27 @@ static unsigned int mtk_ddp_sel_in(enum mtk_ddp_comp_id cur,
> >         return value;
> >  }
> >
> > -static void mtk_ddp_sout_sel(void __iomem *config_regs,
> > -                            enum mtk_ddp_comp_id cur,
> > -                            enum mtk_ddp_comp_id next)
> > +static unsigned int mtk_ddp_sout_sel(void __iomem *config_regs,
> 
> You don't use config_regs anymore, drop it.

ok, will drop it in next version

> 
> > +                                    enum mtk_ddp_comp_id cur,
> > +                                    enum mtk_ddp_comp_id next,
> > +                                    unsigned int *addr)
> >  {
> > +       unsigned int value;
> > +
> >         if (cur == DDP_COMPONENT_BLS && next == DDP_COMPONENT_DSI0) {
> > -               writel_relaxed(BLS_TO_DSI_RDMA1_TO_DPI1,
> > -                              config_regs + DISP_REG_CONFIG_OUT_SEL);
> > +               *addr = DISP_REG_CONFIG_OUT_SEL;
> > +               value = BLS_TO_DSI_RDMA1_TO_DPI1;
> 
> You can directly return BLS_TO_DSI_RDMA1_TO_DPI1.

just format this like mtk_ddp_mout_en and mtk_ddp_sel_in

> 
> >         } else if (cur == DDP_COMPONENT_BLS && next == DDP_COMPONENT_DPI0) {
> > -               writel_relaxed(BLS_TO_DPI_RDMA1_TO_DSI,
> > -                              config_regs + DISP_REG_CONFIG_OUT_SEL);
> > -               writel_relaxed(DSI_SEL_IN_RDMA,
> > -                              config_regs + DISP_REG_CONFIG_DSI_SEL);
> > -               writel_relaxed(DPI_SEL_IN_BLS,
> > -                              config_regs + DISP_REG_CONFIG_DPI_SEL);
> > +               *addr = DISP_REG_CONFIG_OUT_SEL;
> > +               value = BLS_TO_DPI_RDMA1_TO_DSI;
> 
> I (kind of) understand the change above, as you still end up writing
> BLS_TO_DSI_RDMA1_TO_DPI1 in DISP_REG_CONFIG_OUT_SEL.
> 
> This changes the behaviour, as now you only write
> BLS_TO_DPI_RDMA1_TO_DSI to DISP_REG_CONFIG_OUT_SEL, but the previous
> revision of the code would also write to DISP_REG_CONFIG_DSI_SEL and
> DISP_REG_CONFIG_DPI_SEL. Why?
> 

DISP_REG_CONFIG_DSI_SEL set in the next lines.
DPI_SEL_IN_BLS is 0 for DISP_REG_CONFIG_DPI_SEL set, and hardware
default setting is also 0, so this one is no need anymore

> > +       } else if (cur == DDP_COMPONENT_RDMA1 && next == DDP_COMPONENT_DSI0) {
> > +               *addr = DISP_REG_CONFIG_DSI_SEL;
> > +               value = DSI_SEL_IN_RDMA;
> > +       } else {
> > +               value = 0;
> >         }
> > +
> > +       return value;
> >  }
> >
> >  void mtk_ddp_add_comp_to_path(void __iomem *config_regs,
> > @@ -434,7 +440,9 @@ void mtk_ddp_add_comp_to_path(void __iomem *config_regs,
> >                 writel_relaxed(reg, config_regs + addr);
> >         }
> >
> > -       mtk_ddp_sout_sel(config_regs, cur, next);
> > +       value = mtk_ddp_sout_sel(cur, next, &addr);
> > +       if (value)
> > +               writel_relaxed(value, config_regs + addr);
> 
> Why this change? I don't see mtk_ddp_sout_sel being used later in the
> series, so I'm not sure why we don't directly write the value into the
> register.
> 
in the patch "[PATCH 04/18] drm/mediatek: move rdma sout from
mtk_ddp_mout_en into mtk_ddp_sout_sel", i moved all rdma out to here,
rdma only have single out, no multi out.
if keep this format, there will many writel_relaxed in mtk_ddp_sout_sel.
and modify this format like mtk_ddp_mout_en and mtk_ddp_sel_in looks
better.

> 
> 
> >         value = mtk_ddp_sel_in(cur, next, &addr);
> >         if (value) {
> > --
> > 1.8.1.1.dirty
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 03/18] drm/mediatek: redefine mtk_ddp_sout_sel

[Nicolas Boichat]

On Fri, Mar 15, 2019 at 10:06 AM Yongqiang Niu
<yongqiang.niu@mediatek.com> wrote:
>
> On Tue, 2018-12-25 at 11:57 +0800, Nicolas Boichat wrote:
> > On Mon, Dec 24, 2018 at 6:52 PM Yongqiang Niu
> > <yongqiang.niu@mediatek.com> wrote:
> > >
> > > This patch redefine mtk_ddp_sout_sel
> >
> > Can you describe a bit more why you are making this change?
>
> the format of "mtk_ddp_sout_sel"was not flexible, after we add more
> mediatek SOC support, that will be redundant
>
> set this function format like mtk_ddp_mout_en and mtk_ddp_sel_in

This needs to be 2 patches:
 1. Make the change to "cur == DDP_COMPONENT_BLS && next ==
DDP_COMPONENT_DPI0", along with an explanation of why this is
reasonable.
 2. Change the format to look like mtk_ddp_mout_en and mtk_ddp_sel_in

> >
> > > Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
> > > ---
> > >  drivers/gpu/drm/mediatek/mtk_drm_ddp.c | 32 ++++++++++++++++++++------------
> > >  1 file changed, 20 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> > > index adb37e4..592f852 100644
> > > --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> > > +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> > > @@ -405,21 +405,27 @@ static unsigned int mtk_ddp_sel_in(enum mtk_ddp_comp_id cur,
> > >         return value;
> > >  }
> > >
> > > -static void mtk_ddp_sout_sel(void __iomem *config_regs,
> > > -                            enum mtk_ddp_comp_id cur,
> > > -                            enum mtk_ddp_comp_id next)
> > > +static unsigned int mtk_ddp_sout_sel(void __iomem *config_regs,
> >
> > You don't use config_regs anymore, drop it.
>
> ok, will drop it in next version
>
> >
> > > +                                    enum mtk_ddp_comp_id cur,
> > > +                                    enum mtk_ddp_comp_id next,
> > > +                                    unsigned int *addr)
> > >  {
> > > +       unsigned int value;
> > > +
> > >         if (cur == DDP_COMPONENT_BLS && next == DDP_COMPONENT_DSI0) {
> > > -               writel_relaxed(BLS_TO_DSI_RDMA1_TO_DPI1,
> > > -                              config_regs + DISP_REG_CONFIG_OUT_SEL);
> > > +               *addr = DISP_REG_CONFIG_OUT_SEL;
> > > +               value = BLS_TO_DSI_RDMA1_TO_DPI1;
> >
> > You can directly return BLS_TO_DSI_RDMA1_TO_DPI1.
>
> just format this like mtk_ddp_mout_en and mtk_ddp_sel_in

Since there is precedent, sounds ok.

> >
> > >         } else if (cur == DDP_COMPONENT_BLS && next == DDP_COMPONENT_DPI0) {
> > > -               writel_relaxed(BLS_TO_DPI_RDMA1_TO_DSI,
> > > -                              config_regs + DISP_REG_CONFIG_OUT_SEL);
> > > -               writel_relaxed(DSI_SEL_IN_RDMA,
> > > -                              config_regs + DISP_REG_CONFIG_DSI_SEL);
> > > -               writel_relaxed(DPI_SEL_IN_BLS,
> > > -                              config_regs + DISP_REG_CONFIG_DPI_SEL);
> > > +               *addr = DISP_REG_CONFIG_OUT_SEL;
> > > +               value = BLS_TO_DPI_RDMA1_TO_DSI;
> >
> > I (kind of) understand the change above, as you still end up writing
> > BLS_TO_DSI_RDMA1_TO_DPI1 in DISP_REG_CONFIG_OUT_SEL.
> >
> > This changes the behaviour, as now you only write
> > BLS_TO_DPI_RDMA1_TO_DSI to DISP_REG_CONFIG_OUT_SEL, but the previous
> > revision of the code would also write to DISP_REG_CONFIG_DSI_SEL and
> > DISP_REG_CONFIG_DPI_SEL. Why?
> >
>
> DISP_REG_CONFIG_DSI_SEL set in the next lines.

Ok, but where is mtk_ddp_sout_sel(DDP_COMPONENT_RDMA1,
DDP_COMPONENT_DSI0) called from?

Before this change, we just needed to call:
mtk_ddp_sout_sel(DDP_COMPONENT_BLS, DDP_COMPONENT_DSI0)
and the following 3 registers would be modified:
BLS_TO_DPI_RDMA1_TO_DSI, DSI_SEL_IN_RDMA, DPI_SEL_IN_BLS.

Now, to get similar behaviour, we need to call:
mtk_ddp_sout_sel(DDP_COMPONENT_BLS, DDP_COMPONENT_DSI0)
mtk_ddp_sout_sel(DDP_COMPONENT_RDMA1, DDP_COMPONENT_DSI0)

Maybe that's ok, but you really need to explain the reason for this
change in the commit message.

> DPI_SEL_IN_BLS is 0 for DISP_REG_CONFIG_DPI_SEL set, and hardware
> default setting is also 0, so this one is no need anymore

That's somewhat reasonable, but this needs to be at the very least
described in the commit message.

> > > +       } else if (cur == DDP_COMPONENT_RDMA1 && next == DDP_COMPONENT_DSI0) {
> > > +               *addr = DISP_REG_CONFIG_DSI_SEL;
> > > +               value = DSI_SEL_IN_RDMA;
> > > +       } else {
> > > +               value = 0;
> > >         }
> > > +
> > > +       return value;
> > >  }
> > >
> > >  void mtk_ddp_add_comp_to_path(void __iomem *config_regs,
> > > @@ -434,7 +440,9 @@ void mtk_ddp_add_comp_to_path(void __iomem *config_regs,
> > >                 writel_relaxed(reg, config_regs + addr);
> > >         }
> > >
> > > -       mtk_ddp_sout_sel(config_regs, cur, next);
> > > +       value = mtk_ddp_sout_sel(cur, next, &addr);
> > > +       if (value)
> > > +               writel_relaxed(value, config_regs + addr);
> >
> > Why this change? I don't see mtk_ddp_sout_sel being used later in the
> > series, so I'm not sure why we don't directly write the value into the
> > register.
> >
> in the patch "[PATCH 04/18] drm/mediatek: move rdma sout from
> mtk_ddp_mout_en into mtk_ddp_sout_sel", i moved all rdma out to here,
> rdma only have single out, no multi out.
> if keep this format, there will many writel_relaxed in mtk_ddp_sout_sel.
> and modify this format like mtk_ddp_mout_en and mtk_ddp_sel_in looks
> better.
>
> >
> >
> > >         value = mtk_ddp_sel_in(cur, next, &addr);
> > >         if (value) {
> > > --
> > > 1.8.1.1.dirty
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>

Re: [PATCH 10/18] drm/mediatek: add gmc_bits for ovl private data

[Nicolas Boichat]

On Mon, Dec 24, 2018 at 6:53 PM Yongqiang Niu
<yongqiang.niu@mediatek.com> wrote:
>
> This patch add gmc_bits for ovl private data
>
> Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> index 28d1911..afb313c 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> @@ -39,7 +39,9 @@
>  #define DISP_REG_OVL_ADDR_MT8173               0x0f40
>  #define DISP_REG_OVL_ADDR(ovl, n)              ((ovl)->data->addr + 0x20 * (n))
>
> -#define        OVL_RDMA_MEM_GMC        0x40402020
> +#define GMC_THRESHOLD_BITS     16
> +#define GMC_THRESHOLD_HIGH     ((1 << GMC_THRESHOLD_BITS) / 4)
> +#define GMC_THRESHOLD_LOW      ((1 << GMC_THRESHOLD_BITS) / 8)
>
>  #define OVL_CON_BYTE_SWAP      BIT(24)
>  #define OVL_CON_MTX_YUV_TO_RGB (6 << 16)
> @@ -57,6 +59,7 @@
>
>  struct mtk_disp_ovl_data {
>         unsigned int addr;
> +       unsigned int gmc_bits;
>         bool fmt_rgb565_is_0;
>  };
>
> @@ -140,9 +143,23 @@ static unsigned int mtk_ovl_layer_nr(struct mtk_ddp_comp *comp)
>  static void mtk_ovl_layer_on(struct mtk_ddp_comp *comp, unsigned int idx)
>  {
>         unsigned int reg;
> +       unsigned int gmc_thrshd_l;
> +       unsigned int gmc_thrshd_h;
> +       unsigned int gmc_value;
> +       struct mtk_disp_ovl *ovl = comp_to_ovl(comp);
>
>         writel(0x1, comp->regs + DISP_REG_OVL_RDMA_CTRL(idx));
> -       writel(OVL_RDMA_MEM_GMC, comp->regs + DISP_REG_OVL_RDMA_GMC(idx));
> +
> +       gmc_thrshd_l = GMC_THRESHOLD_LOW >>
> +                     (GMC_THRESHOLD_BITS - ovl->data->gmc_bits);
> +       gmc_thrshd_h = GMC_THRESHOLD_HIGH >>
> +                     (GMC_THRESHOLD_BITS - ovl->data->gmc_bits);
> +       if (ovl->data->gmc_bits == 10)
> +               gmc_value = gmc_thrshd_h | gmc_thrshd_h << 16;

I don't really get what this does, but is it intentional that you
don't use gmc_thrshd_l here?

Also, if you only ever use 8 or 10 bits gmc, maybe it's easier to
hard-code the 2 values?
if (ovl->data->gmc_bits == 10)
  gmc_value = OVL_RDMA_MEM_GMC_10BIT;
else
  gmc_value = OVL_RDMA_MEM_GMC_8BIT; //0x40402020

> +       else
> +               gmc_value = gmc_thrshd_l | gmc_thrshd_l << 8 |
> +                           gmc_thrshd_h << 16 | gmc_thrshd_h << 24;
> +       writel(gmc_value, comp->regs + DISP_REG_OVL_RDMA_GMC(idx));
>
>         reg = readl(comp->regs + DISP_REG_OVL_SRC_CON);
>         reg = reg | BIT(idx);
> @@ -324,11 +341,13 @@ static int mtk_disp_ovl_remove(struct platform_device *pdev)
>
>  static const struct mtk_disp_ovl_data mt2701_ovl_driver_data = {
>         .addr = DISP_REG_OVL_ADDR_MT2701,
> +       .gmc_bits = 8,
>         .fmt_rgb565_is_0 = false,
>  };
>
>  static const struct mtk_disp_ovl_data mt8173_ovl_driver_data = {
>         .addr = DISP_REG_OVL_ADDR_MT8173,
> +       .gmc_bits = 8,
>         .fmt_rgb565_is_0 = true,
>  };
>
> --
> 1.8.1.1.dirty
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 10/18] drm/mediatek: add gmc_bits for ovl private data

[Yongqiang Niu]

On Tue, 2018-12-25 at 12:15 +0800, Nicolas Boichat wrote:
> On Mon, Dec 24, 2018 at 6:53 PM Yongqiang Niu
> <yongqiang.niu@mediatek.com> wrote:
> >
> > This patch add gmc_bits for ovl private data
> >
> > Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
> > ---
> >  drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 23 +++++++++++++++++++++--
> >  1 file changed, 21 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> > index 28d1911..afb313c 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> > @@ -39,7 +39,9 @@
> >  #define DISP_REG_OVL_ADDR_MT8173               0x0f40
> >  #define DISP_REG_OVL_ADDR(ovl, n)              ((ovl)->data->addr + 0x20 * (n))
> >
> > -#define        OVL_RDMA_MEM_GMC        0x40402020
> > +#define GMC_THRESHOLD_BITS     16
> > +#define GMC_THRESHOLD_HIGH     ((1 << GMC_THRESHOLD_BITS) / 4)
> > +#define GMC_THRESHOLD_LOW      ((1 << GMC_THRESHOLD_BITS) / 8)
> >
> >  #define OVL_CON_BYTE_SWAP      BIT(24)
> >  #define OVL_CON_MTX_YUV_TO_RGB (6 << 16)
> > @@ -57,6 +59,7 @@
> >
> >  struct mtk_disp_ovl_data {
> >         unsigned int addr;
> > +       unsigned int gmc_bits;
> >         bool fmt_rgb565_is_0;
> >  };
> >
> > @@ -140,9 +143,23 @@ static unsigned int mtk_ovl_layer_nr(struct mtk_ddp_comp *comp)
> >  static void mtk_ovl_layer_on(struct mtk_ddp_comp *comp, unsigned int idx)
> >  {
> >         unsigned int reg;
> > +       unsigned int gmc_thrshd_l;
> > +       unsigned int gmc_thrshd_h;
> > +       unsigned int gmc_value;
> > +       struct mtk_disp_ovl *ovl = comp_to_ovl(comp);
> >
> >         writel(0x1, comp->regs + DISP_REG_OVL_RDMA_CTRL(idx));
> > -       writel(OVL_RDMA_MEM_GMC, comp->regs + DISP_REG_OVL_RDMA_GMC(idx));
> > +
> > +       gmc_thrshd_l = GMC_THRESHOLD_LOW >>
> > +                     (GMC_THRESHOLD_BITS - ovl->data->gmc_bits);
> > +       gmc_thrshd_h = GMC_THRESHOLD_HIGH >>
> > +                     (GMC_THRESHOLD_BITS - ovl->data->gmc_bits);
> > +       if (ovl->data->gmc_bits == 10)
> > +               gmc_value = gmc_thrshd_h | gmc_thrshd_h << 16;
> 
> I don't really get what this does, but is it intentional that you
> don't use gmc_thrshd_l here?
> 
GMC register was set RDMA ultra and pre-ultra threshold.
MT8183 GMC register define is different with other SOC, gmc_thrshd_l not
used here.

> Also, if you only ever use 8 or 10 bits gmc, maybe it's easier to
> hard-code the 2 values?
> if (ovl->data->gmc_bits == 10)
>   gmc_value = OVL_RDMA_MEM_GMC_10BIT;
> else
>   gmc_value = OVL_RDMA_MEM_GMC_8BIT; //0x40402020
> 

our internal maintainer prefer calculate GMC setting with private data
gmc bit instead of hard-core.
and with calculation function that will be more flexible 

> > +       else
> > +               gmc_value = gmc_thrshd_l | gmc_thrshd_l << 8 |
> > +                           gmc_thrshd_h << 16 | gmc_thrshd_h << 24;
> > +       writel(gmc_value, comp->regs + DISP_REG_OVL_RDMA_GMC(idx));
> >
> >         reg = readl(comp->regs + DISP_REG_OVL_SRC_CON);
> >         reg = reg | BIT(idx);
> > @@ -324,11 +341,13 @@ static int mtk_disp_ovl_remove(struct platform_device *pdev)
> >
> >  static const struct mtk_disp_ovl_data mt2701_ovl_driver_data = {
> >         .addr = DISP_REG_OVL_ADDR_MT2701,
> > +       .gmc_bits = 8,
> >         .fmt_rgb565_is_0 = false,
> >  };
> >
> >  static const struct mtk_disp_ovl_data mt8173_ovl_driver_data = {
> >         .addr = DISP_REG_OVL_ADDR_MT8173,
> > +       .gmc_bits = 8,
> >         .fmt_rgb565_is_0 = true,
> >  };
> >
> > --
> > 1.8.1.1.dirty
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 10/18] drm/mediatek: add gmc_bits for ovl private data

[Nicolas Boichat]

On Fri, Mar 15, 2019 at 10:34 AM Yongqiang Niu
<yongqiang.niu@mediatek.com> wrote:
>
> On Tue, 2018-12-25 at 12:15 +0800, Nicolas Boichat wrote:
> > On Mon, Dec 24, 2018 at 6:53 PM Yongqiang Niu
> > <yongqiang.niu@mediatek.com> wrote:
> > >
> > > This patch add gmc_bits for ovl private data
> > >
> > > Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
> > > ---
> > >  drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 23 +++++++++++++++++++++--
> > >  1 file changed, 21 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> > > index 28d1911..afb313c 100644
> > > --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> > > +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> > > @@ -39,7 +39,9 @@
> > >  #define DISP_REG_OVL_ADDR_MT8173               0x0f40
> > >  #define DISP_REG_OVL_ADDR(ovl, n)              ((ovl)->data->addr + 0x20 * (n))
> > >
> > > -#define        OVL_RDMA_MEM_GMC        0x40402020
> > > +#define GMC_THRESHOLD_BITS     16
> > > +#define GMC_THRESHOLD_HIGH     ((1 << GMC_THRESHOLD_BITS) / 4)
> > > +#define GMC_THRESHOLD_LOW      ((1 << GMC_THRESHOLD_BITS) / 8)
> > >
> > >  #define OVL_CON_BYTE_SWAP      BIT(24)
> > >  #define OVL_CON_MTX_YUV_TO_RGB (6 << 16)
> > > @@ -57,6 +59,7 @@
> > >
> > >  struct mtk_disp_ovl_data {
> > >         unsigned int addr;
> > > +       unsigned int gmc_bits;
> > >         bool fmt_rgb565_is_0;
> > >  };
> > >
> > > @@ -140,9 +143,23 @@ static unsigned int mtk_ovl_layer_nr(struct mtk_ddp_comp *comp)
> > >  static void mtk_ovl_layer_on(struct mtk_ddp_comp *comp, unsigned int idx)
> > >  {
> > >         unsigned int reg;
> > > +       unsigned int gmc_thrshd_l;
> > > +       unsigned int gmc_thrshd_h;
> > > +       unsigned int gmc_value;
> > > +       struct mtk_disp_ovl *ovl = comp_to_ovl(comp);
> > >
> > >         writel(0x1, comp->regs + DISP_REG_OVL_RDMA_CTRL(idx));
> > > -       writel(OVL_RDMA_MEM_GMC, comp->regs + DISP_REG_OVL_RDMA_GMC(idx));
> > > +
> > > +       gmc_thrshd_l = GMC_THRESHOLD_LOW >>
> > > +                     (GMC_THRESHOLD_BITS - ovl->data->gmc_bits);
> > > +       gmc_thrshd_h = GMC_THRESHOLD_HIGH >>
> > > +                     (GMC_THRESHOLD_BITS - ovl->data->gmc_bits);
> > > +       if (ovl->data->gmc_bits == 10)
> > > +               gmc_value = gmc_thrshd_h | gmc_thrshd_h << 16;
> >
> > I don't really get what this does, but is it intentional that you
> > don't use gmc_thrshd_l here?
> >
> GMC register was set RDMA ultra and pre-ultra threshold.
> MT8183 GMC register define is different with other SOC, gmc_thrshd_l not
> used here.

Ok cool. My issue is that this really appears to be like a mistake, so
maybe it's better to only compute gmc_thrshd_l in the else branch?

gmc_thrshd_h = GMC_THRESHOLD_HIGH >>
                     (GMC_THRESHOLD_BITS - ovl->data->gmc_bits);
if (ovl->data->gmc_bits == 10) {
     gmc_value = gmc_thrshd_h | gmc_thrshd_h << 16;
} else {
     gmc_thrshd_l = GMC_THRESHOLD_LOW >>
                     (GMC_THRESHOLD_BITS - ovl->data->gmc_bits);
     gmc_value = gmc_thrshd_l | gmc_thrshd_l << 8 |
                          gmc_thrshd_h << 16 | gmc_thrshd_h << 24;
}

> > Also, if you only ever use 8 or 10 bits gmc, maybe it's easier to
> > hard-code the 2 values?
> > if (ovl->data->gmc_bits == 10)
> >   gmc_value = OVL_RDMA_MEM_GMC_10BIT;
> > else
> >   gmc_value = OVL_RDMA_MEM_GMC_8BIT; //0x40402020
> >
>
> our internal maintainer prefer calculate GMC setting with private data
> gmc bit instead of hard-core.
> and with calculation function that will be more flexible

Sounds ok.

> > > +       else
> > > +               gmc_value = gmc_thrshd_l | gmc_thrshd_l << 8 |
> > > +                           gmc_thrshd_h << 16 | gmc_thrshd_h << 24;
> > > +       writel(gmc_value, comp->regs + DISP_REG_OVL_RDMA_GMC(idx));
> > >
> > >         reg = readl(comp->regs + DISP_REG_OVL_SRC_CON);
> > >         reg = reg | BIT(idx);
> > > @@ -324,11 +341,13 @@ static int mtk_disp_ovl_remove(struct platform_device *pdev)
> > >
> > >  static const struct mtk_disp_ovl_data mt2701_ovl_driver_data = {
> > >         .addr = DISP_REG_OVL_ADDR_MT2701,
> > > +       .gmc_bits = 8,
> > >         .fmt_rgb565_is_0 = false,
> > >  };
> > >
> > >  static const struct mtk_disp_ovl_data mt8173_ovl_driver_data = {
> > >         .addr = DISP_REG_OVL_ADDR_MT8173,
> > > +       .gmc_bits = 8,
> > >         .fmt_rgb565_is_0 = true,
> > >  };
> > >
> > > --
> > > 1.8.1.1.dirty
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>

Re: [PATCH 16/18] drm/mediatek: add function mtk_ddp_comp_get_type

[Nicolas Boichat]

On Mon, Dec 24, 2018 at 6:53 PM Yongqiang Niu
<yongqiang.niu@mediatek.com> wrote:
>
> This patch add function mtk_ddp_comp_get_type
>
> Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 10 ++++++++++
>  drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h |  1 +
>  2 files changed, 11 insertions(+)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> index 1c0f9cc..71b565c 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> @@ -350,6 +350,16 @@ int mtk_ddp_comp_get_id(struct device_node *node,
>         return -EINVAL;
>  }
>
> +enum mtk_ddp_comp_type mtk_ddp_comp_get_type(enum mtk_ddp_comp_id comp_id)
> +{
> +       enum mtk_ddp_comp_type comp_type = MTK_DDP_COMP_TYPE_MAX;
> +
> +       if (comp_id < DDP_COMPONENT_ID_MAX)
> +               comp_type = mtk_ddp_matches[comp_id].type;

return mtk_ddp_matches[comp_id].type

> +
> +       return comp_type;

return MTK_DDP_COMP_TYPE_MAX

> +}
> +
>  int mtk_ddp_comp_init(struct device *dev, struct device_node *node,
>                       struct mtk_ddp_comp *comp, enum mtk_ddp_comp_id comp_id,
>                       const struct mtk_ddp_comp_funcs *funcs)
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> index b908172..599e293 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> @@ -198,5 +198,6 @@ void mtk_ddp_write_mask(unsigned int value,
>                         struct mtk_ddp_comp *comp,
>                         unsigned int offset,
>                         unsigned int mask);
> +enum mtk_ddp_comp_type mtk_ddp_comp_get_type(enum mtk_ddp_comp_id comp_id);
>
>  #endif /* MTK_DRM_DDP_COMP_H */
> --
> 1.8.1.1.dirty
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 17/18] drm/mediatek: add ovl0/ovl0_2l usecase

[Nicolas Boichat]

On Mon, Dec 24, 2018 at 6:52 PM Yongqiang Niu
<yongqiang.niu@mediatek.com> wrote:
>
> This patch add ovl0/ovl0_2l usecase
>
> Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 38 ++++++++++++++++++++++++++++++---
>  1 file changed, 35 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> index a5af4be..25cf063 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> @@ -283,6 +283,15 @@ static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc)
>
>         for (i = 0; i < mtk_crtc->ddp_comp_nr; i++) {
>                 struct mtk_ddp_comp *comp = mtk_crtc->ddp_comp[i];
> +               enum mtk_ddp_comp_id prev = DDP_COMPONENT_ID_MAX;
> +
> +               if (i > 0) {
> +                       struct mtk_ddp_comp *comp_prev;
> +
> +                       comp_prev = mtk_crtc->ddp_comp[i - 1];
> +                       prev = comp_prev->id;

Just

if (i > 0)
  prev = mtk_crtc->ddp_comp[i - 1]->id;
else
  prev = DDP_COMPONENT_ID_MAX;

> +               }
> +               mtk_ddp_comp_connect(comp, prev);
>
>                 mtk_ddp_comp_config(comp, width, height, vrefresh, bpc);
>                 mtk_ddp_comp_start(comp);
> @@ -292,10 +301,19 @@ static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc)
>         for (i = 0; i < mtk_crtc->layer_nr; i++) {
>                 struct drm_plane *plane = &mtk_crtc->planes[i];
>                 struct mtk_plane_state *plane_state;
> +               struct mtk_ddp_comp *comp = mtk_crtc->ddp_comp[0];
> +               unsigned int comp_layer_nr = mtk_ddp_comp_layer_nr(comp);
> +               unsigned int local_layer = 0;

No need to init to 0.

>
>                 plane_state = to_mtk_plane_state(plane->state);
> -               mtk_ddp_comp_layer_config(mtk_crtc->ddp_comp[0], i,
> -                                         plane_state);
> +
> +               if (i >= comp_layer_nr) {
> +                       comp = mtk_crtc->ddp_comp[1];
> +                       local_layer = i - comp_layer_nr;
> +               } else {
> +                       local_layer = i;
> +               }
> +               mtk_ddp_comp_layer_config(comp, local_layer, plane_state);
>         }
>
>         return 0;
> @@ -340,6 +358,8 @@ static void mtk_crtc_ddp_config(struct drm_crtc *crtc)
>         struct mtk_crtc_state *state = to_mtk_crtc_state(mtk_crtc->base.state);
>         struct mtk_ddp_comp *comp = mtk_crtc->ddp_comp[0];
>         unsigned int i;
> +       unsigned int comp_layer_nr = mtk_ddp_comp_layer_nr(comp);
> +       unsigned int local_layer = 0;

ditto, don't init to 0.

>
>         /*
>          * TODO: instead of updating the registers here, we should prepare
> @@ -362,7 +382,15 @@ static void mtk_crtc_ddp_config(struct drm_crtc *crtc)
>                         plane_state = to_mtk_plane_state(plane->state);
>
>                         if (plane_state->pending.config) {
> -                               mtk_ddp_comp_layer_config(comp, i, plane_state);
> +                               if (i >= comp_layer_nr) {
> +                                       comp = mtk_crtc->ddp_comp[1];
> +                                       local_layer = i - comp_layer_nr;
> +                               } else {
> +                                       local_layer = i;
> +                               }
> +
> +                               mtk_ddp_comp_layer_config(comp, local_layer,
> +                                                         plane_state);
>                                 plane_state->pending.config = false;
>                         }
>                 }
> @@ -604,6 +632,10 @@ int mtk_drm_crtc_create(struct drm_device *drm_dev,
>         }
>
>         mtk_crtc->layer_nr = mtk_ddp_comp_layer_nr(mtk_crtc->ddp_comp[0]);
> +       if (mtk_crtc->ddp_comp_nr > 1 &&
> +           mtk_ddp_comp_get_type(mtk_crtc->ddp_comp[1]->id) == MTK_DISP_OVL)
> +               mtk_crtc->layer_nr +=
> +               mtk_ddp_comp_layer_nr(mtk_crtc->ddp_comp[1]);
>         mtk_crtc->planes = devm_kcalloc(dev, mtk_crtc->layer_nr,
>                                         sizeof(struct drm_plane),
>                                         GFP_KERNEL);
> --
> 1.8.1.1.dirty
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/18] drm/mediatek: update dt-bindings for mt8183

[CK Hu]

Hi, Yongqiang:

I would like you to add 'dt-bindings' in title.

On Mon, 2018-12-24 at 16:08 +0800, Yongqiang Niu wrote:
> Update device tree binding documention for the display subsystem for
> Mediatek MT8183 SOCs
> 
> Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
> ---
>  .../devicetree/bindings/display/mediatek/mediatek,disp.txt    | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt b/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
> index 8469de5..1c66f39 100644
> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,disp.txt
> @@ -28,9 +28,12 @@ Documentation/devicetree/bindings/display/mediatek/mediatek,dpi.txt.
>  Required properties (all function blocks):
>  - compatible: "mediatek,<chip>-disp-<function>", one of
>  	"mediatek,<chip>-disp-ovl"   - overlay (4 layers, blending, csc)
> +	"mediatek,<chip>-disp-ovl-2l"   - overlay (2 layers, blending, csc)

Other '-' is aliened, so do this one.

>  	"mediatek,<chip>-disp-rdma"  - read DMA / line buffer
>  	"mediatek,<chip>-disp-wdma"  - write DMA
> +	"mediatek,<chip>-disp-ccorr" - color correction
>  	"mediatek,<chip>-disp-color" - color processor
> +	"mediatek,<chip>-disp-dither" - dither

Ditto.

>  	"mediatek,<chip>-disp-aal"   - adaptive ambient light controller
>  	"mediatek,<chip>-disp-gamma" - gamma correction
>  	"mediatek,<chip>-disp-merge" - merge streams from two RDMA sources
> @@ -40,7 +43,7 @@ Required properties (all function blocks):
>  	"mediatek,<chip>-dpi"        - DPI controller, see mediatek,dpi.txt
>  	"mediatek,<chip>-disp-mutex" - display mutex
>  	"mediatek,<chip>-disp-od"    - overdrive
> -  the supported chips are mt2701, mt2712 and mt8173.
> +  the supported chips are mt2701, mt2712, mt8173 and mt8183.
>  - reg: Physical base address and length of the function block register space
>  - interrupts: The interrupt signal from the function block (required, except for
>    merge and split function blocks).
> @@ -71,6 +74,12 @@ mmsys: clock-controller@14000000 {
>  	#clock-cells = <1>;
>  };
>  
> +display_components: dispsys@14000000 {
> +		compatible = "mediatek,mt8183-display";

"mediatek,mt8183-display" is undefined. I think this should be
"mediatek,mt8183-mmsys" and you have met the same problem in [1]. You
may depend on that series to develop your patches.

[1]
http://lists.infradead.org/pipermail/linux-mediatek/2018-November/015860.html

Regards,
CK

> +		reg = <0 0x14000000 0 0x1000>;
> +		power-domains = <&scpsys MT8183_POWER_DOMAIN_DISP>;
> +};
> +
>  ovl0: ovl@1400c000 {
>  	compatible = "mediatek,mt8173-disp-ovl";
>  	reg = <0 0x1400c000 0 0x1000>;

Re: [PATCH 02/18] drm/mediatek: add mutex mod and sof into ddp private data

[CK Hu]

Hi, Yongqiang:

On Mon, 2018-12-24 at 16:08 +0800, Yongqiang Niu wrote:
> This patch add mutex mod and sof into ddp private data

Usually, the commit title shows 'WHAT' does this patch do, commit
message shows 'WHY' does this patch do, and commit body shows 'HOW' does
this patch do. This commit message just show WHAT does this patch do but
does not show WHY. Maybe this is trivial for you but not for everyone.
So describe more about why you do this.

In addition, 'add mutex mode' and 'add sof' are two things, so break
these two modification into two patches.

> 
> Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_drm_ddp.c | 117 ++++++++++++++++++++++++++-------
>  1 file changed, 94 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> index 579ce28..adb37e4 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> @@ -41,11 +41,14 @@
>  #define DISP_REG_CONFIG_DSI_SEL			0x050
>  #define DISP_REG_CONFIG_DPI_SEL			0x064
>  
> +#define MT2701_DISP_MUTEX0_MOD0 0x2C

Lower case for hex number.

> +#define MT2701_DISP_MUTEX0_SOF0  0x30

Align 0x2c and 0x30

> +
>  #define DISP_REG_MUTEX_EN(n)	(0x20 + 0x20 * (n))
>  #define DISP_REG_MUTEX(n)	(0x24 + 0x20 * (n))
>  #define DISP_REG_MUTEX_RST(n)	(0x28 + 0x20 * (n))
> -#define DISP_REG_MUTEX_MOD(n)	(0x2c + 0x20 * (n))
> -#define DISP_REG_MUTEX_SOF(n)	(0x30 + 0x20 * (n))
> +#define DISP_REG_MUTEX_MOD(data, n)	((data)->mutex_mod_reg + 0x20 * (n))
> +#define DISP_REG_MUTEX_SOF(data, n)	((data)->mutex_sof_reg + 0x20 * (n))
>  #define DISP_REG_MUTEX_MOD2(n)	(0x34 + 0x20 * (n))
>  
>  #define INT_MUTEX				BIT(1)
> @@ -147,12 +150,30 @@ struct mtk_disp_mutex {
>  	bool claimed;
>  };
>  
> +enum mtk_ddp_mutex_sof_id {
> +	DDP_MUTEX_SOF_SINGLE_MODE,
> +	DDP_MUTEX_SOF_DSI0,
> +	DDP_MUTEX_SOF_DSI1,
> +	DDP_MUTEX_SOF_DPI0,
> +	DDP_MUTEX_SOF_DPI1,
> +	DDP_MUTEX_SOF_DSI2,
> +	DDP_MUTEX_SOF_DSI3,
> +	DDP_MUTEX_SOF_MAX,
> +};
> +
> +struct mtk_ddp_data {
> +	const unsigned int *mutex_mod;
> +	const unsigned int *mutex_sof;
> +	unsigned int mutex_mod_reg;
> +	unsigned int mutex_sof_reg;
> +};
> +
>  struct mtk_ddp {
>  	struct device			*dev;
>  	struct clk			*clk;
>  	void __iomem			*regs;
>  	struct mtk_disp_mutex		mutex[10];
> -	const unsigned int		*mutex_mod;
> +	const struct mtk_ddp_data	*data;
>  };
>  
>  static const unsigned int mt2701_mutex_mod[DDP_COMPONENT_ID_MAX] = {
> @@ -202,6 +223,51 @@ struct mtk_ddp {
>  	[DDP_COMPONENT_WDMA1] = MT8173_MUTEX_MOD_DISP_WDMA1,
>  };
>  
> +static const unsigned int mt2701_mutex_sof[DDP_MUTEX_SOF_MAX] = {
> +	[DDP_MUTEX_SOF_SINGLE_MODE] = MUTEX_SOF_SINGLE_MODE,
> +	[DDP_MUTEX_SOF_DSI0] = MUTEX_SOF_DSI0,
> +	[DDP_MUTEX_SOF_DSI1] = MUTEX_SOF_DSI1,
> +	[DDP_MUTEX_SOF_DPI0] = MUTEX_SOF_DPI0,
> +};
> +
> +static const unsigned int mt2712_mutex_sof[DDP_MUTEX_SOF_MAX] = {
> +	[DDP_MUTEX_SOF_SINGLE_MODE] = MUTEX_SOF_SINGLE_MODE,
> +	[DDP_MUTEX_SOF_DSI0] = MUTEX_SOF_DSI0,
> +	[DDP_MUTEX_SOF_DSI1] = MUTEX_SOF_DSI1,
> +	[DDP_MUTEX_SOF_DPI0] = MUTEX_SOF_DPI0,
> +	[DDP_MUTEX_SOF_DPI1] = MUTEX_SOF_DPI1,
> +	[DDP_MUTEX_SOF_DSI2] = MUTEX_SOF_DSI2,
> +	[DDP_MUTEX_SOF_DSI3] = MUTEX_SOF_DSI3,
> +};
> +
> +static const unsigned int mt8173_mutex_sof[DDP_MUTEX_SOF_MAX] = {
> +	[DDP_MUTEX_SOF_SINGLE_MODE] = MUTEX_SOF_SINGLE_MODE,
> +	[DDP_MUTEX_SOF_DSI0] = MUTEX_SOF_DSI0,
> +	[DDP_MUTEX_SOF_DSI1] = MUTEX_SOF_DSI1,
> +	[DDP_MUTEX_SOF_DPI0] = MUTEX_SOF_DPI0,
> +};

It looks like that both mt8173_mutex_sof and mt2701_mutex_sof are subset
of mt2712_mutex_sof, so I think you could keep only mt2712_mutex_sof and
mt8173 and mt2701 also use this one.

> +
> +static const struct mtk_ddp_data mt2701_ddp_driver_data = {
> +	.mutex_mod = mt2701_mutex_mod,
> +	.mutex_sof = mt2701_mutex_sof,
> +	.mutex_mod_reg = MT2701_DISP_MUTEX0_MOD0,
> +	.mutex_sof_reg = MT2701_DISP_MUTEX0_SOF0,
> +};
> +
> +static const struct mtk_ddp_data mt2712_ddp_driver_data = {
> +	.mutex_mod = mt2712_mutex_mod,
> +	.mutex_sof = mt2712_mutex_sof,
> +	.mutex_mod_reg = MT2701_DISP_MUTEX0_MOD0,
> +	.mutex_sof_reg = MT2701_DISP_MUTEX0_SOF0,
> +};
> +
> +static const struct mtk_ddp_data mt8173_ddp_driver_data = {
> +	.mutex_mod = mt8173_mutex_mod,
> +	.mutex_sof = mt8173_mutex_sof,
> +	.mutex_mod_reg = MT2701_DISP_MUTEX0_MOD0,
> +	.mutex_sof_reg = MT2701_DISP_MUTEX0_SOF0,
> +};
> +
>  static unsigned int mtk_ddp_mout_en(enum mtk_ddp_comp_id cur,
>  				    enum mtk_ddp_comp_id next,
>  				    unsigned int *addr)
> @@ -446,39 +512,40 @@ void mtk_disp_mutex_add_comp(struct mtk_disp_mutex *mutex,
>  
>  	switch (id) {
>  	case DDP_COMPONENT_DSI0:
> -		reg = MUTEX_SOF_DSI0;
> +		reg = DDP_MUTEX_SOF_DSI0;

I would like you to change 'reg' to 'id' because you change the usage of
this variable.

Regards,
CK

>  		break;
>  	case DDP_COMPONENT_DSI1:
> -		reg = MUTEX_SOF_DSI0;
> +		reg = DDP_MUTEX_SOF_DSI0;
>  		break;
>  	case DDP_COMPONENT_DSI2:
> -		reg = MUTEX_SOF_DSI2;
> +		reg = DDP_MUTEX_SOF_DSI2;
>  		break;
>  	case DDP_COMPONENT_DSI3:
> -		reg = MUTEX_SOF_DSI3;
> +		reg = DDP_MUTEX_SOF_DSI3;
>  		break;
>  	case DDP_COMPONENT_DPI0:
> -		reg = MUTEX_SOF_DPI0;
> +		reg = DDP_MUTEX_SOF_DPI0;
>  		break;
>  	case DDP_COMPONENT_DPI1:
> -		reg = MUTEX_SOF_DPI1;
> +		reg = DDP_MUTEX_SOF_DPI1;
>  		break;
>  	default:
> -		if (ddp->mutex_mod[id] < 32) {
> -			offset = DISP_REG_MUTEX_MOD(mutex->id);
> +		if (ddp->data->mutex_mod[id] < 32) {
> +			offset = DISP_REG_MUTEX_MOD(ddp->data, mutex->id);
>  			reg = readl_relaxed(ddp->regs + offset);
> -			reg |= 1 << ddp->mutex_mod[id];
> +			reg |= 1 << ddp->data->mutex_mod[id];
>  			writel_relaxed(reg, ddp->regs + offset);
>  		} else {
>  			offset = DISP_REG_MUTEX_MOD2(mutex->id);
>  			reg = readl_relaxed(ddp->regs + offset);
> -			reg |= 1 << (ddp->mutex_mod[id] - 32);
> +			reg |= 1 << (ddp->data->mutex_mod[id] - 32);
>  			writel_relaxed(reg, ddp->regs + offset);
>  		}
>  		return;
>  	}
>  
> -	writel_relaxed(reg, ddp->regs + DISP_REG_MUTEX_SOF(mutex->id));
> +	writel_relaxed(ddp->data->mutex_sof[reg],
> +		       ddp->regs + DISP_REG_MUTEX_SOF(ddp->data, mutex->id));
>  }
>  
>  void mtk_disp_mutex_remove_comp(struct mtk_disp_mutex *mutex,
> @@ -499,18 +566,19 @@ void mtk_disp_mutex_remove_comp(struct mtk_disp_mutex *mutex,
>  	case DDP_COMPONENT_DPI0:
>  	case DDP_COMPONENT_DPI1:
>  		writel_relaxed(MUTEX_SOF_SINGLE_MODE,
> -			       ddp->regs + DISP_REG_MUTEX_SOF(mutex->id));
> +			       ddp->regs +
> +			       DISP_REG_MUTEX_SOF(ddp->data, mutex->id));
>  		break;
>  	default:
> -		if (ddp->mutex_mod[id] < 32) {
> -			offset = DISP_REG_MUTEX_MOD(mutex->id);
> +		if (ddp->data->mutex_mod[id] < 32) {
> +			offset = DISP_REG_MUTEX_MOD(ddp->data, mutex->id);
>  			reg = readl_relaxed(ddp->regs + offset);
> -			reg &= ~(1 << ddp->mutex_mod[id]);
> +			reg &= ~(1 << ddp->data->mutex_mod[id]);
>  			writel_relaxed(reg, ddp->regs + offset);
>  		} else {
>  			offset = DISP_REG_MUTEX_MOD2(mutex->id);
>  			reg = readl_relaxed(ddp->regs + offset);
> -			reg &= ~(1 << (ddp->mutex_mod[id] - 32));
> +			reg &= ~(1 << (ddp->data->mutex_mod[id] - 32));
>  			writel_relaxed(reg, ddp->regs + offset);
>  		}
>  		break;
> @@ -585,7 +653,7 @@ static int mtk_ddp_probe(struct platform_device *pdev)
>  		return PTR_ERR(ddp->regs);
>  	}
>  
> -	ddp->mutex_mod = of_device_get_match_data(dev);
> +	ddp->data = of_device_get_match_data(dev);
>  
>  	platform_set_drvdata(pdev, ddp);
>  
> @@ -598,9 +666,12 @@ static int mtk_ddp_remove(struct platform_device *pdev)
>  }
>  
>  static const struct of_device_id ddp_driver_dt_match[] = {
> -	{ .compatible = "mediatek,mt2701-disp-mutex", .data = mt2701_mutex_mod},
> -	{ .compatible = "mediatek,mt2712-disp-mutex", .data = mt2712_mutex_mod},
> -	{ .compatible = "mediatek,mt8173-disp-mutex", .data = mt8173_mutex_mod},
> +	{ .compatible = "mediatek,mt2701-disp-mutex",
> +	  .data = &mt2701_ddp_driver_data},
> +	{ .compatible = "mediatek,mt2712-disp-mutex",
> +	  .data = &mt2712_ddp_driver_data},
> +	{ .compatible = "mediatek,mt8173-disp-mutex",
> +	  .data = &mt8173_ddp_driver_data},
>  	{},
>  };
>  MODULE_DEVICE_TABLE(of, ddp_driver_dt_match);

Re: [PATCH 04/18] drm/mediatek: move rdma sout from mtk_ddp_mout_en into mtk_ddp_sout_sel

[CK Hu]

Hi, Yongqiang:

On Mon, 2018-12-24 at 16:08 +0800, Yongqiang Niu wrote:
> This patch move rdma sout from mtk_ddp_mout_en into mtk_ddp_sout_sel

This patch looks good to me, but you should describe why do you do this.

Regards,
CK

> 
> Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_drm_ddp.c | 90 +++++++++++++++++-----------------
>  1 file changed, 45 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> index 592f852..60cfde7 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> @@ -295,51 +295,6 @@ static unsigned int mtk_ddp_mout_en(enum mtk_ddp_comp_id cur,
>  	} else if (cur == DDP_COMPONENT_OD1 && next == DDP_COMPONENT_RDMA1) {
>  		*addr = DISP_REG_CONFIG_DISP_OD_MOUT_EN;
>  		value = OD1_MOUT_EN_RDMA1;
> -	} else if (cur == DDP_COMPONENT_RDMA0 && next == DDP_COMPONENT_DPI0) {
> -		*addr = DISP_REG_CONFIG_DISP_RDMA0_SOUT_EN;
> -		value = RDMA0_SOUT_DPI0;
> -	} else if (cur == DDP_COMPONENT_RDMA0 && next == DDP_COMPONENT_DPI1) {
> -		*addr = DISP_REG_CONFIG_DISP_RDMA0_SOUT_EN;
> -		value = RDMA0_SOUT_DPI1;
> -	} else if (cur == DDP_COMPONENT_RDMA0 && next == DDP_COMPONENT_DSI1) {
> -		*addr = DISP_REG_CONFIG_DISP_RDMA0_SOUT_EN;
> -		value = RDMA0_SOUT_DSI1;
> -	} else if (cur == DDP_COMPONENT_RDMA0 && next == DDP_COMPONENT_DSI2) {
> -		*addr = DISP_REG_CONFIG_DISP_RDMA0_SOUT_EN;
> -		value = RDMA0_SOUT_DSI2;
> -	} else if (cur == DDP_COMPONENT_RDMA0 && next == DDP_COMPONENT_DSI3) {
> -		*addr = DISP_REG_CONFIG_DISP_RDMA0_SOUT_EN;
> -		value = RDMA0_SOUT_DSI3;
> -	} else if (cur == DDP_COMPONENT_RDMA1 && next == DDP_COMPONENT_DSI1) {
> -		*addr = DISP_REG_CONFIG_DISP_RDMA1_SOUT_EN;
> -		value = RDMA1_SOUT_DSI1;
> -	} else if (cur == DDP_COMPONENT_RDMA1 && next == DDP_COMPONENT_DSI2) {
> -		*addr = DISP_REG_CONFIG_DISP_RDMA1_SOUT_EN;
> -		value = RDMA1_SOUT_DSI2;
> -	} else if (cur == DDP_COMPONENT_RDMA1 && next == DDP_COMPONENT_DSI3) {
> -		*addr = DISP_REG_CONFIG_DISP_RDMA1_SOUT_EN;
> -		value = RDMA1_SOUT_DSI3;
> -	} else if (cur == DDP_COMPONENT_RDMA1 && next == DDP_COMPONENT_DPI0) {
> -		*addr = DISP_REG_CONFIG_DISP_RDMA1_SOUT_EN;
> -		value = RDMA1_SOUT_DPI0;
> -	} else if (cur == DDP_COMPONENT_RDMA1 && next == DDP_COMPONENT_DPI1) {
> -		*addr = DISP_REG_CONFIG_DISP_RDMA1_SOUT_EN;
> -		value = RDMA1_SOUT_DPI1;
> -	} else if (cur == DDP_COMPONENT_RDMA2 && next == DDP_COMPONENT_DPI0) {
> -		*addr = DISP_REG_CONFIG_DISP_RDMA2_SOUT;
> -		value = RDMA2_SOUT_DPI0;
> -	} else if (cur == DDP_COMPONENT_RDMA2 && next == DDP_COMPONENT_DPI1) {
> -		*addr = DISP_REG_CONFIG_DISP_RDMA2_SOUT;
> -		value = RDMA2_SOUT_DPI1;
> -	} else if (cur == DDP_COMPONENT_RDMA2 && next == DDP_COMPONENT_DSI1) {
> -		*addr = DISP_REG_CONFIG_DISP_RDMA2_SOUT;
> -		value = RDMA2_SOUT_DSI1;
> -	} else if (cur == DDP_COMPONENT_RDMA2 && next == DDP_COMPONENT_DSI2) {
> -		*addr = DISP_REG_CONFIG_DISP_RDMA2_SOUT;
> -		value = RDMA2_SOUT_DSI2;
> -	} else if (cur == DDP_COMPONENT_RDMA2 && next == DDP_COMPONENT_DSI3) {
> -		*addr = DISP_REG_CONFIG_DISP_RDMA2_SOUT;
> -		value = RDMA2_SOUT_DSI3;
>  	} else {
>  		value = 0;
>  	}
> @@ -421,6 +376,51 @@ static unsigned int mtk_ddp_sout_sel(void __iomem *config_regs,
>  	} else if (cur == DDP_COMPONENT_RDMA1 && next == DDP_COMPONENT_DSI0) {
>  		*addr = DISP_REG_CONFIG_DSI_SEL;
>  		value = DSI_SEL_IN_RDMA;
> +	} else if (cur == DDP_COMPONENT_RDMA0 && next == DDP_COMPONENT_DPI0) {
> +		*addr = DISP_REG_CONFIG_DISP_RDMA0_SOUT_EN;
> +		value = RDMA0_SOUT_DPI0;
> +	} else if (cur == DDP_COMPONENT_RDMA0 && next == DDP_COMPONENT_DPI1) {
> +		*addr = DISP_REG_CONFIG_DISP_RDMA0_SOUT_EN;
> +		value = RDMA0_SOUT_DPI1;
> +	} else if (cur == DDP_COMPONENT_RDMA0 && next == DDP_COMPONENT_DSI1) {
> +		*addr = DISP_REG_CONFIG_DISP_RDMA0_SOUT_EN;
> +		value = RDMA0_SOUT_DSI1;
> +	} else if (cur == DDP_COMPONENT_RDMA0 && next == DDP_COMPONENT_DSI2) {
> +		*addr = DISP_REG_CONFIG_DISP_RDMA0_SOUT_EN;
> +		value = RDMA0_SOUT_DSI2;
> +	} else if (cur == DDP_COMPONENT_RDMA0 && next == DDP_COMPONENT_DSI3) {
> +		*addr = DISP_REG_CONFIG_DISP_RDMA0_SOUT_EN;
> +		value = RDMA0_SOUT_DSI3;
> +	} else if (cur == DDP_COMPONENT_RDMA1 && next == DDP_COMPONENT_DSI1) {
> +		*addr = DISP_REG_CONFIG_DISP_RDMA1_SOUT_EN;
> +		value = RDMA1_SOUT_DSI1;
> +	} else if (cur == DDP_COMPONENT_RDMA1 && next == DDP_COMPONENT_DSI2) {
> +		*addr = DISP_REG_CONFIG_DISP_RDMA1_SOUT_EN;
> +		value = RDMA1_SOUT_DSI2;
> +	} else if (cur == DDP_COMPONENT_RDMA1 && next == DDP_COMPONENT_DSI3) {
> +		*addr = DISP_REG_CONFIG_DISP_RDMA1_SOUT_EN;
> +		value = RDMA1_SOUT_DSI3;
> +	} else if (cur == DDP_COMPONENT_RDMA1 && next == DDP_COMPONENT_DPI0) {
> +		*addr = DISP_REG_CONFIG_DISP_RDMA1_SOUT_EN;
> +		value = RDMA1_SOUT_DPI0;
> +	} else if (cur == DDP_COMPONENT_RDMA1 && next == DDP_COMPONENT_DPI1) {
> +		*addr = DISP_REG_CONFIG_DISP_RDMA1_SOUT_EN;
> +		value = RDMA1_SOUT_DPI1;
> +	} else if (cur == DDP_COMPONENT_RDMA2 && next == DDP_COMPONENT_DPI0) {
> +		*addr = DISP_REG_CONFIG_DISP_RDMA2_SOUT;
> +		value = RDMA2_SOUT_DPI0;
> +	} else if (cur == DDP_COMPONENT_RDMA2 && next == DDP_COMPONENT_DPI1) {
> +		*addr = DISP_REG_CONFIG_DISP_RDMA2_SOUT;
> +		value = RDMA2_SOUT_DPI1;
> +	} else if (cur == DDP_COMPONENT_RDMA2 && next == DDP_COMPONENT_DSI1) {
> +		*addr = DISP_REG_CONFIG_DISP_RDMA2_SOUT;
> +		value = RDMA2_SOUT_DSI1;
> +	} else if (cur == DDP_COMPONENT_RDMA2 && next == DDP_COMPONENT_DSI2) {
> +		*addr = DISP_REG_CONFIG_DISP_RDMA2_SOUT;
> +		value = RDMA2_SOUT_DSI2;
> +	} else if (cur == DDP_COMPONENT_RDMA2 && next == DDP_COMPONENT_DSI3) {
> +		*addr = DISP_REG_CONFIG_DISP_RDMA2_SOUT;
> +		value = RDMA2_SOUT_DSI3;
>  	} else {
>  		value = 0;
>  	}

Re: [PATCH 05/18] drm/mediatek: add ddp component CCORR

[CK Hu]

Hi, Yongqiang:

On Mon, 2018-12-24 at 16:08 +0800, Yongqiang Niu wrote:
> This patch add ddp component CCORR

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

> Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 32 +++++++++++++++++++++++++++++
>  drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h |  2 ++
>  2 files changed, 34 insertions(+)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> index 54ca794..310c0b9 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> @@ -41,6 +41,12 @@
>  #define DISP_AAL_EN				0x0000
>  #define DISP_AAL_SIZE				0x0030
>  
> +#define DISP_CCORR_EN				0x0000
> +#define CCORR_EN				BIT(0)
> +#define DISP_CCORR_CFG				0x0020
> +#define CCORR_RELAY_MODE			BIT(0)
> +#define DISP_CCORR_SIZE				0x0030
> +
>  #define DISP_GAMMA_EN				0x0000
>  #define DISP_GAMMA_CFG				0x0020
>  #define DISP_GAMMA_SIZE				0x0030
> @@ -131,6 +137,24 @@ static void mtk_aal_stop(struct mtk_ddp_comp *comp)
>  	writel_relaxed(0x0, comp->regs + DISP_AAL_EN);
>  }
>  
> +static void mtk_ccorr_config(struct mtk_ddp_comp *comp, unsigned int w,
> +			     unsigned int h, unsigned int vrefresh,
> +			     unsigned int bpc)
> +{
> +	writel(h << 16 | w, comp->regs + DISP_CCORR_SIZE);
> +	writel(CCORR_RELAY_MODE, comp->regs + DISP_CCORR_CFG);
> +}
> +
> +static void mtk_ccorr_start(struct mtk_ddp_comp *comp)
> +{
> +	writel(CCORR_EN, comp->regs + DISP_CCORR_EN);
> +}
> +
> +static void mtk_ccorr_stop(struct mtk_ddp_comp *comp)
> +{
> +	writel_relaxed(0x0, comp->regs + DISP_CCORR_EN);
> +}
> +
>  static void mtk_gamma_config(struct mtk_ddp_comp *comp, unsigned int w,
>  			     unsigned int h, unsigned int vrefresh,
>  			     unsigned int bpc)
> @@ -179,6 +203,12 @@ static void mtk_gamma_set(struct mtk_ddp_comp *comp,
>  	.stop = mtk_aal_stop,
>  };
>  
> +static const struct mtk_ddp_comp_funcs ddp_ccorr = {
> +	.config = mtk_ccorr_config,
> +	.start = mtk_ccorr_start,
> +	.stop = mtk_ccorr_stop,
> +};
> +
>  static const struct mtk_ddp_comp_funcs ddp_gamma = {
>  	.gamma_set = mtk_gamma_set,
>  	.config = mtk_gamma_config,
> @@ -200,6 +230,7 @@ static void mtk_gamma_set(struct mtk_ddp_comp *comp,
>  	[MTK_DISP_RDMA] = "rdma",
>  	[MTK_DISP_WDMA] = "wdma",
>  	[MTK_DISP_COLOR] = "color",
> +	[MTK_DISP_CCORR] = "ccorr",
>  	[MTK_DISP_AAL] = "aal",
>  	[MTK_DISP_GAMMA] = "gamma",
>  	[MTK_DISP_UFOE] = "ufoe",
> @@ -221,6 +252,7 @@ struct mtk_ddp_comp_match {
>  	[DDP_COMPONENT_AAL0]	= { MTK_DISP_AAL,	0, &ddp_aal },
>  	[DDP_COMPONENT_AAL1]	= { MTK_DISP_AAL,	1, &ddp_aal },
>  	[DDP_COMPONENT_BLS]	= { MTK_DISP_BLS,	0, NULL },
> +	[DDP_COMPONENT_CCORR]	= { MTK_DISP_CCORR,	0, &ddp_ccorr },
>  	[DDP_COMPONENT_COLOR0]	= { MTK_DISP_COLOR,	0, NULL },
>  	[DDP_COMPONENT_COLOR1]	= { MTK_DISP_COLOR,	1, NULL },
>  	[DDP_COMPONENT_DPI0]	= { MTK_DPI,		0, NULL },
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> index 8399229..87ef290 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> @@ -28,6 +28,7 @@ enum mtk_ddp_comp_type {
>  	MTK_DISP_RDMA,
>  	MTK_DISP_WDMA,
>  	MTK_DISP_COLOR,
> +	MTK_DISP_CCORR,
>  	MTK_DISP_AAL,
>  	MTK_DISP_GAMMA,
>  	MTK_DISP_UFOE,
> @@ -44,6 +45,7 @@ enum mtk_ddp_comp_id {
>  	DDP_COMPONENT_AAL0,
>  	DDP_COMPONENT_AAL1,
>  	DDP_COMPONENT_BLS,
> +	DDP_COMPONENT_CCORR,
>  	DDP_COMPONENT_COLOR0,
>  	DDP_COMPONENT_COLOR1,
>  	DDP_COMPONENT_DPI0,

Re: [PATCH 06/18] drm/mediatek: add mmsys private data for ddp path config

[CK Hu]

Hi, Yongqiang:

On Mon, 2018-12-24 at 16:08 +0800, Yongqiang Niu wrote:
> This patch add mmsys private data for ddp path config

Describe WHY do this.

> 
> Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_drm_crtc.c |   4 ++
>  drivers/gpu/drm/mediatek/mtk_drm_ddp.c  | 102 ++++++++++++++++++++++++++------
>  drivers/gpu/drm/mediatek/mtk_drm_ddp.h  |  10 ++++
>  drivers/gpu/drm/mediatek/mtk_drm_drv.c  |  11 ++++
>  drivers/gpu/drm/mediatek/mtk_drm_drv.h  |   4 ++
>  5 files changed, 112 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> index 92ecb9b..a5af4be 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_crtc.c
> @@ -50,6 +50,7 @@ struct mtk_drm_crtc {
>  	bool				pending_planes;
>  
>  	void __iomem			*config_regs;
> +	const struct mtk_mmsys_reg_data *mmsys_reg_data;
>  	struct mtk_disp_mutex		*mutex;
>  	unsigned int			ddp_comp_nr;
>  	struct mtk_ddp_comp		**ddp_comp;
> @@ -271,6 +272,7 @@ static int mtk_crtc_ddp_hw_init(struct mtk_drm_crtc *mtk_crtc)
>  	DRM_DEBUG_DRIVER("mediatek_ddp_ddp_path_setup\n");
>  	for (i = 0; i < mtk_crtc->ddp_comp_nr - 1; i++) {
>  		mtk_ddp_add_comp_to_path(mtk_crtc->config_regs,
> +					 mtk_crtc->mmsys_reg_data,
>  					 mtk_crtc->ddp_comp[i]->id,
>  					 mtk_crtc->ddp_comp[i + 1]->id);
>  		mtk_disp_mutex_add_comp(mtk_crtc->mutex,
> @@ -319,6 +321,7 @@ static void mtk_crtc_ddp_hw_fini(struct mtk_drm_crtc *mtk_crtc)
>  	mtk_disp_mutex_disable(mtk_crtc->mutex);
>  	for (i = 0; i < mtk_crtc->ddp_comp_nr - 1; i++) {
>  		mtk_ddp_remove_comp_from_path(mtk_crtc->config_regs,
> +					      mtk_crtc->mmsys_reg_data,
>  					      mtk_crtc->ddp_comp[i]->id,
>  					      mtk_crtc->ddp_comp[i + 1]->id);
>  		mtk_disp_mutex_remove_comp(mtk_crtc->mutex,
> @@ -561,6 +564,7 @@ int mtk_drm_crtc_create(struct drm_device *drm_dev,
>  		return -ENOMEM;
>  
>  	mtk_crtc->config_regs = priv->config_regs;
> +	mtk_crtc->mmsys_reg_data = priv->reg_data;
>  	mtk_crtc->ddp_comp_nr = path_len;
>  	mtk_crtc->ddp_comp = devm_kmalloc_array(dev, mtk_crtc->ddp_comp_nr,
>  						sizeof(*mtk_crtc->ddp_comp),
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> index 60cfde7..4be3c11 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> @@ -145,6 +145,17 @@
>  #define DPI_SEL_IN_BLS			0x0
>  #define DSI_SEL_IN_RDMA			0x1
>  
> +#define DISP_REG_OVL0_MOUT_EN(data)		((data)->ovl0_mout_en)
> +#define DISP_REG_DPI0_SEL_IN(data)		((data)->dpi0_sel_in)
> +#define DISP_REG_DPI0_SEL_IN_RDMA1(data)	((data)->dpi0_sel_in_rdma1)
> +#define DISP_REG_DSI0_SEL_IN(data)		((data)->dsi0_sel_in)
> +#define DISP_REG_DSI0_SEL_IN_RDMA1(data)	((data)->dsi0_sel_in_rdma1)
> +#define DISP_REG_RDMA0_SOUT_SEL_IN(data)	((data)->rdma0_sout_sel_in)
> +#define DISP_REG_RDMA0_SOUT_COLOR0(data)	((data)->rdma0_sout_color0)
> +#define DISP_REG_RDMA1_SOUT_SEL_IN(data)	((data)->rdma1_sout_sel_in)
> +#define DISP_REG_RDMA1_SOUT_DPI0(data)		((data)->rdma1_sout_dpi0)
> +#define DISP_REG_RDMA1_SOUT_DSI0(data)		((data)->rdma1_sout_dsi0)
> +
>  struct mtk_disp_mutex {
>  	int id;
>  	bool claimed;
> @@ -176,6 +187,19 @@ struct mtk_ddp {
>  	const struct mtk_ddp_data	*data;
>  };
>  
> +struct mtk_mmsys_reg_data {
> +	unsigned int ovl0_mout_en;
> +	unsigned int rdma0_sout_sel_in;
> +	unsigned int rdma0_sout_color0;
> +	unsigned int rdma1_sout_sel_in;
> +	unsigned int rdma1_sout_dpi0;
> +	unsigned int rdma1_sout_dsi0;
> +	unsigned int dpi0_sel_in;
> +	unsigned int dpi0_sel_in_rdma1;
> +	unsigned int dsi0_sel_in;
> +	unsigned int dsi0_sel_in_rdma1;

I would like use 'u32' for these variables.

> +};
> +
>  static const unsigned int mt2701_mutex_mod[DDP_COMPONENT_ID_MAX] = {
>  	[DDP_COMPONENT_BLS] = MT2701_MUTEX_MOD_DISP_BLS,
>  	[DDP_COMPONENT_COLOR0] = MT2701_MUTEX_MOD_DISP_COLOR,
> @@ -268,17 +292,34 @@ struct mtk_ddp {
>  	.mutex_sof_reg = MT2701_DISP_MUTEX0_SOF0,
>  };
>  
> -static unsigned int mtk_ddp_mout_en(enum mtk_ddp_comp_id cur,
> +const struct mtk_mmsys_reg_data mt2701_mmsys_reg_data = {
> +	.ovl0_mout_en = DISP_REG_CONFIG_DISP_OVL_MOUT_EN,
> +	.dsi0_sel_in = DISP_REG_CONFIG_DSI_SEL,
> +	.dsi0_sel_in_rdma1 = DSI_SEL_IN_RDMA,
> +};
> +
> +const struct mtk_mmsys_reg_data mt8173_mmsys_reg_data = {
> +	.ovl0_mout_en = DISP_REG_CONFIG_DISP_OVL0_MOUT_EN,
> +	.rdma1_sout_sel_in = DISP_REG_CONFIG_DISP_RDMA1_SOUT_EN,
> +	.rdma1_sout_dpi0 = RDMA1_SOUT_DPI0,
> +	.dpi0_sel_in = DISP_REG_CONFIG_DPI_SEL_IN,
> +	.dpi0_sel_in_rdma1 = DPI0_SEL_IN_RDMA1,
> +	.dsi0_sel_in = DISP_REG_CONFIG_DSIE_SEL_IN,
> +	.dsi0_sel_in_rdma1 = DSI0_SEL_IN_RDMA1,
> +};
> +
> +static unsigned int mtk_ddp_mout_en(const struct mtk_mmsys_reg_data *data,
> +				    enum mtk_ddp_comp_id cur,
>  				    enum mtk_ddp_comp_id next,
>  				    unsigned int *addr)
>  {
>  	unsigned int value;
>  
>  	if (cur == DDP_COMPONENT_OVL0 && next == DDP_COMPONENT_COLOR0) {
> -		*addr = DISP_REG_CONFIG_DISP_OVL0_MOUT_EN;
> +		*addr = DISP_REG_OVL0_MOUT_EN(data);
>  		value = OVL0_MOUT_EN_COLOR0;
>  	} else if (cur == DDP_COMPONENT_OVL0 && next == DDP_COMPONENT_RDMA0) {
> -		*addr = DISP_REG_CONFIG_DISP_OVL_MOUT_EN;
> +		*addr = DISP_REG_OVL0_MOUT_EN(data);
>  		value = OVL_MOUT_EN_RDMA;
>  	} else if (cur == DDP_COMPONENT_OD0 && next == DDP_COMPONENT_RDMA0) {
>  		*addr = DISP_REG_CONFIG_DISP_OD_MOUT_EN;
> @@ -302,7 +343,8 @@ static unsigned int mtk_ddp_mout_en(enum mtk_ddp_comp_id cur,
>  	return value;
>  }
>  
> -static unsigned int mtk_ddp_sel_in(enum mtk_ddp_comp_id cur,
> +static unsigned int mtk_ddp_sel_in(const struct mtk_mmsys_reg_data *data,
> +				   enum mtk_ddp_comp_id cur,
>  				   enum mtk_ddp_comp_id next,
>  				   unsigned int *addr)
>  {
> @@ -312,14 +354,14 @@ static unsigned int mtk_ddp_sel_in(enum mtk_ddp_comp_id cur,
>  		*addr = DISP_REG_CONFIG_DISP_COLOR0_SEL_IN;
>  		value = COLOR0_SEL_IN_OVL0;
>  	} else if (cur == DDP_COMPONENT_RDMA1 && next == DDP_COMPONENT_DPI0) {
> -		*addr = DISP_REG_CONFIG_DPI_SEL_IN;
> -		value = DPI0_SEL_IN_RDMA1;
> +		*addr = DISP_REG_DPI0_SEL_IN(data);
> +		value = DISP_REG_DPI0_SEL_IN_RDMA1(data);
>  	} else if (cur == DDP_COMPONENT_RDMA1 && next == DDP_COMPONENT_DPI1) {
>  		*addr = DISP_REG_CONFIG_DPI_SEL_IN;
>  		value = DPI1_SEL_IN_RDMA1;
>  	} else if (cur == DDP_COMPONENT_RDMA1 && next == DDP_COMPONENT_DSI0) {
> -		*addr = DISP_REG_CONFIG_DSIE_SEL_IN;
> -		value = DSI0_SEL_IN_RDMA1;
> +		*addr = DISP_REG_DSI0_SEL_IN(data);
> +		value = DISP_REG_DSI0_SEL_IN_RDMA1(data);
>  	} else if (cur == DDP_COMPONENT_RDMA1 && next == DDP_COMPONENT_DSI1) {
>  		*addr = DISP_REG_CONFIG_DSIO_SEL_IN;
>  		value = DSI1_SEL_IN_RDMA1;
> @@ -360,7 +402,7 @@ static unsigned int mtk_ddp_sel_in(enum mtk_ddp_comp_id cur,
>  	return value;
>  }
>  
> -static unsigned int mtk_ddp_sout_sel(void __iomem *config_regs,
> +static unsigned int mtk_ddp_sout_sel(const struct mtk_mmsys_reg_data *data,
>  				     enum mtk_ddp_comp_id cur,
>  				     enum mtk_ddp_comp_id next,
>  				     unsigned int *addr)
> @@ -373,9 +415,6 @@ static unsigned int mtk_ddp_sout_sel(void __iomem *config_regs,
>  	} else if (cur == DDP_COMPONENT_BLS && next == DDP_COMPONENT_DPI0) {
>  		*addr = DISP_REG_CONFIG_OUT_SEL;
>  		value = BLS_TO_DPI_RDMA1_TO_DSI;
> -	} else if (cur == DDP_COMPONENT_RDMA1 && next == DDP_COMPONENT_DSI0) {
> -		*addr = DISP_REG_CONFIG_DSI_SEL;
> -		value = DSI_SEL_IN_RDMA;
>  	} else if (cur == DDP_COMPONENT_RDMA0 && next == DDP_COMPONENT_DPI0) {
>  		*addr = DISP_REG_CONFIG_DISP_RDMA0_SOUT_EN;
>  		value = RDMA0_SOUT_DPI0;
> @@ -401,8 +440,8 @@ static unsigned int mtk_ddp_sout_sel(void __iomem *config_regs,
>  		*addr = DISP_REG_CONFIG_DISP_RDMA1_SOUT_EN;
>  		value = RDMA1_SOUT_DSI3;
>  	} else if (cur == DDP_COMPONENT_RDMA1 && next == DDP_COMPONENT_DPI0) {
> -		*addr = DISP_REG_CONFIG_DISP_RDMA1_SOUT_EN;
> -		value = RDMA1_SOUT_DPI0;
> +		*addr = DISP_REG_RDMA1_SOUT_SEL_IN(data);
> +		value = DISP_REG_RDMA1_SOUT_DPI0(data);
>  	} else if (cur == DDP_COMPONENT_RDMA1 && next == DDP_COMPONENT_DPI1) {
>  		*addr = DISP_REG_CONFIG_DISP_RDMA1_SOUT_EN;
>  		value = RDMA1_SOUT_DPI1;
> @@ -429,22 +468,23 @@ static unsigned int mtk_ddp_sout_sel(void __iomem *config_regs,
>  }
>  
>  void mtk_ddp_add_comp_to_path(void __iomem *config_regs,
> +			      const struct mtk_mmsys_reg_data *reg_data,
>  			      enum mtk_ddp_comp_id cur,
>  			      enum mtk_ddp_comp_id next)
>  {
>  	unsigned int addr, value, reg;
>  
> -	value = mtk_ddp_mout_en(cur, next, &addr);
> +	value = mtk_ddp_mout_en(reg_data, cur, next, &addr);
>  	if (value) {
>  		reg = readl_relaxed(config_regs + addr) | value;
>  		writel_relaxed(reg, config_regs + addr);
>  	}
>  
> -	value = mtk_ddp_sout_sel(cur, next, &addr);
> +	value = mtk_ddp_sout_sel(reg_data, cur, next, &addr);
>  	if (value)
>  		writel_relaxed(value, config_regs + addr);
>  
> -	value = mtk_ddp_sel_in(cur, next, &addr);
> +	value = mtk_ddp_sel_in(reg_data, cur, next, &addr);
>  	if (value) {
>  		reg = readl_relaxed(config_regs + addr) | value;
>  		writel_relaxed(reg, config_regs + addr);
> @@ -452,24 +492,48 @@ void mtk_ddp_add_comp_to_path(void __iomem *config_regs,
>  }
>  
>  void mtk_ddp_remove_comp_from_path(void __iomem *config_regs,
> +				   const struct mtk_mmsys_reg_data *reg_data,
>  				   enum mtk_ddp_comp_id cur,
>  				   enum mtk_ddp_comp_id next)
>  {
>  	unsigned int addr, value, reg;
>  
> -	value = mtk_ddp_mout_en(cur, next, &addr);
> +	value = mtk_ddp_mout_en(reg_data, cur, next, &addr);
>  	if (value) {
>  		reg = readl_relaxed(config_regs + addr) & ~value;
>  		writel_relaxed(reg, config_regs + addr);
>  	}
>  
> -	value = mtk_ddp_sel_in(cur, next, &addr);
> +	value = mtk_ddp_sel_in(reg_data, cur, next, &addr);
>  	if (value) {
>  		reg = readl_relaxed(config_regs + addr) & ~value;
>  		writel_relaxed(reg, config_regs + addr);
>  	}
>  }
>  
> +const struct mtk_mmsys_reg_data *mtk_ddp_get_mmsys_data(enum mtk_mmsys_id id)
> +{
> +	const struct mtk_mmsys_reg_data *data = NULL;
> +
> +	switch (id) {
> +	case MMSYS_MT2701:
> +		data = &mt2701_mmsys_reg_data;
> +		break;
> +	case MMSYS_MT2712:
> +		data = &mt8173_mmsys_reg_data;
> +		break;
> +	case MMSYS_MT8173:
> +		data = &mt8173_mmsys_reg_data;
> +		break;
> +	default:
> +		pr_info("mtk drm not support mmsys id %d\n",
> +			id);
> +		break;
> +	}
> +
> +	return data;
> +}
> +
>  struct mtk_disp_mutex *mtk_disp_mutex_get(struct device *dev, unsigned int id)
>  {
>  	struct mtk_ddp *ddp = dev_get_drvdata(dev);
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp.h b/drivers/gpu/drm/mediatek/mtk_drm_ddp.h
> index f9a7991..ed2b702 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp.h
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp.h
> @@ -19,11 +19,21 @@
>  struct regmap;
>  struct device;
>  struct mtk_disp_mutex;
> +struct mtk_mmsys_reg_data;
> +enum mtk_mmsys_id {
> +	MMSYS_MT2701,
> +	MMSYS_MT2712,
> +	MMSYS_MT8173,
> +	MMSYS_MAX,
> +};
>  
> +const struct mtk_mmsys_reg_data *mtk_ddp_get_mmsys_data(enum mtk_mmsys_id id);
>  void mtk_ddp_add_comp_to_path(void __iomem *config_regs,
> +			      const struct mtk_mmsys_reg_data *reg_data,
>  			      enum mtk_ddp_comp_id cur,
>  			      enum mtk_ddp_comp_id next);
>  void mtk_ddp_remove_comp_from_path(void __iomem *config_regs,
> +				   const struct mtk_mmsys_reg_data *reg_data,
>  				   enum mtk_ddp_comp_id cur,
>  				   enum mtk_ddp_comp_id next);
>  
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> index 6422e99..be6b142 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> @@ -196,6 +196,7 @@ static int mtk_atomic_commit(struct drm_device *drm,
>  	.main_len = ARRAY_SIZE(mt2701_mtk_ddp_main),
>  	.ext_path = mt2701_mtk_ddp_ext,
>  	.ext_len = ARRAY_SIZE(mt2701_mtk_ddp_ext),
> +	.mmsys_id = MMSYS_MT2701,
>  	.shadow_register = true,
>  };
>  
> @@ -206,6 +207,7 @@ static int mtk_atomic_commit(struct drm_device *drm,
>  	.ext_len = ARRAY_SIZE(mt2712_mtk_ddp_ext),
>  	.third_path = mt2712_mtk_ddp_third,
>  	.third_len = ARRAY_SIZE(mt2712_mtk_ddp_third),
> +	.mmsys_id = MMSYS_MT2712,
>  };
>  
>  static const struct mtk_mmsys_driver_data mt8173_mmsys_driver_data = {
> @@ -213,6 +215,7 @@ static int mtk_atomic_commit(struct drm_device *drm,
>  	.main_len = ARRAY_SIZE(mt8173_mtk_ddp_main),
>  	.ext_path = mt8173_mtk_ddp_ext,
>  	.ext_len = ARRAY_SIZE(mt8173_mtk_ddp_ext),
> +	.mmsys_id = MMSYS_MT8173,
>  };
>  
>  static int mtk_drm_kms_init(struct drm_device *drm)
> @@ -461,6 +464,14 @@ static int mtk_drm_probe(struct platform_device *pdev)
>  	INIT_WORK(&private->commit.work, mtk_atomic_work);
>  	private->data = of_device_get_match_data(dev);
>  
> +	private->reg_data = mtk_ddp_get_mmsys_data(private->data->mmsys_id);
> +	if (IS_ERR(private->reg_data)) {
> +		ret = PTR_ERR(private->config_regs);
> +		pr_info("Failed to get mmsys register data: %d\n",
> +			ret);
> +		return ret;
> +	}
> +
>  	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	private->config_regs = devm_ioremap_resource(dev, mem);
>  	if (IS_ERR(private->config_regs)) {
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.h b/drivers/gpu/drm/mediatek/mtk_drm_drv.h
> index ecc00ca..11de7f8 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.h
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.h
> @@ -15,6 +15,7 @@
>  #define MTK_DRM_DRV_H
>  
>  #include <linux/io.h>
> +#include "mtk_drm_ddp.h"
>  #include "mtk_drm_ddp_comp.h"
>  
>  #define MAX_CRTC	3
> @@ -36,6 +37,8 @@ struct mtk_mmsys_driver_data {
>  	const enum mtk_ddp_comp_id *third_path;
>  	unsigned int third_len;
>  
> +	enum mtk_mmsys_id mmsys_id;
> +
>  	bool shadow_register;
>  };
>  
> @@ -48,6 +51,7 @@ struct mtk_drm_private {
>  	struct device_node *mutex_node;
>  	struct device *mutex_dev;
>  	void __iomem *config_regs;
> +	const struct mtk_mmsys_reg_data *reg_data;

Why not move reg_data into data? Both could be got by
of_device_get_match_data().

Regards,
CK

>  	struct device_node *comp_node[DDP_COMPONENT_ID_MAX];
>  	struct mtk_ddp_comp *ddp_comp[DDP_COMPONENT_ID_MAX];
>  	const struct mtk_mmsys_driver_data *data;

Re: [PATCH 07/18] drm/mediatek: add commponent OVL0_2L

[CK Hu]

Hi, Yongqiang:

On Mon, 2018-12-24 at 16:08 +0800, Yongqiang Niu wrote:
> This patch add commponent OVL0_2L
> 
> Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 1 +
>  drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> index 310c0b9..1b2d9e9 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> @@ -266,6 +266,7 @@ struct mtk_ddp_comp_match {
>  	[DDP_COMPONENT_OD1]	= { MTK_DISP_OD,	1, &ddp_od },
>  	[DDP_COMPONENT_OVL0]	= { MTK_DISP_OVL,	0, NULL },
>  	[DDP_COMPONENT_OVL1]	= { MTK_DISP_OVL,	1, NULL },
> +	[DDP_COMPONENT_OVL0_2L]	= { MTK_DISP_OVL,	2, NULL },

If ovl_2l is different than ovl, I think it's better to add new type for
ovl_2l.

Regards,
CK

>  	[DDP_COMPONENT_PWM0]	= { MTK_DISP_PWM,	0, NULL },
>  	[DDP_COMPONENT_PWM1]	= { MTK_DISP_PWM,	1, NULL },
>  	[DDP_COMPONENT_PWM2]	= { MTK_DISP_PWM,	2, NULL },
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> index 87ef290..3c74b63 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> @@ -58,6 +58,7 @@ enum mtk_ddp_comp_id {
>  	DDP_COMPONENT_OD0,
>  	DDP_COMPONENT_OD1,
>  	DDP_COMPONENT_OVL0,
> +	DDP_COMPONENT_OVL0_2L,
>  	DDP_COMPONENT_OVL1,
>  	DDP_COMPONENT_PWM0,
>  	DDP_COMPONENT_PWM1,

Re: [PATCH 09/18] drm/mediatek: add component DITHER

[CK Hu]

Hi, Yongqiang:

On Mon, 2018-12-24 at 16:08 +0800, Yongqiang Niu wrote:
> This patch add component DITHER
> 

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

> Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 32 +++++++++++++++++++++++++++++
>  drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h |  2 ++
>  2 files changed, 34 insertions(+)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> index 63f4b1e..a97e27b 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> @@ -47,6 +47,12 @@
>  #define CCORR_RELAY_MODE			BIT(0)
>  #define DISP_CCORR_SIZE				0x0030
>  
> +#define DISP_DITHER_EN				0x0000
> +#define DITHER_EN				BIT(0)
> +#define DISP_DITHER_CFG                        0x0020
> +#define DITHER_RELAY_MODE			BIT(0)
> +#define DISP_DITHER_SIZE			0x0030
> +
>  #define DISP_GAMMA_EN				0x0000
>  #define DISP_GAMMA_CFG				0x0020
>  #define DISP_GAMMA_SIZE				0x0030
> @@ -155,6 +161,24 @@ static void mtk_ccorr_stop(struct mtk_ddp_comp *comp)
>  	writel_relaxed(0x0, comp->regs + DISP_CCORR_EN);
>  }
>  
> +static void mtk_dither_config(struct mtk_ddp_comp *comp, unsigned int w,
> +			      unsigned int h, unsigned int vrefresh,
> +			      unsigned int bpc)
> +{
> +	writel(h << 16 | w, comp->regs + DISP_DITHER_SIZE);
> +	writel(DITHER_RELAY_MODE, comp->regs + DISP_DITHER_CFG);
> +}
> +
> +static void mtk_dither_start(struct mtk_ddp_comp *comp)
> +{
> +	writel(DITHER_EN, comp->regs + DISP_DITHER_EN);
> +}
> +
> +static void mtk_dither_stop(struct mtk_ddp_comp *comp)
> +{
> +	writel_relaxed(0x0, comp->regs + DISP_DITHER_EN);
> +}
> +
>  static void mtk_gamma_config(struct mtk_ddp_comp *comp, unsigned int w,
>  			     unsigned int h, unsigned int vrefresh,
>  			     unsigned int bpc)
> @@ -209,6 +233,12 @@ static void mtk_gamma_set(struct mtk_ddp_comp *comp,
>  	.stop = mtk_ccorr_stop,
>  };
>  
> +static const struct mtk_ddp_comp_funcs ddp_dither = {
> +	.config = mtk_dither_config,
> +	.start = mtk_dither_start,
> +	.stop = mtk_dither_stop,
> +};
> +
>  static const struct mtk_ddp_comp_funcs ddp_gamma = {
>  	.gamma_set = mtk_gamma_set,
>  	.config = mtk_gamma_config,
> @@ -233,6 +263,7 @@ static void mtk_gamma_set(struct mtk_ddp_comp *comp,
>  	[MTK_DISP_CCORR] = "ccorr",
>  	[MTK_DISP_AAL] = "aal",
>  	[MTK_DISP_GAMMA] = "gamma",
> +	[MTK_DISP_DITHER] = "dither",
>  	[MTK_DISP_UFOE] = "ufoe",
>  	[MTK_DSI] = "dsi",
>  	[MTK_DPI] = "dpi",
> @@ -255,6 +286,7 @@ struct mtk_ddp_comp_match {
>  	[DDP_COMPONENT_CCORR]	= { MTK_DISP_CCORR,	0, &ddp_ccorr },
>  	[DDP_COMPONENT_COLOR0]	= { MTK_DISP_COLOR,	0, NULL },
>  	[DDP_COMPONENT_COLOR1]	= { MTK_DISP_COLOR,	1, NULL },
> +	[DDP_COMPONENT_DITHER]	= { MTK_DISP_DITHER,	0, &ddp_dither },
>  	[DDP_COMPONENT_DPI0]	= { MTK_DPI,		0, NULL },
>  	[DDP_COMPONENT_DPI1]	= { MTK_DPI,		1, NULL },
>  	[DDP_COMPONENT_DSI0]	= { MTK_DSI,		0, NULL },
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> index 6905647..b0064c52 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> @@ -29,6 +29,7 @@ enum mtk_ddp_comp_type {
>  	MTK_DISP_WDMA,
>  	MTK_DISP_COLOR,
>  	MTK_DISP_CCORR,
> +	MTK_DISP_DITHER,
>  	MTK_DISP_AAL,
>  	MTK_DISP_GAMMA,
>  	MTK_DISP_UFOE,
> @@ -48,6 +49,7 @@ enum mtk_ddp_comp_id {
>  	DDP_COMPONENT_CCORR,
>  	DDP_COMPONENT_COLOR0,
>  	DDP_COMPONENT_COLOR1,
> +	DDP_COMPONENT_DITHER,
>  	DDP_COMPONENT_DPI0,
>  	DDP_COMPONENT_DPI1,
>  	DDP_COMPONENT_DSI0,

Re: [PATCH 11/18] drm/medaitek: add layer_nr for ovl private data

[CK Hu]

Hi, Yongqiang:

On Mon, 2018-12-24 at 16:08 +0800, Yongqiang Niu wrote:
> This patch add layer_nr for ovl private data
> 

I think you do this because ovl-2l and ovl share the same driver and the
ovl layer is different, so this patch is a preparation for ovl-2l and
ovl share the same driver. Describe more information for this patch.

Regards,
CK

> Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> index afb313c..a0ab760 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> @@ -60,6 +60,7 @@
>  struct mtk_disp_ovl_data {
>  	unsigned int addr;
>  	unsigned int gmc_bits;
> +	unsigned int layer_nr;
>  	bool fmt_rgb565_is_0;
>  };
>  
> @@ -137,7 +138,9 @@ static void mtk_ovl_config(struct mtk_ddp_comp *comp, unsigned int w,
>  
>  static unsigned int mtk_ovl_layer_nr(struct mtk_ddp_comp *comp)
>  {
> -	return 4;
> +	struct mtk_disp_ovl *ovl = comp_to_ovl(comp);
> +
> +	return ovl->data->layer_nr;
>  }
>  
>  static void mtk_ovl_layer_on(struct mtk_ddp_comp *comp, unsigned int idx)
> @@ -342,12 +345,14 @@ static int mtk_disp_ovl_remove(struct platform_device *pdev)
>  static const struct mtk_disp_ovl_data mt2701_ovl_driver_data = {
>  	.addr = DISP_REG_OVL_ADDR_MT2701,
>  	.gmc_bits = 8,
> +	.layer_nr = 4,
>  	.fmt_rgb565_is_0 = false,
>  };
>  
>  static const struct mtk_disp_ovl_data mt8173_ovl_driver_data = {
>  	.addr = DISP_REG_OVL_ADDR_MT8173,
>  	.gmc_bits = 8,
> +	.layer_nr = 4,
>  	.fmt_rgb565_is_0 = true,
>  };
>  

Re: [PATCH 13/18] drm/mediatek: add ddp write register common api

[CK Hu]

Hi, Yongqiang:

On Mon, 2018-12-24 at 16:08 +0800, Yongqiang Niu wrote:
> This patch add ddp write register common api
> 

I could not see anywhere you use these function. If the function is
useless, drop this patch.

Regards,
CK

> Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 24 ++++++++++++++++++++++++
>  drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h |  9 +++++++++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> index a97e27b..1c0f9cc 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> @@ -84,6 +84,30 @@
>  #define DITHER_ADD_LSHIFT_G(x)			(((x) & 0x7) << 4)
>  #define DITHER_ADD_RSHIFT_G(x)			(((x) & 0x7) << 0)
>  
> +void mtk_ddp_write(unsigned int value, struct mtk_ddp_comp *comp,
> +		   unsigned int offset)
> +{
> +	writel(value, comp->regs + offset);
> +}
> +
> +void mtk_ddp_write_relaxed(unsigned int value,
> +			   struct mtk_ddp_comp *comp,
> +			   unsigned int offset)
> +{
> +	writel_relaxed(value, comp->regs + offset);
> +}
> +
> +void mtk_ddp_write_mask(unsigned int value,
> +			struct mtk_ddp_comp *comp,
> +			unsigned int offset,
> +			unsigned int mask)
> +{
> +	unsigned int tmp = readl(comp->regs + offset);
> +
> +	tmp = (tmp & ~mask) | (value & mask);
> +	writel(tmp, comp->regs + offset);
> +}
> +
>  void mtk_dither_set(struct mtk_ddp_comp *comp, unsigned int bpc,
>  		    unsigned int CFG)
>  {
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> index f2ab0b3..b908172 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h
> @@ -189,5 +189,14 @@ int mtk_ddp_comp_init(struct device *dev, struct device_node *comp_node,
>  void mtk_ddp_comp_unregister(struct drm_device *drm, struct mtk_ddp_comp *comp);
>  void mtk_dither_set(struct mtk_ddp_comp *comp, unsigned int bpc,
>  		    unsigned int CFG);
> +void mtk_ddp_write(unsigned int value, struct mtk_ddp_comp *comp,
> +		   unsigned int offset);
> +void mtk_ddp_write_relaxed(unsigned int value,
> +			   struct mtk_ddp_comp *comp,
> +			   unsigned int offset);
> +void mtk_ddp_write_mask(unsigned int value,
> +			struct mtk_ddp_comp *comp,
> +			unsigned int offset,
> +			unsigned int mask);
>  
>  #endif /* MTK_DRM_DDP_COMP_H */

Re: [PATCH 14/18] drm/mediatek: add connect function for ovl

[CK Hu]

Hi, Yongqiang:

On Mon, 2018-12-24 at 16:08 +0800, Yongqiang Niu wrote:
> This patch add connect function for ovl

Could you describe more about how ovl-2l works? I guess that ovl-2l is a
ovl hardware which has 3 layer, the bottom two layer is from DRAM and
the top layer is from another hardware (maybe the ovl). If my guess is
correct, why not just implement this function in mtk_ovl_layer_on()? In
mtk_ovl_layer_on(), you could do as

if (idx == ovl->data->layer_nr) {
	mtk_ddp_write_mask((1 << 2), comp,
			DISP_REG_OVL_DATAPATH_CON, OVL_BGCLR_SEL_IN);

	return;
}

So does mtk_ovl_layer_off().

Regards,
CK

> 
> Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> index a0ab760..3b2ce77 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c
> @@ -27,6 +27,8 @@
>  #define DISP_REG_OVL_EN				0x000c
>  #define DISP_REG_OVL_RST			0x0014
>  #define DISP_REG_OVL_ROI_SIZE			0x0020
> +#define DISP_REG_OVL_DATAPATH_CON		0x0024
> +#define OVL_BGCLR_SEL_IN                        BIT(2)
>  #define DISP_REG_OVL_ROI_BGCLR			0x0028
>  #define DISP_REG_OVL_SRC_CON			0x002c
>  #define DISP_REG_OVL_CON(n)			(0x0030 + 0x20 * (n))
> @@ -245,6 +247,19 @@ static void mtk_ovl_layer_config(struct mtk_ddp_comp *comp, unsigned int idx,
>  		mtk_ovl_layer_on(comp, idx);
>  }
>  
> +static void mtk_ovl_connect(struct mtk_ddp_comp *comp,
> +			    enum mtk_ddp_comp_id prev)
> +{
> +	int is_ovl = 0;
> +
> +	if (prev == DDP_COMPONENT_OVL0 || prev == DDP_COMPONENT_OVL1 ||
> +	    prev == DDP_COMPONENT_OVL0_2L || prev == DDP_COMPONENT_OVL1_2L)
> +		is_ovl = 1;
> +
> +	mtk_ddp_write_mask((is_ovl << 2), comp,
> +			   DISP_REG_OVL_DATAPATH_CON, OVL_BGCLR_SEL_IN);
> +}
> +
>  static const struct mtk_ddp_comp_funcs mtk_disp_ovl_funcs = {
>  	.config = mtk_ovl_config,
>  	.start = mtk_ovl_start,
> @@ -255,6 +270,7 @@ static void mtk_ovl_layer_config(struct mtk_ddp_comp *comp, unsigned int idx,
>  	.layer_on = mtk_ovl_layer_on,
>  	.layer_off = mtk_ovl_layer_off,
>  	.layer_config = mtk_ovl_layer_config,
> +	.connect = mtk_ovl_connect,
>  };
>  
>  static int mtk_disp_ovl_bind(struct device *dev, struct device *master,

Re: [PATCH 15/18] drm/mediatek: add RDMA1 fifo size into RDMA private data

[CK Hu]

Hi, Yongqiang:

On Mon, 2018-12-24 at 16:08 +0800, Yongqiang Niu wrote:
> This patch add RDMA1 fifo size into RDMA private data
> 
> Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_disp_rdma.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_rdma.c b/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
> index b0a5cff..3f9b4d4 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_rdma.c
> @@ -53,12 +53,14 @@
>  #define RDMA_FIFO_PSEUDO_SIZE(bytes)			(((bytes) / 16) << 16)
>  #define RDMA_OUTPUT_VALID_FIFO_THRESHOLD(bytes)		((bytes) / 16)
>  #define RDMA_FIFO_SIZE(rdma)			((rdma)->data->fifo_size)
> +#define RDMA_FIFO_SIZE1(rdma)			((rdma)->data->fifo_size1)
>  #define DISP_RDMA_MEM_START_ADDR		0x0f00
>  
>  #define RDMA_MEM_GMC				0x40402020
>  
>  struct mtk_disp_rdma_data {
>  	unsigned int fifo_size;
> +	unsigned int fifo_size1;
>  };
>  
>  /**
> @@ -137,11 +139,17 @@ static void mtk_rdma_config(struct mtk_ddp_comp *comp, unsigned int width,
>  {
>  	unsigned int threshold;
>  	unsigned int reg;
> +	unsigned int rdma_fifo_size;
>  	struct mtk_disp_rdma *rdma = comp_to_rdma(comp);
>  
>  	rdma_update_bits(comp, DISP_REG_RDMA_SIZE_CON_0, 0xfff, width);
>  	rdma_update_bits(comp, DISP_REG_RDMA_SIZE_CON_1, 0xfffff, height);
>  
> +	if (comp->id == DDP_COMPONENT_RDMA1)
> +		rdma_fifo_size = RDMA_FIFO_SIZE1(rdma);
> +	else
> +		rdma_fifo_size = RDMA_FIFO_SIZE(rdma);
> +

It looks like that mt81830-rdma0 and mt8183-rdma1 are different in
hardware, so I think fifo_size should be decided by something from
device tree rather than from its name. Maybe add a property 'FIFO_SIZE'
in rdma device node or create an additional compatible string for
mt8183-rdma1.

Regards,
CK

>  	/*
>  	 * Enable FIFO underflow since DSI and DPI can't be blocked.
>  	 * Keep the FIFO pseudo size reset default of 8 KiB. Set the
> @@ -149,8 +157,12 @@ static void mtk_rdma_config(struct mtk_ddp_comp *comp, unsigned int width,
>  	 * account for blanking, and with a pixel depth of 4 bytes:
>  	 */
>  	threshold = width * height * vrefresh * 4 * 7 / 1000000;
> +
> +	if (threshold > rdma_fifo_size)
> +		threshold = rdma_fifo_size;
> +
>  	reg = RDMA_FIFO_UNDERFLOW_EN |
> -	      RDMA_FIFO_PSEUDO_SIZE(RDMA_FIFO_SIZE(rdma)) |
> +	      RDMA_FIFO_PSEUDO_SIZE(rdma_fifo_size) |
>  	      RDMA_OUTPUT_VALID_FIFO_THRESHOLD(threshold);
>  	writel(reg, comp->regs + DISP_REG_RDMA_FIFO_CON);
>  }
> @@ -330,10 +342,12 @@ static int mtk_disp_rdma_remove(struct platform_device *pdev)
>  
>  static const struct mtk_disp_rdma_data mt2701_rdma_driver_data = {
>  	.fifo_size = SZ_4K,
> +	.fifo_size1 = SZ_4K,
>  };
>  
>  static const struct mtk_disp_rdma_data mt8173_rdma_driver_data = {
>  	.fifo_size = SZ_8K,
> +	.fifo_size1 = SZ_8K,
>  };
>  
>  static const struct of_device_id mtk_disp_rdma_driver_dt_match[] = {