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

[Laurent Pinchart]

Hi Dmitry,

On Fri, Aug 26, 2022 at 04:52:12PM +0300, Dmitry Baryshkov wrote:
> On 26/08/2022 14:55, Laurent Pinchart wrote:
> > 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.

Maybe we can leave dynamic selection of the number of lanes for dual-DSI
out at this point ? I have no experienced with bonded DSI, is it typical
to have to switch between single and bonded links at runtime (to be
precise, at mode set time, not while the display is on) ?

> > 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.

A long time ago, I worked on converting the omapdrm driver to the DRM
bridge API. It was using internal bridge and panel drivers with an API
specific to omapdrm, and it was based on a similar principle: enabling
or disabling an output went from the last component in the chain, which
was the responsible for calling into its parent explicitly, with a
bus-specific API. DRM bridge, on the other hand, doesn't use a recursive
model but sequences the whole pipeline with a fixed order. This has led
to be pre-enable/enable split, and even that isn't enough. Moving from
the omapdrm model to the DRM bridge model was difficult and took lots of
time and effort, and I'm now increasingly thinking the omapdrm model got
it right, only too early to convince enough people.

> >>> 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.

That's fine.

> (*) 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.

-- 
Regards,

Laurent Pinchart