linux-kernel - Re: [PATCH] drm: bridge: samsung-dsim: Don't use FORCE_STOP

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <11fe6f0e-49e7-44d4-a31a-3b739f77489f@kontron.de>
Date:   Tue, 14 Nov 2023 15:29:33 +0100
From:   Frieder Schrempf <frieder.schrempf@...tron.de>
To:     Michael Walle <mwalle@...nel.org>, Inki Dae <inki.dae@...sung.com>,
        Jagan Teki <jagan@...rulasolutions.com>,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        Andrzej Hajda <andrzej.hajda@...el.com>,
        Neil Armstrong <neil.armstrong@...aro.org>,
        Robert Foss <rfoss@...nel.org>,
        Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
        Jonas Karlman <jonas@...boo.se>,
        Jernej Skrabec <jernej.skrabec@...il.com>,
        David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>,
        Tim Harvey <tharvey@...eworks.com>,
        Alexander Stein <alexander.stein@...tq-group.com>
Cc:     dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm: bridge: samsung-dsim: Don't use FORCE_STOP_STATE

Hi Michael,

On 13.11.23 17:43, Michael Walle wrote:
> The FORCE_STOP_STATE bit is unsuitable to force the DSI link into LP-11
> mode. It seems the bridge internally queues DSI packets and when the
> FORCE_STOP_STATE bit is cleared, they are sent in close succession
> without any useful timing (this also means that the DSI lanes won't go
> into LP-11 mode). The length of this gibberish varies between 1ms and
> 5ms. This sometimes breaks an attached bridge (TI SN65DSI84 in this
> case). In our case, the bridge will fail in about 1 per 500 reboots.
> 
> The FORCE_STOP_STATE handling was introduced to have the DSI lanes in
> LP-11 state during the .pre_enable phase. But as it turns out, none of
> this is needed at all. Between samsung_dsim_init() and
> samsung_dsim_set_display_enable() the lanes are already in LP-11 mode.
> The code as it was before commit 20c827683de0 ("drm: bridge:
> samsung-dsim: Fix init during host transfer") and 0c14d3130654 ("drm:
> bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec") was correct
> in this regard.
> 
> This patch basically reverts both commits. It was tested on an i.MX8M
> SoC with an SN65DSI84 bridge. The signals were probed and the DSI
> packets were decoded during initialization and link start-up. After this
> patch the first DSI packet on the link is a VSYNC packet and the timing
> is correct.
> 
> Command mode between .pre_enable and .enable was also briefly tested by
> a quick hack. There was no DSI link partner which would have responded,
> but it was made sure the DSI packet was send on the link. As a side
> note, the command mode seems to just work in HS mode. I couldn't find
> that the bridge will handle commands in LP mode.
> 
> Fixes: 20c827683de0 ("drm: bridge: samsung-dsim: Fix init during host transfer")
> Fixes: 0c14d3130654 ("drm: bridge: samsung-dsim: Fix i.MX8M enable flow to meet spec")
> Signed-off-by: Michael Walle <mwalle@...nel.org>

Thanks for the fix. Your explanation sounds convincing.

Unfortunately I'm currently not able to understand why I had to
introduce these changes in the first place. What I tried to fix was
exactly this kind of issue where the display stays black every few
hundred boot cycles.

My current guess would be that the issue I was seeing was already fixed
with dd9e329af723 ("drm/bridge: ti-sn65dsi83: Fix enable/disable flow to
meet spec") and I didn't properly test both changes separately.

My cheap scope is not able to capture the DSI signals and I admit that
we didn't use our more expensive equipment to verify the changes back then.

Instead, we had an automated test setup to do cyclic on/off switching
for the display and check for a black screen using a sensor. It is quite
a hassle to set up and I'm currently not planning to spend that much
effort to verify this change again.

Anyway, I currently don't see any reasons to not revert my changes. Your
revert looks correct and seems to work fine as far as I can tell.

Reviewed-by: Frieder Schrempf <frieder.schrempf@...tron.de>

Thanks
Frieder