Re: [PATCH v17 07/21] soc: mediatek: mmsys: modify reset controller for MT8195 vdosys1

Re: [PATCH v17 07/21] soc: mediatek: mmsys: modify reset controller for MT8195 vdosys1

[Matthias Brugger]

On 16/04/2022 04:07, Nancy.Lin wrote:
> MT8195 vdosys1 has more than 32 reset bits and a different reset base
> than other chips. Modify mmsys for support 64 bit and different reset
> base.
> 
> Signed-off-by: Nancy.Lin <nancy.lin@mediatek.com>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>   drivers/soc/mediatek/mt8195-mmsys.h |  1 +
>   drivers/soc/mediatek/mtk-mmsys.c    | 39 ++++++++++++++++++-----------
>   drivers/soc/mediatek/mtk-mmsys.h    |  1 +
>   3 files changed, 27 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/soc/mediatek/mt8195-mmsys.h b/drivers/soc/mediatek/mt8195-mmsys.h
> index 5469073e3073..0a286fa5a824 100644
> --- a/drivers/soc/mediatek/mt8195-mmsys.h
> +++ b/drivers/soc/mediatek/mt8195-mmsys.h
> @@ -139,6 +139,7 @@
>   #define MT8195_VDO1_MIXER_SOUT_SEL_IN				0xf68
>   #define MT8195_MIXER_SOUT_SEL_IN_FROM_DISP_MIXER			0
>   
> +#define MT8195_VDO1_SW0_RST_B		0x1d0
>   #define MT8195_VDO1_MERGE0_ASYNC_CFG_WD	0xe30
>   #define MT8195_VDO1_HDRBE_ASYNC_CFG_WD	0xe70
>   #define MT8195_VDO1_HDR_TOP_CFG		0xd00
> diff --git a/drivers/soc/mediatek/mtk-mmsys.c b/drivers/soc/mediatek/mtk-mmsys.c
> index ea04aa2c3840..d7c806f9e494 100644
> --- a/drivers/soc/mediatek/mtk-mmsys.c
> +++ b/drivers/soc/mediatek/mtk-mmsys.c
> @@ -20,6 +20,8 @@
>   #include "mt8195-mmsys.h"
>   #include "mt8365-mmsys.h"
>   
> +#define MMSYS_SW_RESET_PER_REG 32
> +
>   static const struct mtk_mmsys_driver_data mt2701_mmsys_driver_data = {
>   	.clk_driver = "clk-mt2701-mm",
>   	.routes = mmsys_default_routing_table,
> @@ -86,6 +88,7 @@ static const struct mtk_mmsys_driver_data mt8173_mmsys_driver_data = {
>   	.routes = mmsys_default_routing_table,
>   	.num_routes = ARRAY_SIZE(mmsys_default_routing_table),
>   	.sw0_rst_offset = MT8183_MMSYS_SW0_RST_B,
> +	.num_resets = 32,
>   };
>   
>   static const struct mtk_mmsys_match_data mt8173_mmsys_match_data = {
> @@ -100,6 +103,7 @@ static const struct mtk_mmsys_driver_data mt8183_mmsys_driver_data = {
>   	.routes = mmsys_mt8183_routing_table,
>   	.num_routes = ARRAY_SIZE(mmsys_mt8183_routing_table),
>   	.sw0_rst_offset = MT8183_MMSYS_SW0_RST_B,
> +	.num_resets = 32,
>   };
>   
>   static const struct mtk_mmsys_match_data mt8183_mmsys_match_data = {
> @@ -114,6 +118,7 @@ static const struct mtk_mmsys_driver_data mt8186_mmsys_driver_data = {
>   	.routes = mmsys_mt8186_routing_table,
>   	.num_routes = ARRAY_SIZE(mmsys_mt8186_routing_table),
>   	.sw0_rst_offset = MT8186_MMSYS_SW0_RST_B,
> +	.num_resets = 32,
>   };
>   
>   static const struct mtk_mmsys_match_data mt8186_mmsys_match_data = {
> @@ -148,6 +153,8 @@ static const struct mtk_mmsys_driver_data mt8195_vdosys1_driver_data = {
>   	.clk_driver = "clk-mt8195-vdo1",
>   	.routes = mmsys_mt8195_routing_table,
>   	.num_routes = ARRAY_SIZE(mmsys_mt8195_routing_table),
> +	.sw0_rst_offset = MT8195_VDO1_SW0_RST_B,
> +	.num_resets = 64,
>   };
>   
>   static const struct mtk_mmsys_match_data mt8195_mmsys_match_data = {
> @@ -234,18 +241,22 @@ static int mtk_mmsys_reset_update(struct reset_controller_dev *rcdev, unsigned l
>   {
>   	struct mtk_mmsys *mmsys = container_of(rcdev, struct mtk_mmsys, rcdev);
>   	unsigned long flags;
> +	u32 offset;
>   	u32 reg;
>   
> +	offset = (id / MMSYS_SW_RESET_PER_REG) * sizeof(u32);
> +	id = id % MMSYS_SW_RESET_PER_REG;
> +
>   	spin_lock_irqsave(&mmsys->lock, flags);
>   
> -	reg = readl_relaxed(mmsys->regs + mmsys->data->sw0_rst_offset);
> +	reg = readl_relaxed(mmsys->regs + mmsys->data->sw0_rst_offset + offset);
>   
>   	if (assert)
>   		reg &= ~BIT(id);
>   	else
>   		reg |= BIT(id);
>   
> -	writel_relaxed(reg, mmsys->regs + mmsys->data->sw0_rst_offset);
> +	writel_relaxed(reg, mmsys->regs + mmsys->data->sw0_rst_offset + offset);
>   
>   	spin_unlock_irqrestore(&mmsys->lock, flags);
>   
> @@ -360,18 +371,6 @@ static int mtk_mmsys_probe(struct platform_device *pdev)
>   		return ret;
>   	}
>   
> -	spin_lock_init(&mmsys->lock);
> -
> -	mmsys->rcdev.owner = THIS_MODULE;
> -	mmsys->rcdev.nr_resets = 32;
> -	mmsys->rcdev.ops = &mtk_mmsys_reset_ops;
> -	mmsys->rcdev.of_node = pdev->dev.of_node;
> -	ret = devm_reset_controller_register(&pdev->dev, &mmsys->rcdev);
> -	if (ret) {
> -		dev_err(&pdev->dev, "Couldn't register mmsys reset controller: %d\n", ret);
> -		return ret;
> -	}
> -

I'm not sure why you move that code block. It's not explained in the commit message.

Regards,
Matthias

Re: [PATCH v17 06/21] soc: mediatek: add cmdq support of mtk-mmsys config API for mt8195 vdosys1

[Matthias Brugger]

On 16/04/2022 04:07, Nancy.Lin wrote:
> Add cmdq support for mtk-mmsys config API.
> The mmsys config register settings need to take effect with the other
> HW settings(like OVL_ADAPTOR...) at the same vblanking time.
> 
> If we use CPU to write the mmsys reg, we can't guarantee all the
> settings can be written in the same vblanking time.
> Cmdq is used for this purpose. We prepare all the related HW settings
> in one cmdq packet. The first command in the packet is "wait stream done",
> and then following with all the HW settings. After the cmdq packet is
> flush to GCE HW. The GCE waits for the "stream done event" to coming
> and then starts flushing all the HW settings. This can guarantee all
> the settings flush in the same vblanking.
> 
> Signed-off-by: Nancy.Lin <nancy.lin@mediatek.com>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>   drivers/soc/mediatek/mtk-mmsys.c       | 50 ++++++++++++++++++--------
>   include/linux/soc/mediatek/mtk-mmsys.h | 15 +++++---
>   2 files changed, 47 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/soc/mediatek/mtk-mmsys.c b/drivers/soc/mediatek/mtk-mmsys.c
> index 7b262cefef85..ea04aa2c3840 100644
> --- a/drivers/soc/mediatek/mtk-mmsys.c
> +++ b/drivers/soc/mediatek/mtk-mmsys.c
> @@ -177,6 +177,7 @@ struct mtk_mmsys {
>   	spinlock_t lock; /* protects mmsys_sw_rst_b reg */
>   	struct reset_controller_dev rcdev;
>   	phys_addr_t io_start;
> +	struct cmdq_client_reg cmdq_base;
>   };
>   
>   static int mtk_mmsys_find_match_drvdata(struct mtk_mmsys *mmsys,
> @@ -280,46 +281,61 @@ static const struct reset_control_ops mtk_mmsys_reset_ops = {
>   	.reset = mtk_mmsys_reset,
>   };
>   
> -static void mtk_mmsys_write_reg(struct device *dev, u32 offset, u32 mask, u32 val)
> +static void mtk_mmsys_write_reg(struct device *dev, u32 offset, u32 mask, u32 val,
> +				struct cmdq_pkt *cmdq_pkt)
>   {
>   	struct mtk_mmsys *mmsys = dev_get_drvdata(dev);
>   	u32 tmp;
>   
> -	tmp = readl(mmsys->regs + offset);
> -	tmp = (tmp & ~mask) | val;
> -	writel(tmp, mmsys->regs + offset);
> +#if IS_REACHABLE(CONFIG_MTK_CMDQ)
> +	if (cmdq_pkt && mmsys->cmdq_base.size) {
> +		cmdq_pkt_write_mask(cmdq_pkt, mmsys->cmdq_base.subsys,
> +				    mmsys->cmdq_base.offset + offset, val,
> +				    mask);
If we put here:

    return;
}
#endif

> +	} else {
> +#endif
> +		tmp = readl(mmsys->regs + offset);
> +		tmp = (tmp & ~mask) | val;
> +		writel(tmp, mmsys->regs + offset);
> +#if IS_REACHABLE(CONFIG_MTK_CMDQ)
> +	}
> +#endif

Then we can get rid of this IS_REACHABLE. Other then that patch looks good.

Matthias

>   }
>   
> -void mtk_mmsys_merge_async_config(struct device *dev, int idx, int width, int height)
> +void mtk_mmsys_merge_async_config(struct device *dev, int idx, int width,
> +				  int height, struct cmdq_pkt *cmdq_pkt)
>   {
>   	mtk_mmsys_write_reg(dev, MT8195_VDO1_MERGE0_ASYNC_CFG_WD + 0x10 * idx,
> -			    ~0, height << 16 | width);
> +			    ~0, height << 16 | width, cmdq_pkt);
>   }
>   EXPORT_SYMBOL_GPL(mtk_mmsys_merge_async_config);
>   
> -void mtk_mmsys_hdr_confing(struct device *dev, int be_width, int be_height)
> +void mtk_mmsys_hdr_confing(struct device *dev, int be_width, int be_height,
> +			   struct cmdq_pkt *cmdq_pkt)
>   {
>   	mtk_mmsys_write_reg(dev, MT8195_VDO1_HDRBE_ASYNC_CFG_WD, ~0,
> -			    be_height << 16 | be_width);
> +			    be_height << 16 | be_width, cmdq_pkt);
>   }
>   EXPORT_SYMBOL_GPL(mtk_mmsys_hdr_confing);
>   
>   void mtk_mmsys_mixer_in_config(struct device *dev, int idx, bool alpha_sel, u16 alpha,
> -			       u8 mode, u32 biwidth)
> +			       u8 mode, u32 biwidth, struct cmdq_pkt *cmdq_pkt)
>   {
>   	mtk_mmsys_write_reg(dev, MT8195_VDO1_MIXER_IN1_ALPHA + (idx - 1) * 4, ~0,
> -			    alpha << 16 | alpha);
> +			    alpha << 16 | alpha, cmdq_pkt);
>   	mtk_mmsys_write_reg(dev, MT8195_VDO1_HDR_TOP_CFG, BIT(19 + idx),
> -			    alpha_sel << (19 + idx));
> +			    alpha_sel << (19 + idx), cmdq_pkt);
>   	mtk_mmsys_write_reg(dev, MT8195_VDO1_MIXER_IN1_PAD + (idx - 1) * 4,
> -			    GENMASK(31, 16) | GENMASK(1, 0), biwidth << 16 | mode);
> +			    GENMASK(31, 16) | GENMASK(1, 0),
> +			    biwidth << 16 | mode, cmdq_pkt);
>   }
>   EXPORT_SYMBOL_GPL(mtk_mmsys_mixer_in_config);
>   
> -void mtk_mmsys_mixer_in_channel_swap(struct device *dev, int idx, bool channel_swap)
> +void mtk_mmsys_mixer_in_channel_swap(struct device *dev, int idx, bool channel_swap,
> +				     struct cmdq_pkt *cmdq_pkt)
>   {
>   	mtk_mmsys_write_reg(dev, MT8195_VDO1_MIXER_IN1_PAD + (idx - 1) * 4, BIT(4),
> -			    channel_swap << 4);
> +			    channel_swap << 4, cmdq_pkt);
>   }
>   EXPORT_SYMBOL_GPL(mtk_mmsys_mixer_in_channel_swap);
>   
> @@ -377,6 +393,12 @@ static int mtk_mmsys_probe(struct platform_device *pdev)
>   		mmsys->data = match_data->drv_data[0];
>   	}
>   
> +#if IS_REACHABLE(CONFIG_MTK_CMDQ)
> +	ret = cmdq_dev_get_client_reg(dev, &mmsys->cmdq_base, 0);
> +	if (ret)
> +		dev_dbg(dev, "No mediatek,gce-client-reg!\n");
> +#endif
> +
>   	platform_set_drvdata(pdev, mmsys);
>   
>   	clks = platform_device_register_data(&pdev->dev, mmsys->data->clk_driver,
> diff --git a/include/linux/soc/mediatek/mtk-mmsys.h b/include/linux/soc/mediatek/mtk-mmsys.h
> index fe620929b0f9..7a73305390ba 100644
> --- a/include/linux/soc/mediatek/mtk-mmsys.h
> +++ b/include/linux/soc/mediatek/mtk-mmsys.h
> @@ -6,6 +6,10 @@
>   #ifndef __MTK_MMSYS_H
>   #define __MTK_MMSYS_H
>   
> +#include <linux/mailbox_controller.h>
> +#include <linux/mailbox/mtk-cmdq-mailbox.h>
> +#include <linux/soc/mediatek/mtk-cmdq.h>
> +
>   enum mtk_ddp_comp_id;
>   struct device;
>   
> @@ -73,13 +77,16 @@ void mtk_mmsys_ddp_disconnect(struct device *dev,
>   			      enum mtk_ddp_comp_id cur,
>   			      enum mtk_ddp_comp_id next);
>   
> -void mtk_mmsys_merge_async_config(struct device *dev, int idx, int width, int height);
> +void mtk_mmsys_merge_async_config(struct device *dev, int idx, int width,
> +				  int height, struct cmdq_pkt *cmdq_pkt);
>   
> -void mtk_mmsys_hdr_confing(struct device *dev, int be_width, int be_height);
> +void mtk_mmsys_hdr_confing(struct device *dev, int be_width, int be_height,
> +			   struct cmdq_pkt *cmdq_pkt);
>   
>   void mtk_mmsys_mixer_in_config(struct device *dev, int idx, bool alpha_sel, u16 alpha,
> -			       u8 mode, u32 biwidth);
> +			       u8 mode, u32 biwidth, struct cmdq_pkt *cmdq_pkt);
>   
> -void mtk_mmsys_mixer_in_channel_swap(struct device *dev, int idx, bool channel_swap);
> +void mtk_mmsys_mixer_in_channel_swap(struct device *dev, int idx, bool channel_swap,
> +				     struct cmdq_pkt *cmdq_pkt);
>   
>   #endif /* __MTK_MMSYS_H */

Re: [PATCH v17 11/21] drm/mediatek: add display merge start/stop API for cmdq support

[Matthias Brugger]

On 16/04/2022 04:07, Nancy.Lin wrote:
> Add merge start/stop API for cmdq support. The ovl_adaptor merges
> are configured with each drm plane update. Need to enable/disable
> merge with cmdq making sure all the settings taken effect in the
> same vblank.
> 
> Signed-off-by: Nancy.Lin <nancy.lin@mediatek.com>
> Reviewed-by: Chun-Kuang Hu <chunkuang.hu@kernel.org>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>   drivers/gpu/drm/mediatek/mtk_disp_drv.h   |  2 ++
>   drivers/gpu/drm/mediatek/mtk_disp_merge.c | 20 +++++++++++++++++---
>   2 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_drv.h b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> index 53aa988dde3b..43a412525b75 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> @@ -66,6 +66,8 @@ void mtk_merge_stop(struct device *dev);
>   void mtk_merge_advance_config(struct device *dev, unsigned int l_w, unsigned int r_w,
>   			      unsigned int h, unsigned int vrefresh, unsigned int bpc,
>   			      struct cmdq_pkt *cmdq_pkt);
> +void mtk_merge_start_cmdq(struct device *dev, struct cmdq_pkt *cmdq_pkt);
> +void mtk_merge_stop_cmdq(struct device *dev, struct cmdq_pkt *cmdq_pkt);
>   
>   void mtk_ovl_bgclr_in_on(struct device *dev);
>   void mtk_ovl_bgclr_in_off(struct device *dev);
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_merge.c b/drivers/gpu/drm/mediatek/mtk_disp_merge.c
> index 40da0555416d..c0d9b43b2a66 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_merge.c
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_merge.c
> @@ -67,17 +67,31 @@ struct mtk_disp_merge {
>   };
>   
>   void mtk_merge_start(struct device *dev)

Probably not my call, but wouldn't it make sense to enhance mtk_merge_start to 
pass 'struct cmdq_pkt *cmdq_pkt' directly instead of adding this 'adapter'?

In the end this is up to Chun-Kuang.

Regards,
Matthias

> +{
> +	mtk_merge_start_cmdq(dev, NULL);
> +}
> +
> +void mtk_merge_stop(struct device *dev)
>   {
>   	struct mtk_disp_merge *priv = dev_get_drvdata(dev);
>   
> -	writel(MERGE_EN, priv->regs + DISP_REG_MERGE_CTRL);
> +	mtk_merge_stop_cmdq(dev, NULL);
>   }
>   
> -void mtk_merge_stop(struct device *dev)
> +void mtk_merge_start_cmdq(struct device *dev, struct cmdq_pkt *cmdq_pkt)
> +{
> +	struct mtk_disp_merge *priv = dev_get_drvdata(dev);
> +
> +	mtk_ddp_write(cmdq_pkt, 1, &priv->cmdq_reg, priv->regs,
> +		      DISP_REG_MERGE_CTRL);
> +}
> +
> +void mtk_merge_stop_cmdq(struct device *dev, struct cmdq_pkt *cmdq_pkt)
>   {
>   	struct mtk_disp_merge *priv = dev_get_drvdata(dev);
>   
> -	writel(0x0, priv->regs + DISP_REG_MERGE_CTRL);
> +	mtk_ddp_write(cmdq_pkt, 0, &priv->cmdq_reg, priv->regs,
> +		      DISP_REG_MERGE_CTRL);
>   }
>   
>   static void mtk_merge_fifo_setting(struct mtk_disp_merge *priv,

Re: [PATCH v17 11/21] drm/mediatek: add display merge start/stop API for cmdq support

[Chun-Kuang Hu]

Hi, Matthias:

Matthias Brugger <matthias.bgg@gmail.com> 於 2022年4月22日 週五 下午7:48寫道：
>
>
>
> On 16/04/2022 04:07, Nancy.Lin wrote:
> > Add merge start/stop API for cmdq support. The ovl_adaptor merges
> > are configured with each drm plane update. Need to enable/disable
> > merge with cmdq making sure all the settings taken effect in the
> > same vblank.
> >
> > Signed-off-by: Nancy.Lin <nancy.lin@mediatek.com>
> > Reviewed-by: Chun-Kuang Hu <chunkuang.hu@kernel.org>
> > Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> > ---
> >   drivers/gpu/drm/mediatek/mtk_disp_drv.h   |  2 ++
> >   drivers/gpu/drm/mediatek/mtk_disp_merge.c | 20 +++++++++++++++++---
> >   2 files changed, 19 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/mediatek/mtk_disp_drv.h b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> > index 53aa988dde3b..43a412525b75 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> > +++ b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> > @@ -66,6 +66,8 @@ void mtk_merge_stop(struct device *dev);
> >   void mtk_merge_advance_config(struct device *dev, unsigned int l_w, unsigned int r_w,
> >                             unsigned int h, unsigned int vrefresh, unsigned int bpc,
> >                             struct cmdq_pkt *cmdq_pkt);
> > +void mtk_merge_start_cmdq(struct device *dev, struct cmdq_pkt *cmdq_pkt);
> > +void mtk_merge_stop_cmdq(struct device *dev, struct cmdq_pkt *cmdq_pkt);
> >
> >   void mtk_ovl_bgclr_in_on(struct device *dev);
> >   void mtk_ovl_bgclr_in_off(struct device *dev);
> > diff --git a/drivers/gpu/drm/mediatek/mtk_disp_merge.c b/drivers/gpu/drm/mediatek/mtk_disp_merge.c
> > index 40da0555416d..c0d9b43b2a66 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_disp_merge.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_disp_merge.c
> > @@ -67,17 +67,31 @@ struct mtk_disp_merge {
> >   };
> >
> >   void mtk_merge_start(struct device *dev)
>
> Probably not my call, but wouldn't it make sense to enhance mtk_merge_start to
> pass 'struct cmdq_pkt *cmdq_pkt' directly instead of adding this 'adapter'?

In [1], mtk_merge_start() should match the function prototype of
mtk_ddp_comp_funcs.start, so keep the non-cmdq interface.

static const struct mtk_ddp_comp_funcs ddp_merge = {
 .clk_enable = mtk_merge_clk_enable,
 .clk_disable = mtk_merge_clk_disable,
 .start = mtk_merge_start,
 .stop = mtk_merge_stop,
 .config = mtk_merge_config,
};

[1] https://patchwork.kernel.org/project/linux-mediatek/patch/20220419094143.9561-5-jason-jh.lin@mediatek.com/

Regards,
Chun-Kuang.

>
> In the end this is up to Chun-Kuang.
>
> Regards,
> Matthias
>
> > +{
> > +     mtk_merge_start_cmdq(dev, NULL);
> > +}
> > +
> > +void mtk_merge_stop(struct device *dev)
> >   {
> >       struct mtk_disp_merge *priv = dev_get_drvdata(dev);
> >
> > -     writel(MERGE_EN, priv->regs + DISP_REG_MERGE_CTRL);
> > +     mtk_merge_stop_cmdq(dev, NULL);
> >   }
> >
> > -void mtk_merge_stop(struct device *dev)
> > +void mtk_merge_start_cmdq(struct device *dev, struct cmdq_pkt *cmdq_pkt)
> > +{
> > +     struct mtk_disp_merge *priv = dev_get_drvdata(dev);
> > +
> > +     mtk_ddp_write(cmdq_pkt, 1, &priv->cmdq_reg, priv->regs,
> > +                   DISP_REG_MERGE_CTRL);
> > +}
> > +
> > +void mtk_merge_stop_cmdq(struct device *dev, struct cmdq_pkt *cmdq_pkt)
> >   {
> >       struct mtk_disp_merge *priv = dev_get_drvdata(dev);
> >
> > -     writel(0x0, priv->regs + DISP_REG_MERGE_CTRL);
> > +     mtk_ddp_write(cmdq_pkt, 0, &priv->cmdq_reg, priv->regs,
> > +                   DISP_REG_MERGE_CTRL);
> >   }
> >
> >   static void mtk_merge_fifo_setting(struct mtk_disp_merge *priv,

Re: [PATCH v17 11/21] drm/mediatek: add display merge start/stop API for cmdq support

[Matthias Brugger]

On 25/04/2022 15:32, Chun-Kuang Hu wrote:
> Hi, Matthias:
> 
> Matthias Brugger <matthias.bgg@gmail.com> 於 2022年4月22日 週五 下午7:48寫道：
>>
>>
>>
>> On 16/04/2022 04:07, Nancy.Lin wrote:
>>> Add merge start/stop API for cmdq support. The ovl_adaptor merges
>>> are configured with each drm plane update. Need to enable/disable
>>> merge with cmdq making sure all the settings taken effect in the
>>> same vblank.
>>>
>>> Signed-off-by: Nancy.Lin <nancy.lin@mediatek.com>
>>> Reviewed-by: Chun-Kuang Hu <chunkuang.hu@kernel.org>
>>> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>>> ---
>>>    drivers/gpu/drm/mediatek/mtk_disp_drv.h   |  2 ++
>>>    drivers/gpu/drm/mediatek/mtk_disp_merge.c | 20 +++++++++++++++++---
>>>    2 files changed, 19 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_drv.h b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
>>> index 53aa988dde3b..43a412525b75 100644
>>> --- a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
>>> +++ b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
>>> @@ -66,6 +66,8 @@ void mtk_merge_stop(struct device *dev);
>>>    void mtk_merge_advance_config(struct device *dev, unsigned int l_w, unsigned int r_w,
>>>                              unsigned int h, unsigned int vrefresh, unsigned int bpc,
>>>                              struct cmdq_pkt *cmdq_pkt);
>>> +void mtk_merge_start_cmdq(struct device *dev, struct cmdq_pkt *cmdq_pkt);
>>> +void mtk_merge_stop_cmdq(struct device *dev, struct cmdq_pkt *cmdq_pkt);
>>>
>>>    void mtk_ovl_bgclr_in_on(struct device *dev);
>>>    void mtk_ovl_bgclr_in_off(struct device *dev);
>>> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_merge.c b/drivers/gpu/drm/mediatek/mtk_disp_merge.c
>>> index 40da0555416d..c0d9b43b2a66 100644
>>> --- a/drivers/gpu/drm/mediatek/mtk_disp_merge.c
>>> +++ b/drivers/gpu/drm/mediatek/mtk_disp_merge.c
>>> @@ -67,17 +67,31 @@ struct mtk_disp_merge {
>>>    };
>>>
>>>    void mtk_merge_start(struct device *dev)
>>
>> Probably not my call, but wouldn't it make sense to enhance mtk_merge_start to
>> pass 'struct cmdq_pkt *cmdq_pkt' directly instead of adding this 'adapter'?
> 
> In [1], mtk_merge_start() should match the function prototype of
> mtk_ddp_comp_funcs.start, so keep the non-cmdq interface.

Well other callbacks, namely config hass cmdq_pkt. So we could change the 
callback in a similar way is was done in
d0afe37f5209 ("drm/mediatek: support CMDQ interface in ddp component")


But as i said it's up to you. You are the maintainer.

Regards,
Matthias

> 
> static const struct mtk_ddp_comp_funcs ddp_merge = {
>   .clk_enable = mtk_merge_clk_enable,
>   .clk_disable = mtk_merge_clk_disable,
>   .start = mtk_merge_start,
>   .stop = mtk_merge_stop,
>   .config = mtk_merge_config,
> };
> 
> [1] https://patchwork.kernel.org/project/linux-mediatek/patch/20220419094143.9561-5-jason-jh.lin@mediatek.com/
> 
> Regards,
> Chun-Kuang.
> 
>>
>> In the end this is up to Chun-Kuang.
>>
>> Regards,
>> Matthias
>>
>>> +{
>>> +     mtk_merge_start_cmdq(dev, NULL);
>>> +}
>>> +
>>> +void mtk_merge_stop(struct device *dev)
>>>    {
>>>        struct mtk_disp_merge *priv = dev_get_drvdata(dev);
>>>
>>> -     writel(MERGE_EN, priv->regs + DISP_REG_MERGE_CTRL);
>>> +     mtk_merge_stop_cmdq(dev, NULL);
>>>    }
>>>
>>> -void mtk_merge_stop(struct device *dev)
>>> +void mtk_merge_start_cmdq(struct device *dev, struct cmdq_pkt *cmdq_pkt)
>>> +{
>>> +     struct mtk_disp_merge *priv = dev_get_drvdata(dev);
>>> +
>>> +     mtk_ddp_write(cmdq_pkt, 1, &priv->cmdq_reg, priv->regs,
>>> +                   DISP_REG_MERGE_CTRL);
>>> +}
>>> +
>>> +void mtk_merge_stop_cmdq(struct device *dev, struct cmdq_pkt *cmdq_pkt)
>>>    {
>>>        struct mtk_disp_merge *priv = dev_get_drvdata(dev);
>>>
>>> -     writel(0x0, priv->regs + DISP_REG_MERGE_CTRL);
>>> +     mtk_ddp_write(cmdq_pkt, 0, &priv->cmdq_reg, priv->regs,
>>> +                   DISP_REG_MERGE_CTRL);
>>>    }
>>>
>>>    static void mtk_merge_fifo_setting(struct mtk_disp_merge *priv,

Re: [PATCH v17 03/21] dt-bindings: mediatek: add ethdr definition for mt8195

[Philipp Zabel]

Hi Nancy,

On Sa, 2022-04-16 at 10:07 +0800, Nancy.Lin wrote:
> Add vdosys1 ETHDR definition.
> 
> Signed-off-by: Nancy.Lin <nancy.lin@mediatek.com>
> Reviewed-by: Chun-Kuang Hu <chunkuang.hu@kernel.org>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  .../display/mediatek/mediatek,ethdr.yaml      | 158 ++++++++++++++++++
>  1 file changed, 158 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/mediatek/mediatek,ethdr.yaml
> 
> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,ethdr.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,ethdr.yaml
> new file mode 100644
> index 000000000000..e8303c28a361
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,ethdr.yaml
> @@ -0,0 +1,158 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/mediatek/mediatek,ethdr.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Mediatek Ethdr Device Tree Bindings
> +
> +maintainers:
> +  - Chun-Kuang Hu <chunkuang.hu@kernel.org>
> +  - Philipp Zabel <p.zabel@pengutronix.de>
> +
> +description: |
> +  ETHDR is designed for HDR video and graphics conversion in the external display path.
> +  It handles multiple HDR input types and performs tone mapping, color space/color
> +  format conversion, and then combine different layers, output the required HDR or
> +  SDR signal to the subsequent display path. This engine is composed of two video
> +  frontends, two graphic frontends, one video backend and a mixer. ETHDR has two
> +  DMA function blocks, DS and ADL. These two function blocks read the pre-programmed
> +  registers from DRAM and set them to HW in the v-blanking period.
> +
> +properties:
> +  compatible:
> +    items:
> +      - const: mediatek,mt8195-disp-ethdr
> +  reg:
> +    maxItems: 7
> +  reg-names:
> +    items:
> +      - const: mixer
> +      - const: vdo_fe0
> +      - const: vdo_fe1
> +      - const: gfx_fe0
> +      - const: gfx_fe1
> +      - const: vdo_be
> +      - const: adl_ds
> +  interrupts:
> +    minItems: 1
> +  iommus:
> +    description: The compatible property is DMA function blocks.
> +      Should point to the respective IOMMU block with master port as argument,
> +      see Documentation/devicetree/bindings/iommu/mediatek,iommu.yaml for
> +      details.
> +    minItems: 1
> +    maxItems: 2
> +  clocks:
> +    items:
> +      - description: mixer clock
> +      - description: video frontend 0 clock
> +      - description: video frontend 1 clock
> +      - description: graphic frontend 0 clock
> +      - description: graphic frontend 1 clock
> +      - description: video backend clock
> +      - description: autodownload and menuload clock
> +      - description: video frontend 0 async clock
> +      - description: video frontend 1 async clock
> +      - description: graphic frontend 0 async clock
> +      - description: graphic frontend 1 async clock
> +      - description: video backend async clock
> +      - description: ethdr top clock
> +  clock-names:
> +    items:
> +      - const: mixer
> +      - const: vdo_fe0
> +      - const: vdo_fe1
> +      - const: gfx_fe0
> +      - const: gfx_fe1
> +      - const: vdo_be
> +      - const: adl_ds
> +      - const: vdo_fe0_async
> +      - const: vdo_fe1_async
> +      - const: gfx_fe0_async
> +      - const: gfx_fe1_async
> +      - const: vdo_be_async
> +      - const: ethdr_top
> +  power-domains:
> +    maxItems: 1
> +  resets:
> +    maxItems: 5

Please add a reset-names property and name these resets.

regards
Philipp

Re: [PATCH v17 13/21] drm/mediatek: add display merge async reset control

[Philipp Zabel]

On Sa, 2022-04-16 at 10:07 +0800, Nancy.Lin wrote:
> Add merge async reset control in mtk_merge_stop. Async hw doesn't do self
> reset on each sof signal(start of frame), so need to reset the async to
> clear the hw status for the next merge start.
> 
> Signed-off-by: Nancy.Lin <nancy.lin@mediatek.com>
> Reviewed-by: CK Hu <ck.hu@mediatek.com>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_disp_merge.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_merge.c b/drivers/gpu/drm/mediatek/mtk_disp_merge.c
> index 9dca145cfb71..177473fa8160 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_merge.c
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_merge.c
> @@ -8,6 +8,7 @@
>  #include <linux/of_device.h>
>  #include <linux/of_irq.h>
>  #include <linux/platform_device.h>
> +#include <linux/reset.h>
>  #include <linux/soc/mediatek/mtk-cmdq.h>
>  
> 
> 
> 
>  #include "mtk_drm_ddp_comp.h"
> @@ -79,6 +80,9 @@ void mtk_merge_stop(struct device *dev)
>  	struct mtk_disp_merge *priv = dev_get_drvdata(dev);
>  
> 
> 
> 
>  	mtk_merge_stop_cmdq(dev, NULL);
> +
> +	if (priv->async_clk)
> +		device_reset_optional(dev);

To avoid the overhead of looking up the reset control in the device
tree every time, it would be better to request a reset control during
probe using devm_reset_control_get_optional_exclusive(). Here you'd
just call reset_control_reset().

regards
Philipp

Re: [PATCH v17 14/21] drm/mediatek: add ETHDR support for MT8195

[Philipp Zabel]

On Sa, 2022-04-16 at 10:07 +0800, Nancy.Lin wrote:
> ETHDR is a part of ovl_adaptor.
> ETHDR is designed for HDR video and graphics conversion in the external
> display path. It handles multiple HDR input types and performs tone
> mapping, color space/color format conversion, and then combine
> different layers, output the required HDR or SDR signal to the
> subsequent display path.
> 
> Signed-off-by: Nancy.Lin <nancy.lin@mediatek.com>
> Reviewed-by: Chun-Kuang Hu <chunkuang.hu@kernel.org>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
[...]
> +static int mtk_ethdr_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct mtk_ethdr *priv;
> +	int ret;
> +	int i;
> +
> +	dev_info(dev, "%s+\n", __func__);

Left-over debug statements?

> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < ETHDR_ID_MAX; i++) {
> +		priv->ethdr_comp[i].dev = dev;
> +		priv->ethdr_comp[i].regs = of_iomap(dev->of_node, i);
> +#if IS_REACHABLE(CONFIG_MTK_CMDQ)
> +		ret = cmdq_dev_get_client_reg(dev,
> +					      &priv->ethdr_comp[i].cmdq_base, i);
> +		if (ret)
> +			dev_dbg(dev, "get mediatek,gce-client-reg fail!\n");
> +#endif
> +		dev_dbg(dev, "[DRM]regs:0x%p, node:%d\n", priv->ethdr_comp[i].regs, i);
> +	}
> +
> +	for (i = 0; i < ETHDR_CLK_NUM; i++)
> +		priv->ethdr_clk[i].id = ethdr_clk_str[i];
> +	ret = devm_clk_bulk_get_optional(dev, ETHDR_CLK_NUM, priv->ethdr_clk);
> +	if (ret)
> +		return ret;
> +
> +	priv->irq = platform_get_irq(pdev, 0);
> +	if (priv->irq < 0)
> +		priv->irq = 0;
> +
> +	if (priv->irq) {
> +		ret = devm_request_irq(dev, priv->irq, mtk_ethdr_irq_handler,
> +				       IRQF_TRIGGER_NONE, dev_name(dev), priv);
> +		if (ret < 0) {
> +			dev_err(dev, "Failed to request irq %d: %d\n", priv->irq, ret);
> +			return ret;
> +		}
> +	}
> +
> +	priv->reset_ctl = devm_reset_control_array_get_optional_exclusive(dev);

This is missing error handling. You could use dev_err_probe() here.

regards
Philipp