Re: [PATCH v8 06/14] drm: bridge: samsung-dsim: Handle proper DSI host initialization

[Jagan Teki <jagan@amarulasolutions.com>]

On Tue, Dec 6, 2022 at 2:32 PM Frieder Schrempf
<frieder.schrempf@kontron.de> wrote:
>
> On 05.12.22 16:37, Frieder Schrempf wrote:
> > Hi Dave,
> >
> > On 05.12.22 16:20, Dave Stevenson wrote:
> >> Hi Frieder
> >>
> >> On Mon, 5 Dec 2022 at 07:30, Frieder Schrempf
> >> <frieder.schrempf@kontron.de> wrote:
> >>>
> >>> On 02.12.22 15:55, Dave Stevenson wrote:
> >>>> Hi Marek
> >>>>
> >>>> On Fri, 2 Dec 2022 at 12:21, Marek Vasut <marex@denx.de> wrote:
> >>>>>
> >>>>> On 12/2/22 11:52, Marek Szyprowski wrote:
> >>>>>> Hi,
> >>>>>>
> >>>>>> Sorry for delay, I was on a sick leave last 2 weeks.
> >>>>>>
> >>>>>> On 28.11.2022 15:43, Jagan Teki wrote:
> >>>>>>> ,On Sat, Nov 26, 2022 at 3:44 AM Marek Vasut <marex@denx.de> wrote:
> >>>>>>>> On 11/23/22 21:09, Jagan Teki wrote:
> >>>>>>>>> On Sat, Nov 19, 2022 at 7:45 PM Marek Vasut <marex@denx.de> wrote:
> >>>>>>>>>> On 11/17/22 14:04, Marek Szyprowski wrote:
> >>>>>>>>>>> On 17.11.2022 05:58, Marek Vasut wrote:
> >>>>>>>>>>>> On 11/10/22 19:38, Jagan Teki wrote:
> >>>>>>>>>>>>> DSI host initialization handling in previous exynos dsi driver has
> >>>>>>>>>>>>> some pitfalls. It initializes the host during host transfer() hook
> >>>>>>>>>>>>> that is indeed not the desired call flow for I2C and any other DSI
> >>>>>>>>>>>>> configured downstream bridges.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Host transfer() is usually triggered for downstream DSI panels or
> >>>>>>>>>>>>> bridges and I2C-configured-DSI bridges miss these host initialization
> >>>>>>>>>>>>> as these downstream bridges use bridge operations hooks like pre_enable,
> >>>>>>>>>>>>> and enable in order to initialize or set up the host.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> This patch is trying to handle the host init handler to satisfy all
> >>>>>>>>>>>>> downstream panels and bridges. Added the DSIM_STATE_REINITIALIZED state
> >>>>>>>>>>>>> flag to ensure that host init is also done on first cmd transfer, this
> >>>>>>>>>>>>> helps existing DSI panels work on exynos platform (form Marek
> >>>>>>>>>>>>> Szyprowski).
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> v8, v7, v6, v5:
> >>>>>>>>>>>>> * none
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> v4:
> >>>>>>>>>>>>> * update init handling to ensure host init done on first cmd transfer
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> v3:
> >>>>>>>>>>>>> * none
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> v2:
> >>>>>>>>>>>>> * check initialized state in samsung_dsim_init
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> v1:
> >>>>>>>>>>>>> * keep DSI init in host transfer
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> >>>>>>>>>>>>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> >>>>>>>>>>>>> ---
> >>>>>>>>>>>>>       drivers/gpu/drm/bridge/samsung-dsim.c | 25 +++++++++++++++++--------
> >>>>>>>>>>>>>       include/drm/bridge/samsung-dsim.h     |  5 +++--
> >>>>>>>>>>>>>       2 files changed, 20 insertions(+), 10 deletions(-)
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c
> >>>>>>>>>>>>> b/drivers/gpu/drm/bridge/samsung-dsim.c
> >>>>>>>>>>>>> index bb1f45fd5a88..ec7e01ae02ea 100644
> >>>>>>>>>>>>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> >>>>>>>>>>>>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> >>>>>>>>>>>>> @@ -1234,12 +1234,17 @@ static void samsung_dsim_disable_irq(struct
> >>>>>>>>>>>>> samsung_dsim *dsi)
> >>>>>>>>>>>>>           disable_irq(dsi->irq);
> >>>>>>>>>>>>>       }
> >>>>>>>>>>>>>       -static int samsung_dsim_init(struct samsung_dsim *dsi)
> >>>>>>>>>>>>> +static int samsung_dsim_init(struct samsung_dsim *dsi, unsigned int
> >>>>>>>>>>>>> flag)
> >>>>>>>>>>>>>       {
> >>>>>>>>>>>>>           const struct samsung_dsim_driver_data *driver_data =
> >>>>>>>>>>>>> dsi->driver_data;
> >>>>>>>>>>>>>       +    if (dsi->state & flag)
> >>>>>>>>>>>>> +        return 0;
> >>>>>>>>>>>>> +
> >>>>>>>>>>>>>           samsung_dsim_reset(dsi);
> >>>>>>>>>>>>> -    samsung_dsim_enable_irq(dsi);
> >>>>>>>>>>>>> +
> >>>>>>>>>>>>> +    if (!(dsi->state & DSIM_STATE_INITIALIZED))
> >>>>>>>>>>>>> +        samsung_dsim_enable_irq(dsi);
> >>>>>>>>>>>>>             if (driver_data->reg_values[RESET_TYPE] == DSIM_FUNCRST)
> >>>>>>>>>>>>>               samsung_dsim_enable_lane(dsi, BIT(dsi->lanes) - 1);
> >>>>>>>>>>>>> @@ -1250,6 +1255,8 @@ static int samsung_dsim_init(struct
> >>>>>>>>>>>>> samsung_dsim *dsi)
> >>>>>>>>>>>>>           samsung_dsim_set_phy_ctrl(dsi);
> >>>>>>>>>>>>>           samsung_dsim_init_link(dsi);
> >>>>>>>>>>>>>       +    dsi->state |= flag;
> >>>>>>>>>>>>> +
> >>>>>>>>>>>>>           return 0;
> >>>>>>>>>>>>>       }
> >>>>>>>>>>>>>       @@ -1269,6 +1276,10 @@ static void
> >>>>>>>>>>>>> samsung_dsim_atomic_pre_enable(struct drm_bridge *bridge,
> >>>>>>>>>>>>>           }
> >>>>>>>>>>>>>             dsi->state |= DSIM_STATE_ENABLED;
> >>>>>>>>>>>>> +
> >>>>>>>>>>>>> +    ret = samsung_dsim_init(dsi, DSIM_STATE_INITIALIZED);
> >>>>>>>>>>>>> +    if (ret)
> >>>>>>>>>>>>> +        return;
> >>>>>>>>>>>>>       }
> >>>>>>>>>>>>>         static void samsung_dsim_atomic_enable(struct drm_bridge *bridge,
> >>>>>>>>>>>>> @@ -1458,12 +1469,9 @@ static ssize_t
> >>>>>>>>>>>>> samsung_dsim_host_transfer(struct mipi_dsi_host *host,
> >>>>>>>>>>>>>           if (!(dsi->state & DSIM_STATE_ENABLED))
> >>>>>>>>>>>>>               return -EINVAL;
> >>>>>>>>>>>>>       -    if (!(dsi->state & DSIM_STATE_INITIALIZED)) {
> >>>>>>>>>>>>> -        ret = samsung_dsim_init(dsi);
> >>>>>>>>>>>>> -        if (ret)
> >>>>>>>>>>>>> -            return ret;
> >>>>>>>>>>>>> -        dsi->state |= DSIM_STATE_INITIALIZED;
> >>>>>>>>>>>>> -    }
> >>>>>>>>>>>>> +    ret = samsung_dsim_init(dsi, DSIM_STATE_REINITIALIZED);
> >>>>>>>>>>>> This triggers full controller reset and reprogramming upon first
> >>>>>>>>>>>> command transfer, is such heavy handed reload really necessary ?
> >>>>>>>>>>> Yes it is, otherwise the proper DSI panels doesn't work with Exynos DRM
> >>>>>>>>>>> DSI. If this is a real issue for you, then maybe the driver could do the
> >>>>>>>>>>> initialization conditionally, in prepare() callback in case of IMX and
> >>>>>>>>>>> on the first transfer in case of Exynos?
> >>>>>>>>>> That's odd , it does actually break panel support for me, without this
> >>>>>>>>>> double reset the panel works again. But I have to wonder, why would such
> >>>>>>>>>> a full reset be necessary at all , even on the exynos ?
> >>>>>>>>> Is it breaking samsung_dsim_reset from host_transfer? maybe checking
> >>>>>>>>> whether a reset is required before calling it might fix the issue.  I
> >>>>>>>>> agree with double initialization is odd but it seems it is required on
> >>>>>>>>> some panels in Exynos, I think tweaking them and adjusting the panel
> >>>>>>>>> code might resolve this discrepancy.
> >>>>>>>> Can someone provide further details on the exynos problem ?
> >>>>>>> If I'm correct this sequence is required in order to work the existing
> >>>>>>> panel/bridges on exynos. Adjusting these panel/bridge codes can
> >>>>>>> possibly fix the sequence further.
> >>>>>>>
> >>>>>>> Marek Szyprowski, please add if you have anything.
> >>>>>>
> >>>>>>
> >>>>>> Well, frankly speaking the double initialization is not a correct
> >>>>>> sequence, but this is the only one that actually works on Exynos based
> >>>>>> boards with DSI panels after moving the initialization to bridge's
> >>>>>> .prepare() callback.
> >>>>>
> >>>>> Somehow, I suspect this is related to the LP11 mode handling, which
> >>>>> differs for different panels, right ? I think the RPi people worked on
> >>>>> fixing that.
> >>>>>
> >>>>> +CC Dave
> >>>>
> >>>> Yes. I've just sent out a v3 of that patch set.
> >>>>
> >>>> Hopefully setting the pre_enable_prev_first flag on your peripheral's
> >>>> drm_bridge, or prepare_prev_first if a drm_panel, will result in a
> >>>> more sensible initialisation order for your panel.
> >>>>
> >>>> Note that host_transfer should ensure that the host is initialised, as
> >>>> it is valid to call it with the host in any state. If it has to
> >>>> initialise, then it should deinitialise once the transfer has
> >>>> completed.
> >>>>
> >>>> Dave
> >>>>
> >>>>>> I've already explained this and shared the results
> >>>>>> of my investigation in my replies to the previous versions of this
> >>>>>> patchset. The original Exynos DSI driver does the initialization on the
> >>>>>> first DSI command. This however doesn't work for Jagan with I2C
> >>>>>> controlled panels/bridges, so he moved the initialization to the
> >>>>>> .prepare() callback, what broke the Exynos case (in-short - all tested
> >>>>>> panels works fine only if the DSI host initialization is done AFTER
> >>>>>> turning the panel's power on). For more information, see this thread:
> >>>>>> https://eur04.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fall%2Fe96197f9-948a-997e-5453-9f9d179b5f5a%40samsung.com%2F&amp;data=05%7C01%7Cfrieder.schrempf%40kontron.de%7Cee7b57ee420e45a73b1d08dad6d45306%7C8c9d3c973fd941c8a2b1646f3942daf1%7C0%7C0%7C638058504671330145%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=TQIIKNa4OVGP1dZo3tM%2FOMO3dlXrjLr04U%2FJFhd2rAs%3D&amp;reserved=0
> >>>>>>
> >>>>>> Now, the more I think of it, the more I'm convinced that we simply
> >>>>>> should add a hack based on the HW type: do the initialization in
> >>>>>> .prepare() for non-Exynos case and before the first transfer for the
> >>>>>> Exynos case, as there is no way to detect the panel/next bridge type
> >>>>>> (I2C or DSI controlled).
> >>>>>
> >>>>> Let's see what Dave has to say about this, maybe there is some further help.
> >>>
> >>> Could we agree on adding the HW type based hack Marek S. proposed as a
> >>> quick fix?
> >>>
> >>> This patchset was tested on Exynos so it's likely to not break any
> >>> existing setups. And for i.MX8, this is a new driver so there's not
> >>> really a requirement to have all setups working/supported from the
> >>> beginning.
> >>>
> >>> Also having one or two hacks (marked with FIXME) in the code doesn't
> >>> hurt. As we can see there are drafts to fix them in conjunction with
> >>> changes in the DRM framework.
> >>>
> >>> This has been pending for months and in my opinion if there's a chance
> >>> to get this into v6.2-rc1 we should take it.
> >>
> >> My patchset was sent in March with no one seeming to care enough to review it.
> >
> > I wasn't referring to your patchset but rather to the Samsung DSIM
> > bridge transformation patchset.
> >
> > My point was simply to not try getting everything done in one big step
> > because this will fail. The patchset this refers to needs testing on two
> > separate platforms which is painful enough (thanks to Marek for covering
> > the Exynos side!). I think we should focus on getting the DSIM bridge
> > transformation merged and accept a few small hacks that will be taken
> > care of in the next step.
> >
> >>
> >> If the situation is that your devices fall into the same camp as those
> >> for vc4 (the host needs to be initialised before the peripheral), at
> >> least verifying that would be useful before rushing into a hack.
> >>
> >> Your other comment references using a TI SN65DSI84. I know for certain
> >> that falls into the category of needing the DSI bus initialised before
> >> it is brought out of reset.
> >
> > I'm actually working on this right now and when I received your message
> > I was about to start typing a reply to your patchset.
> >
> > The SN65DSI84 works with the i.MX8MM DSIM even using the default order
> > of host init after peripheral init in our setup, but this configuration
> > doesn't seem to be stable and occasionally the bridge doesn't come up
> > properly.
> >
> > We are still in the process of verifying if the reversed order fixes
> > this reliably. But regardless of the results, without the reversal the
> > initialization sequence is way out of spec and we need to fix this in
> > any case.
> >
> > See here for my testing branch including some follow-up patches that
> > improve the initialization flow for my setup:
> > https://git.kontron-electronics.de/sw/misc/linux/-/commits/v6.1-dsim-mx8mm.
>
> To recap my thoughts on the two hacks for the DSIM bridge driver
> discussed before:
>
> (1) Passing null to previous bridge in samsung_dsim_attach()
> (2) Always initialize host on first transfer (see this patch, 06/14)
>
> My wild guess would be that both could be fixed up properly in the long
> run by the following changes:
>
> * Apply Dave's patchset [1]
> * Set pre_enable_prev_first flag in the downstream bridge drivers and
>   fix init flow if required ([2] for ti-sn65dsi83)
> * Fix DSIM init to keep data lanes in LP11 until enable() is called [3]
> * Only call init on transfer when not already initialized and deinit
>   after transfer (tbd)
>
> As that route needs proper testing on the affected hardware setups and
> includes changes to other drivers and the framework, I would suggest the
> following for the v9 of this patchset:

I did it on drm-misc-next [1].

>
> * Keep hack (1)

This has gone, not needed.

> * Make hack (2) dependent on the platform (Exynos)

I think with pre_enable_prev_first flag the Exynos pipeline will start
from bridge funcs like pre_enable so initializing the host will work
for exynos to work. [2]

Marek Sz. Can you confirm this?

[1] https://gitlab.com/openedev/kernel/-/commits/imx8mm-dsi-v9
[2] https://gitlab.com/openedev/kernel/-/commit/95ab71b797310952284fabfbc8476a9831902c5c

Jagan.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel