[PATCH v2 1/2] drm: bridge: samsung-dsim: Initialize bridge on attach

[Marek Vasut]

On 7/16/24 9:50 PM, Michael Walle wrote:
>>>>>> Thank you for testing and keeping up with this. I will wait for more
>>>>>> feedback if there is any (Frieder? Lucas? Michael?). If there are no
>>>>>> objections, then I can merge it in a week or two ?
>>>>>
>>>>> I'll try to use your approach on the tc358775. Hopefully, I'll find
>>>>> some time this week.
>>>>
>>>> So ... I wonder ... shall I apply these patches or not ?
>>>
>>> As mentioned on IRC, I tried it to port it for the mediatek DSI
>>> host, but I gave up and got doubts that this is the way to go. I
>>> think this is too invasive (in a sense that it changes behavior)
>>
>> I would argue it makes the behavior well defined. If that breaks some
>> drivers that depended on the undefined behavior before, those should be
>> fixed too.
> 
> Then this behavior should be documented (and accepted) in the
> corresponding section:
> https://docs.kernel.org/gpu/drm-kms-helpers.html#mipi-dsi-bridge-operation

I think so too.

> This will help DSI host driver developers and we can point all the
> host DSI driver maintainers to that document along with the proper
> implementation :)
> 
>>> and not that easy to implement on other drivers.
>>
>> How so ? At least the DSIM and STM32 DW DSI host can switch lanes to
>> LP11 state. Is the mediatek host not capable of that ?
> 
> The controller is certainly capable to do that. But the changes
> seems pretty invasive to me and I fear some fallout. Although it's
> basically just one line for the DSIM, you seem to be moving the init
> of the DSIM to an earlier point(?). I'm no expert with all the DRM
> stuff, so I might be wrong here.

I am moving some of the initialization to an earlier point, but only 
enough of it to configure the lanes to LP11 state before the next 
bridge(s) start to (pre)enable themselves. And the DSIM controller is 
RPM suspended again after the lanes are in LP11.

>>> Given that this requirement is far more common across DSI bridges,
>>> I'd favor a more general solution which isn't a workaround.
>>
>> I think we only had a look at the TI DSI83 / ICN6211 / Toshiba TC358767
>> bridges, but we did not look at many panels, did we ? Do panels require
>> lanes in non-LP11 state on start up ?
> 
> I'm not talking about panels, just bridges. It's not just one bridge
> with a weird behavior but most bridges.

What do you refer to by "weird behavior" ? It seems the DSI bridges we 
looked at all required data lanes in LP11 state on start up one way or 
the other, didn't they ?

>> Was there any progress on the generic LP11 solution, I think you did
>> mention something was in progress ? How would that even look like ?
> 
> I had a new callback implemented:
> https://lore.kernel.org/r/20240506-tc358775-fix-powerup-v1-1-545dcf00b8dd@kernel.org/
> 
> Not sure if that's any better though.

Wouldn't that callback be called by various controllers at various 
stages of initialization , without consistency on when that callback 
will be called ? That would be my concern.

At least here, the expectation is that the controller would put data 
lanes into LP11 before the next bridge can even pre_enable itself , 
which is not perfect though, because if a bridge needs DSI clock to 
probe() itself and then data lanes in LP11 to probe itself, that is a 
really bad situation (and the TC358767 is capable of being used that 
way, although this is currently not supported by the kernel driver and 
there is no real interest in supporting it).