(EXT) Re: (EXT) [PATCH v2 00/12] drm: bridge: Add Samsung MIPI DSIM bridge

[Alexander Stein]

Hi Marek,

Am Freitag, 6. Mai 2022, 10:57:05 CEST schrieb Marek Szyprowski:
> Hi Alexander,
> 
> On 05.05.2022 13:55, Alexander Stein wrote:
> > Am Donnerstag, 5. Mai 2022, 09:38:48 CEST schrieb Jagan Teki:
> >> On Thu, May 5, 2022 at 12:57 PM Alexander Stein
> >> 
> >> <alexander.stein at ew.tq-group.com> wrote:
> >>> Hello Jagan,
> >>> 
> >>> thanks for the second version of this patchset.
> >>> 
> >>> Am Mittwoch, 4. Mai 2022, 13:40:09 CEST schrieb Jagan Teki:
> >>>> This series supports common bridge support for Samsung MIPI DSIM
> >>>> which is used in Exynos and i.MX8MM SoC's.
> >>>> 
> >>>> Previous v1 can be available here [1].
> >>>> 
> >>>> The final bridge supports both the Exynos and i.MX8MM DSI devices.
> >>>> 
> >>>> On, summary this patch-set break the entire DSIM driver into
> >>>> - platform specific glue code for platform ops, component_ops.
> >>>> - common bridge driver which handle platform glue init and invoke.
> >>>> 
> >>>> Patch 0000:   Samsung DSIM bridge
> >>>> 
> >>>> Patch 0001:   Common lookup code for OF-graph or child
> >>>> 
> >>>> Patch 0002:   platform init flag via driver_data
> >>>> 
> >>>> Patch 0003/10:  bridge fixes, atomic API's
> >>>> 
> >>>> Patch 0011:   document fsl,imx8mm-mipi-dsim
> >>>> 
> >>>> Patch 0012:   add i.MX8MM DSIM support
> >>>> 
> >>>> Tested in Engicam i.Core MX8M Mini SoM.
> >>>> 
> >>>> Anyone interested, please have a look on this repo [2]
> >>>> 
> >>>> [2]
> >>>> https://protect2.fireeye.com/v1/url?k=569d5207-09066afa-569cd948-000ba
> >>>> bff317b-7f7572918a36c54e&q=1&e=1305c5cc-33c8-467e-a498-6862a854cf94&u=h
> >>>> ttps%3A%2F%2Fgithub.com%2Fopenedev%2Fkernel%2Ftree%2Fimx8mm-dsi-v2 [1]
> >>>> https://patchwork.kernel.org/project/dri-devel/cover/20220408162108.184
> >>>> 5
> >>>> 83-> 1-jagan at amarulasolutions.com/
> >>>> 
> >>>> Any inputs?
> >>> 
> >>> I was able to get my LVDS display running using this driver and an LVDS
> >>> bridge. Actually my setup is similar to yours. My chain is like this:
> >>> MIPI-DSI -> sn65dsi83 -> LVDS panel
> >>> I noticed some things though:
> >>> My setup only works if I use less than 4 lanes. See [1]. When using 4
> >>> lanes
> >>> the image is flickering, but the content is "visible". Your DT has only
> >>> 2
> >>> lanes configured, do you have the possibility to use 4 lanes? I have no
> >>> idea how to tackle this. It might be the DSIM side or the bridge side.
> >>> Apparently the downstream kernel from NXP supports 4 lanes, if I can
> >>> trust
> >>> the config. I have no way to verify this though.
> >> 
> >> What is dsi_lvds_bridge node? have you added your dts changes on top
> >> of imx8mm-dsi-v2 branch I'm pointing it.
> >> 
> >> I will check 4 lanes and let you know.
> >> 
> >>> Another thing is I get the following warning
> >>> 
> >>>> sn65dsi83 2-002d: Unsupported LVDS bus format 0x100a, please check
> >>>> output
> >>> 
> >>> bridge driver. Falling back to SPWG24.
> >> 
> >> This couldn't be much affected but will fix it.
> > 
> > I found the cause. You need the following diff:
> > ----8<-----
> > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
> > b/drivers/gpu/drm/bridge/ samsung-dsim.c
> > index 138323dec0eb..7fb96dc7bb2e 100644
> > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > @@ -1427,7 +1427,7 @@ static int samsung_dsim_attach(struct drm_bridge
> > *bridge,
> > 
> >   {
> >   
> >          struct samsung_dsim *dsi = bridge_to_dsi(bridge);
> > 
> > -       return drm_bridge_attach(bridge->encoder, dsi->out_bridge, NULL,
> > flags);
> > +       return drm_bridge_attach(bridge->encoder, dsi->out_bridge, bridge,
> > flags);
> > 
> >   }
> >   
> >   static const struct drm_bridge_funcs samsung_dsim_bridge_funcs = {
> > 
> > ----8<-----
> 
> Well, basically, the above change breaks DSI panels. :(

That's too bad :( I wonder why actually this breaks DSI setups. From my 
understanding, the diff above seems correct, even for DSI panels.
But I don't know a DSI setup in detail or the bridge/panel code involved or 
which part breaks with this change.

> I've spent another evening playing with that code and I have some more
> thoughts...
> 
> I agree that logically this should be like you pointed. However the the
> code has been hacked in such a way, that it forces a proper order of
> pre-enable operations of the DSI and the client (panel, next bridge).
> This works somehow with a chain of 2 entities (Trats board: DSI and a
> panel) or even 3 entities (Arndale board: DSI, TC358764 bridge, panel),
> but probably it fails in your case.

Well, setting e.g. the bus format from panel -> bridge -> bridge ->... -> 
encoder seems sensible to me. It should be similar for both names setups as 
well. Essentially the Arndale is quite a similar setup to my and Jagan's one.
The actual reason it fails for me is that this list is created incorrectly, 
which should also be the case for Arndale.

> I really have no clue how to fix this mess. It has been pointed many
> times that this insane per-order call chain of the pre_enable()
> operations is completely useless for the DSI hardware and noone pointed
> how to solve this. Exynos DSI (and VC4) called those operations directly
> to achieve proper order. So what happened? Now Exynos DSI got converted
> to the generic bridge call chain. To get it working with existing hw,
> the order of the bridges has been hacked. Probably in the next few
> releases more mess will come to get around this known issue, especially
> when support for the next set of imx boards is added.
> 
> I'm really open to help fixing this issue. I've spent a lot of time
> analyzing this code and I have boards to test. Just please give me some
> advice how to avoid this reverse-order call chain of the pre_enable()
> operations in the widely accepted, non-hacky way.

In the first place I'm inclined to raise a warning in drm_bridge_attach() if 
previous is NULL and encoder->bridge_chain is not empty. This means that you 
are adding two "root"-bridges which seems wrong to me.

There is also some documentation regarding 'special care dsi' in drivers/gpu/
drm/drm_bridge.c. There is some distinction between a DSI host using 
components or not. But I have no knowledge about those components. 
That being said, I would assume that the Exynos conversion using a DRM bridge 
now might needs some additional changes.

Best regards,
Alexander