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: <acfd12b7966710172aa92fdf5f19a05c@kernel.org>
Date:   Tue, 14 Nov 2023 09:52:33 +0100
From:   Michael Walle <mwalle@...nel.org>
To:     Alexander Stein <alexander.stein@...tq-group.com>
Cc:     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>,
        Frieder Schrempf <frieder.schrempf@...tron.de>,
        Tim Harvey <tharvey@...eworks.com>,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm: bridge: samsung-dsim: Don't use FORCE_STOP_STATE

Hi Alexander,

>> 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.
> 
> Apparently LP-11 is actually entered with the call to
> samsung_dsim_enable_lane(), but I don't know about other requisites on 
> that
> matter. Unfortunately documentation lacks a lot in that regard.

Which is called during samsung_dsim_atomic_pre_enable(). So I'm not sure
why the FORCE_STOP_STATE was needed at all. Lanes will be in LP-11 mode
if the video stream isn't enabled.

>> 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.
> 
> At which point does SN65DSI84 require LP-11?

I guess I have the same requirement as Frieder (we use the same bridge).
According to the datasheet, the DSI data must be in LP-11 when releasing
EN. According to the init sequence:
  - asserting EN
  - configure CSRs
  - enable video stream

Although not, stated explicitly, LP-11 should also be active during CSR
writes.

But after all, the DSIM driver should adhere to the linux requirement,
which Frieder cited [1].

> You have access to a DSI/D-PHY analyzer?

A pretty fast oscilloscope with an DSI decoder. So we can analyze one 
DSI
lane.

>> 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.
> 
> AFAIK ti-sn65dsi83.c only uses I2C for communication. Did you send DSI 
> read/
> writes instead?

Yeah the bridge only supports I2C. I just wanted to try that a DSI 
command
will still work after this patch. As a quick hack, I just added an
mipi_dsi_generic_write() to sn65dsi83_atomic_pre_enable() and made sure
there is a DSI write packet on the actual link.

-michael

> 
> best regards,
> Alexander
> 
>> 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>
>> ---
>> Let me know wether this should be two commits each reverting one, but 
>> both
>> commits appeared first in kernel 6.5.
>> 

[1] 
https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation