Re: [PATCH v3 5/6] drm/mediatek: support CMDQ interface in ddp component

From: CK Hu <ck.hu@mediatek.com>
To: Bibby Hsieh <bibby.hsieh@mediatek.com>
Cc: David Airlie <airlied@linux.ie>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	Daniel Vetter <daniel.vetter@ffwll.ch>,
	<dri-devel@lists.freedesktop.org>,
	<linux-mediatek@lists.infradead.org>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	YT Shen <yt.shen@mediatek.com>,
	Thierry Reding <thierry.reding@gmail.com>,
	<linux-arm-kernel@lists.infradead.org>, <tfiga@chromium.org>,
	<drinkcat@chromium.org>, <linux-kernel@vger.kernel.org>,
	<srv_heupstream@mediatek.com>,
	Yongqiang Niu <yongqiang.niu@mediatek.com>
Subject: Re: [PATCH v3 5/6] drm/mediatek: support CMDQ interface in ddp component
Date: Thu, 5 Dec 2019 12:50:48 +0800	[thread overview]
Message-ID: <1575521448.24783.11.camel@mtksdaap41> (raw)
In-Reply-To: <20191204094441.5116-6-bibby.hsieh@mediatek.com>

Hi, Bibby:

On Wed, 2019-12-04 at 17:44 +0800, Bibby Hsieh wrote:
> The CMDQ (Command Queue) in MT8183 is used to help
> update all relevant display controller registers
> with critical time limation.
> This patch add cmdq interface in ddp_comp interface,
> let all ddp_comp interface can support cpu/cmdq function
> at the same time.
> 
> Signed-off-by: YT Shen <yt.shen@mediatek.com>
> Signed-off-by: CK Hu <ck.hu@mediatek.com>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> Signed-off-by: Bibby Hsieh <bibby.hsieh@mediatek.com>
> Signed-off-by: Yongqiang Niu <yongqiang.niu@mediatek.com>
> ---
>  drivers/gpu/drm/mediatek/mtk_disp_color.c   |   7 +-
>  drivers/gpu/drm/mediatek/mtk_disp_ovl.c     |  65 +++++-----
>  drivers/gpu/drm/mediatek/mtk_disp_rdma.c    |  43 ++++---
>  drivers/gpu/drm/mediatek/mtk_drm_crtc.c     |   8 +-
>  drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 136 +++++++++++++++-----
>  drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h |  31 +++--
>  6 files changed, 197 insertions(+), 93 deletions(-)
> 

[snip]

> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> index 3407d38aff8f..e93e46726de6 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c
> @@ -12,7 +12,8 @@
>  #include <linux/of_irq.h>
>  #include <linux/of_platform.h>
>  #include <linux/platform_device.h>
> -
> +#include <drm/drmP.h>
> +#include <linux/soc/mediatek/mtk-cmdq.h>
>  #include "mtk_drm_drv.h"
>  #include "mtk_drm_plane.h"
>  #include "mtk_drm_ddp_comp.h"
> @@ -76,36 +77,82 @@
>  #define DITHER_ADD_LSHIFT_G(x)			(((x) & 0x7) << 4)
>  #define DITHER_ADD_RSHIFT_G(x)			(((x) & 0x7) << 0)
>  
> +void mtk_ddp_write(struct cmdq_pkt *cmdq_pkt, unsigned int value,
> +		   struct mtk_ddp_comp *comp, unsigned int offset)
> +{
> +	if (cmdq_pkt)
> +#ifdef CONFIG_MTK_CMDQ
> +		cmdq_pkt_write(cmdq_pkt, comp->subsys,
> +			       comp->regs_pa + offset, value);
> +#endif
> +	else
> +		writel(value, comp->regs + offset);

If CONFIG_MTK_CMDQ is not defined, this code would become

	if (cmdq_pkt)

	else
		writel(value, comp->regs + offset);

> +}
> +
> +void mtk_ddp_write_relaxed(struct cmdq_pkt *cmdq_pkt, unsigned int value,
> +			   struct mtk_ddp_comp *comp,
> +			   unsigned int offset)
> +{
> +	if (cmdq_pkt)
> +#ifdef CONFIG_MTK_CMDQ
> +		cmdq_pkt_write(cmdq_pkt, comp->subsys,
> +			       comp->regs_pa + offset, value);
> +#endif
> +	else
> +		writel_relaxed(value, comp->regs + offset);
> +}
> +
> +void mtk_ddp_write_mask(struct cmdq_pkt *cmdq_pkt,
> +			unsigned int value,
> +			struct mtk_ddp_comp *comp,
> +			unsigned int offset,
> +			unsigned int mask)
> +{
> +	if (cmdq_pkt) {
> +#ifdef CONFIG_MTK_CMDQ
> +		cmdq_pkt_write_mask(cmdq_pkt, comp->subsys,
> +				    comp->regs_pa + offset, value, mask);
> +#endif
> +	} else {
> +		u32 tmp = readl(comp->regs + offset);
> +
> +		tmp = (tmp & ~mask) | (value & mask);
> +		writel(tmp, comp->regs + offset);
> +	}
> +}
> +

[snip]

>  
>  static void mtk_gamma_start(struct mtk_ddp_comp *comp)
> @@ -324,6 +371,9 @@ int mtk_ddp_comp_init(struct device *dev, struct device_node *node,
>  		      const struct mtk_ddp_comp_funcs *funcs)
>  {
>  	struct platform_device *comp_pdev;
> +	struct resource res;
> +	struct cmdq_client_reg *cmdq_reg;

I think you could simply defined as below, so you need not to allocate
and free.

struct cmdq_client_reg cmdq_reg;

Regards,
CK

> +	int ret = 0;
>  
>  	if (comp_id < 0 || comp_id >= DDP_COMPONENT_ID_MAX)
>  		return -EINVAL;
> @@ -358,6 +408,34 @@ int mtk_ddp_comp_init(struct device *dev, struct device_node *node,
>  	}
>  	comp->dev = &comp_pdev->dev;
>  
> +#ifdef CONFIG_MTK_CMDQ
> +	if (of_address_to_resource(node, 0, &res) != 0) {
> +		dev_err(dev, "Missing reg in %s node\n",
> +			node->full_name);
> +		return -EINVAL;
> +	}
> +	comp->regs_pa = res.start;
> +
> +	comp_pdev = of_find_device_by_node(node);
> +	if (!comp_pdev) {
> +		dev_warn(dev, "Waiting for component device %s\n",
> +			 node->full_name);
> +		return -EPROBE_DEFER;
> +	}
> +
> +	cmdq_reg = kzalloc(sizeof(*cmdq_reg), GFP_KERNEL);
> +	if (!cmdq_reg)
> +		return -EINVAL;
> +
> +	ret = cmdq_dev_get_client_reg(&comp_pdev->dev, cmdq_reg, 0);
> +	if (ret != 0)
> +		dev_dbg(&comp_pdev->dev,
> +			"get mediatek,gce-client-reg fail!\n");
> +	else
> +		comp->subsys = cmdq_reg->subsys;
> +
> +	kfree(cmdq_reg);
> +#endif
>  	return 0;
>  }
>  