Re: [PATCH v4 02/12] drm: bridge: Add Samsung DSIM bridge driver

[Marek Szyprowski <m.szyprowski@samsung.com>]

Hi Jagan,

On 16.09.2022 12:21, Jagan Teki wrote:
> On Fri, Sep 16, 2022 at 1:58 PM Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
>> On 14.09.2022 11:39, Jagan Teki wrote:
>>> On Wed, Sep 14, 2022 at 2:51 PM Marek Szyprowski
>>> <m.szyprowski@samsung.com> wrote:
>>>> On 13.09.2022 19:29, Jagan Teki wrote:
>>>>> On Wed, Sep 7, 2022 at 3:34 PM Marek Szyprowski
>>>>> <m.szyprowski@samsung.com> wrote:
>>>>>> On 06.09.2022 21:07, Jagan Teki wrote:
>>>>>>> On Mon, Sep 5, 2022 at 4:54 PM Marek Szyprowski
>>>>>>> <m.szyprowski@samsung.com> wrote:
>>>>>>>> On 02.09.2022 12:47, Marek Szyprowski wrote:
>>>>>>>>> On 29.08.2022 20:40, Jagan Teki wrote:
>>>>>>>>>> Samsung MIPI DSIM controller is common DSI IP that can be used in
>>>>>>>>>> various
>>>>>>>>>> SoCs like Exynos, i.MX8M Mini/Nano.
>>>>>>>>>>
>>>>>>>>>> In order to access this DSI controller between various platform SoCs,
>>>>>>>>>> the ideal way to incorporate this in the drm stack is via the drm bridge
>>>>>>>>>> driver.
>>>>>>>>>>
>>>>>>>>>> This patch is trying to differentiate platform-specific and bridge
>>>>>>>>>> driver
>>>>>>>>>> code and keep maintaining the exynos_drm_dsi.c code as platform-specific
>>>>>>>>>> glue code and samsung-dsim.c as a common bridge driver code.
>>>>>>>>>>
>>>>>>>>>> - Exynos specific glue code is exynos specific te_irq, host_attach, and
>>>>>>>>>>        detach code along with conventional component_ops.
>>>>>>>>>>
>>>>>>>>>> - Samsung DSIM is a bridge driver which is common across all
>>>>>>>>>> platforms and
>>>>>>>>>>        the respective platform-specific glue will initialize at the end
>>>>>>>>>> of the
>>>>>>>>>>        probe. The platform-specific operations and other glue calls will
>>>>>>>>>> invoke
>>>>>>>>>>        on associate code areas.
>>>>>>>>>>
>>>>>>>>>> v4:
>>>>>>>>>> * include Inki Dae in MAINTAINERS
>>>>>>>>>> * remove dsi_driver probe in exynos_drm_drv to support multi-arch build
>>>>>>>>> This breaks Exynos DRM completely as the Exynos DRM driver is not able
>>>>>>>>> to wait until the DSI driver is probed and registered as component.
>>>>>>>>>
>>>>>>>>> I will show how to rework this the way it is done in
>>>>>>>>> drivers/gpu/drm/exynos/exynos_dp.c and
>>>>>>>>> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c soon...
>>>>>>>> I've finally had some time to implement such approach, see
>>>>>>>> https://protect2.fireeye.com/v1/url?k=c5d024d9-a4ab8e4e-c5d1af96-74fe4860001d-625a8324a9797375&q=1&e=489b94d4-84fb-408e-b679-a8d27acf2930&u=https%3A%2F%2Fgithub.com%2Fmszyprow%2Flinux%2Ftree%2Fv6.0-dsi-v4-reworked
>>>>>>>>
>>>>>>>> If you want me to send the patches against your v4 patchset, let me
>>>>>>>> know, but imho my changes are much more readable after squashing to the
>>>>>>>> original patches.
>>>>>>>>
>>>>>>>> Now the driver is fully multi-arch safe and ready for further
>>>>>>>> extensions. I've removed the weak functions, reworked the way the
>>>>>>>> plat_data is used (dropped the patch related to it) and restored
>>>>>>>> exynos-dsi driver as a part of the Exynos DRM drivers/subsystem. Feel
>>>>>>>> free to resend the above as v5 after testing on your hardware. At least
>>>>>>>> it properly works now on all Exynos boards I have, both compiled into
>>>>>>>> the kernel or as modules.
>>>>>>> Thanks. I've seen the repo added on top of Dave patches - does it mean
>>>>>>> these depends on Dave changes as well?
>>>>>> Yes and no. My rework doesn't change anything with this dependency. It
>>>>>> comes from my patch "drm: exynos: dsi: Restore proper bridge chain
>>>>>> order" already included in your series (patch #1). Without it exynos-dsi
>>>>>> driver hacks the list of bridges to ensure the order of pre_enable calls
>>>>>> needed for proper operation. This works somehow with DSI panels on my
>>>>>> test systems, but it has been reported that it doesn't work with a bit
>>>>>> more complex display pipelines. Only that patch depends on the Dave's
>>>>>> patches. If you remove it, you would need to adjust the code in the
>>>>>> exynos_drm_dsi.c and samsung-dsim.c respectively. imho it would be
>>>>>> better to keep it and merge Dave's patches together with dsi changes, as
>>>>>> they are the first real client of it.
>>>>> I think the Dave patches especially "drm/bridge: Introduce
>>>>> pre_enable_upstream_first to alter bridge init order" seems not 100%
>>>>> relevant to this series as they affect bridge chain call flow
>>>>> globally. Having a separate series for that makes sense to me. I'm
>>>>> sending v5 by excluding those parts.
>>>> If so then drop the "drm: exynos: dsi: Restore proper bridge chain
>>>> order" patch and adjust code respectively in samsung-dsim.c. Without the
>>>> Dave's patches, that one doesn't make sense.
>>> Doesn't it break Exynos?
>> No it won't. Lack of the "drm: exynos: dsi: Restore proper bridge chain
>> order" patch doesn't change much against the current state of the driver.
>>
>> Here is my rework of your v4 patchset without the mentioned patch and
>> Dave's patches:
>>
>> https://protect2.fireeye.com/v1/url?k=6282936d-3d19aa74-62831822-000babff3793-9317af6e2b207460&q=1&e=cd36cc51-8faa-4812-880a-8242739a86bd&u=https%3A%2F%2Fgithub.com%2Fmszyprow%2Flinux%2Ftree%2Fv6.0-dsi-v4-reworked-minimal
> We have one problem with getting bus format from previous bridge if we
> pass NULL in bridge_func.attach()
> https://protect2.fireeye.com/v1/url?k=bdc3e18a-e258d893-bdc26ac5-000babff3793-1dd81e5acf9d7f83&q=1&e=cd36cc51-8faa-4812-880a-8242739a86bd&u=https%3A%2F%2Fgithub.com%2Fmszyprow%2Flinux%2Fcommit%2F0fa57e33b3bf866efc4c17ab20eec28d6e07b3e9%23diff-3fe873f1ada5f1dfcf2a50ac114bdab3ea7b026d12278648ca40809d3fa1a331R1321
>
> Booting the video as it assigns default bus format if the previous bus
> format is unknown.
>
> [    1.635984] samsung-dsim 32e10000.dsi:
> [drm:samsung_dsim_host_attach] Attached sn65dsi83 device
> [    1.648067] [drm] Initialized mxsfb-drm 1.0.0 20160824 for
> 32e00000.lcdif on minor 0
> [    1.658726] mmc0: SDHCI controller on 30b40000.mmc [30b40000.mmc]
> using ADMA
> [    1.681893] sn65dsi83 3-002c: Unsupported LVDS bus format 0x100a,
> please check output bridge driver. Falling back to SPWG24.
>
> Does passing the bridge to drm_bridge_attach is working on your platform?
> return drm_bridge_attach(bridge->encoder, dsi->out_bridge, bridge, flags);

Nope, it fails:

[drm] Initialized exynos 1.1.0 20180330 for exynos-drm on minor 0
exynos-dsi 11c80000.dsi: [drm:samsung_dsim_host_attach] Attached s6e8aa0 
device
panel-samsung-s6e8aa0 11c80000.dsi.0: error -22 setting maximum return 
packet size to 3
panel-samsung-s6e8aa0 11c80000.dsi.0: read id failed

I've already pointed that it makes sense only together with Dave's patches.

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