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
