by Chun-Kuang Hu

[permalink] [raw]

Subject: Re: [v7, PATCH 7/7] drm/mediatek: add support for mediatek SOC MT8183

Hi, Yongqiang:

Yongqiang Niu <[email protected]> 於 2020年7月27日週一下午3:54寫道：
>
> On Sat, 2020-07-25 at 07:24 +0800, Chun-Kuang Hu wrote:
> > Hi Yongqiang:
> >
> > Yongqiang Niu <[email protected]> 於 2020年7月23日週四上午10:15寫道：
> > >
> > > This patch add support for mediatek SOC MT8183
> > > 1.ovl_2l share driver with ovl
> >
> > I think this is done in [1], [2], [3], this patch just add the support
> > of mt8183-ovl and mt8183-ovl-2l.
> >
> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/gpu/drm/mediatek?h=v5.8-rc6&id=132c6e250ed745443973cada8db17cdbaebdf551
> > [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/gpu/drm/mediatek?h=v5.8-rc6&id=318462d1a568634ba09263cc730cb0fb1d56c2b3
> > [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/gpu/drm/mediatek?h=v5.8-rc6&id=57148baac8b78461e394953cfd5317bde8f795ab
> >
> > > 2.rdma1 share drive with rdma0, but fifo size is different
> >
> > I think this is done in [4], this patch just add the support of mt8183-rdma.
> >
> > [4] https://patchwork.kernel.org/patch/11679549/
> >
> > > 3.add mt8183 mutex private data, and mmsys private data
> > > 4.add mt8183 main and external path module for crtc create
> >
> > The fourth item is the mmsys private data in third item, so you need
> > not to repeat it.
> >
> > >
> > > Signed-off-by: Yongqiang Niu <[email protected]>
> > > ---
> > > drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 18 ++++++++++++
> > > drivers/gpu/drm/mediatek/mtk_disp_rdma.c | 6 ++++
> > > drivers/gpu/drm/mediatek/mtk_drm_ddp.c | 47 ++++++++++++++++++++++++++++++++
> > > drivers/gpu/drm/mediatek/mtk_drm_drv.c | 43 +++++++++++++++++++++++++++++
> > > 4 files changed, 114 insertions(+)
> > >
> >
> > [snip]
> >
> > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> > > index 014c1bb..60788c1 100644
> > > --- a/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> > > +++ b/drivers/gpu/drm/mediatek/mtk_drm_ddp.c
> > > @@ -15,6 +15,8 @@
> > >
> > > #define MT2701_DISP_MUTEX0_MOD0 0x2c
> > > #define MT2701_DISP_MUTEX0_SOF0 0x30
> > > +#define MT8183_DISP_MUTEX0_MOD0 0x30
> > > +#define MT8183_DISP_MUTEX0_SOF0 0x2c
> > >
> > > #define DISP_REG_MUTEX_EN(n) (0x20 + 0x20 * (n))
> > > #define DISP_REG_MUTEX(n) (0x24 + 0x20 * (n))
> > > @@ -25,6 +27,18 @@
> > >
> > > #define INT_MUTEX BIT(1)
> > >
> > > +#define MT8183_MUTEX_MOD_DISP_RDMA0 0
> > > +#define MT8183_MUTEX_MOD_DISP_RDMA1 1
> > > +#define MT8183_MUTEX_MOD_DISP_OVL0 9
> > > +#define MT8183_MUTEX_MOD_DISP_OVL0_2L 10
> > > +#define MT8183_MUTEX_MOD_DISP_OVL1_2L 11
> > > +#define MT8183_MUTEX_MOD_DISP_WDMA0 12
> > > +#define MT8183_MUTEX_MOD_DISP_COLOR0 13
> > > +#define MT8183_MUTEX_MOD_DISP_CCORR0 14
> > > +#define MT8183_MUTEX_MOD_DISP_AAL0 15
> > > +#define MT8183_MUTEX_MOD_DISP_GAMMA0 16
> > > +#define MT8183_MUTEX_MOD_DISP_DITHER0 17
> > > +
> > > #define MT8173_MUTEX_MOD_DISP_OVL0 11
> > > #define MT8173_MUTEX_MOD_DISP_OVL1 12
> > > #define MT8173_MUTEX_MOD_DISP_RDMA0 13
> > > @@ -74,6 +88,10 @@
> > > #define MUTEX_SOF_DSI2 5
> > > #define MUTEX_SOF_DSI3 6
> > >
> > > +#define MT8183_MUTEX_SOF_DPI0 2
> > > +#define MT8183_MUTEX_EOF_DSI0 (MUTEX_SOF_DSI0 << 6)
> > > +#define MT8183_MUTEX_EOF_DPI0 (MT8183_MUTEX_SOF_DPI0 << 6)
> > > +
> > >
> > > struct mtk_disp_mutex {
> > > int id;
> > > @@ -153,6 +171,20 @@ struct mtk_ddp {
> > > [DDP_COMPONENT_WDMA1] = MT8173_MUTEX_MOD_DISP_WDMA1,
> > > };
> > >
> > > +static const unsigned int mt8183_mutex_mod[DDP_COMPONENT_ID_MAX] = {
> > > + [DDP_COMPONENT_AAL0] = MT8183_MUTEX_MOD_DISP_AAL0,
> > > + [DDP_COMPONENT_CCORR] = MT8183_MUTEX_MOD_DISP_CCORR0,
> > > + [DDP_COMPONENT_COLOR0] = MT8183_MUTEX_MOD_DISP_COLOR0,
> > > + [DDP_COMPONENT_DITHER] = MT8183_MUTEX_MOD_DISP_DITHER0,
> > > + [DDP_COMPONENT_GAMMA] = MT8183_MUTEX_MOD_DISP_GAMMA0,
> > > + [DDP_COMPONENT_OVL0] = MT8183_MUTEX_MOD_DISP_OVL0,
> > > + [DDP_COMPONENT_OVL_2L0] = MT8183_MUTEX_MOD_DISP_OVL0_2L,
> > > + [DDP_COMPONENT_OVL_2L1] = MT8183_MUTEX_MOD_DISP_OVL1_2L,
> > > + [DDP_COMPONENT_RDMA0] = MT8183_MUTEX_MOD_DISP_RDMA0,
> > > + [DDP_COMPONENT_RDMA1] = MT8183_MUTEX_MOD_DISP_RDMA1,
> > > + [DDP_COMPONENT_WDMA0] = MT8183_MUTEX_MOD_DISP_WDMA0,
> > > +};
> > > +
> > > static const unsigned int mt2712_mutex_sof[DDP_MUTEX_SOF_DSI3 + 1] = {
> > > [DDP_MUTEX_SOF_SINGLE_MODE] = MUTEX_SOF_SINGLE_MODE,
> > > [DDP_MUTEX_SOF_DSI0] = MUTEX_SOF_DSI0,
> > > @@ -163,6 +195,12 @@ struct mtk_ddp {
> > > [DDP_MUTEX_SOF_DSI3] = MUTEX_SOF_DSI3,
> > > };
> > >
> > > +static const unsigned int mt8183_mutex_sof[DDP_MUTEX_SOF_DSI3 + 1] = {
> > > + [DDP_MUTEX_SOF_SINGLE_MODE] = MUTEX_SOF_SINGLE_MODE,
> > > + [DDP_MUTEX_SOF_DSI0] = MUTEX_SOF_DSI0 | MT8183_MUTEX_EOF_DSI0,
> >
> > I think this array is for 'sof', so you should drop MT8183_MUTEX_EOF_DSI0.
> >
> > > + [DDP_MUTEX_SOF_DPI0] = MT8183_MUTEX_SOF_DPI0 | MT8183_MUTEX_EOF_DPI0,
> >
> > Ditto.
> >
> > Regards,
> > Chun-Kuang.
>
> MT8183 mutex setting is different with before SoC.
> if we do not set EOF, Overlay hardware will not receive frame done irq,
> and will display error.

Please add comment for this because this is not a trivial thing.

Regards,
Chun-Kuang.

> >
> > > +};
> > > +
> > > static const struct mtk_ddp_data mt2701_ddp_driver_data = {
> > > .mutex_mod = mt2701_mutex_mod,
> > > .mutex_sof = mt2712_mutex_sof,
> > > @@ -184,6 +222,13 @@ struct mtk_ddp {
> > > .mutex_sof_reg = MT2701_DISP_MUTEX0_SOF0,
> > > };
> > >
>

2020-07-28 10:26:15

by Enric Balletbo Serra

[permalink] [raw]

Subject: Re: [v7, PATCH 2/7] mtk-mmsys: add mmsys private data

Hi Yongqiang,

Missatge de Yongqiang Niu <[email protected]> del dia ds., 25
de jul. 2020 a les 5:29:
>
> On Thu, 2020-07-23 at 11:32 +0200, Enric Balletbo Serra wrote:
> > Hi Yongqiang Niu,
> >
> > Thank you for your patch.
> >
> > Missatge de Yongqiang Niu <[email protected]> del dia dj., 23
> > de jul. 2020 a les 4:05:
> > >
> > > add mmsys private data
> > >
> >
> > I think this change requires a better explanation of what you are
> > doing. Although I'm really uncomfortable with this change, why you
> > need to create a new mt2701-mmsys file?
>
> reason:
> 1.there will more and more Mediatek Soc upstream, and the display path
> connection function mtk_mmsys_ddp_mout_en, mtk_mmsys_ddp_sel_in and
> mtk_mmsys_ddp_sout_sel will complicated more and more,
> 2. many of the connection are only used in some SoC, and useless for
> other SoC and not readable,
> 3. if we add a new SoC connection, we need check is this affect other
> Soc,
> >
> > > Feature: drm/mediatek
> >
> > Remove this.
> next version will remove this
> >
> > > Signed-off-by: Yongqiang Niu <[email protected]>
> > > ---
> > > drivers/soc/mediatek/Makefile | 1 +
> > > drivers/soc/mediatek/mmsys/Makefile | 2 +
> > > drivers/soc/mediatek/mmsys/mt2701-mmsys.c | 250 +++++++++++++++++++++++++++
> > > drivers/soc/mediatek/mtk-mmsys.c | 271 +++++-------------------------
> > > include/linux/soc/mediatek/mtk-mmsys.h | 15 ++
> > > 5 files changed, 314 insertions(+), 225 deletions(-)
> > > create mode 100644 drivers/soc/mediatek/mmsys/Makefile
> > > create mode 100644 drivers/soc/mediatek/mmsys/mt2701-mmsys.c
> > >
> > > diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile
> > > index 2afa7b9..b37ac2c 100644
> > > --- a/drivers/soc/mediatek/Makefile
> > > +++ b/drivers/soc/mediatek/Makefile
> > > @@ -3,3 +3,4 @@ obj-$(CONFIG_MTK_CMDQ) += mtk-cmdq-helper.o
> > > obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o
> > > obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o
> > > obj-$(CONFIG_MTK_MMSYS) += mtk-mmsys.o
> > > +obj-$(CONFIG_MTK_MMSYS) += mmsys/
> > > diff --git a/drivers/soc/mediatek/mmsys/Makefile b/drivers/soc/mediatek/mmsys/Makefile
> > > new file mode 100644
> > > index 0000000..33b0dab
> > > --- /dev/null
> > > +++ b/drivers/soc/mediatek/mmsys/Makefile
> > > @@ -0,0 +1,2 @@
> > > +# SPDX-License-Identifier: GPL-2.0-only
> > > +obj-y += mt2701-mmsys.o
> > > diff --git a/drivers/soc/mediatek/mmsys/mt2701-mmsys.c b/drivers/soc/mediatek/mmsys/mt2701-mmsys.c
> > > new file mode 100644
> > > index 0000000..b8e53b0
> > > --- /dev/null
> > > +++ b/drivers/soc/mediatek/mmsys/mt2701-mmsys.c
> > > @@ -0,0 +1,250 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +//
> > > +// Copyright (c) 2020 MediaTek Inc.
> > > +
> > > +#include <linux/device.h>
> > > +#include <linux/io.h>
> > > +#include <linux/of_device.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/soc/mediatek/mtk-mmsys.h>
> > > +
> > > +#define DISP_REG_CONFIG_DISP_OVL0_MOUT_EN 0x040
> > > +#define DISP_REG_CONFIG_DISP_OVL1_MOUT_EN 0x044
> > > +#define DISP_REG_CONFIG_DISP_OD_MOUT_EN 0x048
> > > +#define DISP_REG_CONFIG_DISP_GAMMA_MOUT_EN 0x04c
> > > +#define DISP_REG_CONFIG_DISP_UFOE_MOUT_EN 0x050
> > > +#define DISP_REG_CONFIG_DISP_COLOR0_SEL_IN 0x084
> > > +#define DISP_REG_CONFIG_DISP_COLOR1_SEL_IN 0x088
> > > +#define DISP_REG_CONFIG_DSIE_SEL_IN 0x0a4
> > > +#define DISP_REG_CONFIG_DSIO_SEL_IN 0x0a8
> > > +#define DISP_REG_CONFIG_DPI_SEL_IN 0x0ac
> > > +#define DISP_REG_CONFIG_DISP_RDMA2_SOUT 0x0b8
> > > +#define DISP_REG_CONFIG_DISP_RDMA0_SOUT_EN 0x0c4
> > > +#define DISP_REG_CONFIG_DISP_RDMA1_SOUT_EN 0x0c8
> > > +#define DISP_REG_CONFIG_MMSYS_CG_CON0 0x100
> > > +
> > > +#define DISP_REG_CONFIG_DISP_OVL_MOUT_EN 0x030
> > > +#define DISP_REG_CONFIG_OUT_SEL 0x04c
> > > +#define DISP_REG_CONFIG_DSI_SEL 0x050
> > > +#define DISP_REG_CONFIG_DPI_SEL 0x064
> > > +
> > > +#define OVL0_MOUT_EN_COLOR0 0x1
> > > +#define OD_MOUT_EN_RDMA0 0x1
> > > +#define OD1_MOUT_EN_RDMA1 BIT(16)
> > > +#define UFOE_MOUT_EN_DSI0 0x1
> > > +#define COLOR0_SEL_IN_OVL0 0x1
> > > +#define OVL1_MOUT_EN_COLOR1 0x1
> > > +#define GAMMA_MOUT_EN_RDMA1 0x1
> > > +#define RDMA0_SOUT_DPI0 0x2
> > > +#define RDMA0_SOUT_DPI1 0x3
> > > +#define RDMA0_SOUT_DSI1 0x1
> > > +#define RDMA0_SOUT_DSI2 0x4
> > > +#define RDMA0_SOUT_DSI3 0x5
> > > +#define RDMA1_SOUT_DPI0 0x2
> > > +#define RDMA1_SOUT_DPI1 0x3
> > > +#define RDMA1_SOUT_DSI1 0x1
> > > +#define RDMA1_SOUT_DSI2 0x4
> > > +#define RDMA1_SOUT_DSI3 0x5
> > > +#define RDMA2_SOUT_DPI0 0x2
> > > +#define RDMA2_SOUT_DPI1 0x3
> > > +#define RDMA2_SOUT_DSI1 0x1
> > > +#define RDMA2_SOUT_DSI2 0x4
> > > +#define RDMA2_SOUT_DSI3 0x5
> > > +#define DPI0_SEL_IN_RDMA1 0x1
> > > +#define DPI0_SEL_IN_RDMA2 0x3
> > > +#define DPI1_SEL_IN_RDMA1 (0x1 << 8)
> > > +#define DPI1_SEL_IN_RDMA2 (0x3 << 8)
> > > +#define DSI0_SEL_IN_RDMA1 0x1
> > > +#define DSI0_SEL_IN_RDMA2 0x4
> > > +#define DSI1_SEL_IN_RDMA1 0x1
> > > +#define DSI1_SEL_IN_RDMA2 0x4
> > > +#define DSI2_SEL_IN_RDMA1 (0x1 << 16)
> > > +#define DSI2_SEL_IN_RDMA2 (0x4 << 16)
> > > +#define DSI3_SEL_IN_RDMA1 (0x1 << 16)
> > > +#define DSI3_SEL_IN_RDMA2 (0x4 << 16)
> > > +#define COLOR1_SEL_IN_OVL1 0x1
> > > +
> > > +#define OVL_MOUT_EN_RDMA 0x1
> > > +#define BLS_TO_DSI_RDMA1_TO_DPI1 0x8
> > > +#define BLS_TO_DPI_RDMA1_TO_DSI 0x2
> > > +#define DSI_SEL_IN_BLS 0x0
> > > +#define DPI_SEL_IN_BLS 0x0
> > > +#define DSI_SEL_IN_RDMA 0x1
> > > +
> > > +static unsigned int mtk_mmsys_ddp_mout_en(enum mtk_ddp_comp_id cur,
> > > + enum mtk_ddp_comp_id next,
> > > + unsigned int *addr)
> > > +{
> >
> > Can't be reused this function for all devices? You did in the previous
> > series, why not now?
>
> can not reused, that why need add mmsys private data like before
> version.

You did in the previous series, would that mean that your previous
series were wrong or nonworking or break on some SoCs?

I agree that you need per SoC data, what I am complaining is the idea
of having pointers to functions instead of just having the registers
or the media paths per SoC. You can have private data for registers,
something like this:

static const struct mtk_mmsys_driver_data mt8183_mmsys_driver_data = {
.clk_driver = "clk-mt8183-mm",
.regs = {
.ovl0_mout_en = MT8183_DISP_OVL0_MOUT_EN,
.rdma0_sout_sel_in = MT8183_DISP_RDMA0_SOUT_SEL_IN,
.rdma0_sout_color0 = MT8183_RDMA0_SOUT_COLOR0,
.rdma1_sout_sel_in = MT8183_DISP_RDMA1_SOUT_SEL_IN,
.rdma1_sout_dsi0 = MT8183_RDMA1_SOUT_DSI0,
.dpi0_sel_in = MT8183_DISP_DPI0_SEL_IN,
.dpi0_sel_in_rdma1 = MT8183_DPI0_SEL_IN_RDMA1,
.dsi0_sel_in = MT8183_DISP_DSI0_SEL_IN,
.dsi0_sel_in_rdma1 = MT8183_DSI0_SEL_IN_RDMA1,
},
};

And you can also have the media configuration paths in the driver
data, similar to what you did for mt8183 but not for the others. But,
I'd recommend to do this in a second step, and go step-by-step. Your
v6 and v7 are quite different, but IMHO I'm more in favor of what you
did for v6 (as a first step to support mt8183) than this patchset. For
v7 I was expecting just to move the bits that needed to mtk-mmsys, not
such rework. Said all that, I'm not the maintainer and is up to the
maintainer to decide the direction that driver should take.

> but that still not a good idea.
>
> the next path like this will be more readable, and more easy to maintain
> https://patchwork.kernel.org/patch/11679539/

Ack, that's more readable and probably should be the direction to go,
always assuming we have the *data* (mmsys_path_sel, etc.) per SoC and
the functions mtk_mmsys_ddp_mout_en(), mtk_mmsys_ddp_sel_in(),
mtk_mmsys_ddp_sout_sel() common for all SoCs.

Thanks,
Enric

[snip]