| Message ID | 20230911074233.31556-18-shawn.sung@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add display driver for MT8188 VDOSYS1 | expand |
Hi, Hsiao-chien: On Mon, 2023-09-11 at 15:42 +0800, Hsiao Chien Sung wrote: > Padding is a new display module on MT8188, it provides ability > to add pixels to width and height of a layer with specified colors. > > Due to hardware design, Mixer in VDOSYS1 requires width of a layer > to be 2-pixel-align, or 4-pixel-align when ETHDR is enabled, > we need Padding to deal with odd width. > > Please notice that even if the Padding is in bypass mode, > settings in register must be cleared to 0, > or undefined behaviors could happen. You just set padding to bypass mode and not clear settings to 0. Any thing wrong? Regards, CK > > Signed-off-by: Hsiao Chien Sung <shawn.sung@mediatek.com> > --- > drivers/gpu/drm/mediatek/Makefile | 3 +- > drivers/gpu/drm/mediatek/mtk_disp_drv.h | 3 + > drivers/gpu/drm/mediatek/mtk_drm_drv.c | 1 + > drivers/gpu/drm/mediatek/mtk_drm_drv.h | 2 +- > drivers/gpu/drm/mediatek/mtk_padding.c | 136 > ++++++++++++++++++++++++ > 5 files changed, 143 insertions(+), 2 deletions(-) > create mode 100644 drivers/gpu/drm/mediatek/mtk_padding.c > > diff --git a/drivers/gpu/drm/mediatek/Makefile > b/drivers/gpu/drm/mediatek/Makefile > index d4d193f60271..5e4436403b8d 100644 > --- a/drivers/gpu/drm/mediatek/Makefile > +++ b/drivers/gpu/drm/mediatek/Makefile > @@ -16,7 +16,8 @@ mediatek-drm-y := mtk_disp_aal.o \ > mtk_dsi.o \ > mtk_dpi.o \ > mtk_ethdr.o \ > - mtk_mdp_rdma.o > + mtk_mdp_rdma.o \ > + mtk_padding.o > > obj-$(CONFIG_DRM_MEDIATEK) += mediatek-drm.o > > diff --git a/drivers/gpu/drm/mediatek/mtk_disp_drv.h > b/drivers/gpu/drm/mediatek/mtk_disp_drv.h > index 2254038519e1..f9fdb1268aa5 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_padding_clk_enable(struct device *dev); > +void mtk_padding_clk_disable(struct device *dev); > +void mtk_padding_config(struct device *dev, struct cmdq_pkt > *cmdq_pkt); > #endif > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c > b/drivers/gpu/drm/mediatek/mtk_drm_drv.c > index 93552d76b6e7..cde69f39a066 100644 > --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c > +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c > @@ -973,6 +973,7 @@ static struct platform_driver * const > mtk_drm_drivers[] = { > &mtk_dsi_driver, > &mtk_ethdr_driver, > &mtk_mdp_rdma_driver, > + &mtk_padding_driver, > }; > > static int __init mtk_drm_init(void) > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.h > b/drivers/gpu/drm/mediatek/mtk_drm_drv.h > index eb2fd45941f0..562f2db47add 100644 > --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.h > +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.h > @@ -64,5 +64,5 @@ extern struct platform_driver mtk_dpi_driver; > extern struct platform_driver mtk_dsi_driver; > extern struct platform_driver mtk_ethdr_driver; > extern struct platform_driver mtk_mdp_rdma_driver; > - > +extern struct platform_driver mtk_padding_driver; > #endif /* MTK_DRM_DRV_H */ > diff --git a/drivers/gpu/drm/mediatek/mtk_padding.c > b/drivers/gpu/drm/mediatek/mtk_padding.c > new file mode 100644 > index 000000000000..bbb9c5e286ce > --- /dev/null > +++ b/drivers/gpu/drm/mediatek/mtk_padding.c > @@ -0,0 +1,136 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (c) 2023 MediaTek Inc. > + */ > + > +#include <linux/clk.h> > +#include <linux/component.h> > +#include <linux/module.h> > +#include <linux/of_device.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" > + > +/** > + * struct mtk_padding - basic information of Padding > + * @clk: Clock of the module > + * @regs: Virtual address of the Padding for CPU to access > + * @cmdq_reg: CMDQ setting of the Padding > + * > + * Every Padding should have different clock source, register base, > and > + * CMDQ settings, we stored these differences all together. > + */ > +struct mtk_padding { > + struct clk *clk; > + void __iomem *regs; > + struct cmdq_client_reg cmdq_reg; > +}; > + > +int mtk_padding_clk_enable(struct device *dev) > +{ > + struct mtk_padding *padding = dev_get_drvdata(dev); > + > + return clk_prepare_enable(padding->clk); > +} > + > +void mtk_padding_clk_disable(struct device *dev) > +{ > + struct mtk_padding *padding = dev_get_drvdata(dev); > + > + clk_disable_unprepare(padding->clk); > +} > + > +void mtk_padding_config(struct device *dev, struct cmdq_pkt > *cmdq_pkt) > +{ > + struct mtk_padding *padding = dev_get_drvdata(dev); > + > + /* bypass padding */ > + mtk_ddp_write_mask(cmdq_pkt, GENMASK(1, 0), &padding->cmdq_reg, > padding->regs, 0, > + GENMASK(1, 0)); > +} > + > +static int mtk_padding_bind(struct device *dev, struct device > *master, void *data) > +{ > + return 0; > +} > + > +static void mtk_padding_unbind(struct device *dev, struct device > *master, void *data) > +{ > +} > + > +static const struct component_ops mtk_padding_component_ops = { > + .bind = mtk_padding_bind, > + .unbind = mtk_padding_unbind, > +}; > + > +static int mtk_padding_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct mtk_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); > + } > + > + 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); > + > + ret = devm_pm_runtime_enable(dev); > + if (ret) > + return ret; > + > + ret = component_add(dev, &mtk_padding_component_ops); > + if (ret) { > + pm_runtime_disable(dev); > + return dev_err_probe(dev, ret, "failed to add > component\n"); > + } > + > + return 0; > +} > + > +static int mtk_padding_remove(struct platform_device *pdev) > +{ > + component_del(&pdev->dev, &mtk_padding_component_ops); > + return 0; > +} > + > +static const struct of_device_id mtk_padding_driver_dt_match[] = { > + { .compatible = "mediatek,mt8188-padding" }, > + { /* sentinel */ } > +}; > +MODULE_DEVICE_TABLE(of, mtk_padding_driver_dt_match); > + > +struct platform_driver mtk_padding_driver = { > + .probe = mtk_padding_probe, > + .remove = mtk_padding_remove, > + .driver = { > + .name = "mediatek-padding", > + .owner = THIS_MODULE, > + .of_match_table = mtk_padding_driver_dt_match, > + }, > +}; > -- > 2.18.0 >
Hi CK, On Thu, 2023-09-28 at 03:05 +0000, CK Hu (胡俊光) wrote: > Hi, Hsiao-chien: > > On Mon, 2023-09-11 at 15:42 +0800, Hsiao Chien Sung wrote: > > Padding is a new display module on MT8188, it provides ability > > to add pixels to width and height of a layer with specified colors. > > > > Due to hardware design, Mixer in VDOSYS1 requires width of a layer > > to be 2-pixel-align, or 4-pixel-align when ETHDR is enabled, > > we need Padding to deal with odd width. > > > > Please notice that even if the Padding is in bypass mode, > > settings in register must be cleared to 0, > > or undefined behaviors could happen. > > You just set padding to bypass mode and not clear settings to 0. Any > thing wrong? > Since the deafult value of all the registers in Padding is zero, and we are not using Padding currently, it's fine if we just set padding to bypass mode witout clearing other registers. The comment is just a reminder in case we forget it in the future. Regards, Hsiao Chien Sung
Il 28/09/23 05:39, Shawn Sung (宋孝謙) ha scritto: > Hi CK, > > On Thu, 2023-09-28 at 03:05 +0000, CK Hu (胡俊光) wrote: >> Hi, Hsiao-chien: >> >> On Mon, 2023-09-11 at 15:42 +0800, Hsiao Chien Sung wrote: >>> Padding is a new display module on MT8188, it provides ability >>> to add pixels to width and height of a layer with specified colors. >>> >>> Due to hardware design, Mixer in VDOSYS1 requires width of a layer >>> to be 2-pixel-align, or 4-pixel-align when ETHDR is enabled, >>> we need Padding to deal with odd width. >>> >>> Please notice that even if the Padding is in bypass mode, >>> settings in register must be cleared to 0, >>> or undefined behaviors could happen. >> >> You just set padding to bypass mode and not clear settings to 0. Any >> thing wrong? >> > > Since the deafult value of all the registers in Padding is zero, and > we are not using Padding currently, it's fine if we just set padding to > bypass mode witout clearing other registers. > > The comment is just a reminder in case we forget it in the future. Do *not* rely on default register values, because you don't know what booted Linux in the first place: you shall *not* expect a clean state and you shall *not* expect a clean boot. Besides, what I see is that you're setting GENMASK(1, 0) without explaining why in the code: you have to add at least the definitions for PADDING_EN and PADDING_BYPASS. I also don't see why you shouldn't add at least basic handling for this block, as it looks easy enough: after all, you anyway have to make sure that the registers are cleared - might as well just add a little more effort on top and actually set them to meaningful values? That's ultimately your choice, but I don't want to see any GENMASK(31,0) write even for register clearing. Please make this driver proper. Thanks, Angelo > > Regards, > Hsiao Chien Sung
Hi Angelo, On Thu, 2023-09-28 at 12:24 +0200, AngeloGioacchino Del Regno wrote: > Il 28/09/23 05:39, Shawn Sung (宋孝謙) ha scritto: > > Hi CK, > > > > On Thu, 2023-09-28 at 03:05 +0000, CK Hu (胡俊光) wrote: > > > Hi, Hsiao-chien: > > > > > > On Mon, 2023-09-11 at 15:42 +0800, Hsiao Chien Sung wrote: > > > > Padding is a new display module on MT8188, it provides ability > > > > to add pixels to width and height of a layer with specified > > > > colors. > > > > > > > > Due to hardware design, Mixer in VDOSYS1 requires width of a > > > > layer > > > > to be 2-pixel-align, or 4-pixel-align when ETHDR is enabled, > > > > we need Padding to deal with odd width. > > > > > > > > Please notice that even if the Padding is in bypass mode, > > > > settings in register must be cleared to 0, > > > > or undefined behaviors could happen. > > > > > > You just set padding to bypass mode and not clear settings to 0. > > > Any > > > thing wrong? > > > > > > > Since the deafult value of all the registers in Padding is zero, > > and > > we are not using Padding currently, it's fine if we just set > > padding to > > bypass mode witout clearing other registers. > > > > The comment is just a reminder in case we forget it in the future. > > Do *not* rely on default register values, because you don't know what > booted > Linux in the first place: you shall *not* expect a clean state and > you shall > *not* expect a clean boot. > > Besides, what I see is that you're setting GENMASK(1, 0) without > explaining > why in the code: you have to add at least the definitions for > PADDING_EN and > PADDING_BYPASS. > > I also don't see why you shouldn't add at least basic handling for > this block, > as it looks easy enough: after all, you anyway have to make sure that > the > registers are cleared - might as well just add a little more effort > on top > and actually set them to meaningful values? That's ultimately your > choice, but > I don't want to see any GENMASK(31,0) write even for register > clearing. > > Please make this driver proper. > Thank you for the suggestions. I'll implement it in the next version. Thanks, Shawn
diff --git a/drivers/gpu/drm/mediatek/Makefile b/drivers/gpu/drm/mediatek/Makefile index d4d193f60271..5e4436403b8d 100644 --- a/drivers/gpu/drm/mediatek/Makefile +++ b/drivers/gpu/drm/mediatek/Makefile @@ -16,7 +16,8 @@ mediatek-drm-y := mtk_disp_aal.o \ mtk_dsi.o \ mtk_dpi.o \ mtk_ethdr.o \ - mtk_mdp_rdma.o + mtk_mdp_rdma.o \ + mtk_padding.o obj-$(CONFIG_DRM_MEDIATEK) += mediatek-drm.o diff --git a/drivers/gpu/drm/mediatek/mtk_disp_drv.h b/drivers/gpu/drm/mediatek/mtk_disp_drv.h index 2254038519e1..f9fdb1268aa5 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_padding_clk_enable(struct device *dev); +void mtk_padding_clk_disable(struct device *dev); +void mtk_padding_config(struct device *dev, struct cmdq_pkt *cmdq_pkt); #endif diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c index 93552d76b6e7..cde69f39a066 100644 --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c @@ -973,6 +973,7 @@ static struct platform_driver * const mtk_drm_drivers[] = { &mtk_dsi_driver, &mtk_ethdr_driver, &mtk_mdp_rdma_driver, + &mtk_padding_driver, }; static int __init mtk_drm_init(void) diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.h b/drivers/gpu/drm/mediatek/mtk_drm_drv.h index eb2fd45941f0..562f2db47add 100644 --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.h +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.h @@ -64,5 +64,5 @@ extern struct platform_driver mtk_dpi_driver; extern struct platform_driver mtk_dsi_driver; extern struct platform_driver mtk_ethdr_driver; extern struct platform_driver mtk_mdp_rdma_driver; - +extern struct platform_driver mtk_padding_driver; #endif /* MTK_DRM_DRV_H */ diff --git a/drivers/gpu/drm/mediatek/mtk_padding.c b/drivers/gpu/drm/mediatek/mtk_padding.c new file mode 100644 index 000000000000..bbb9c5e286ce --- /dev/null +++ b/drivers/gpu/drm/mediatek/mtk_padding.c @@ -0,0 +1,136 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (c) 2023 MediaTek Inc. + */ + +#include <linux/clk.h> +#include <linux/component.h> +#include <linux/module.h> +#include <linux/of_device.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" + +/** + * struct mtk_padding - basic information of Padding + * @clk: Clock of the module + * @regs: Virtual address of the Padding for CPU to access + * @cmdq_reg: CMDQ setting of the Padding + * + * Every Padding should have different clock source, register base, and + * CMDQ settings, we stored these differences all together. + */ +struct mtk_padding { + struct clk *clk; + void __iomem *regs; + struct cmdq_client_reg cmdq_reg; +}; + +int mtk_padding_clk_enable(struct device *dev) +{ + struct mtk_padding *padding = dev_get_drvdata(dev); + + return clk_prepare_enable(padding->clk); +} + +void mtk_padding_clk_disable(struct device *dev) +{ + struct mtk_padding *padding = dev_get_drvdata(dev); + + clk_disable_unprepare(padding->clk); +} + +void mtk_padding_config(struct device *dev, struct cmdq_pkt *cmdq_pkt) +{ + struct mtk_padding *padding = dev_get_drvdata(dev); + + /* bypass padding */ + mtk_ddp_write_mask(cmdq_pkt, GENMASK(1, 0), &padding->cmdq_reg, padding->regs, 0, + GENMASK(1, 0)); +} + +static int mtk_padding_bind(struct device *dev, struct device *master, void *data) +{ + return 0; +} + +static void mtk_padding_unbind(struct device *dev, struct device *master, void *data) +{ +} + +static const struct component_ops mtk_padding_component_ops = { + .bind = mtk_padding_bind, + .unbind = mtk_padding_unbind, +}; + +static int mtk_padding_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct mtk_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); + } + + 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); + + ret = devm_pm_runtime_enable(dev); + if (ret) + return ret; + + ret = component_add(dev, &mtk_padding_component_ops); + if (ret) { + pm_runtime_disable(dev); + return dev_err_probe(dev, ret, "failed to add component\n"); + } + + return 0; +} + +static int mtk_padding_remove(struct platform_device *pdev) +{ + component_del(&pdev->dev, &mtk_padding_component_ops); + return 0; +} + +static const struct of_device_id mtk_padding_driver_dt_match[] = { + { .compatible = "mediatek,mt8188-padding" }, + { /* sentinel */ } +}; +MODULE_DEVICE_TABLE(of, mtk_padding_driver_dt_match); + +struct platform_driver mtk_padding_driver = { + .probe = mtk_padding_probe, + .remove = mtk_padding_remove, + .driver = { + .name = "mediatek-padding", + .owner = THIS_MODULE, + .of_match_table = mtk_padding_driver_dt_match, + }, +};
Padding is a new display module on MT8188, it provides ability to add pixels to width and height of a layer with specified colors. Due to hardware design, Mixer in VDOSYS1 requires width of a layer to be 2-pixel-align, or 4-pixel-align when ETHDR is enabled, we need Padding to deal with odd width. Please notice that even if the Padding is in bypass mode, settings in register must be cleared to 0, or undefined behaviors could happen. Signed-off-by: Hsiao Chien Sung <shawn.sung@mediatek.com> --- drivers/gpu/drm/mediatek/Makefile | 3 +- drivers/gpu/drm/mediatek/mtk_disp_drv.h | 3 + drivers/gpu/drm/mediatek/mtk_drm_drv.c | 1 + drivers/gpu/drm/mediatek/mtk_drm_drv.h | 2 +- drivers/gpu/drm/mediatek/mtk_padding.c | 136 ++++++++++++++++++++++++ 5 files changed, 143 insertions(+), 2 deletions(-) create mode 100644 drivers/gpu/drm/mediatek/mtk_padding.c -- 2.18.0