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

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

On Wed, 2025-02-12 at 04:08 +0000, CK Hu (胡俊光) wrote:
> Hi, Paul:
> 
> On Fri, 2025-01-10 at 20: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>
> > ---
> >  drivers/gpu/drm/mediatek/Makefile           |   1 +
> >  drivers/gpu/drm/mediatek/mtk_ddp_comp.c     |   1 +
> >  drivers/gpu/drm/mediatek/mtk_ddp_comp.h     |   1 +
> >  drivers/gpu/drm/mediatek/mtk_disp_drv.h     |  14 ++
> >  drivers/gpu/drm/mediatek/mtk_disp_outproc.c | 244
> > ++++++++++++++++++++
> >  drivers/gpu/drm/mediatek/mtk_disp_outproc.h |  22 ++
> >  drivers/gpu/drm/mediatek/mtk_drm_drv.c      |   1 +
> >  drivers/gpu/drm/mediatek/mtk_drm_drv.h      |   1 +
> >  8 files changed, 285 insertions(+)
> >  create mode 100644 drivers/gpu/drm/mediatek/mtk_disp_outproc.c
> >  create mode 100644 drivers/gpu/drm/mediatek/mtk_disp_outproc.h
> > 
> > 
//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..af473eb2fb77
> > --- /dev/null
> > +++ b/drivers/gpu/drm/mediatek/mtk_disp_outproc.c
> > @@ -0,0 +1,244 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) 2021 MediaTek Inc.
> > + */
> > +
> > +#include <drm/drm_fourcc.h>
> > +#include <drm/drm_framebuffer.h>
> > +#include <linux/clk.h>
> > +#include <linux/component.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 <linux/of.h>
> > +#include "mtk_drm_drv.h"
> > +#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
> > 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;
> > +
> > +	writel(0x0, priv->regs + DISP_REG_OVL_OUTPROC_INTSTA);
> > +
> > +	if (!priv->vblank_cb)
> > +		return IRQ_NONE;
> > +
> > +	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);
> > +	unsigned int tmp;
> > +
> > +	dev_dbg(dev, "%s-w:%d, h:%d\n", __func__, w, h);
> > +
> > +	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);
> 
> Move bypass shadow to mtk_disp_outproc_start().
> 

Sure, we will move the bypass shadow to mtk_disp_outproc_start().
> > +
> > +	mtk_ddp_write_mask(cmdq_pkt, h << 16 | w, &priv->cmdq_reg,
> > priv->regs,
> > +			   DISP_REG_OVL_OUTPROC_ROI_SIZE, ~0);
> 
> mtk_ddp_write();
> 
> > +	mtk_ddp_write_mask(cmdq_pkt, DATAPATH_CON_OUTPUT_CLAMP,
> > &priv->cmdq_reg, priv->regs,
> > +			   DISP_REG_OVL_OUTPROC_DATAPATH_CON,
> > DATAPATH_CON_OUTPUT_CLAMP);
> > +}
> > +
> > +void mtk_disp_outproc_start(struct device *dev)
> > +{
> > +	struct mtk_disp_outproc *priv = dev_get_drvdata(dev);
> > +
> > +	mtk_ddp_write_mask(NULL, OVL_OUTPROC_RST, &priv->cmdq_reg,
> > priv->regs,
> > +			   DISP_REG_OVL_OUTPROC_RST,
> > OVL_OUTPROC_RST);
> 
> writel();
> 

Sure, I will use the writel() to replace the mtk_ddp_write_mask().
> > +	mtk_ddp_write_mask(NULL, 0, &priv->cmdq_reg, priv->regs,
> > +			   DISP_REG_OVL_OUTPROC_RST,
> > OVL_OUTPROC_RST);
> 
> Some hardware reset before start.
> Some hardware reset after stop.
> 
> Could align this?
> 
Sure, we will align this part.
> > +	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);
> > +}
> > +
> > +void mtk_disp_outproc_stop(struct device *dev)
> > +{
> > +	struct mtk_disp_outproc *priv = dev_get_drvdata(dev);
> > +
> > +	mtk_ddp_write(NULL, 0, &priv->cmdq_reg, priv->regs,
> > +		      DISP_REG_OVL_OUTPROC_INTEN);
> 
> OVL driver does not disable interrupt when stop.
> Align this with OVL driver.
> 
Sure I will check the driver and align this part.

> 
//snip
> > +#if IS_REACHABLE(CONFIG_MTK_CMDQ)
> > +	ret = cmdq_dev_get_client_reg(dev, &priv->cmdq_reg, 0);
> > +	if (ret)
> > +		dev_dbg(dev, "No mediatek,gce-client-reg\n");
> > +#endif
> > +
> > +	if (of_property_read_u32_index(dev->of_node, "interrupts",
> > 0, &ret)) {
> > +		dev_dbg(dev, "interrupts not defined\n");
> 
> In binding document, interrupt is required.
> So return with error code.
> 
The interrupt is not required, I have rewrite the binding document. 
> > +	} else {
> > +		priv->irq = platform_get_irq(pdev, 0);
> > +		if (priv->irq < 0)
> > +			priv->irq = 0;
> 
> In binding document, interrupt is required.
> So return with error code.
> 
> Regards,
> CK
> 
The interrupt is not required, I have rewrite the binding document. 
The next upstream version of patch would fix this problem


Best, 

Paul
>