Hi Jagan,
Thank you for the patch.
On Mon, Feb 15, 2021 at 01:11:00AM +0530, Jagan Teki wrote:
drm_bridge_attach has stacked the bridge chain, so the bridge that gets pushed last can trigger its bridge function pre_enable first from drm_atomic_bridge_chain_pre_enable.
This indeed gives a chance to trigger slave bridge pre_enable first without triggering its host bridge pre_enable for the usual host to slave device model like DSI host with panel slave.
For fully enabled bridge drivers, host bridge pre_enable has all host related clock, reset, PHY configuration code that needs to initialized before sending commands or configuration from a slave to communicate its host.
Queue the bridge chain instead of stacking it so-that the bridges that got enqueued first can have a chance to trigger first.
First of all, won't thus break all the drivers that currently rely on the existing behaviour ?
Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Thomas Zimmermann tzimmermann@suse.de Signed-off-by: Jagan Teki jagan@amarulasolutions.com
Changes for v3:
- new patch
drivers/gpu/drm/drm_bridge.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index 64f0effb52ac..e75d1a080c55 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -191,9 +191,9 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, bridge->encoder = encoder;
if (previous)
list_add(&bridge->chain_node, &previous->chain_node);
list_add_tail(&bridge->chain_node, &previous->chain_node);
list_add(&bridge->chain_node, &encoder->bridge_chain);
list_add_tail(&bridge->chain_node, &encoder->bridge_chain);
Then, this will create a really weird order, as the list will contain bridges in the reverse order. Assuming three bridges, A, B and C, which are connected at the hardware level as follows:
Encoder -> A -> B -> C
the list would contain
Encoder -> C -> B -> A
This isn't intuitive, and if you want to reverse the order in which bridge operations are called, it would be better to do so in the operations themselves, for instance replacing list_for_each_entry_reverse() with list_for_each_entry() in drm_atomic_bridge_chain_pre_enable(). Still, this will likely break drivers that depend on the existing order, so I don't think that's an acceptable solution as-is.
if (bridge->funcs->attach) { ret = bridge->funcs->attach(bridge, flags);