[PATCH v15 00/16] drm: Add Samsung MIPI DSIM bridge

Tue Mar 7 07:55:12 UTC 2023

Hi Marek,

On Tue, Mar 7, 2023 at 4:11 AM Marek Szyprowski
<m.szyprowski at samsung.com> 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 at samsung.com> wrote:
> >> On 04.03.2023 19:59, Jagan Teki wrote:
> >>> On Sat, Mar 4, 2023 at 3:56 AM Marek Szyprowski
> >>> <m.szyprowski at samsung.com> 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.

Thanks,
Jagan.