Re: [PATCH v3 14/17] drm/mediatek: add OUTPROC support for MT8196

Paul-pl Chen (陳柏霖)
2025-07-04T07:27:21Z

On Wed, 2025-06-18 at 08:23 +0000, CK Hu (胡俊光) wrote:
> On Thu, 2025-05-15 at 17:34 +0800, paul-pl.chen wrote:
> > From: Nancy Lin <nancy.lin@mediatek.com>
> >
> > OUTPROC handles the post-stage of pixel processing in
> > the overlapping procedure.OUTPROC manages pixels for
> > gamma correction and ensures that pixel values are
> > within the correct range.
> >
> > Signed-off-by: Nancy Lin <nancy.lin@mediatek.com>
> > Signed-off-by: Paul-pl Chen <paul-pl.chen@mediatek.com>
> > ---
>
> [snip]
>
> > +void mtk_disp_outproc_config(struct device *dev, unsigned int w,
> > +      unsigned int h, unsigned int
> > vrefresh,
> > +      unsigned int bpc, struct cmdq_pkt
> > *cmdq_pkt)
> > +{
> > + struct mtk_disp_outproc *priv = dev_get_drvdata(dev);
> > + u32 tmp;
> > +
> > + dev_dbg(dev, "%s-w:%d, h:%d\n", __func__, w, h);
> > +
> > + writel((h << 16) | w, priv->regs +
> > DISP_REG_OVL_OUTPROC_ROI_SIZE);
> > +
> > + tmp = readl(priv->regs +
> > DISP_REG_OVL_OUTPROC_DATAPATH_CON);
> > + tmp = (tmp & ~OVL_OUTPROC_DATAPATH_CON_OUTPUT_CLAMP) |
> > +   (OVL_OUTPROC_DATAPATH_CON_OUTPUT_CLAMP &
> > OVL_OUTPROC_DATAPATH_CON_OUTPUT_CLAMP);
>
> This would simplified as
>
> tmp |= OVL_OUTPROC_DATAPATH_CON_OUTPUT_CLAMP;
>

OK, I will refine the code to simplify this part.

> > + writel(tmp, priv->regs +
> > DISP_REG_OVL_OUTPROC_DATAPATH_CON);
> > +}
> > +
> > +void mtk_disp_outproc_start(struct device *dev)
> > +{
> > + struct mtk_disp_outproc *priv = dev_get_drvdata(dev);
> > + unsigned int tmp;
> > +
> > + tmp = readl(priv->regs +
> > DISP_REG_OVL_OUTPROC_SHADOW_CTRL);
> > + tmp = tmp | OVL_OUTPROC_BYPASS_SHADOW;
>
> tmp |= OVL_OUTPROC_BYPASS_SHADOW;

Understood, I will refine the code to simplify this part.
>
> > + writel(tmp, priv->regs +
> > DISP_REG_OVL_OUTPROC_SHADOW_CTRL);
> > +
> > + writel(0, priv->regs + DISP_REG_OVL_OUTPROC_INTSTA);
>
> To align OVL driver, clear interrupt status in enable_vblank()
> function.
>
>

Understood, I will refine the code and move
"the clear interrupt status" to function of enable_vblank().
> > +
> > + tmp = readl(priv->regs + DISP_REG_OVL_OUTPROC_EN);
> > + tmp = (tmp & ~OVL_OUTPROC_OVL_EN) | (OVL_OUTPROC_OVL_EN &
> > OVL_OUTPROC_OVL_EN);
>
> This would be simplified as
>
> tmp |= OVL_OUTPROC_OVL_EN;
>

Understood, I will simplify the code.

> One question. In mtk_disp_outproc_stop(), you overwrite the whole
> register of  DISP_REG_OVL_OUTPROC_EN.
> Why here not overwrite the whole register?
>
> Regards,
> CK
>
Hi Ck, I will perform an experiment to verify this part.

Best, Paul
> > + writel(tmp, priv->regs + DISP_REG_OVL_OUTPROC_EN);
> > +}
> > +
> > +void mtk_disp_outproc_stop(struct device *dev)
> > +{
> > + struct mtk_disp_outproc *priv = dev_get_drvdata(dev);
> > +
> > + writel(0, priv->regs + DISP_REG_OVL_OUTPROC_EN);
> > + writel(OVL_OUTPROC_RST, priv->regs +
> > DISP_REG_OVL_OUTPROC_RST);
> > + writel(0, priv->regs + DISP_REG_OVL_OUTPROC_RST);
> > +}
> > +
>