Hi Marek, On Tue, Mar 7, 2023 at 1:25 PM Jagan Teki <jagan@xxxxxxxxxxxxxxxxxxxx> wrote: > > Hi Marek, > > On Tue, Mar 7, 2023 at 4:11 AM Marek Szyprowski > <m.szyprowski@xxxxxxxxxxx> wrote: > > > > Dear Jagan, > > > > On 06.03.2023 18:24, Jagan Teki wrote: > > > On Mon, Mar 6, 2023 at 4:32 PM Marek Szyprowski > > > <m.szyprowski@xxxxxxxxxxx> wrote: > > >> On 04.03.2023 19:59, Jagan Teki wrote: > > >>> On Sat, Mar 4, 2023 at 3:56 AM Marek Szyprowski > > >>> <m.szyprowski@xxxxxxxxxxx> wrote: > > >>>> On 03.03.2023 15:51, Jagan Teki wrote: > > >>>>> This series supports common bridge support for Samsung MIPI DSIM > > >>>>> which is used in Exynos and i.MX8MM SoC's. > > >>>>> > > >>>>> The final bridge supports both the Exynos and i.MX8M Mini/Nano/Plus. > > >>>>> > > >>>>> Inki Dae: please note that this series added on top of exynos-drm-next > > >>>>> since few exynos dsi changes are not been part of drm-misc-next. > > >>>>> Request you to pick these via exynos-drm-next, or let me know if you > > >>>>> have any comments? > > >>>> I gave it a try on Exynos TM2e and unfortunately it nukes again: > > >>>> > > >>>> exynos-drm exynos-drm: bound 13970000.hdmi (ops hdmi_component_ops) > > >>>> Unable to handle kernel paging request at virtual address 003d454d414e5675 > > >>>> ... > > >>>> [003d454d414e5675] address between user and kernel address ranges > > >>>> Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP > > >>>> Modules linked in: > > >>>> CPU: 4 PID: 9 Comm: kworker/u16:0 Not tainted 6.2.0-next-20230303+ #13341 > > >>>> Hardware name: Samsung TM2E board (DT) > > >>>> Workqueue: events_unbound deferred_probe_work_func > > >>>> pstate: 000000c5 (nzcv daIF -PAN -UAO -TCO -DIT -SSBS BTYPE=--) > > >>>> pc : drm_connector_list_iter_next+0x58/0x100 > > >>>> lr : drm_connector_list_iter_next+0x2c/0x100 > > >>>> sp : ffff80000bbab910 > > >>>> ... > > >>>> Call trace: > > >>>> drm_connector_list_iter_next+0x58/0x100 > > >>>> drm_mode_config_reset+0xfc/0x144 > > >>>> exynos_drm_bind+0x160/0x1b8 > > >>>> try_to_bring_up_aggregate_device+0x168/0x1d4 > > >>>> __component_add+0xa8/0x170 > > >>>> component_add+0x14/0x20 > > >>>> hdmi_probe+0x3fc/0x6d4 > > >>>> platform_probe+0x68/0xd8 > > >>>> really_probe+0x148/0x2b4 > > >>>> __driver_probe_device+0x78/0xe0 > > >>>> driver_probe_device+0xd8/0x160 > > >>>> __device_attach_driver+0xb8/0x138 > > >>>> bus_for_each_drv+0x84/0xe0 > > >>>> __device_attach+0xa8/0x1b0 > > >>>> device_initial_probe+0x14/0x20 > > >>>> bus_probe_device+0xb0/0xb4 > > >>>> deferred_probe_work_func+0x8c/0xc8 > > >>>> process_one_work+0x288/0x6c8 > > >>>> worker_thread+0x24c/0x450 > > >>>> kthread+0x118/0x11c > > >>>> ret_from_fork+0x10/0x20 > > >>> This looks not related to dsi to me since there is no exynos_drm_dsi > > >>> call in the trace. look hdmi related. Moreover, I think the exynos dsi > > >>> worked well on v10 and I couldn't find any potential differences in > > >>> terms of call flow change. > > >>> https://gitlab.com/openedev/kernel/-/commits/imx8mm-dsi-v10 > > >> Well, the issue is definitely related to this patchset. On Friday, due > > >> to other kernel messages, I missed the most important part of the log: > > >> > > >> [drm] Exynos DRM: using 13800000.decon device for DMA mapping operations > > >> exynos-drm exynos-drm: bound 13800000.decon (ops decon_component_ops) > > >> exynos-drm exynos-drm: bound 13880000.decon (ops decon_component_ops) > > >> exynos-dsi 13900000.dsi: [drm:samsung_dsim_host_attach] Attached s6e3hf2 > > >> device > > >> exynos-dsi 13900000.dsi: request interrupt failed with -22 > > >> panel-samsung-s6e3ha2: probe of 13900000.dsi.0 failed with error -22 > > >> exynos-drm exynos-drm: bound 13900000.dsi (ops exynos_dsi_component_ops) > > >> exynos-drm exynos-drm: bound 13930000.mic (ops exynos_mic_component_ops) > > >> > > >> It looks that the are at least 2 issues. The first one related to TE > > >> interrupt registration, the second is broken error path, which should > > >> free allocated resources and stop DRM from binding/initialization. > > >> > > >> This patch fixes the issue (TE gpio/interrupt is optional!): > > >> > > >> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c > > >> b/drivers/gpu/drm/bridge/samsung-dsim.c > > >> index b4a5348b763c..ed83495fe105 100644 > > >> --- a/drivers/gpu/drm/bridge/samsung-dsim.c > > >> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c > > >> @@ -1518,7 +1518,9 @@ static int samsung_dsim_register_te_irq(struct > > >> samsung_dsim *dsi, struct device > > >> int ret; > > >> > > >> dsi->te_gpio = devm_gpiod_get_optional(dev, "te", GPIOD_IN); > > >> - if (IS_ERR(dsi->te_gpio)) > > >> + if (!dsi->te_gpio) > > >> + return 0; > > >> + else if (IS_ERR(dsi->te_gpio)) > > > I think I missed this change from v10 where Marek V commented to move > > > this in dsim instead of in Exynos. anyway, I will correct this. > > > > > >> return dev_err_probe(dev, PTR_ERR(dsi->te_gpio), > > >> "failed to get te GPIO\n"); > > >> > > >> te_gpio_irq = gpiod_to_irq(dsi->te_gpio); > > >> > > >> > > >> The error path in samsung_dsim_host_attach() after > > >> samsung_dsim_register_te_irq() failure is wrong. It lacks cleaning up > > >> the allocated resources (removing the bridge, detaing the dsi device). > > > This is because of the same discussion of moving TE GPIO in dsim instead exynos. > > > > Well, I'm not very happy about adding more and more features/changes to > > this single patch. I think that once we got the first version that was > > working on both Exynos and IMX, all other changes should be done > > incrementally to avoid this kind of issues... > > This is what I thought too, v10 was that ready to merge patchset. > Since it was delayed it is obvious to have more comments from other > developers in the community. This is what happened with the TE GPIO > change. > > > > > > > > How about register TE GPIO before calling host attach like this, > > > > > > --- a/drivers/gpu/drm/bridge/samsung-dsim.c > > > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c > > > @@ -1598,9 +1598,6 @@ static int samsung_dsim_host_attach(struct > > > mipi_dsi_host *host, > > > > > > drm_bridge_add(&dsi->bridge); > > > > > > - if (pdata->host_ops && pdata->host_ops->attach) > > > - pdata->host_ops->attach(dsi, device); > > > - > > > /* > > > * This is a temporary solution and should be made by more generic way. > > > * > > > @@ -1613,6 +1610,9 @@ static int samsung_dsim_host_attach(struct > > > mipi_dsi_host *host, > > > return ret; > > > } > > > > > > + if (pdata->host_ops && pdata->host_ops->attach) > > > + pdata->host_ops->attach(dsi, device); > > > + > > > dsi->lanes = device->lanes; > > > dsi->format = device->format; > > > dsi->mode_flags = device->mode_flags; > > > > > > Would you please check this? > > > > Yes. This fixes the error path. > > Okay. I'm sending v16 with the above two changes. Would you please test this v16 for confirmation before sending it to the mailing list? https://github.com/openedev/kernel/commits/imx8mm-dsi-v16 Thanks, Jagan.
