Right now, the chaining order of
pre_enable/enable/disable/post_disable looks like this:
pre_enable: start from connector and move to encoder
enable: start from encoder and move to connector
disable: start from connector and move to encoder
post_disable: start from encoder and move to connector
In the above, it can be seen that at least pre_enable() and
post_disable() are opposites of each other and enable() and disable()
are opposites. However, it seems broken that pre_enable() and enable()
would not move in the same direction. In other parts of Linux you can
see that various stages move in the same order. For instance, during
system suspend the "early" calls run in the same order as the normal
calls run in the same order as the "late" calls run in the same order
as the "noirq" calls.
Let fix the above so that it makes more sense. Now we'll have:
pre_enable: start from encoder and move to connector
enable: start from encoder and move to connector
disable: start from connector and move to encoder
post_disable: start from connector and move to encoder
This order is chosen because if there are parent-child relationships
anywhere I would expect that the encoder would be a parent and the
connector a child--not the other way around.
This can be important when using the DP AUX bus to instantiate a
panel. The DP AUX bus is likely part of a bridge driver and is a
parent of the panel. We'd like the bridge to be pre_enabled before the
panel and the panel to be post_disabled before the
bridge. Specifically, this allows pm_runtime_put_sync_suspend() in a
bridge driver's post_suspend to work properly even a panel is under
it.
NOTE: it's entirely possible that this change could break someone who
was relying on the old order. Hopefully this isn't the case, but if
this does break someone it seems like it's better to do it sonner
rather than later so we can fix everyone to handle the order that
makes the most sense.
A FURTHER NOTE: Looking closer at commit 4e5763f03e10 ("drm/bridge:
ti-sn65dsi86: Wrap panel with panel-bridge") you can see that patch
inadvertently changed the order of things. The order used to be
correct (panel prepare was at the tail of the bridge enable) but it
became backwards. We'll restore the original order with this patch.
Fixes: 4e5763f03e10 ("drm/bridge: ti-sn65dsi86: Wrap panel with panel-bridge")
Fixes: 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked list")
Signed-off-by: Douglas Anderson <[email protected]>
---
drivers/gpu/drm/drm_bridge.c | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index c96847fc0ebc..98808af59afd 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -583,18 +583,14 @@ EXPORT_SYMBOL(drm_bridge_chain_mode_set);
void drm_bridge_chain_pre_enable(struct drm_bridge *bridge)
{
struct drm_encoder *encoder;
- struct drm_bridge *iter;
if (!bridge)
return;
encoder = bridge->encoder;
- list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
- if (iter->funcs->pre_enable)
- iter->funcs->pre_enable(iter);
-
- if (iter == bridge)
- break;
+ list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
+ if (bridge->funcs->pre_enable)
+ bridge->funcs->pre_enable(bridge);
}
}
EXPORT_SYMBOL(drm_bridge_chain_pre_enable);
@@ -684,26 +680,30 @@ void drm_atomic_bridge_chain_post_disable(struct drm_bridge *bridge,
struct drm_atomic_state *old_state)
{
struct drm_encoder *encoder;
+ struct drm_bridge *iter;
if (!bridge)
return;
encoder = bridge->encoder;
- list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
- if (bridge->funcs->atomic_post_disable) {
+ list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
+ if (iter->funcs->atomic_post_disable) {
struct drm_bridge_state *old_bridge_state;
old_bridge_state =
drm_atomic_get_old_bridge_state(old_state,
- bridge);
+ iter);
if (WARN_ON(!old_bridge_state))
return;
- bridge->funcs->atomic_post_disable(bridge,
- old_bridge_state);
- } else if (bridge->funcs->post_disable) {
- bridge->funcs->post_disable(bridge);
+ iter->funcs->atomic_post_disable(iter,
+ old_bridge_state);
+ } else if (iter->funcs->post_disable) {
+ iter->funcs->post_disable(iter);
}
+
+ if (iter == bridge)
+ break;
}
}
EXPORT_SYMBOL(drm_atomic_bridge_chain_post_disable);
--
2.33.0.1079.g6e70778dc9-goog
Hi,
On Thu, Oct 21, 2021 at 1:21 PM Sam Ravnborg <[email protected]> wrote:
>
> Hi Douglas,
>
> On Thu, Oct 21, 2021 at 12:29:01PM -0700, Douglas Anderson wrote:
> > Right now, the chaining order of
> > pre_enable/enable/disable/post_disable looks like this:
> >
> > pre_enable: start from connector and move to encoder
> > enable: start from encoder and move to connector
> > disable: start from connector and move to encoder
> > post_disable: start from encoder and move to connector
> >
> > In the above, it can be seen that at least pre_enable() and
> > post_disable() are opposites of each other and enable() and disable()
> > are opposites. However, it seems broken that pre_enable() and enable()
> > would not move in the same direction. In other parts of Linux you can
> > see that various stages move in the same order. For instance, during
> > system suspend the "early" calls run in the same order as the normal
> > calls run in the same order as the "late" calls run in the same order
> > as the "noirq" calls.
> >
> > Let fix the above so that it makes more sense. Now we'll have:
> >
> > pre_enable: start from encoder and move to connector
> > enable: start from encoder and move to connector
> > disable: start from connector and move to encoder
> > post_disable: start from connector and move to encoder
> >
> > This order is chosen because if there are parent-child relationships
> > anywhere I would expect that the encoder would be a parent and the
> > connector a child--not the other way around.
>
> This makes good sense as you describe it. I hope others can add more
> useful feedback.
> Added Andrzej Hajda <[email protected]> to the mail, as he have
> expressed concerns with the chain of bridges before.
>
> >
> > This can be important when using the DP AUX bus to instantiate a
> > panel. The DP AUX bus is likely part of a bridge driver and is a
> > parent of the panel. We'd like the bridge to be pre_enabled before the
> > panel and the panel to be post_disabled before the
> > bridge. Specifically, this allows pm_runtime_put_sync_suspend() in a
> > bridge driver's post_suspend to work properly even a panel is under
> > it.
> >
> > NOTE: it's entirely possible that this change could break someone who
> > was relying on the old order. Hopefully this isn't the case, but if
> > this does break someone it seems like it's better to do it sonner
> > rather than later so we can fix everyone to handle the order that
> > makes the most sense.
> >
> > A FURTHER NOTE: Looking closer at commit 4e5763f03e10 ("drm/bridge:
> > ti-sn65dsi86: Wrap panel with panel-bridge") you can see that patch
> > inadvertently changed the order of things. The order used to be
> > correct (panel prepare was at the tail of the bridge enable) but it
> > became backwards. We'll restore the original order with this patch.
> >
> > Fixes: 4e5763f03e10 ("drm/bridge: ti-sn65dsi86: Wrap panel with panel-bridge")
> > Fixes: 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked list")
> > Signed-off-by: Douglas Anderson <[email protected]>
>
> To make the patch complete the descriptions in drm_bridge_funcs
> need to be updated to reflect the new reality.
Ah, oops! Sure, I'll plan on a v2 with this but I'll wait for more feedback.
> > drivers/gpu/drm/drm_bridge.c | 28 ++++++++++++++--------------
> > 1 file changed, 14 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > index c96847fc0ebc..98808af59afd 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -583,18 +583,14 @@ EXPORT_SYMBOL(drm_bridge_chain_mode_set);
> > void drm_bridge_chain_pre_enable(struct drm_bridge *bridge)
>
> If you, or someone else, could r-b or ack the pending patches to remove
> this function, this part of the patch would no longer be needed.
OK. I will likely be able to take a look next week. Given that I'm
helping Philip bringup a board with ps8640 it looks like your patch
series will be quite relevant! I guess it would be good to figure out
what would be the best order to land them. In my case we need this fix
to be easy to pick back to fix the behavior on the Chrome OS 5.4 tree.
My fix is easy to pick back, but perhaps yours is as well. Of course
we could also just make a local divergent change in our tree if need
be, too.
> > {
> > struct drm_encoder *encoder;
> > - struct drm_bridge *iter;
> >
> > if (!bridge)
> > return;
> >
> > encoder = bridge->encoder;
> > - list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
> > - if (iter->funcs->pre_enable)
> > - iter->funcs->pre_enable(iter);
> > -
> > - if (iter == bridge)
> > - break;
> > + list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
> > + if (bridge->funcs->pre_enable)
> > + bridge->funcs->pre_enable(bridge);
> > }
> > }
> > EXPORT_SYMBOL(drm_bridge_chain_pre_enable);
> > @@ -684,26 +680,30 @@ void drm_atomic_bridge_chain_post_disable(struct drm_bridge *bridge,
> > struct drm_atomic_state *old_state)
> > {
> > struct drm_encoder *encoder;
> > + struct drm_bridge *iter;
> s/iter/bridge/ would make the patch simpler
> And then the bridge argument could be last_bridge or something.
> This would IMO increase readability of the code and make the patch smaller.
Yeah, I debated this too. I was trying to match
drm_bridge_chain_disable() and in my mind keeping the two functions
matching is more important than keeping this patch small. Certainly I
could add another patch in the series to rename "bridge" to
"last_bridge" and "iter" to "bridge" in that function, but that
defeats the goal of reducing churn... ...and clearly whoever wrote
drm_bridge_chain_disable() liked "iter" better. :-P
In any case, I'll change it as you say if everyone likes it better,
but otherwise I'll leave it as I have it.
> > if (!bridge)
> > return;
> >
> > encoder = bridge->encoder;
> > - list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
> > - if (bridge->funcs->atomic_post_disable) {
> > + list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
> > + if (iter->funcs->atomic_post_disable) {
> > struct drm_bridge_state *old_bridge_state;
> >
> > old_bridge_state =
> > drm_atomic_get_old_bridge_state(old_state,
> > - bridge);
> > + iter);
> > if (WARN_ON(!old_bridge_state))
> > return;
> >
> > - bridge->funcs->atomic_post_disable(bridge,
> > - old_bridge_state);
> > - } else if (bridge->funcs->post_disable) {
> > - bridge->funcs->post_disable(bridge);
> > + iter->funcs->atomic_post_disable(iter,
> > + old_bridge_state);
> > + } else if (iter->funcs->post_disable) {
> > + iter->funcs->post_disable(iter);
> > }
> > +
> > + if (iter == bridge)
> > + break;
> I cannot see why this is needed, we are at the end of the list here
> anyway.
It's because you can start at something that's not the first bridge in
the chain. See commit bab5cca7e609 ("drm/bridge: Fix the stop
condition of drm_bridge_chain_pre_enable()")
Hi Doug,
I see this patch fixes the order for
drm_bridge_chain_pre_enable() and drm_atomic_bridge_chain_post_disable().
For completeness, shouldn't we also fix the order for
drm_atomic_bridge_chain_pre_enable() and drm_bridge_chain_post_disable()?
Surely, if Sam's pending patches will land first, there is no need to
fix the non_atomic versions.
On Thu, Oct 21, 2021 at 1:41 PM Sam Ravnborg <[email protected]> wrote:
>
> Hi Douglas,
>
> > > > void drm_bridge_chain_pre_enable(struct drm_bridge *bridge)
> > >
> > > If you, or someone else, could r-b or ack the pending patches to remove
> > > this function, this part of the patch would no longer be needed.
> >
> > OK. I will likely be able to take a look next week. Given that I'm
> > helping Philip bringup a board with ps8640 it looks like your patch
> > series will be quite relevant! I guess it would be good to figure out
> > what would be the best order to land them. In my case we need this fix
> > to be easy to pick back to fix the behavior on the Chrome OS 5.4 tree.
> > My fix is easy to pick back, but perhaps yours is as well. Of course
> > we could also just make a local divergent change in our tree if need
> > be, too.
> I do not mind the order - so whatever works for you guys.
> The only concern here is that we should not gain new users.
>
> >
> > > > {
> > > > struct drm_encoder *encoder;
> > > > - struct drm_bridge *iter;
> > > >
> > > > if (!bridge)
> > > > return;
> > > >
> > > > encoder = bridge->encoder;
> > > > - list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
> > > > - if (iter->funcs->pre_enable)
> > > > - iter->funcs->pre_enable(iter);
> > > > -
> > > > - if (iter == bridge)
> > > > - break;
> > > > + list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
> > > > + if (bridge->funcs->pre_enable)
> > > > + bridge->funcs->pre_enable(bridge);
> > > > }
> > > > }
> > > > EXPORT_SYMBOL(drm_bridge_chain_pre_enable);
> > > > @@ -684,26 +680,30 @@ void drm_atomic_bridge_chain_post_disable(struct drm_bridge *bridge,
> > > > struct drm_atomic_state *old_state)
> > > > {
> > > > struct drm_encoder *encoder;
> > > > + struct drm_bridge *iter;
> > > s/iter/bridge/ would make the patch simpler
> > > And then the bridge argument could be last_bridge or something.
> > > This would IMO increase readability of the code and make the patch smaller.
> >
> > Yeah, I debated this too. I was trying to match
> > drm_bridge_chain_disable() and in my mind keeping the two functions
> > matching is more important than keeping this patch small.
> Well, drm_bridge_chain_disable() is about to be deleted. So that the
> wrong one to look at.
>
> > Certainly I
> > could add another patch in the series to rename "bridge" to
> > "last_bridge" and "iter" to "bridge" in that function, but that
> > defeats the goal of reducing churn... ...and clearly whoever wrote
> > drm_bridge_chain_disable() liked "iter" better. :-P
> >
> > In any case, I'll change it as you say if everyone likes it better,
> > but otherwise I'll leave it as I have it.
>
> >
> >
> > > > if (!bridge)
> > > > return;
> > > >
> > > > encoder = bridge->encoder;
> > > > - list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
> > > > - if (bridge->funcs->atomic_post_disable) {
> > > > + list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
> > > > + if (iter->funcs->atomic_post_disable) {
> > > > struct drm_bridge_state *old_bridge_state;
> > > >
> > > > old_bridge_state =
> > > > drm_atomic_get_old_bridge_state(old_state,
> > > > - bridge);
> > > > + iter);
> > > > if (WARN_ON(!old_bridge_state))
> > > > return;
> > > >
> > > > - bridge->funcs->atomic_post_disable(bridge,
> > > > - old_bridge_state);
> > > > - } else if (bridge->funcs->post_disable) {
> > > > - bridge->funcs->post_disable(bridge);
> > > > + iter->funcs->atomic_post_disable(iter,
> > > > + old_bridge_state);
> > > > + } else if (iter->funcs->post_disable) {
> > > > + iter->funcs->post_disable(iter);
> > > > }
> > > > +
> > > > + if (iter == bridge)
> > > > + break;
> > > I cannot see why this is needed, we are at the end of the list here
> > > anyway.
> I see, please include this change in your changelog and add it to the
> documentation in drm_bridge_h.
>
> Sam
Hi Doug,
Thank you for the patch.
On Thu, Oct 21, 2021 at 12:29:01PM -0700, Douglas Anderson wrote:
> Right now, the chaining order of
> pre_enable/enable/disable/post_disable looks like this:
>
> pre_enable: start from connector and move to encoder
> enable: start from encoder and move to connector
> disable: start from connector and move to encoder
> post_disable: start from encoder and move to connector
>
> In the above, it can be seen that at least pre_enable() and
> post_disable() are opposites of each other and enable() and disable()
> are opposites. However, it seems broken that pre_enable() and enable()
> would not move in the same direction. In other parts of Linux you can
> see that various stages move in the same order. For instance, during
> system suspend the "early" calls run in the same order as the normal
> calls run in the same order as the "late" calls run in the same order
> as the "noirq" calls.
>
> Let fix the above so that it makes more sense. Now we'll have:
>
> pre_enable: start from encoder and move to connector
> enable: start from encoder and move to connector
> disable: start from connector and move to encoder
> post_disable: start from connector and move to encoder
>
> This order is chosen because if there are parent-child relationships
> anywhere I would expect that the encoder would be a parent and the
> connector a child--not the other way around.
>
> This can be important when using the DP AUX bus to instantiate a
> panel. The DP AUX bus is likely part of a bridge driver and is a
> parent of the panel. We'd like the bridge to be pre_enabled before the
> panel and the panel to be post_disabled before the
> bridge. Specifically, this allows pm_runtime_put_sync_suspend() in a
> bridge driver's post_suspend to work properly even a panel is under
> it.
>
> NOTE: it's entirely possible that this change could break someone who
> was relying on the old order. Hopefully this isn't the case, but if
> this does break someone it seems like it's better to do it sonner
> rather than later so we can fix everyone to handle the order that
> makes the most sense.
I'm less hopeful than you are on this, changing the order of operations
is very risky. I'm also concerned about hardware damage, the pre-enable
operation is often used to power up devices, and powering up a source
before a sink is dangerous as many devices don't like having I/O voltage
applied to their pins before they get powered up. If you really want to
land this, the patch needs very very broad testing, as well as a plan to
address the power up issue.
> A FURTHER NOTE: Looking closer at commit 4e5763f03e10 ("drm/bridge:
> ti-sn65dsi86: Wrap panel with panel-bridge") you can see that patch
> inadvertently changed the order of things. The order used to be
> correct (panel prepare was at the tail of the bridge enable) but it
> became backwards. We'll restore the original order with this patch.
>
> Fixes: 4e5763f03e10 ("drm/bridge: ti-sn65dsi86: Wrap panel with panel-bridge")
> Fixes: 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked list")
> Signed-off-by: Douglas Anderson <[email protected]>
> ---
>
> drivers/gpu/drm/drm_bridge.c | 28 ++++++++++++++--------------
> 1 file changed, 14 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index c96847fc0ebc..98808af59afd 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -583,18 +583,14 @@ EXPORT_SYMBOL(drm_bridge_chain_mode_set);
> void drm_bridge_chain_pre_enable(struct drm_bridge *bridge)
> {
> struct drm_encoder *encoder;
> - struct drm_bridge *iter;
>
> if (!bridge)
> return;
>
> encoder = bridge->encoder;
> - list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
> - if (iter->funcs->pre_enable)
> - iter->funcs->pre_enable(iter);
> -
> - if (iter == bridge)
> - break;
> + list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
> + if (bridge->funcs->pre_enable)
> + bridge->funcs->pre_enable(bridge);
> }
> }
> EXPORT_SYMBOL(drm_bridge_chain_pre_enable);
> @@ -684,26 +680,30 @@ void drm_atomic_bridge_chain_post_disable(struct drm_bridge *bridge,
> struct drm_atomic_state *old_state)
> {
> struct drm_encoder *encoder;
> + struct drm_bridge *iter;
>
> if (!bridge)
> return;
>
> encoder = bridge->encoder;
> - list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
> - if (bridge->funcs->atomic_post_disable) {
> + list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
> + if (iter->funcs->atomic_post_disable) {
> struct drm_bridge_state *old_bridge_state;
>
> old_bridge_state =
> drm_atomic_get_old_bridge_state(old_state,
> - bridge);
> + iter);
> if (WARN_ON(!old_bridge_state))
> return;
>
> - bridge->funcs->atomic_post_disable(bridge,
> - old_bridge_state);
> - } else if (bridge->funcs->post_disable) {
> - bridge->funcs->post_disable(bridge);
> + iter->funcs->atomic_post_disable(iter,
> + old_bridge_state);
> + } else if (iter->funcs->post_disable) {
> + iter->funcs->post_disable(iter);
> }
> +
> + if (iter == bridge)
> + break;
> }
> }
> EXPORT_SYMBOL(drm_atomic_bridge_chain_post_disable);
--
Regards,
Laurent Pinchart
Hi All,
On 21.10.2021 22:21, Sam Ravnborg wrote:
> Hi Douglas,
>
> On Thu, Oct 21, 2021 at 12:29:01PM -0700, Douglas Anderson wrote:
>> Right now, the chaining order of
>> pre_enable/enable/disable/post_disable looks like this:
>>
>> pre_enable: start from connector and move to encoder
>> enable: start from encoder and move to connector
>> disable: start from connector and move to encoder
>> post_disable: start from encoder and move to connector
>>
>> In the above, it can be seen that at least pre_enable() and
>> post_disable() are opposites of each other and enable() and disable()
>> are opposites. However, it seems broken that pre_enable() and enable()
>> would not move in the same direction. In other parts of Linux you can
>> see that various stages move in the same order. For instance, during
>> system suspend the "early" calls run in the same order as the normal
>> calls run in the same order as the "late" calls run in the same order
>> as the "noirq" calls.
>>
>> Let fix the above so that it makes more sense. Now we'll have:
>>
>> pre_enable: start from encoder and move to connector
>> enable: start from encoder and move to connector
>> disable: start from connector and move to encoder
>> post_disable: start from connector and move to encoder
>>
>> This order is chosen because if there are parent-child relationships
>> anywhere I would expect that the encoder would be a parent and the
>> connector a child--not the other way around.
> This makes good sense as you describe it. I hope others can add more
> useful feedback.
> Added Andrzej Hajda <[email protected]> to the mail, as he have
> expressed concerns with the chain of bridges before.
Thanks Sam, but I am not sure about useful feedback - when I see bridge
chain issues it automatically triggers "whining mode" in my head :)
>
>> This can be important when using the DP AUX bus to instantiate a
>> panel. The DP AUX bus is likely part of a bridge driver and is a
>> parent of the panel. We'd like the bridge to be pre_enabled before the
>> panel and the panel to be post_disabled before the
>> bridge. Specifically, this allows pm_runtime_put_sync_suspend() in a
>> bridge driver's post_suspend to work properly even a panel is under
>> it.
>>
>> NOTE: it's entirely possible that this change could break someone who
>> was relying on the old order. Hopefully this isn't the case, but if
>> this does break someone it seems like it's better to do it sonner
>> rather than later so we can fix everyone to handle the order that
>> makes the most sense.
It will break for sure. So the question is: if it is worth changing?
New order seems good for eDP, DSI sinks [1], probably other as well.
Old order is better for example for THC63LVD1024 [2 p. 20], I guess for
many other sinks as well.
I am not even sure if it is protocol specific (LVDS, RGB, HDMI,...), or
it depends on specific hw pairs (source->sink).
This is why I complain about the bridge chain - assumption that one
fixed call order will work for all setups seems to me ridiculous.
Going back to the question - changing the order from fixed one to
another fixed one will not solve general issue.
What can we do then?
1. Configurable call order? Probably doable: every chain element should
expose info if it's call should be before or after source, then some
core helper will create queue of callbacks. Seems quite complicated,
hides the logic from implementer and not fully flexible (for example,
there are protocols which require to perform sth on source, then on
sink, then again on the source).
2. Stop using bridge chain and call sink ops directly from the source
(this is what Exynos and VC4 do): is flexible and straightforward, gives
full control to the source.
3. Use different abstractions to enforce proper initialization order
(like extending mipi_dsi_host_ops): requires existence of transport bus
abstraction (only DSI at the moment(?)).
... other ideas?
Another idea, connected to the subject - some protocols require some
negotiations between source and sink bus format, or more steps than
pre_enable, enable ops to establish link. I wonder if encapsulating
drm_bridge in some protocol specific struct wouldn't be a solution, it
can be helpful as well in case of the subject.
For example:
struct drm_bridge_edp {
const struct drm_bridge_edp_funcs *funcs;
struct drm_bridge base;
...
};
Then source could promote bridge pointer to bridge_edp pointer (if
applicable) and perform edp specific stuff. To make it working well
pre-enable order should be as proposed in this patchsets (encoder ->
connector), as the source should initiate negotiations.
Btw this encapsulation stuff above asks to rename drm_bridge to
drm_sink, otherwise it would be confusing as bridges have two ends.
Regards
Andrzej
[1]: I use term sink as short equivalent for 'bridges AND panels'
(another issue in DRMs).
[2]: https://www.mouser.com/datasheet/2/286/THC63LVD1024-1396205.pdf
Regards
Andrzej
>>
>> A FURTHER NOTE: Looking closer at commit 4e5763f03e10 ("drm/bridge:
>> ti-sn65dsi86: Wrap panel with panel-bridge") you can see that patch
>> inadvertently changed the order of things. The order used to be
>> correct (panel prepare was at the tail of the bridge enable) but it
>> became backwards. We'll restore the original order with this patch.
>>
>> Fixes: 4e5763f03e10 ("drm/bridge: ti-sn65dsi86: Wrap panel with panel-bridge")
>> Fixes: 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked list")
>> Signed-off-by: Douglas Anderson <[email protected]>
> To make the patch complete the descriptions in drm_bridge_funcs
> need to be updated to reflect the new reality.
>
>> ---
>>
>> drivers/gpu/drm/drm_bridge.c | 28 ++++++++++++++--------------
>> 1 file changed, 14 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>> index c96847fc0ebc..98808af59afd 100644
>> --- a/drivers/gpu/drm/drm_bridge.c
>> +++ b/drivers/gpu/drm/drm_bridge.c
>> @@ -583,18 +583,14 @@ EXPORT_SYMBOL(drm_bridge_chain_mode_set);
>> void drm_bridge_chain_pre_enable(struct drm_bridge *bridge)
> If you, or someone else, could r-b or ack the pending patches to remove
> this function, this part of the patch would no longer be needed.
>
>> {
>> struct drm_encoder *encoder;
>> - struct drm_bridge *iter;
>>
>> if (!bridge)
>> return;
>>
>> encoder = bridge->encoder;
>> - list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
>> - if (iter->funcs->pre_enable)
>> - iter->funcs->pre_enable(iter);
>> -
>> - if (iter == bridge)
>> - break;
>> + list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
>> + if (bridge->funcs->pre_enable)
>> + bridge->funcs->pre_enable(bridge);
>> }
>> }
>> EXPORT_SYMBOL(drm_bridge_chain_pre_enable);
>> @@ -684,26 +680,30 @@ void drm_atomic_bridge_chain_post_disable(struct drm_bridge *bridge,
>> struct drm_atomic_state *old_state)
>> {
>> struct drm_encoder *encoder;
>> + struct drm_bridge *iter;
> s/iter/bridge/ would make the patch simpler
> And then the bridge argument could be last_bridge or something.
> This would IMO increase readability of the code and make the patch smaller.
>>
>> if (!bridge)
>> return;
>>
>> encoder = bridge->encoder;
>> - list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
>> - if (bridge->funcs->atomic_post_disable) {
>> + list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
>> + if (iter->funcs->atomic_post_disable) {
>> struct drm_bridge_state *old_bridge_state;
>>
>> old_bridge_state =
>> drm_atomic_get_old_bridge_state(old_state,
>> - bridge);
>> + iter);
>> if (WARN_ON(!old_bridge_state))
>> return;
>>
>> - bridge->funcs->atomic_post_disable(bridge,
>> - old_bridge_state);
>> - } else if (bridge->funcs->post_disable) {
>> - bridge->funcs->post_disable(bridge);
>> + iter->funcs->atomic_post_disable(iter,
>> + old_bridge_state);
>> + } else if (iter->funcs->post_disable) {
>> + iter->funcs->post_disable(iter);
>> }
>> +
>> + if (iter == bridge)
>> + break;
> I cannot see why this is needed, we are at the end of the list here
> anyway.
>
>
>> }
>> }
>> EXPORT_SYMBOL(drm_atomic_bridge_chain_post_disable);
> Sam
Hello,
On Mon, Oct 25, 2021 at 01:00:10PM +0200, Andrzej Hajda wrote:
> On 21.10.2021 22:21, Sam Ravnborg wrote:
> > On Thu, Oct 21, 2021 at 12:29:01PM -0700, Douglas Anderson wrote:
> >> Right now, the chaining order of
> >> pre_enable/enable/disable/post_disable looks like this:
> >>
> >> pre_enable: start from connector and move to encoder
> >> enable: start from encoder and move to connector
> >> disable: start from connector and move to encoder
> >> post_disable: start from encoder and move to connector
> >>
> >> In the above, it can be seen that at least pre_enable() and
> >> post_disable() are opposites of each other and enable() and disable()
> >> are opposites. However, it seems broken that pre_enable() and enable()
> >> would not move in the same direction. In other parts of Linux you can
> >> see that various stages move in the same order. For instance, during
> >> system suspend the "early" calls run in the same order as the normal
> >> calls run in the same order as the "late" calls run in the same order
> >> as the "noirq" calls.
> >>
> >> Let fix the above so that it makes more sense. Now we'll have:
> >>
> >> pre_enable: start from encoder and move to connector
> >> enable: start from encoder and move to connector
> >> disable: start from connector and move to encoder
> >> post_disable: start from connector and move to encoder
> >>
> >> This order is chosen because if there are parent-child relationships
> >> anywhere I would expect that the encoder would be a parent and the
> >> connector a child--not the other way around.
> >
> > This makes good sense as you describe it. I hope others can add more
> > useful feedback.
> > Added Andrzej Hajda <[email protected]> to the mail, as he have
> > expressed concerns with the chain of bridges before.
>
> Thanks Sam, but I am not sure about useful feedback - when I see bridge
> chain issues it automatically triggers "whining mode" in my head :)
>
> >> This can be important when using the DP AUX bus to instantiate a
> >> panel. The DP AUX bus is likely part of a bridge driver and is a
> >> parent of the panel. We'd like the bridge to be pre_enabled before the
> >> panel and the panel to be post_disabled before the
> >> bridge. Specifically, this allows pm_runtime_put_sync_suspend() in a
> >> bridge driver's post_suspend to work properly even a panel is under
> >> it.
> >>
> >> NOTE: it's entirely possible that this change could break someone who
> >> was relying on the old order. Hopefully this isn't the case, but if
> >> this does break someone it seems like it's better to do it sonner
> >> rather than later so we can fix everyone to handle the order that
> >> makes the most sense.
>
> It will break for sure. So the question is: if it is worth changing?
>
> New order seems good for eDP, DSI sinks [1], probably other as well.
>
> Old order is better for example for THC63LVD1024 [2 p. 20], I guess for
> many other sinks as well.
>
> I am not even sure if it is protocol specific (LVDS, RGB, HDMI,...), or
> it depends on specific hw pairs (source->sink).
>
> This is why I complain about the bridge chain - assumption that one
> fixed call order will work for all setups seems to me ridiculous.
>
> Going back to the question - changing the order from fixed one to
> another fixed one will not solve general issue.
>
> What can we do then?
>
> 1. Configurable call order? Probably doable: every chain element should
> expose info if it's call should be before or after source, then some
> core helper will create queue of callbacks. Seems quite complicated,
> hides the logic from implementer and not fully flexible (for example,
> there are protocols which require to perform sth on source, then on
> sink, then again on the source).
>
> 2. Stop using bridge chain and call sink ops directly from the source
> (this is what Exynos and VC4 do): is flexible and straightforward, gives
> full control to the source.
And breaks interoperability, because different sources end up calling
operations in different orders. We end up having different sinks that
expect calls in different ways, and divide the world in sink/source
groups that don't interoperate :-(
> 3. Use different abstractions to enforce proper initialization order
> (like extending mipi_dsi_host_ops): requires existence of transport bus
> abstraction (only DSI at the moment(?)).
A real bus seems overkill, but we could have drm_bridge operations
specific to particular hardware interfaces.
> ... other ideas?
I don't like it because of the amount of work it would require to switch
to such a model, but I'm really starting to think that a variation of
the second option would be best, where the sink controls the source
instead of having the source controlling the sink. It's the sink that
knows about its enabling/disabling sequence, and about how the source
needs to be controlled to match it.
> Another idea, connected to the subject - some protocols require some
> negotiations between source and sink bus format, or more steps than
> pre_enable, enable ops to establish link. I wonder if encapsulating
> drm_bridge in some protocol specific struct wouldn't be a solution, it
> can be helpful as well in case of the subject.
>
> For example:
>
> struct drm_bridge_edp {
>
> const struct drm_bridge_edp_funcs *funcs;
>
> struct drm_bridge base;
>
> ...
>
> };
>
> Then source could promote bridge pointer to bridge_edp pointer (if
> applicable) and perform edp specific stuff. To make it working well
> pre-enable order should be as proposed in this patchsets (encoder ->
> connector), as the source should initiate negotiations.
>
> Btw this encapsulation stuff above asks to rename drm_bridge to
> drm_sink, otherwise it would be confusing as bridges have two ends.
drm_sink would be equally confusing when used for devices that have a
sink and a source :-) I'm not against a rename though, if we can find a
better name.
> Regards
>
> Andrzej
>
>
> [1]: I use term sink as short equivalent for 'bridges AND panels'
> (another issue in DRMs).
>
> [2]: https://www.mouser.com/datasheet/2/286/THC63LVD1024-1396205.pdf
>
> >> A FURTHER NOTE: Looking closer at commit 4e5763f03e10 ("drm/bridge:
> >> ti-sn65dsi86: Wrap panel with panel-bridge") you can see that patch
> >> inadvertently changed the order of things. The order used to be
> >> correct (panel prepare was at the tail of the bridge enable) but it
> >> became backwards. We'll restore the original order with this patch.
> >>
> >> Fixes: 4e5763f03e10 ("drm/bridge: ti-sn65dsi86: Wrap panel with panel-bridge")
> >> Fixes: 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked list")
> >> Signed-off-by: Douglas Anderson <[email protected]>
> >
> > To make the patch complete the descriptions in drm_bridge_funcs
> > need to be updated to reflect the new reality.
> >
> >> ---
> >>
> >> drivers/gpu/drm/drm_bridge.c | 28 ++++++++++++++--------------
> >> 1 file changed, 14 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> >> index c96847fc0ebc..98808af59afd 100644
> >> --- a/drivers/gpu/drm/drm_bridge.c
> >> +++ b/drivers/gpu/drm/drm_bridge.c
> >> @@ -583,18 +583,14 @@ EXPORT_SYMBOL(drm_bridge_chain_mode_set);
> >> void drm_bridge_chain_pre_enable(struct drm_bridge *bridge)
> >
> > If you, or someone else, could r-b or ack the pending patches to remove
> > this function, this part of the patch would no longer be needed.
> >
> >> {
> >> struct drm_encoder *encoder;
> >> - struct drm_bridge *iter;
> >>
> >> if (!bridge)
> >> return;
> >>
> >> encoder = bridge->encoder;
> >> - list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
> >> - if (iter->funcs->pre_enable)
> >> - iter->funcs->pre_enable(iter);
> >> -
> >> - if (iter == bridge)
> >> - break;
> >> + list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
> >> + if (bridge->funcs->pre_enable)
> >> + bridge->funcs->pre_enable(bridge);
> >> }
> >> }
> >> EXPORT_SYMBOL(drm_bridge_chain_pre_enable);
> >> @@ -684,26 +680,30 @@ void drm_atomic_bridge_chain_post_disable(struct drm_bridge *bridge,
> >> struct drm_atomic_state *old_state)
> >> {
> >> struct drm_encoder *encoder;
> >> + struct drm_bridge *iter;
> >
> > s/iter/bridge/ would make the patch simpler
> > And then the bridge argument could be last_bridge or something.
> > This would IMO increase readability of the code and make the patch smaller.
> >>
> >> if (!bridge)
> >> return;
> >>
> >> encoder = bridge->encoder;
> >> - list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
> >> - if (bridge->funcs->atomic_post_disable) {
> >> + list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
> >> + if (iter->funcs->atomic_post_disable) {
> >> struct drm_bridge_state *old_bridge_state;
> >>
> >> old_bridge_state =
> >> drm_atomic_get_old_bridge_state(old_state,
> >> - bridge);
> >> + iter);
> >> if (WARN_ON(!old_bridge_state))
> >> return;
> >>
> >> - bridge->funcs->atomic_post_disable(bridge,
> >> - old_bridge_state);
> >> - } else if (bridge->funcs->post_disable) {
> >> - bridge->funcs->post_disable(bridge);
> >> + iter->funcs->atomic_post_disable(iter,
> >> + old_bridge_state);
> >> + } else if (iter->funcs->post_disable) {
> >> + iter->funcs->post_disable(iter);
> >> }
> >> +
> >> + if (iter == bridge)
> >> + break;
> >
> > I cannot see why this is needed, we are at the end of the list here
> > anyway.
> >
> >> }
> >> }
> >> EXPORT_SYMBOL(drm_atomic_bridge_chain_post_disable);
--
Regards,
Laurent Pinchart
Hi,
On 25.10.2021 13:21, Laurent Pinchart wrote:
> Hello,
>
> On Mon, Oct 25, 2021 at 01:00:10PM +0200, Andrzej Hajda wrote:
>> On 21.10.2021 22:21, Sam Ravnborg wrote:
>>> On Thu, Oct 21, 2021 at 12:29:01PM -0700, Douglas Anderson wrote:
>>>> Right now, the chaining order of
>>>> pre_enable/enable/disable/post_disable looks like this:
>>>>
>>>> pre_enable: start from connector and move to encoder
>>>> enable: start from encoder and move to connector
>>>> disable: start from connector and move to encoder
>>>> post_disable: start from encoder and move to connector
>>>>
>>>> In the above, it can be seen that at least pre_enable() and
>>>> post_disable() are opposites of each other and enable() and disable()
>>>> are opposites. However, it seems broken that pre_enable() and enable()
>>>> would not move in the same direction. In other parts of Linux you can
>>>> see that various stages move in the same order. For instance, during
>>>> system suspend the "early" calls run in the same order as the normal
>>>> calls run in the same order as the "late" calls run in the same order
>>>> as the "noirq" calls.
>>>>
>>>> Let fix the above so that it makes more sense. Now we'll have:
>>>>
>>>> pre_enable: start from encoder and move to connector
>>>> enable: start from encoder and move to connector
>>>> disable: start from connector and move to encoder
>>>> post_disable: start from connector and move to encoder
>>>>
>>>> This order is chosen because if there are parent-child relationships
>>>> anywhere I would expect that the encoder would be a parent and the
>>>> connector a child--not the other way around.
>>> This makes good sense as you describe it. I hope others can add more
>>> useful feedback.
>>> Added Andrzej Hajda <[email protected]> to the mail, as he have
>>> expressed concerns with the chain of bridges before.
>> Thanks Sam, but I am not sure about useful feedback - when I see bridge
>> chain issues it automatically triggers "whining mode" in my head :)
>>
>>>> This can be important when using the DP AUX bus to instantiate a
>>>> panel. The DP AUX bus is likely part of a bridge driver and is a
>>>> parent of the panel. We'd like the bridge to be pre_enabled before the
>>>> panel and the panel to be post_disabled before the
>>>> bridge. Specifically, this allows pm_runtime_put_sync_suspend() in a
>>>> bridge driver's post_suspend to work properly even a panel is under
>>>> it.
>>>>
>>>> NOTE: it's entirely possible that this change could break someone who
>>>> was relying on the old order. Hopefully this isn't the case, but if
>>>> this does break someone it seems like it's better to do it sonner
>>>> rather than later so we can fix everyone to handle the order that
>>>> makes the most sense.
>> It will break for sure. So the question is: if it is worth changing?
>>
>> New order seems good for eDP, DSI sinks [1], probably other as well.
>>
>> Old order is better for example for THC63LVD1024 [2 p. 20], I guess for
>> many other sinks as well.
>>
>> I am not even sure if it is protocol specific (LVDS, RGB, HDMI,...), or
>> it depends on specific hw pairs (source->sink).
>>
>> This is why I complain about the bridge chain - assumption that one
>> fixed call order will work for all setups seems to me ridiculous.
>>
>> Going back to the question - changing the order from fixed one to
>> another fixed one will not solve general issue.
>>
>> What can we do then?
>>
>> 1. Configurable call order? Probably doable: every chain element should
>> expose info if it's call should be before or after source, then some
>> core helper will create queue of callbacks. Seems quite complicated,
>> hides the logic from implementer and not fully flexible (for example,
>> there are protocols which require to perform sth on source, then on
>> sink, then again on the source).
>>
>> 2. Stop using bridge chain and call sink ops directly from the source
>> (this is what Exynos and VC4 do): is flexible and straightforward, gives
>> full control to the source.
> And breaks interoperability, because different sources end up calling
> operations in different orders. We end up having different sinks that
> expect calls in different ways, and divide the world in sink/source
> groups that don't interoperate :-(
I have an impression you describe current status :) More seriously, it
is matter of proper specification/documentation/implementations of the
operations. If we really need strict constraints we could try to
implement them on protocol level.
>
>> 3. Use different abstractions to enforce proper initialization order
>> (like extending mipi_dsi_host_ops): requires existence of transport bus
>> abstraction (only DSI at the moment(?)).
> A real bus seems overkill, but we could have drm_bridge operations
> specific to particular hardware interfaces.
>
>> ... other ideas?
> I don't like it because of the amount of work it would require to switch
> to such a model, but I'm really starting to think that a variation of
> the second option would be best, where the sink controls the source
> instead of having the source controlling the sink. It's the sink that
> knows about its enabling/disabling sequence, and about how the source
> needs to be controlled to match it.
I am afraid it depends on the protocol and cross-calls (source->sink,
sink->source) can be hard to avoid in case of some protocols.
>
>> Another idea, connected to the subject - some protocols require some
>> negotiations between source and sink bus format, or more steps than
>> pre_enable, enable ops to establish link. I wonder if encapsulating
>> drm_bridge in some protocol specific struct wouldn't be a solution, it
>> can be helpful as well in case of the subject.
>>
>> For example:
>>
>> struct drm_bridge_edp {
>>
>> const struct drm_bridge_edp_funcs *funcs;
>>
>> struct drm_bridge base;
>>
>> ...
>>
>> };
>>
>> Then source could promote bridge pointer to bridge_edp pointer (if
>> applicable) and perform edp specific stuff. To make it working well
>> pre-enable order should be as proposed in this patchsets (encoder ->
>> connector), as the source should initiate negotiations.
>>
>> Btw this encapsulation stuff above asks to rename drm_bridge to
>> drm_sink, otherwise it would be confusing as bridges have two ends.
> drm_sink would be equally confusing when used for devices that have a
> sink and a source :-) I'm not against a rename though, if we can find a
> better name.
But in this case we are interested only in sink part of the bridge (OR
panel). If source is looking for specific bridge or panel
(drm_of_find_panel_or_bridge), it is in fact looking for sink.
Regards
Andrzej
>
>> Regards
>>
>> Andrzej
>>
>>
>> [1]: I use term sink as short equivalent for 'bridges AND panels'
>> (another issue in DRMs).
>>
>> [2]: https://www.mouser.com/datasheet/2/286/THC63LVD1024-1396205.pdf
>>
>>>> A FURTHER NOTE: Looking closer at commit 4e5763f03e10 ("drm/bridge:
>>>> ti-sn65dsi86: Wrap panel with panel-bridge") you can see that patch
>>>> inadvertently changed the order of things. The order used to be
>>>> correct (panel prepare was at the tail of the bridge enable) but it
>>>> became backwards. We'll restore the original order with this patch.
>>>>
>>>> Fixes: 4e5763f03e10 ("drm/bridge: ti-sn65dsi86: Wrap panel with panel-bridge")
>>>> Fixes: 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked list")
>>>> Signed-off-by: Douglas Anderson <[email protected]>
>>> To make the patch complete the descriptions in drm_bridge_funcs
>>> need to be updated to reflect the new reality.
>>>
>>>> ---
>>>>
>>>> drivers/gpu/drm/drm_bridge.c | 28 ++++++++++++++--------------
>>>> 1 file changed, 14 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>>>> index c96847fc0ebc..98808af59afd 100644
>>>> --- a/drivers/gpu/drm/drm_bridge.c
>>>> +++ b/drivers/gpu/drm/drm_bridge.c
>>>> @@ -583,18 +583,14 @@ EXPORT_SYMBOL(drm_bridge_chain_mode_set);
>>>> void drm_bridge_chain_pre_enable(struct drm_bridge *bridge)
>>> If you, or someone else, could r-b or ack the pending patches to remove
>>> this function, this part of the patch would no longer be needed.
>>>
>>>> {
>>>> struct drm_encoder *encoder;
>>>> - struct drm_bridge *iter;
>>>>
>>>> if (!bridge)
>>>> return;
>>>>
>>>> encoder = bridge->encoder;
>>>> - list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
>>>> - if (iter->funcs->pre_enable)
>>>> - iter->funcs->pre_enable(iter);
>>>> -
>>>> - if (iter == bridge)
>>>> - break;
>>>> + list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
>>>> + if (bridge->funcs->pre_enable)
>>>> + bridge->funcs->pre_enable(bridge);
>>>> }
>>>> }
>>>> EXPORT_SYMBOL(drm_bridge_chain_pre_enable);
>>>> @@ -684,26 +680,30 @@ void drm_atomic_bridge_chain_post_disable(struct drm_bridge *bridge,
>>>> struct drm_atomic_state *old_state)
>>>> {
>>>> struct drm_encoder *encoder;
>>>> + struct drm_bridge *iter;
>>> s/iter/bridge/ would make the patch simpler
>>> And then the bridge argument could be last_bridge or something.
>>> This would IMO increase readability of the code and make the patch smaller.
>>>>
>>>> if (!bridge)
>>>> return;
>>>>
>>>> encoder = bridge->encoder;
>>>> - list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
>>>> - if (bridge->funcs->atomic_post_disable) {
>>>> + list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
>>>> + if (iter->funcs->atomic_post_disable) {
>>>> struct drm_bridge_state *old_bridge_state;
>>>>
>>>> old_bridge_state =
>>>> drm_atomic_get_old_bridge_state(old_state,
>>>> - bridge);
>>>> + iter);
>>>> if (WARN_ON(!old_bridge_state))
>>>> return;
>>>>
>>>> - bridge->funcs->atomic_post_disable(bridge,
>>>> - old_bridge_state);
>>>> - } else if (bridge->funcs->post_disable) {
>>>> - bridge->funcs->post_disable(bridge);
>>>> + iter->funcs->atomic_post_disable(iter,
>>>> + old_bridge_state);
>>>> + } else if (iter->funcs->post_disable) {
>>>> + iter->funcs->post_disable(iter);
>>>> }
>>>> +
>>>> + if (iter == bridge)
>>>> + break;
>>> I cannot see why this is needed, we are at the end of the list here
>>> anyway.
>>>
>>>> }
>>>> }
>>>> EXPORT_SYMBOL(drm_atomic_bridge_chain_post_disable);
Hi,
On Mon, Oct 25, 2021 at 1:12 PM Andrzej Hajda <[email protected]> wrote:
>
> >>>> This can be important when using the DP AUX bus to instantiate a
> >>>> panel. The DP AUX bus is likely part of a bridge driver and is a
> >>>> parent of the panel. We'd like the bridge to be pre_enabled before the
> >>>> panel and the panel to be post_disabled before the
> >>>> bridge. Specifically, this allows pm_runtime_put_sync_suspend() in a
> >>>> bridge driver's post_suspend to work properly even a panel is under
> >>>> it.
So with further thought, I _think_ I actually have a solution to my
problem at hand (pm_runtime_put_sync_suspend() is returning -EBUSY in
the ps8640 bridge driver with Philip's most recent patches). I need to
get access to the hardware to test it, but I suspect that the bridge
driver just needs this in its probe:
pm_suspend_ignore_children(dev, true);
Basically I think we just need to tell PM Runtime to "butt out" and
not try to manage power between the bridge driver and the panel even
though there's a parent-child relationship between them (when you use
DP AUX bus).
If that works then the urgency of this problem goes _way_ down for me.
While it still seems like we could run into problems at least they
won't be affecting me anymore in the short term.
> >>>> NOTE: it's entirely possible that this change could break someone who
> >>>> was relying on the old order. Hopefully this isn't the case, but if
> >>>> this does break someone it seems like it's better to do it sonner
> >>>> rather than later so we can fix everyone to handle the order that
> >>>> makes the most sense.
> >> It will break for sure. So the question is: if it is worth changing?
> >>
> >> New order seems good for eDP, DSI sinks [1], probably other as well.
> >>
> >> Old order is better for example for THC63LVD1024 [2 p. 20], I guess for
> >> many other sinks as well.
> >>
> >> I am not even sure if it is protocol specific (LVDS, RGB, HDMI,...), or
> >> it depends on specific hw pairs (source->sink).
> >>
> >> This is why I complain about the bridge chain - assumption that one
> >> fixed call order will work for all setups seems to me ridiculous.
> >>
> >> Going back to the question - changing the order from fixed one to
> >> another fixed one will not solve general issue.
> >>
> >> What can we do then?
> >>
> >> 1. Configurable call order? Probably doable: every chain element should
> >> expose info if it's call should be before or after source, then some
> >> core helper will create queue of callbacks. Seems quite complicated,
> >> hides the logic from implementer and not fully flexible (for example,
> >> there are protocols which require to perform sth on source, then on
> >> sink, then again on the source).
> >>
> >> 2. Stop using bridge chain and call sink ops directly from the source
> >> (this is what Exynos and VC4 do): is flexible and straightforward, gives
> >> full control to the source.
> > And breaks interoperability, because different sources end up calling
> > operations in different orders. We end up having different sinks that
> > expect calls in different ways, and divide the world in sink/source
> > groups that don't interoperate :-(
>
>
> I have an impression you describe current status :) More seriously, it
> is matter of proper specification/documentation/implementations of the
> operations. If we really need strict constraints we could try to
> implement them on protocol level.
>
>
> >
> >> 3. Use different abstractions to enforce proper initialization order
> >> (like extending mipi_dsi_host_ops): requires existence of transport bus
> >> abstraction (only DSI at the moment(?)).
> > A real bus seems overkill, but we could have drm_bridge operations
> > specific to particular hardware interfaces.
> >
> >> ... other ideas?
> > I don't like it because of the amount of work it would require to switch
> > to such a model, but I'm really starting to think that a variation of
> > the second option would be best, where the sink controls the source
> > instead of having the source controlling the sink. It's the sink that
> > knows about its enabling/disabling sequence, and about how the source
> > needs to be controlled to match it.
>
>
> I am afraid it depends on the protocol and cross-calls (source->sink,
> sink->source) can be hard to avoid in case of some protocols.
I'll continue to point out that I'm wearing a noob hat on my head (so
take my opinions for what they're worth), but I'm biased towards
somehow sticking with the existing "bridge chains" just because it
feels like re-designing everything is not something that will get done
in a reasonable time frame. I'm not saying that it's not worth doing
(I have no idea!) and if there's someone who has the vision for it and
wants to make it work then great! ...but personally it would be a bit
much.
How about this as a proposal if it's not too ugly:
1. Add two new bridge functions: after_pre_enable() and
before_post_disable(). Yeah, I don't like the names much either, so
yell if you have better ones.
2. The existing drm_atomic_bridge_chain_pre_enable() /
drm_bridge_chain_pre_enable() (until it's deleted) will call BOTH
pre_enable() and after_pre_enable()
3. The existing drm_atomic_bridge_chain_post_disable() /
drm_bridge_chain_post_disable() (until it's deleted) will call BOTH
before_post_disable() and post_disable()
4. The order for calls for after_pre_enable() and
before_post_disable() will match the "enable" calls.
Writing it out:
pre_enable: start from connector and move to encoder
after_pre_enable: start from encoder and move to connector
enable: start from encoder and move to connector
disable: start from connector and move to encoder
before_post_disable: start from encoder and move to connector
post_disable: start from encoder and move to connector
Then the eDP panel code would move to using only after_pre_enable() /
before_post_disable() for powering itself on/off because This is
saying that for eDP panels that you'd want to power the panel up after
the connector. ...and if there was some eDP panel that was different
it wouldn't be too hard to add a quirk for that panel and have it
power on in pre_enable.
The question is: is the above just a hack to get us out of the current
situation or is it a reasonable design? Let's think about it. In
general the above design allows bridges closer to the connector to
control whether they are powered before or after bridges that are
farther from the connector. So if we have:
encoder (A) -> bridge (B) -> bridge (C) -> connector (D)
* D is in control of whether it gets powered before or after A/B/C
* C is in control of whether it gets powered before or after A/B
* B is in control of whether it gets powered before or after A
Here the encoder has the least flexibility. In much of the discussion
so far, however, it sounded like the connector was the piece that
needed the most control, so I think this is right.
Now let's think about if we could do better, like perhaps giving "full
flexibility" where everything in the chain can request whether they
get pre_enable() before or after their sources / sinks. Looking at
this a little, I don't think the "full flexibility" can really work
and also accomplish interoperability well. You can simply imagine two
bridges, like (B) and (C) above, where:
* Bridge B says that it needs to be pre_enabled before its output (AKA Bridge C)
* Bridge C says that it needs to be pre_enabled before its input (AKA Bridge B)
That would simply be impossible to resolve. Trying to figure out what
to do here and in general all the rules for honoring the requests for
all the bridges in the chain seems really complex and I'm not
convinced that the result would be better than the simple "give the
control to whatever's closer to the connector" that my scheme
accomplishes.
So I guess I'd summarize by saying that the above proposal, while not
a panacea, seems like it has real beneficial properties and maybe
isn't a "hack"?
-Doug
Hi Andrzej,
On Mon, Oct 25, 2021 at 10:11:47PM +0200, Andrzej Hajda wrote:
> On 25.10.2021 13:21, Laurent Pinchart wrote:
> > On Mon, Oct 25, 2021 at 01:00:10PM +0200, Andrzej Hajda wrote:
> >> On 21.10.2021 22:21, Sam Ravnborg wrote:
> >>> On Thu, Oct 21, 2021 at 12:29:01PM -0700, Douglas Anderson wrote:
> >>>> Right now, the chaining order of
> >>>> pre_enable/enable/disable/post_disable looks like this:
> >>>>
> >>>> pre_enable: start from connector and move to encoder
> >>>> enable: start from encoder and move to connector
> >>>> disable: start from connector and move to encoder
> >>>> post_disable: start from encoder and move to connector
> >>>>
> >>>> In the above, it can be seen that at least pre_enable() and
> >>>> post_disable() are opposites of each other and enable() and disable()
> >>>> are opposites. However, it seems broken that pre_enable() and enable()
> >>>> would not move in the same direction. In other parts of Linux you can
> >>>> see that various stages move in the same order. For instance, during
> >>>> system suspend the "early" calls run in the same order as the normal
> >>>> calls run in the same order as the "late" calls run in the same order
> >>>> as the "noirq" calls.
> >>>>
> >>>> Let fix the above so that it makes more sense. Now we'll have:
> >>>>
> >>>> pre_enable: start from encoder and move to connector
> >>>> enable: start from encoder and move to connector
> >>>> disable: start from connector and move to encoder
> >>>> post_disable: start from connector and move to encoder
> >>>>
> >>>> This order is chosen because if there are parent-child relationships
> >>>> anywhere I would expect that the encoder would be a parent and the
> >>>> connector a child--not the other way around.
> >>>
> >>> This makes good sense as you describe it. I hope others can add more
> >>> useful feedback.
> >>> Added Andrzej Hajda <[email protected]> to the mail, as he have
> >>> expressed concerns with the chain of bridges before.
> >>
> >> Thanks Sam, but I am not sure about useful feedback - when I see bridge
> >> chain issues it automatically triggers "whining mode" in my head :)
> >>
> >>>> This can be important when using the DP AUX bus to instantiate a
> >>>> panel. The DP AUX bus is likely part of a bridge driver and is a
> >>>> parent of the panel. We'd like the bridge to be pre_enabled before the
> >>>> panel and the panel to be post_disabled before the
> >>>> bridge. Specifically, this allows pm_runtime_put_sync_suspend() in a
> >>>> bridge driver's post_suspend to work properly even a panel is under
> >>>> it.
> >>>>
> >>>> NOTE: it's entirely possible that this change could break someone who
> >>>> was relying on the old order. Hopefully this isn't the case, but if
> >>>> this does break someone it seems like it's better to do it sonner
> >>>> rather than later so we can fix everyone to handle the order that
> >>>> makes the most sense.
> >>
> >> It will break for sure. So the question is: if it is worth changing?
> >>
> >> New order seems good for eDP, DSI sinks [1], probably other as well.
> >>
> >> Old order is better for example for THC63LVD1024 [2 p. 20], I guess for
> >> many other sinks as well.
> >>
> >> I am not even sure if it is protocol specific (LVDS, RGB, HDMI,...), or
> >> it depends on specific hw pairs (source->sink).
> >>
> >> This is why I complain about the bridge chain - assumption that one
> >> fixed call order will work for all setups seems to me ridiculous.
> >>
> >> Going back to the question - changing the order from fixed one to
> >> another fixed one will not solve general issue.
> >>
> >> What can we do then?
> >>
> >> 1. Configurable call order? Probably doable: every chain element should
> >> expose info if it's call should be before or after source, then some
> >> core helper will create queue of callbacks. Seems quite complicated,
> >> hides the logic from implementer and not fully flexible (for example,
> >> there are protocols which require to perform sth on source, then on
> >> sink, then again on the source).
> >>
> >> 2. Stop using bridge chain and call sink ops directly from the source
> >> (this is what Exynos and VC4 do): is flexible and straightforward, gives
> >> full control to the source.
> >
> > And breaks interoperability, because different sources end up calling
> > operations in different orders. We end up having different sinks that
> > expect calls in different ways, and divide the world in sink/source
> > groups that don't interoperate :-(
>
> I have an impression you describe current status :) More seriously, it
> is matter of proper specification/documentation/implementations of the
> operations. If we really need strict constraints we could try to
> implement them on protocol level.
I certainly wouldn't complain if we had better documented operations :-)
> >> 3. Use different abstractions to enforce proper initialization order
> >> (like extending mipi_dsi_host_ops): requires existence of transport bus
> >> abstraction (only DSI at the moment(?)).
> >
> > A real bus seems overkill, but we could have drm_bridge operations
> > specific to particular hardware interfaces.
> >
> >> ... other ideas?
> >
> > I don't like it because of the amount of work it would require to switch
> > to such a model, but I'm really starting to think that a variation of
> > the second option would be best, where the sink controls the source
> > instead of having the source controlling the sink. It's the sink that
> > knows about its enabling/disabling sequence, and about how the source
> > needs to be controlled to match it.
>
> I am afraid it depends on the protocol and cross-calls (source->sink,
> sink->source) can be hard to avoid in case of some protocols.
Do you have any particular protocol in mind ?
> >> Another idea, connected to the subject - some protocols require some
> >> negotiations between source and sink bus format, or more steps than
> >> pre_enable, enable ops to establish link. I wonder if encapsulating
> >> drm_bridge in some protocol specific struct wouldn't be a solution, it
> >> can be helpful as well in case of the subject.
> >>
> >> For example:
> >>
> >> struct drm_bridge_edp {
> >>
> >> const struct drm_bridge_edp_funcs *funcs;
> >>
> >> struct drm_bridge base;
> >>
> >> ...
> >>
> >> };
> >>
> >> Then source could promote bridge pointer to bridge_edp pointer (if
> >> applicable) and perform edp specific stuff. To make it working well
> >> pre-enable order should be as proposed in this patchsets (encoder ->
> >> connector), as the source should initiate negotiations.
> >>
> >> Btw this encapsulation stuff above asks to rename drm_bridge to
> >> drm_sink, otherwise it would be confusing as bridges have two ends.
> >
> > drm_sink would be equally confusing when used for devices that have a
> > sink and a source :-) I'm not against a rename though, if we can find a
> > better name.
>
> But in this case we are interested only in sink part of the bridge (OR
> panel). If source is looking for specific bridge or panel
> (drm_of_find_panel_or_bridge), it is in fact looking for sink.
I'm find using "sink" as part of a function name that would look up a
sink, but the device itself isn't necessarily just a sink, so we need a
more generic name.
> >> [1]: I use term sink as short equivalent for 'bridges AND panels'
> >> (another issue in DRMs).
> >>
> >> [2]: https://www.mouser.com/datasheet/2/286/THC63LVD1024-1396205.pdf
> >>
> >>>> A FURTHER NOTE: Looking closer at commit 4e5763f03e10 ("drm/bridge:
> >>>> ti-sn65dsi86: Wrap panel with panel-bridge") you can see that patch
> >>>> inadvertently changed the order of things. The order used to be
> >>>> correct (panel prepare was at the tail of the bridge enable) but it
> >>>> became backwards. We'll restore the original order with this patch.
> >>>>
> >>>> Fixes: 4e5763f03e10 ("drm/bridge: ti-sn65dsi86: Wrap panel with panel-bridge")
> >>>> Fixes: 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked list")
> >>>> Signed-off-by: Douglas Anderson <[email protected]>
> >>>
> >>> To make the patch complete the descriptions in drm_bridge_funcs
> >>> need to be updated to reflect the new reality.
> >>>
> >>>> ---
> >>>>
> >>>> drivers/gpu/drm/drm_bridge.c | 28 ++++++++++++++--------------
> >>>> 1 file changed, 14 insertions(+), 14 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> >>>> index c96847fc0ebc..98808af59afd 100644
> >>>> --- a/drivers/gpu/drm/drm_bridge.c
> >>>> +++ b/drivers/gpu/drm/drm_bridge.c
> >>>> @@ -583,18 +583,14 @@ EXPORT_SYMBOL(drm_bridge_chain_mode_set);
> >>>> void drm_bridge_chain_pre_enable(struct drm_bridge *bridge)
> >>> If you, or someone else, could r-b or ack the pending patches to remove
> >>> this function, this part of the patch would no longer be needed.
> >>>
> >>>> {
> >>>> struct drm_encoder *encoder;
> >>>> - struct drm_bridge *iter;
> >>>>
> >>>> if (!bridge)
> >>>> return;
> >>>>
> >>>> encoder = bridge->encoder;
> >>>> - list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
> >>>> - if (iter->funcs->pre_enable)
> >>>> - iter->funcs->pre_enable(iter);
> >>>> -
> >>>> - if (iter == bridge)
> >>>> - break;
> >>>> + list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
> >>>> + if (bridge->funcs->pre_enable)
> >>>> + bridge->funcs->pre_enable(bridge);
> >>>> }
> >>>> }
> >>>> EXPORT_SYMBOL(drm_bridge_chain_pre_enable);
> >>>> @@ -684,26 +680,30 @@ void drm_atomic_bridge_chain_post_disable(struct drm_bridge *bridge,
> >>>> struct drm_atomic_state *old_state)
> >>>> {
> >>>> struct drm_encoder *encoder;
> >>>> + struct drm_bridge *iter;
> >>> s/iter/bridge/ would make the patch simpler
> >>> And then the bridge argument could be last_bridge or something.
> >>> This would IMO increase readability of the code and make the patch smaller.
> >>>>
> >>>> if (!bridge)
> >>>> return;
> >>>>
> >>>> encoder = bridge->encoder;
> >>>> - list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
> >>>> - if (bridge->funcs->atomic_post_disable) {
> >>>> + list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
> >>>> + if (iter->funcs->atomic_post_disable) {
> >>>> struct drm_bridge_state *old_bridge_state;
> >>>>
> >>>> old_bridge_state =
> >>>> drm_atomic_get_old_bridge_state(old_state,
> >>>> - bridge);
> >>>> + iter);
> >>>> if (WARN_ON(!old_bridge_state))
> >>>> return;
> >>>>
> >>>> - bridge->funcs->atomic_post_disable(bridge,
> >>>> - old_bridge_state);
> >>>> - } else if (bridge->funcs->post_disable) {
> >>>> - bridge->funcs->post_disable(bridge);
> >>>> + iter->funcs->atomic_post_disable(iter,
> >>>> + old_bridge_state);
> >>>> + } else if (iter->funcs->post_disable) {
> >>>> + iter->funcs->post_disable(iter);
> >>>> }
> >>>> +
> >>>> + if (iter == bridge)
> >>>> + break;
> >>> I cannot see why this is needed, we are at the end of the list here
> >>> anyway.
> >>>
> >>>> }
> >>>> }
> >>>> EXPORT_SYMBOL(drm_atomic_bridge_chain_post_disable);
--
Regards,
Laurent Pinchart
On 27.10.2021 01:25, Laurent Pinchart wrote:
> Hi Andrzej,
>
> On Mon, Oct 25, 2021 at 10:11:47PM +0200, Andrzej Hajda wrote:
>> On 25.10.2021 13:21, Laurent Pinchart wrote:
>>> On Mon, Oct 25, 2021 at 01:00:10PM +0200, Andrzej Hajda wrote:
>>>> On 21.10.2021 22:21, Sam Ravnborg wrote:
>>>>> On Thu, Oct 21, 2021 at 12:29:01PM -0700, Douglas Anderson wrote:
>>>>>> Right now, the chaining order of
>>>>>> pre_enable/enable/disable/post_disable looks like this:
>>>>>>
>>>>>> pre_enable: start from connector and move to encoder
>>>>>> enable: start from encoder and move to connector
>>>>>> disable: start from connector and move to encoder
>>>>>> post_disable: start from encoder and move to connector
>>>>>>
>>>>>> In the above, it can be seen that at least pre_enable() and
>>>>>> post_disable() are opposites of each other and enable() and disable()
>>>>>> are opposites. However, it seems broken that pre_enable() and enable()
>>>>>> would not move in the same direction. In other parts of Linux you can
>>>>>> see that various stages move in the same order. For instance, during
>>>>>> system suspend the "early" calls run in the same order as the normal
>>>>>> calls run in the same order as the "late" calls run in the same order
>>>>>> as the "noirq" calls.
>>>>>>
>>>>>> Let fix the above so that it makes more sense. Now we'll have:
>>>>>>
>>>>>> pre_enable: start from encoder and move to connector
>>>>>> enable: start from encoder and move to connector
>>>>>> disable: start from connector and move to encoder
>>>>>> post_disable: start from connector and move to encoder
>>>>>>
>>>>>> This order is chosen because if there are parent-child relationships
>>>>>> anywhere I would expect that the encoder would be a parent and the
>>>>>> connector a child--not the other way around.
>>>>> This makes good sense as you describe it. I hope others can add more
>>>>> useful feedback.
>>>>> Added Andrzej Hajda <[email protected]> to the mail, as he have
>>>>> expressed concerns with the chain of bridges before.
>>>> Thanks Sam, but I am not sure about useful feedback - when I see bridge
>>>> chain issues it automatically triggers "whining mode" in my head :)
>>>>
>>>>>> This can be important when using the DP AUX bus to instantiate a
>>>>>> panel. The DP AUX bus is likely part of a bridge driver and is a
>>>>>> parent of the panel. We'd like the bridge to be pre_enabled before the
>>>>>> panel and the panel to be post_disabled before the
>>>>>> bridge. Specifically, this allows pm_runtime_put_sync_suspend() in a
>>>>>> bridge driver's post_suspend to work properly even a panel is under
>>>>>> it.
>>>>>>
>>>>>> NOTE: it's entirely possible that this change could break someone who
>>>>>> was relying on the old order. Hopefully this isn't the case, but if
>>>>>> this does break someone it seems like it's better to do it sonner
>>>>>> rather than later so we can fix everyone to handle the order that
>>>>>> makes the most sense.
>>>> It will break for sure. So the question is: if it is worth changing?
>>>>
>>>> New order seems good for eDP, DSI sinks [1], probably other as well.
>>>>
>>>> Old order is better for example for THC63LVD1024 [2 p. 20], I guess for
>>>> many other sinks as well.
>>>>
>>>> I am not even sure if it is protocol specific (LVDS, RGB, HDMI,...), or
>>>> it depends on specific hw pairs (source->sink).
>>>>
>>>> This is why I complain about the bridge chain - assumption that one
>>>> fixed call order will work for all setups seems to me ridiculous.
>>>>
>>>> Going back to the question - changing the order from fixed one to
>>>> another fixed one will not solve general issue.
>>>>
>>>> What can we do then?
>>>>
>>>> 1. Configurable call order? Probably doable: every chain element should
>>>> expose info if it's call should be before or after source, then some
>>>> core helper will create queue of callbacks. Seems quite complicated,
>>>> hides the logic from implementer and not fully flexible (for example,
>>>> there are protocols which require to perform sth on source, then on
>>>> sink, then again on the source).
>>>>
>>>> 2. Stop using bridge chain and call sink ops directly from the source
>>>> (this is what Exynos and VC4 do): is flexible and straightforward, gives
>>>> full control to the source.
>>> And breaks interoperability, because different sources end up calling
>>> operations in different orders. We end up having different sinks that
>>> expect calls in different ways, and divide the world in sink/source
>>> groups that don't interoperate :-(
>> I have an impression you describe current status :) More seriously, it
>> is matter of proper specification/documentation/implementations of the
>> operations. If we really need strict constraints we could try to
>> implement them on protocol level.
> I certainly wouldn't complain if we had better documented operations :-)
>
>>>> 3. Use different abstractions to enforce proper initialization order
>>>> (like extending mipi_dsi_host_ops): requires existence of transport bus
>>>> abstraction (only DSI at the moment(?)).
>>> A real bus seems overkill, but we could have drm_bridge operations
>>> specific to particular hardware interfaces.
>>>
>>>> ... other ideas?
>>> I don't like it because of the amount of work it would require to switch
>>> to such a model, but I'm really starting to think that a variation of
>>> the second option would be best, where the sink controls the source
>>> instead of having the source controlling the sink. It's the sink that
>>> knows about its enabling/disabling sequence, and about how the source
>>> needs to be controlled to match it.
>> I am afraid it depends on the protocol and cross-calls (source->sink,
>> sink->source) can be hard to avoid in case of some protocols.
> Do you have any particular protocol in mind ?
I guess I made my claim too early, as I do not know how would you like
to implement this "sink controls the source".
So the main question is why do you think sink should control the source,
leaving aside the current model (which looks for me source control sink)
I do not see why sink->source would be better than source->sink.
Going back to your question in Exynos DSI there were following stages,
according to vendor code:
1. DSI host on.
2. DSI dev on.
3. DSI host init.
4. DSI dev init.
5. DSI host start video.
6. DSI dev display_on.
How would you put it into framework?
>
>>>> Another idea, connected to the subject - some protocols require some
>>>> negotiations between source and sink bus format, or more steps than
>>>> pre_enable, enable ops to establish link. I wonder if encapsulating
>>>> drm_bridge in some protocol specific struct wouldn't be a solution, it
>>>> can be helpful as well in case of the subject.
>>>>
>>>> For example:
>>>>
>>>> struct drm_bridge_edp {
>>>>
>>>> const struct drm_bridge_edp_funcs *funcs;
>>>>
>>>> struct drm_bridge base;
>>>>
>>>> ...
>>>>
>>>> };
>>>>
>>>> Then source could promote bridge pointer to bridge_edp pointer (if
>>>> applicable) and perform edp specific stuff. To make it working well
>>>> pre-enable order should be as proposed in this patchsets (encoder ->
>>>> connector), as the source should initiate negotiations.
>>>>
>>>> Btw this encapsulation stuff above asks to rename drm_bridge to
>>>> drm_sink, otherwise it would be confusing as bridges have two ends.
>>> drm_sink would be equally confusing when used for devices that have a
>>> sink and a source :-) I'm not against a rename though, if we can find a
>>> better name.
>> But in this case we are interested only in sink part of the bridge (OR
>> panel). If source is looking for specific bridge or panel
>> (drm_of_find_panel_or_bridge), it is in fact looking for sink.
> I'm find using "sink" as part of a function name that would look up a
> sink, but the device itself isn't necessarily just a sink, so we need a
> more generic name.
source is looking for the sink, and using only the sink, what is behind
the sink is sink's private matter :)
I guess you have an approach that all these bridges/sinks/crtcs/.... are
parts of drm_dev and drm_dev should control them directly. My approach
is more local - they are part of drm_dev but mainly they 'talks' only to
their neighbours- sinks or sources, from drm_dev they require only few
global settings, and drm_dev controls them indirectly.
Regards
Andrzej
>
>>>> [1]: I use term sink as short equivalent for 'bridges AND panels'
>>>> (another issue in DRMs).
>>>>
>>>> [2]: https://www.mouser.com/datasheet/2/286/THC63LVD1024-1396205.pdf
>>>>
>>>>>> A FURTHER NOTE: Looking closer at commit 4e5763f03e10 ("drm/bridge:
>>>>>> ti-sn65dsi86: Wrap panel with panel-bridge") you can see that patch
>>>>>> inadvertently changed the order of things. The order used to be
>>>>>> correct (panel prepare was at the tail of the bridge enable) but it
>>>>>> became backwards. We'll restore the original order with this patch.
>>>>>>
>>>>>> Fixes: 4e5763f03e10 ("drm/bridge: ti-sn65dsi86: Wrap panel with panel-bridge")
>>>>>> Fixes: 05193dc38197 ("drm/bridge: Make the bridge chain a double-linked list")
>>>>>> Signed-off-by: Douglas Anderson <[email protected]>
>>>>> To make the patch complete the descriptions in drm_bridge_funcs
>>>>> need to be updated to reflect the new reality.
>>>>>
>>>>>> ---
>>>>>>
>>>>>> drivers/gpu/drm/drm_bridge.c | 28 ++++++++++++++--------------
>>>>>> 1 file changed, 14 insertions(+), 14 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
>>>>>> index c96847fc0ebc..98808af59afd 100644
>>>>>> --- a/drivers/gpu/drm/drm_bridge.c
>>>>>> +++ b/drivers/gpu/drm/drm_bridge.c
>>>>>> @@ -583,18 +583,14 @@ EXPORT_SYMBOL(drm_bridge_chain_mode_set);
>>>>>> void drm_bridge_chain_pre_enable(struct drm_bridge *bridge)
>>>>> If you, or someone else, could r-b or ack the pending patches to remove
>>>>> this function, this part of the patch would no longer be needed.
>>>>>
>>>>>> {
>>>>>> struct drm_encoder *encoder;
>>>>>> - struct drm_bridge *iter;
>>>>>>
>>>>>> if (!bridge)
>>>>>> return;
>>>>>>
>>>>>> encoder = bridge->encoder;
>>>>>> - list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
>>>>>> - if (iter->funcs->pre_enable)
>>>>>> - iter->funcs->pre_enable(iter);
>>>>>> -
>>>>>> - if (iter == bridge)
>>>>>> - break;
>>>>>> + list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
>>>>>> + if (bridge->funcs->pre_enable)
>>>>>> + bridge->funcs->pre_enable(bridge);
>>>>>> }
>>>>>> }
>>>>>> EXPORT_SYMBOL(drm_bridge_chain_pre_enable);
>>>>>> @@ -684,26 +680,30 @@ void drm_atomic_bridge_chain_post_disable(struct drm_bridge *bridge,
>>>>>> struct drm_atomic_state *old_state)
>>>>>> {
>>>>>> struct drm_encoder *encoder;
>>>>>> + struct drm_bridge *iter;
>>>>> s/iter/bridge/ would make the patch simpler
>>>>> And then the bridge argument could be last_bridge or something.
>>>>> This would IMO increase readability of the code and make the patch smaller.
>>>>>>
>>>>>> if (!bridge)
>>>>>> return;
>>>>>>
>>>>>> encoder = bridge->encoder;
>>>>>> - list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
>>>>>> - if (bridge->funcs->atomic_post_disable) {
>>>>>> + list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
>>>>>> + if (iter->funcs->atomic_post_disable) {
>>>>>> struct drm_bridge_state *old_bridge_state;
>>>>>>
>>>>>> old_bridge_state =
>>>>>> drm_atomic_get_old_bridge_state(old_state,
>>>>>> - bridge);
>>>>>> + iter);
>>>>>> if (WARN_ON(!old_bridge_state))
>>>>>> return;
>>>>>>
>>>>>> - bridge->funcs->atomic_post_disable(bridge,
>>>>>> - old_bridge_state);
>>>>>> - } else if (bridge->funcs->post_disable) {
>>>>>> - bridge->funcs->post_disable(bridge);
>>>>>> + iter->funcs->atomic_post_disable(iter,
>>>>>> + old_bridge_state);
>>>>>> + } else if (iter->funcs->post_disable) {
>>>>>> + iter->funcs->post_disable(iter);
>>>>>> }
>>>>>> +
>>>>>> + if (iter == bridge)
>>>>>> + break;
>>>>> I cannot see why this is needed, we are at the end of the list here
>>>>> anyway.
>>>>>
>>>>>> }
>>>>>> }
>>>>>> EXPORT_SYMBOL(drm_atomic_bridge_chain_post_disable);