Re: [PATCH v2 12/15] drm/mediatek: add OUTPROC support for MT8196

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

On Mon, 2025-03-24 at 09:00 +0000, CK Hu (胡俊光) wrote:
> On Fri, 2025-03-21 at 17:33 +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]
> 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_disp_outproc.c
> > b/drivers/gpu/drm/mediatek/mtk_disp_outproc.c
> > new file mode 100644
> > index 000000000000..a7c6d1982bca
> > --- /dev/null
> > +++ b/drivers/gpu/drm/mediatek/mtk_disp_outproc.c
> > @@ -0,0 +1,242 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) 2025 MediaTek Inc.
> > + */
> > +
> > +#include <drm/drm_fourcc.h>
> > +#include <drm/drm_framebuffer.h>
> > +#include <linux/clk.h>
> > +#include <linux/component.h>
> > +#include <linux/of.h>
> > +#include <linux/of_device.h>
> > +#include <linux/of_address.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/reset.h>
> > +#include <linux/soc/mediatek/mtk-cmdq.h>
> > +#include <linux/soc/mediatek/mtk-mmsys.h>
> > +
> > +#include "mtk_crtc.h"
> > +#include "mtk_ddp_comp.h"
> > +#include "mtk_drm_drv.h"
> 
> Alphabetic order.
> 
> OK, I will fix the Alphabetic order.
> 
> > +#include "mtk_disp_outproc.h"
> > +
> > +#define
> > DISP_REG_OVL_OUTPROC_INTEN				0x004
> > +#define
> > OVL_OUTPROC_FME_CPL_INTEN					BIT(1)
> > +#define
> > DISP_REG_OVL_OUTPROC_INTSTA				0x008
> > +#define
> > DISP_REG_OVL_OUTPROC_DATAPATH_CON			0x010
> > +#define
> > OVL_OUTPROC_DATAPATH_CON_OUTPUT_CLAMP				BIT(26)
> > +
> > +#define
> > DISP_REG_OVL_OUTPROC_EN					0x020
> > +#define
> > OVL_OUTPROC_OVL_EN						BIT(0)
> > +#define
> > DISP_REG_OVL_OUTPROC_RST				0x024
> > +#define
> > OVL_OUTPROC_RST							BIT(0)
> > +#define
> > DISP_REG_OVL_OUTPROC_SHADOW_CTRL			0x028
> > +#define
> > OVL_OUTPROC_BYPASS_SHADOW					BIT(2)
> > +#define
> > DISP_REG_OVL_OUTPROC_ROI_SIZE				0x030
> > +
> > +struct mtk_disp_outproc {
> > +	void __iomem		*regs;
> > +	struct clk		*clk;
> > +	void			(*vblank_cb)(void *data);
> > +	void			*vblank_cb_data;
> > +	int			irq;
> > +	struct cmdq_client_reg	cmdq_reg;
> > +};
> > +
> > +void mtk_disp_outproc_register_vblank_cb(struct device *dev,
> > +					 void (*vblank_cb)(void
> > *),
> > +					 void *vblank_cb_data)
> > +{
> > +	struct mtk_disp_outproc *priv = dev_get_drvdata(dev);
> > +
> > +	priv->vblank_cb = vblank_cb;
> > +	priv->vblank_cb_data = vblank_cb_data;
> > +}
> > +
> > +void mtk_disp_outproc_unregister_vblank_cb(struct device *dev)
> > +{
> > +	struct mtk_disp_outproc *priv = dev_get_drvdata(dev);
> > +
> > +	priv->vblank_cb = NULL;
> > +	priv->vblank_cb_data = NULL;
> > +}
> > +
> > +void mtk_disp_outproc_enable_vblank(struct device *dev)
> > +{
> > +	struct mtk_disp_outproc *priv = dev_get_drvdata(dev);
> > +
> > +	writel(OVL_OUTPROC_FME_CPL_INTEN, priv->regs +
> > DISP_REG_OVL_OUTPROC_INTEN);
> > +}
> > +
> > +void mtk_disp_outproc_disable_vblank(struct device *dev)
> > +{
> > +	struct mtk_disp_outproc *priv = dev_get_drvdata(dev);
> > +
> > +	writel(0x0, priv->regs + DISP_REG_OVL_OUTPROC_INTEN);
> > +}
> > +
> > +static irqreturn_t mtk_disp_outproc_irq_handler(int irq, void
> > *dev_id)
> > +{
> > +	struct mtk_disp_outproc *priv = dev_id;
> > +	u32 val;
> > +
> > +	val = readl(priv->regs + DISP_REG_OVL_OUTPROC_INTSTA);
> > +	if (!val)
> > +		return IRQ_NONE;
> > +
> > +	writel(0x0, priv->regs + DISP_REG_OVL_OUTPROC_INTSTA);
> > +
> > +	if (priv->vblank_cb)
> > +		priv->vblank_cb(priv->vblank_cb_data);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +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);
> > +
> > +	dev_dbg(dev, "%s-w:%d, h:%d\n", __func__, w, h);
> > +
> > +	//move mtk_ddp_write_mask to mtk_ddp_write
> 
> I think this comment is redundant.

Sure, this redundant comment will be removed at the next version.
> 
> > +	mtk_ddp_write(cmdq_pkt, h << 16 | w, &priv->cmdq_reg,
> > priv->regs,
> > +		      DISP_REG_OVL_OUTPROC_ROI_SIZE);
> > +	mtk_ddp_write_mask(cmdq_pkt,
> > OVL_OUTPROC_DATAPATH_CON_OUTPUT_CLAMP,
> > +			   &priv->cmdq_reg, priv->regs,
> > +			   DISP_REG_OVL_OUTPROC_DATAPATH_CON,
> > +			   OVL_OUTPROC_DATAPATH_CON_OUTPUT_CLAMP);
> > +}
> > +
> > +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;
> > +	writel(tmp, priv->regs +
> > DISP_REG_OVL_OUTPROC_SHADOW_CTRL);
> > +
> > +	mtk_ddp_write(NULL, 0, &priv->cmdq_reg, priv->regs,
> > +		      DISP_REG_OVL_OUTPROC_INTSTA);
> > +	mtk_ddp_write_mask(NULL, OVL_OUTPROC_OVL_EN, &priv-
> > >cmdq_reg, priv->regs,
> > +			   DISP_REG_OVL_OUTPROC_EN,
> > OVL_OUTPROC_OVL_EN);
> 
> Use readl() and writel().
> 
> Understood, I will use `writel` to replace the 'mtk_ddp_write'.

> > +}
> > +
> > +void mtk_disp_outproc_stop(struct device *dev)
> > +{
> > +	struct mtk_disp_outproc *priv = dev_get_drvdata(dev);
> > +
> > +	mtk_ddp_write_mask(NULL, 0, &priv->cmdq_reg, priv->regs,
> > +			   DISP_REG_OVL_OUTPROC_EN,
> > OVL_OUTPROC_OVL_EN);
> > +	mtk_ddp_write_mask(NULL, OVL_OUTPROC_RST, &priv->cmdq_reg,
> > priv->regs,
> > +			   DISP_REG_OVL_OUTPROC_RST,
> > OVL_OUTPROC_RST);
> > +	mtk_ddp_write_mask(NULL, 0, &priv->cmdq_reg, priv->regs,
> > +			   DISP_REG_OVL_OUTPROC_RST,
> > OVL_OUTPROC_RST);
> 
> Use readl() and writel().
Understood, I will use `writel` to replace the 'mtk_ddp_write'.
> 
> > +}
> > +
> > +
> 
> 
> 
Best regards, 
Paul