Hi Dave, Am 06.07.22 um 12:27 schrieb Dave Stevenson: > Hi Frieder. > > Apologies Lucas - I missed your response. > > On Wed, 6 Jul 2022 at 08:09, Frieder Schrempf > <frieder.schrempf@xxxxxxxxxx> wrote: >> >> Am 10.06.22 um 09:52 schrieb Lucas Stach: >>> Hi, >>> >>> Am Mittwoch, dem 11.05.2022 um 16:58 +0200 schrieb Marek Szyprowski: >>>> Hi Dave, >>>> >>>> On 05.04.2022 13:43, Dave Stevenson wrote: >>>>> On Fri, 18 Mar 2022 at 12:25, Dave Stevenson >>>>> <dave.stevenson@xxxxxxxxxxxxxxx> wrote: >>>>>> On Fri, 4 Mar 2022 at 15:18, Dave Stevenson >>>>>> <dave.stevenson@xxxxxxxxxxxxxxx> wrote: >>>>>>> Hi All >>>>>> A gentle ping on this series. Any comments on the approach? >>>>>> Thanks. >>>>> I realise the merge window has just closed and therefore folks have >>>>> been busy, but no responses on this after a month? >>>>> >>>>> Do I give up and submit a patch to document that DSI is broken and no one cares? >>>> >>>> Thanks for pointing this patchset in the 'drm: bridge: Add Samsung MIPI >>>> DSIM bridge' thread, otherwise I would miss it since I'm not involved >>>> much in the DRM development. >>>> >>>> This resolves most of the issues in the Exynos DSI and its recent >>>> conversion to the drm bridge framework. I've added the needed >>>> prepare_upstream_first flags to the panels and everything works fine >>>> without the bridge chain order hacks. >>>> >>>> Feel free to add: >>>> >>>> Tested-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx> >>>> >>>> >>>> The only remaining thing to resolve is the moment of enabling DSI host. >>>> The proper sequence is: >>>> >>>> 1. host power on, 2. device power on, 3. host init, 4. device init, 5. >>>> video enable. >>>> >>>> #1 is done in dsi's pre_enable, #2 is done in panel's prepare. #3 was so >>>> far done in the first host transfer call, which usually happens in >>>> panel's prepare, then the #4 happens. Then video enable is done in the >>>> enable callbacks. >>>> >>>> Jagan wants to move it to the dsi host pre_enable() to let it work with >>>> DSI bridges controlled over different interfaces >>>> (https://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fall%2F20220504114021.33265-6-jagan%40amarulasolutions.com%2F&data=05%7C01%7Cfrieder.schrempf%40kontron.de%7Ca82478efb8434ac07dfb08da5f3a1b75%7C8c9d3c973fd941c8a2b1646f3942daf1%7C0%7C0%7C637927000416444951%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=vgnJ8L7fhloUV83p%2Be6ziUKHbL9apqcLWpDvMUcOOoY%3D&reserved=0 >>>> ). This however fails on Exynos with DSI panels, because when dsi's >>>> pre_enable is called, the dsi device is not yet powered. I've discussed >>>> this with Andrzej Hajda and we came to the conclusion that this can be >>>> resolved by adding the init() callback to the struct mipi_dsi_host_ops. >>>> Then DSI client (next bridge or panel) would call it after powering self >>>> on, but before sending any DSI commands in its pre_enable/prepare functions. >>>> >>>> I've prepared a prototype of such solution. This approach finally >>>> resolved all the initialization issues! The bridge chain finally matches >>>> the hardware, no hack are needed, and everything is controlled by the >>>> DRM core. This prototype also includes the Jagan's patches, which add >>>> IMX support to Samsung DSIM. If one is interested, here is my git repo >>>> with all the PoC patches: >>>> >>>> https://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fmszyprow%2Flinux%2Ftree%2Fv5.18-next-20220511-dsi-rework&data=05%7C01%7Cfrieder.schrempf%40kontron.de%7Ca82478efb8434ac07dfb08da5f3a1b75%7C8c9d3c973fd941c8a2b1646f3942daf1%7C0%7C0%7C637927000416444951%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=ka2wikbVhc4RxFGARjTh0ixGKiaNHloW9dVkejA5kEg%3D&reserved=0 >>> >>> While this needs rework on the bridge chip side, I fear that we need >>> something like that to allow the bridge to control the sequencing of >>> the DSI host init. While most bridges that aren't controlled via the >>> DSI channel might be fine with just initializing the host right before >>> a video signal is driven, there are some that need a different >>> sequencing. >>> >>> The chip I'm currently looking at is a TC368767, where the datasheet >>> states that the DSI lanes must be in LP-11 before the reset is >>> released. While the datasheet doesn't specify what happens if that >>> sequence is violated, Marek Vasut found that the chip enters a test >>> mode if the lanes are not in LP-11 at that point and I can confirm this >>> observation. >> >> The SN65DSI84 also has this requirement according to the datasheet. > > SN65DSI8[3|4|5] need LP-11 before the chip comes out of reset, but > that only happens as part of enabling the bridge chain. This patch set > allows it to request the DSI host to be initialised first in order to > meet that constraint, which is common with many DSI sink devices. > > Lucas' comment with TC368767 is that it is doing other things for > display negotiation over the AUX channel prior to enabling the video > path, and that is needing the DSI interface to be enabled and in LP-11 > much earlier (and potentially clock lane in HS if not using an > external clock). Ok, got it know. I see this is a separate issue that is specific to devices that need the link for video mode negotiation early. Thanks for the explanation! > >>> Now with the TC358767 being a DSI to (e)DP converter, we need to >>> release the chip from reset pretty early to establish the DP AUX >>> connection to communicate with the display, in order to find out which >>> video modes we can drive. As the chip is controlled via i2c in my case, >>> initializing the DSI host on first DSI command transaction is out and >>> doing so before the bridge pre_enable is way too late. >>> >>> What I would need for this chip to work properly is an explicit call, >>> like the mipi_dsi_host_init() added in the PoC above, to allow the >>> bridge driver to kick the DSI host initialization before releasing the >>> chip from reset state. >> >> So to sum up on the missing parts: >> >> 1. This series needs more reviews, but is generally agreed on. >> 2. Jagan will integrate Marek's mipi_dsi_host_init() PoC when respinning >> his series "drm: bridge: Add Samsung MIPI DSIM bridge". > > I'm still not clear as to whether Marek's mipi_dsi_host_init is needed > in the majority of cases. > Exynos appeared to be trying to check for no contention as part of the > initialisation to LP-11, and that isn't necessary. No one has come > back on that one. > > Adding a mipi_dsi_host_init would potentially solve the additional > issue for TC358767. > However it leaves a number of open questions. The first I can think of > is that there are no defined mechanisms for specifying the link > frequency prior to having a video mode set. Yes struct mipi_dsi_device > has hs_rate, but that is defined as the maximum HS rate that the > device supports, and therefore open to variation in the > implementation. Exynos has various vendor specific DT parameters to > configure link frequencies, which ought to be standardised if that is > the preferred approach. Having standardized DT properties to configure the early link parameters could be a possible solution. > >> 3. Bridge drivers need to be adjusted to call mipi_dsi_host_init() if >> required. > > Which hopefully is only the weirder devices such as TC358767. > SN65DSI8[3|4|5] do not need this, but they will need > pre_enable_upstream_first if/when the enable logic is shifted from > atomic_enable to atomic_pre_enable. Got it, thanks for clarifying! Thanks Frieder
