| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1420fdda-07cf-734a-c29a-ff09a45e58d3@collabora.com>
Date: Wed, 7 Jun 2023 09:36:53 +0200
From: AngeloGioacchino Del Regno
<angelogioacchino.delregno@...labora.com>
To: Hsiao Chien Sung <shawn.sung@...iatek.com>,
Matthias Brugger <matthias.bgg@...il.com>,
Philipp Zabel <p.zabel@...gutronix.de>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>
Cc: linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-mediatek@...ts.infradead.org, devicetree@...r.kernel.org,
Project_Global_Chrome_Upstream_Group@...iatek.com,
Chun-Kuang Hu <chunkuang.hu@...nel.org>,
Singo Chang <singo.chang@...iatek.com>,
Nancy Lin <nancy.lin@...iatek.com>,
Jason-JH Lin <jason-jh.lin@...iatek.com>,
Fei Shao <fshao@...gle.com>
Subject: Re: [PATCH v1 6/6] drm/mediatek: mt8188: Add VDOSYS1 PADDING driver
Il 07/06/23 08:11, Hsiao Chien Sung ha scritto:
> Add VDOSYS1 PADDING driver.
Please expand on the commit description, say what PADDING does, what is it for.
>
> Signed-off-by: Hsiao Chien Sung <shawn.sung@...iatek.com>
> ---
> drivers/gpu/drm/mediatek/Makefile | 1 +
> drivers/gpu/drm/mediatek/mtk_disp_drv.h | 3 +
> .../gpu/drm/mediatek/mtk_disp_ovl_adaptor.c | 43 +++++-
> drivers/gpu/drm/mediatek/mtk_disp_padding.c | 134 ++++++++++++++++++
> drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 1 +
> drivers/gpu/drm/mediatek/mtk_drm_drv.c | 1 +
> drivers/gpu/drm/mediatek/mtk_drm_drv.h | 2 +-
> 7 files changed, 181 insertions(+), 4 deletions(-)
> create mode 100644 drivers/gpu/drm/mediatek/mtk_disp_padding.c
>
> diff --git a/drivers/gpu/drm/mediatek/Makefile b/drivers/gpu/drm/mediatek/Makefile
> index d4d193f60271..753b7cb264d6 100644
> --- a/drivers/gpu/drm/mediatek/Makefile
> +++ b/drivers/gpu/drm/mediatek/Makefile
> @@ -7,6 +7,7 @@ mediatek-drm-y := mtk_disp_aal.o \
> mtk_disp_merge.o \
> mtk_disp_ovl.o \
> mtk_disp_ovl_adaptor.o \
> + mtk_disp_padding.o \
> mtk_disp_rdma.o \
> mtk_drm_crtc.o \
> mtk_drm_ddp_comp.o \
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_drv.h b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> index 2254038519e1..7200519a2670 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_drv.h
> @@ -157,4 +157,7 @@ void mtk_mdp_rdma_config(struct device *dev, struct mtk_mdp_rdma_cfg *cfg,
> const u32 *mtk_mdp_rdma_get_formats(struct device *dev);
> size_t mtk_mdp_rdma_get_num_formats(struct device *dev);
>
> +int mtk_disp_padding_clk_enable(struct device *dev);
> +void mtk_disp_padding_clk_disable(struct device *dev);
> +void mtk_disp_padding_config(struct device *dev, struct cmdq_pkt *cmdq_pkt);
> #endif
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c b/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
> index e1d8d4765af8..5f775144e8c1 100644
> --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl_adaptor.c
> @@ -27,6 +27,7 @@
>
> enum mtk_ovl_adaptor_comp_type {
> OVL_ADAPTOR_TYPE_RDMA = 0,
> + OVL_ADAPTOR_TYPE_PADDING,
> OVL_ADAPTOR_TYPE_MERGE,
> OVL_ADAPTOR_TYPE_ETHDR,
> OVL_ADAPTOR_TYPE_NUM,
> @@ -41,6 +42,14 @@ enum mtk_ovl_adaptor_comp_id {
> OVL_ADAPTOR_MDP_RDMA5,
> OVL_ADAPTOR_MDP_RDMA6,
> OVL_ADAPTOR_MDP_RDMA7,
> + OVL_ADAPTOR_DISP_PADDING0,
> + OVL_ADAPTOR_DISP_PADDING1,
> + OVL_ADAPTOR_DISP_PADDING2,
> + OVL_ADAPTOR_DISP_PADDING3,
> + OVL_ADAPTOR_DISP_PADDING4,
> + OVL_ADAPTOR_DISP_PADDING5,
> + OVL_ADAPTOR_DISP_PADDING6,
> + OVL_ADAPTOR_DISP_PADDING7,
> OVL_ADAPTOR_MERGE0,
> OVL_ADAPTOR_MERGE1,
> OVL_ADAPTOR_MERGE2,
> @@ -63,6 +72,7 @@ struct mtk_disp_ovl_adaptor {
>
> static const char * const private_comp_stem[OVL_ADAPTOR_TYPE_NUM] = {
> [OVL_ADAPTOR_TYPE_RDMA] = "vdo1-rdma",
> + [OVL_ADAPTOR_TYPE_PADDING] = "vdo1-padding",
> [OVL_ADAPTOR_TYPE_MERGE] = "merge",
> [OVL_ADAPTOR_TYPE_ETHDR] = "ethdr",
> };
> @@ -76,6 +86,14 @@ static const struct ovl_adaptor_comp_match comp_matches[OVL_ADAPTOR_ID_MAX] = {
> [OVL_ADAPTOR_MDP_RDMA5] = { OVL_ADAPTOR_TYPE_RDMA, DDP_COMPONENT_MDP_RDMA5, 5 },
> [OVL_ADAPTOR_MDP_RDMA6] = { OVL_ADAPTOR_TYPE_RDMA, DDP_COMPONENT_MDP_RDMA6, 6 },
> [OVL_ADAPTOR_MDP_RDMA7] = { OVL_ADAPTOR_TYPE_RDMA, DDP_COMPONENT_MDP_RDMA7, 7 },
> + [OVL_ADAPTOR_DISP_PADDING0] = { OVL_ADAPTOR_TYPE_PADDING, DDP_COMPONENT_DISP_PADDING0, 0 },
> + [OVL_ADAPTOR_DISP_PADDING1] = { OVL_ADAPTOR_TYPE_PADDING, DDP_COMPONENT_DISP_PADDING1, 1 },
> + [OVL_ADAPTOR_DISP_PADDING2] = { OVL_ADAPTOR_TYPE_PADDING, DDP_COMPONENT_DISP_PADDING2, 2 },
> + [OVL_ADAPTOR_DISP_PADDING3] = { OVL_ADAPTOR_TYPE_PADDING, DDP_COMPONENT_DISP_PADDING3, 3 },
> + [OVL_ADAPTOR_DISP_PADDING4] = { OVL_ADAPTOR_TYPE_PADDING, DDP_COMPONENT_DISP_PADDING4, 4 },
> + [OVL_ADAPTOR_DISP_PADDING5] = { OVL_ADAPTOR_TYPE_PADDING, DDP_COMPONENT_DISP_PADDING5, 5 },
> + [OVL_ADAPTOR_DISP_PADDING6] = { OVL_ADAPTOR_TYPE_PADDING, DDP_COMPONENT_DISP_PADDING6, 6 },
> + [OVL_ADAPTOR_DISP_PADDING7] = { OVL_ADAPTOR_TYPE_PADDING, DDP_COMPONENT_DISP_PADDING7, 7 },
> [OVL_ADAPTOR_MERGE0] = { OVL_ADAPTOR_TYPE_MERGE, DDP_COMPONENT_MERGE1, 1 },
> [OVL_ADAPTOR_MERGE1] = { OVL_ADAPTOR_TYPE_MERGE, DDP_COMPONENT_MERGE2, 2 },
> [OVL_ADAPTOR_MERGE2] = { OVL_ADAPTOR_TYPE_MERGE, DDP_COMPONENT_MERGE3, 3 },
> @@ -92,6 +110,8 @@ void mtk_ovl_adaptor_layer_config(struct device *dev, unsigned int idx,
> struct mtk_mdp_rdma_cfg rdma_config = {0};
> struct device *rdma_l;
> struct device *rdma_r;
> + struct device *padding_l;
> + struct device *padding_r;
> struct device *merge;
> struct device *ethdr;
> const struct drm_format_info *fmt_info = drm_format_info(pending->format);
> @@ -108,6 +128,8 @@ void mtk_ovl_adaptor_layer_config(struct device *dev, unsigned int idx,
>
> rdma_l = ovl_adaptor->ovl_adaptor_comp[OVL_ADAPTOR_MDP_RDMA0 + 2 * idx];
> rdma_r = ovl_adaptor->ovl_adaptor_comp[OVL_ADAPTOR_MDP_RDMA0 + 2 * idx + 1];
> + padding_l = ovl_adaptor->ovl_adaptor_comp[OVL_ADAPTOR_DISP_PADDING0 + 2 * idx];
> + padding_r = ovl_adaptor->ovl_adaptor_comp[OVL_ADAPTOR_DISP_PADDING0 + 2 * idx + 1];
> merge = ovl_adaptor->ovl_adaptor_comp[OVL_ADAPTOR_MERGE0 + idx];
> ethdr = ovl_adaptor->ovl_adaptor_comp[OVL_ADAPTOR_ETHDR0];
>
> @@ -143,10 +165,15 @@ void mtk_ovl_adaptor_layer_config(struct device *dev, unsigned int idx,
> rdma_config.color_encoding = pending->color_encoding;
> mtk_mdp_rdma_config(rdma_l, &rdma_config, cmdq_pkt);
>
> + if (padding_l)
> + mtk_disp_padding_config(padding_l, cmdq_pkt);
> +
> if (use_dual_pipe) {
> rdma_config.x_left = l_w;
> rdma_config.width = r_w;
> mtk_mdp_rdma_config(rdma_r, &rdma_config, cmdq_pkt);
> + if (padding_r)
> + mtk_disp_padding_config(padding_r, cmdq_pkt);
> }
>
> mtk_merge_start_cmdq(merge, cmdq_pkt);
> @@ -206,8 +233,10 @@ int mtk_ovl_adaptor_clk_enable(struct device *dev)
> comp = ovl_adaptor->ovl_adaptor_comp[i];
> if (!comp)
> continue;
> - if (i < OVL_ADAPTOR_MERGE0)
> + if (i < OVL_ADAPTOR_DISP_PADDING0)
> ret = mtk_mdp_rdma_clk_enable(comp);
> + else if (i < OVL_ADAPTOR_MERGE0)
> + ret = mtk_disp_padding_clk_enable(comp);
> else if (i < OVL_ADAPTOR_ETHDR0)
> ret = mtk_merge_clk_enable(comp);
> else
> @@ -225,8 +254,10 @@ int mtk_ovl_adaptor_clk_enable(struct device *dev)
> comp = ovl_adaptor->ovl_adaptor_comp[i];
> if (!comp)
> continue;
> - if (i < OVL_ADAPTOR_MERGE0)
> + if (i < OVL_ADAPTOR_DISP_PADDING0)
> mtk_mdp_rdma_clk_disable(comp);
> + else if (i < OVL_ADAPTOR_MERGE0)
> + mtk_disp_padding_clk_disable(comp);
> else if (i < OVL_ADAPTOR_ETHDR0)
> mtk_merge_clk_disable(comp);
> else
> @@ -255,9 +286,12 @@ void mtk_ovl_adaptor_clk_disable(struct device *dev)
> comp = ovl_adaptor->ovl_adaptor_comp[i];
> if (!comp)
> continue;
> - if (i < OVL_ADAPTOR_MERGE0) {
> + if (i < OVL_ADAPTOR_DISP_PADDING0) {
Now we're counting three branches. I'd say that a switch() would be a better
fit at this point.
> mtk_mdp_rdma_clk_disable(comp);
> pm_runtime_put(comp);
> + } else if (i < OVL_ADAPTOR_MERGE0) {
> + mtk_disp_padding_clk_disable(comp);
> + pm_runtime_put(comp);
> } else if (i < OVL_ADAPTOR_ETHDR0) {
> mtk_merge_clk_disable(comp);
> } else {
> @@ -389,6 +423,9 @@ static const struct of_device_id mtk_ovl_adaptor_comp_dt_ids[] = {
> {
> .compatible = "mediatek,mt8188-vdo1-rdma",
> .data = (void *)OVL_ADAPTOR_TYPE_RDMA,
> + }, {
> + .compatible = "mediatek,mt8188-vdo1-padding",
> + .data = (void *)OVL_ADAPTOR_TYPE_PADDING,
> }, {
> .compatible = "mediatek,mt8188-disp-merge",
> .data = (void *)OVL_ADAPTOR_TYPE_MERGE,
> diff --git a/drivers/gpu/drm/mediatek/mtk_disp_padding.c b/drivers/gpu/drm/mediatek/mtk_disp_padding.c
> new file mode 100644
> index 000000000000..5722aa57d628
> --- /dev/null
> +++ b/drivers/gpu/drm/mediatek/mtk_disp_padding.c
> @@ -0,0 +1,134 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2022 MediaTek Inc.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/component.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/of_irq.h>
I don't see any interrupt parsing action here, why are you including of_irq.h?
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/soc/mediatek/mtk-cmdq.h>
> +
> +#include "mtk_disp_drv.h"
> +#include "mtk_drm_crtc.h"
> +#include "mtk_drm_ddp_comp.h"
> +
> +/*
For kerneldoc, this should be:
/**
> + * struct mtk_disp_padding - DISP_RDMA driver structure
Also, this is not the DISP_RDMA driver structure
> + * @data: local driver data
...and there's no @data member, plus, you're not documenting other members.
> + */
> +struct mtk_disp_padding {
> + struct clk *clk;
> + void __iomem *regs;
> + struct cmdq_client_reg cmdq_reg;
> +};
> +
> +static int mtk_disp_padding_bind(struct device *dev, struct device *master,
> + void *data)
> +{
> + return 0;
> +}
> +
> +static void mtk_disp_padding_unbind(struct device *dev, struct device *master,
> + void *data)
> +{
> +}
> +
> +static const struct component_ops mtk_disp_padding_component_ops = {
> + .bind = mtk_disp_padding_bind,
> + .unbind = mtk_disp_padding_unbind,
> +};
> +
> +static int mtk_disp_padding_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct mtk_disp_padding *priv;
> + struct resource *res;
> + int ret;
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->clk = devm_clk_get(dev, NULL);
> + if (IS_ERR(priv->clk)) {
> + dev_err(dev, "failed to get clk\n");
> + return PTR_ERR(priv->clk);
> + }
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + priv->regs = devm_ioremap_resource(dev, res);
priv->regs = devm_platform_get_and_ioremap_resource(pdev, 0, res);
> + if (IS_ERR(priv->regs)) {
> + dev_err(dev, "failed to do ioremap\n");
> + return PTR_ERR(priv->regs);
> + }
> +
> +#if IS_REACHABLE(CONFIG_MTK_CMDQ)
> + ret = cmdq_dev_get_client_reg(dev, &priv->cmdq_reg, 0);
> + if (ret) {
> + dev_err(dev, "failed to get gce client reg\n");
> + return ret;
> + }
> +#endif
> +
> + platform_set_drvdata(pdev, priv);
> +
> + pm_runtime_enable(dev);
ret = devm_pm_runtime_enable(dev);
if (ret)
return ret;
> +
> + ret = component_add(dev, &mtk_disp_padding_component_ops);
> + if (ret) {
> + pm_runtime_disable(dev);
> + dev_err(dev, "failed to add component: %d\n", ret);
return dev_err_probe( .... )
> + }
> +
> + return ret;
return 0;
> +}
> +
> +static int mtk_disp_padding_remove(struct platform_device *pdev)
> +{
> + component_del(&pdev->dev, &mtk_disp_padding_component_ops);
> + pm_runtime_disable(&pdev->dev);
If you use devm_pm_runtime_enable, you don't have to call pm_runtime_disable here.
> + return 0;
> +}
> +
> +static const struct of_device_id mtk_disp_padding_driver_dt_match[] = {
> + { .compatible = "mediatek,mt8188-vdo1-padding" },
> + {},
{ /* sentinel */ }
}
> +};
> +MODULE_DEVICE_TABLE(of, mtk_disp_padding_driver_dt_match);
> +
> +struct platform_driver mtk_disp_padding_driver = {
> + .probe = mtk_disp_padding_probe,
> + .remove = mtk_disp_padding_remove,
> + .driver = {
> + .name = "mediatek-disp-padding",
> + .owner = THIS_MODULE,
> + .of_match_table = mtk_disp_padding_driver_dt_match,
> + },
> +};
> +
> +int mtk_disp_padding_clk_enable(struct device *dev)
> +{
> + struct mtk_disp_padding *padding = dev_get_drvdata(dev);
> +
> + return clk_prepare_enable(padding->clk);
> +}
> +
> +void mtk_disp_padding_clk_disable(struct device *dev)
> +{
> + struct mtk_disp_padding *padding = dev_get_drvdata(dev);
> +
> + clk_disable_unprepare(padding->clk);
> +}
> +
> +void mtk_disp_padding_config(struct device *dev, struct cmdq_pkt *cmdq_pkt)
> +{
> + struct mtk_disp_padding *padding = dev_get_drvdata(dev);
> +
> + // bypass padding
C-style comments please.
> + mtk_ddp_write_mask(cmdq_pkt,
> + 0b11, &padding->cmdq_reg, padding->regs, 0, 0b11);
Please add definitions and use GENMASK().
> +}
Move those functions before the probe function please.
Regards,
Angelo
Powered by blists - more mailing lists