Re: [PATCH 1/2] drm/mediatek: Support CMDQ interface in ddp component

[Nicolas Boichat]

On Fri, Aug 30, 2019 at 3:41 PM Bibby Hsieh <bibby.hsieh@xxxxxxxxxxxx> wrote:
>
> The CMDQ (Command Queue) in MT8183 is used to help
> update all relevant display controller registers
> with critical time limation.
> This patch add cmdq interface in ddp_comp interface,
> let all ddp_comp interface can support cpu/cmdq function
> at the same time.
>
> Signed-off-by: YT Shen <yt.shen@xxxxxxxxxxxx>
> Signed-off-by: CK Hu <ck.hu@xxxxxxxxxxxx>
> Signed-off-by: Philipp Zabel <p.zabel@xxxxxxxxxxxxxx>
> Signed-off-by: Bibby Hsieh <bibby.hsieh@xxxxxxxxxxxx>
> Signed-off-by: Yongqiang Niu <yongqiang.niu@xxxxxxxxxxxx>
> --- [snip]
>  static void mtk_gamma_set(struct mtk_ddp_comp *comp,
> -                         struct drm_crtc_state *state)
> +                         struct drm_crtc_state *state,
> +                         struct cmdq_pkt *cmdq_pkt)
>  {
> -       unsigned int i, reg;
> +       unsigned int i;
>         struct drm_color_lut *lut;
>         void __iomem *lut_base;
>         u32 word;
>
>         if (state->gamma_lut) {
> -               reg = readl(comp->regs + DISP_GAMMA_CFG);
> -               reg = reg | GAMMA_LUT_EN;
> -               writel(reg, comp->regs + DISP_GAMMA_CFG);
> +               mtk_ddp_write_mask(cmdq_pkt, GAMMA_LUT_EN, comp,
> +                                  DISP_GAMMA_CFG, GAMMA_LUT_EN);
>                 lut_base = comp->regs + DISP_GAMMA_LUT;
>                 lut = (struct drm_color_lut *)state->gamma_lut->data;
>                 for (i = 0; i < MTK_LUT_SIZE; i++) {
>                         word = (((lut[i].red >> 6) & LUT_10BIT_MASK) << 20) +
>                                 (((lut[i].green >> 6) & LUT_10BIT_MASK) << 10) +
>                                 ((lut[i].blue >> 6) & LUT_10BIT_MASK);
> -                       writel(word, (lut_base + i * 4));
> +                       mtk_ddp_write(cmdq_pkt, word, comp,
> +                                     (unsigned int)(lut_base + i * 4));

Guenter pointed out that this looks quite wrong. We should have:
unsigned int lut_base;
lut_base = DISP_GAMMA_LUT;
mtk_ddp_write(cmdq_pkt, word, comp, lut_base + i * 4);

Or more simply:
mtk_ddp_write(cmdq_pkt, word, comp, DISP_GAMMA_LUT + i * 4);

>                 }
>         }