On 13.12.2022 13:20, Marek Szyprowski wrote: > On 13.12.2022 11:40, Jagan Teki wrote: >> On Tue, Dec 13, 2022 at 2:28 PM Marek Szyprowski >> <m.szyprow...@samsung.com> wrote: >>> On 12.12.2022 16:33, Jagan Teki wrote: >>> >>> On Mon, Dec 12, 2022 at 8:52 PM Marek Szyprowski >>> <m.szyprow...@samsung.com> wrote: >>> >>> On 12.12.2022 09:43, Marek Szyprowski wrote: >>> >>> On 12.12.2022 09:32, Jagan Teki wrote: >>> >>> On Mon, Dec 12, 2022 at 1:56 PM Marek Szyprowski >>> <m.szyprow...@samsung.com> wrote: >>> >>> Hi Jagan, >>> >>> On 09.12.2022 16:23, Jagan Teki wrote: >>> >>> The existing drm panels and bridges in Exynos required host >>> initialization during the first DSI command transfer even though >>> the initialization was done before. >>> >>> This host reinitialization is handled via DSIM_STATE_REINITIALIZED >>> flag and triggers from host transfer. >>> >>> Do this exclusively for Exynos. >>> >>> Initial logic is derived from Marek Szyprowski changes. >>> >>> Signed-off-by: Marek Szyprowski <m.szyprow...@samsung.com> >>> Signed-off-by: Jagan Teki <ja...@amarulasolutions.com> >>> --- >>> Changes from v9: >>> - derived from v8 >>> - added comments >>> >>> drivers/gpu/drm/bridge/samsung-dsim.c | 15 ++++++++++++++- >>> include/drm/bridge/samsung-dsim.h | 5 +++-- >>> 2 files changed, 17 insertions(+), 3 deletions(-) >>> >>> The following chunk is missing compared to v8: >>> >>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c >>> b/drivers/gpu/drm/bridge/samsung-dsim.c >>> index 6e9ad955ebd3..6a9403cb92ae 100644 >>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c >>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c >>> @@ -1315,7 +1315,9 @@ static int samsung_dsim_init(struct samsung_dsim >>> *dsi, unsigned int flag) >>> return 0; >>> >>> samsung_dsim_reset(dsi); >>> - samsung_dsim_enable_irq(dsi); >>> + >>> + if (!(dsi->state & DSIM_STATE_INITIALIZED)) >>> + samsung_dsim_enable_irq(dsi); >>> >>> Is this really required? does it make sure that the IRQ does not >>> enable twice? >>> >>> That's what that check does. Without the 'if (!(dsi->state & >>> DSIM_STATE_INITIALIZED))' check, the irqs will be enabled twice (first >>> from pre_enable, then from the first transfer), what leads to a >>> warning from irq core. >>> >>> I've just noticed that we also would need to clear the >>> DSIM_STATE_REINITIALIZED flag in dsim_suspend. >>> >>> However I've found that a bit simpler patch would keep the current code >>> flow for Exynos instead of this reinitialization hack. This can be >>> applied on the "[PATCH v9 09/18] drm: bridge: samsung-dsim: Add host >>> init in pre_enable" patch: >>> >>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c >>> b/drivers/gpu/drm/bridge/samsung-dsim.c >>> index 0b2e52585485..acc95c61ae45 100644 >>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c >>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c >>> @@ -1291,9 +1291,11 @@ static void >>> samsung_dsim_atomic_pre_enable(struct >>> drm_bridge *bridge, >>> >>> dsi->state |= DSIM_STATE_ENABLED; >>> >>> - ret = samsung_dsim_init(dsi, DSIM_STATE_INITIALIZED); >>> - if (ret) >>> - return; >>> + if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) { >>> + ret = samsung_dsim_init(dsi, DSIM_STATE_INITIALIZED); >>> + if (ret) >>> + return; >>> + } >>> >>> Sorry, I don't understand this. Does it mean Exynos doesn't need to >>> init host in pre_enable? If I remember correctly even though the host >>> is initialized it has to reinitialize during the first transfer - This >>> is what the Exynos requirement is. Please correct or explain here. >>> >>> This is a matter of enabling power regulator(s) in the right order >>> and doing the host initialization in the right moment. It was never >>> a matter of re-initialization. See the current code for the >>> reference (it uses the same approach as my above change). I've >>> already explained that here: >>> >>> https://lore.kernel.org/all/e96197f9-948a-997e-5453-9f9d179b5...@samsung.com/ >>> >>> >>> >>> If you would like to see the exact proper moment of the dsi host >>> initialization on the Exynos see the code here: >>> >>> https://protect2.fireeye.com/v1/url?k=5dc33900-0258001f-5dc2b24f-000babdfecba-f7c1a2a1905c83ca&q=1&e=f086bfdb-9055-48bd-b9c2-5dffb6c0d558&u=https%3A%2F%2Fgithub.com%2Fmszyprow%2Flinux%2Ftree%2Fv5.18-next-20220511-dsi-rework >>> >>> and patches adding mipi_dsi_host_init() to panel/bridge drivers. >> As I said before, the downstream bridge needs an explicit call to host >> init via mipi_dsi_host_init - this is indeed not a usual use-case >> scenario. Let's handle this with a REINIT fix and see if we can update >> this later to handle both scenarios. >> >> Would you please test this repo, I have included all. >> >> https://gitlab.com/openedev/kernel/-/commits/imx8mm-dsi-v10 > > This doesn't work on TM2e board. Give me some time to find why... > The following change is missing in "drm: bridge: Generalize Exynos-DSI driver into a Samsung DSIM bridge" patch:
diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c index 1dbff2bee93f..81828b5ee0ac 100644 --- a/drivers/gpu/drm/bridge/samsung-dsim.c +++ b/drivers/gpu/drm/bridge/samsung-dsim.c @@ -1745,6 +1745,7 @@ int samsung_dsim_probe(struct platform_device *pdev) dsi->bridge.funcs = &samsung_dsim_bridge_funcs; dsi->bridge.of_node = dev->of_node; dsi->bridge.type = DRM_MODE_CONNECTOR_DSI; + dsi->bridge.pre_enable_prev_first = true; /* DE_LOW: i.MX8M Mini/Nano LCDIF-DSIM glue logic inverts HS/VS/DE */ if (dsi->plat_data->hw_type == DSIM_TYPE_IMX8MM) After adding the above, all my test platform works fine. BTW, I still think that it is worth replacing the "drm: exynos: dsi: Add host initialization in pre_enable" patch with the following simple change and propagate it to bridge/samsung-dsim.c: diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c index fdaf514b39f2..071b74d60dcb 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c @@ -254,6 +254,9 @@ struct exynos_dsi_transfer { #define DSIM_STATE_CMD_LPM BIT(2) #define DSIM_STATE_VIDOUT_AVAILABLE BIT(3) +#define exynos_dsi_hw_is_exynos(hw) \ + ((hw) >= DSIM_TYPE_EXYNOS3250 && (hw) <= DSIM_TYPE_EXYNOS5433) + enum exynos_dsi_type { DSIM_TYPE_EXYNOS3250, DSIM_TYPE_EXYNOS4210, @@ -1344,6 +1347,9 @@ static int exynos_dsi_init(struct exynos_dsi *dsi) { const struct exynos_dsi_driver_data *driver_data = dsi->driver_data; + if (dsi->state & DSIM_STATE_INITIALIZED) + return 0; + exynos_dsi_reset(dsi); exynos_dsi_enable_irq(dsi); @@ -1356,6 +1362,8 @@ static int exynos_dsi_init(struct exynos_dsi *dsi) exynos_dsi_set_phy_ctrl(dsi); exynos_dsi_init_link(dsi); + dsi->state |= DSIM_STATE_INITIALIZED; + return 0; } @@ -1411,6 +1419,12 @@ static void exynos_dsi_atomic_pre_enable(struct drm_bridge *bridge, } dsi->state |= DSIM_STATE_ENABLED; + + if (!exynos_dsi_hw_is_exynos(dsi->plat_data->hw_type)) { + ret = exynos_dsi_init(dsi); + if (ret) + return; + } } static void exynos_dsi_atomic_enable(struct drm_bridge *bridge, @@ -1557,12 +1571,9 @@ static ssize_t exynos_dsi_host_transfer(struct mipi_dsi_host *host, if (!(dsi->state & DSIM_STATE_ENABLED)) return -EINVAL; - if (!(dsi->state & DSIM_STATE_INITIALIZED)) { - ret = exynos_dsi_init(dsi); - if (ret) - return ret; - dsi->state |= DSIM_STATE_INITIALIZED; - } + ret = exynos_dsi_init(dsi); + if (ret) + return ret; ret = mipi_dsi_create_packet(&xfer.packet, msg); if (ret < 0) Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland