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

["Paul-pl Chen (陳柏霖)" <Paul-pl.Chen@mediatek.com>]

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);
> > +}
> > +
>