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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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...


> 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.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux