[RFC] drm/bridge: adv7533: remove dynamic lane switching from adv7533 bridge

[Dmitry Baryshkov]

On 26/08/2022 14:55, Laurent Pinchart wrote:
> Hello,
> 
> On Fri, Aug 26, 2022 at 01:17:43PM +0300, Dmitry Baryshkov wrote:
>> On 22/08/2022 19:31, Dmitry Baryshkov wrote:
>>> On 09/08/2022 22:40, Laurent Pinchart wrote:
>>>> On Mon, Aug 08, 2022 at 05:35:30PM -0700, Abhinav Kumar wrote:
>>>>> adv7533 bridge tries to dynamically switch lanes based on the
>>>>> mode by detaching and attaching the mipi dsi device.
>>>>>
>>>>> This approach is incorrect because as per the DSI spec the
>>>>> number of lanes is fixed at the time of system design or initial
>>>>> configuration and may not change dynamically.
>>>>
>>>> Is that really so ? The number of lanes connected on the board is
>>>> certainlyset at design time, but a lower number of lanes can be used at
>>>> runtime. It shouldn't change dynamically while the display is on, but it
>>>> could change at mode set time.
>>>
>>> I'm not sure if I interpreted the standard correctly, but I tended to
>>> have the same interpretation as Abhinav did.
>>>
>>> Anyway, even if we allow the drivers to switch the amount of lanes, this
>>> should not happen in the form of detach()/attach() cycle. The drivers
> 
> Agreed.
> 
>>> use mipi_dsi_attach() as way to signal the DSI hosts that the sink
>>> device is ready. Some of DSI hosts (including MSM one) would bind
>>> components from the attach callback.
>>>
>>> If we were to support dynamically changing the amount of lanes, I would
>>> ask for additional mipi_dsi API call telling the host to switch the
>>> amount of lanes. And note that this can open the can of worms. Different
>>> hosts might have different requirements here. For example for the MSM
>>> platform the amount of lanes is programmed during bridge_pre_enable
>>> chain call, so it is possible to just set the amount of lanes following
>>> the msm_dsi_device::lanes. Other hosts might have other requirements.
> 
> Conceptually, I would expect the number of effective lanes to be
> selected at mode set time, because it has to be done while the output is
> disabled.

There is one tightly coupled question. The dual-DSI (or bonded-DSI) 
mode. Currently it is exposed as two independent DSI hosts. If we allow 
changing the amount of DSI lanes at runtime, bonded DSI mode would 
become complicated by fixing amount of lanes for each of outputs (or 
switching them in tight loop). And then comes the question of switching 
between single-DSI and bonded-DSI at runtime.

> With the atomic API for bridges the .mode_set() operation is
> deprecated, so .pre_enable() sounds best, but there's a potential issue:
> the .pre_enable() operation is called from sink to source. It means that
> a DSI sink .pre_enable() operation would need to call a DSI host
> operation to set (or maybe negotiate instead of just setting a fixed
> value) the number of lanes first if it wants to control the sink through
> DCS at .pre_enable() time. We'd have to see how that fits.

What is the fate of the patchset that implemented 'parent-first' opt-in 
for the drm_bridge chains? It was supposed to solve this this kind of 
issues. I'm asking because until it is merged some DSI hosts (e.g. the 
msm dsi) turn on the power in .mode_set() to allow the pre_enable() 
callbacks work when the DSI link is in LP11 mode.

Back then I voted for the explicit mipi_dsi_power_on kind of calls, 
which would allow the sink bridge to prepare for the DSI powerup (e.g. 
by setting the amount of lanes), power up the DSI host, putting the link 
into LP11 and after that communicate with the sink using the DSI data 
lanes.

> 
>>> Thus said I'd suggest accepting this patch and maybe working on the
>>> API/support for the lane switching as a followup.
> 
> I don't have a personal need for the ADV7533 so I won't really complain
> about any potential regression this may introduce (given that it fixes a
> deadlock maybe things are completely broken already and nothing can
> regress). I'd like to see this fixed though, doing it as a follow up is
> too often a way to avoid doing it at all :-)

I don't know if this sounds like a promise, we are supporting several 
devices which use adv75xx (including famous dragonboard410c and less 
known Inforce ifc6510). So it might be (*) in our interest to restore 
this functionality. However as it requires adding additional API, design 
& negotiations might take some time.

(*) might be if it limits the functionality of the device by limiting 
support for different modes. If not... why care then?


> In any case, the commit message should be reworded to explain the
> rationale and what needs to be done. Adding a TODO or FIXME comment in
> the code would also help.
> 

-- 
With best wishes
Dmitry