Hi,
On 25/10/2019 15:29, Neil Armstrong wrote:
On 23/10/2019 17:44, Boris Brezillon wrote:
So that each element in the chain can easily access its predecessor. This will be needed to support bus format negotiation between elements of the bridge chain.
Signed-off-by: Boris Brezillon boris.brezillon@collabora.com
Changes in v3:
- None
Changes in v2:
- Adjust things to the "dummy encoder bridge" change (patch 2 in this series)
drivers/gpu/drm/drm_bridge.c | 171 ++++++++++++++++++++++------------ drivers/gpu/drm/drm_encoder.c | 16 +--- include/drm/drm_bridge.h | 12 ++- include/drm/drm_encoder.h | 9 +- 4 files changed, 135 insertions(+), 73 deletions(-)
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index 54c874493c57..c5cf8a9c4237 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c
[...]
@@ -426,15 +471,23 @@ EXPORT_SYMBOL(drm_atomic_bridge_chain_post_disable); void drm_atomic_bridge_chain_pre_enable(struct drm_bridge *bridge, struct drm_atomic_state *state) {
- struct drm_encoder *encoder;
- struct drm_bridge *iter;
- if (!bridge) return;
- drm_atomic_bridge_chain_pre_enable(bridge->next, state);
- encoder = bridge->encoder;
- list_for_each_entry_reverse(iter, &bridge->encoder->bridge_chain,
chain_node) {
This should use the encoder local variable in list_for_each_entry_reverse()
if (iter->funcs->atomic_pre_enable)iter->funcs->atomic_pre_enable(iter, state);else if (iter->funcs->pre_enable)iter->funcs->pre_enable(iter);
- if (bridge->funcs->atomic_pre_enable)
bridge->funcs->atomic_pre_enable(bridge, state);- else if (bridge->funcs->pre_enable)
bridge->funcs->pre_enable(bridge);
if (iter == bridge)break;- }
} EXPORT_SYMBOL(drm_atomic_bridge_chain_pre_enable);
@@ -453,15 +506,19 @@ EXPORT_SYMBOL(drm_atomic_bridge_chain_pre_enable); void drm_atomic_bridge_chain_enable(struct drm_bridge *bridge, struct drm_atomic_state *state) {
- struct drm_encoder *encoder;
- if (!bridge) return;
- if (bridge->funcs->atomic_enable)
bridge->funcs->atomic_enable(bridge, state);- else if (bridge->funcs->enable)
bridge->funcs->enable(bridge);- drm_atomic_bridge_chain_enable(bridge->next, state);
- encoder = bridge->encoder;
- list_for_each_entry_from(bridge, &bridge->encoder->bridge_chain,
chain_node) {
This should use encoder instead of bridge->encoder otherwise bridge will change and bridge->encoder->bridge_chain won't be valid during the for_each and cause the following :
[ 79.082861] WARNING: CPU: 2 PID: 1999 at drivers/gpu/drm/drm_bridge.c:607 drm_atomic_bridge_chain_enable+0xac/0xc0 ... [ 79.210153] drm_atomic_bridge_chain_enable+0xac/0xc0 [ 79.215156] drm_atomic_helper_commit_modeset_enables+0x138/0x248 [ 79.221190] drm_atomic_helper_commit_tail_rpm+0x2c/0x78 [ 79.226452] commit_tail+0x50/0xc0 [ 79.229815] drm_atomic_helper_commit+0xe8/0x168 [ 79.234386] drm_atomic_commit+0x48/0x58 [ 79.238269] drm_client_modeset_commit_atomic.isra.15+0x184/0x220 [ 79.244305] drm_client_modeset_commit_force+0x64/0x1a0 [ 79.249482] drm_fb_helper_restore_fbdev_mode_unlocked+0x70/0xe8 [ 79.255432] drm_fbdev_client_restore+0x14/0x20 [ 79.259920] drm_client_dev_restore+0x80/0xd8 [ 79.264231] drm_lastclose+0x4c/0x58 [ 79.267765] drm_release+0xa8/0x178
if (bridge->funcs->atomic_enable)bridge->funcs->atomic_enable(bridge, state);else if (bridge->funcs->enable)bridge->funcs->enable(bridge);- }
} EXPORT_SYMBOL(drm_atomic_bridge_chain_enable);
diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c index 4fe9e723e227..e555281f43d4 100644 --- a/drivers/gpu/drm/drm_encoder.c +++ b/drivers/gpu/drm/drm_encoder.c @@ -140,6 +140,7 @@ int drm_encoder_init(struct drm_device *dev, goto out_put; }
- INIT_LIST_HEAD(&encoder->bridge_chain); list_add_tail(&encoder->head, &dev->mode_config.encoder_list); encoder->index = dev->mode_config.num_encoder++;
@@ -160,23 +161,16 @@ EXPORT_SYMBOL(drm_encoder_init); void drm_encoder_cleanup(struct drm_encoder *encoder) { struct drm_device *dev = encoder->dev;
struct drm_bridge *bridge, *next;
/* Note that the encoder_list is considered to be static; should we
- remove the drm_encoder at runtime we would have to decrement all
- the indices on the drm_encoder after us in the encoder_list.
*/
- if (encoder->bridge) {
struct drm_bridge *bridge;struct drm_bridge *next;bridge = drm_bridge_chain_get_first_bridge(encoder);while (bridge) {next = bridge->next;drm_bridge_detach(bridge);bridge = next;}- }
list_for_each_entry_safe(bridge, next, &encoder->bridge_chain,
chain_node)drm_bridge_detach(bridge);drm_mode_object_unregister(dev, &encoder->base); kfree(encoder->name);
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index 27eef63ce0ff..3ab16c95e59e 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -384,8 +384,8 @@ struct drm_bridge { struct drm_device *dev; /** @encoder: encoder to which this bridge is connected */ struct drm_encoder *encoder;
- /** @next: the next bridge in the encoder chain */
- struct drm_bridge *next;
- /** @chain_node: used to form a bridge chain */
- struct list_head chain_node;
#ifdef CONFIG_OF /** @of_node: device node pointer to the bridge */ struct device_node *of_node; @@ -420,7 +420,10 @@ int drm_bridge_attach(struct drm_encoder *encoder, struct drm_bridge *bridge, static inline struct drm_bridge * drm_bridge_chain_get_next_bridge(struct drm_bridge *bridge) {
- return bridge->next;
- if (list_is_last(&bridge->chain_node, &bridge->encoder->bridge_chain))
return NULL;- return list_next_entry(bridge, chain_node);
}
/** @@ -434,7 +437,8 @@ drm_bridge_chain_get_next_bridge(struct drm_bridge *bridge) static inline struct drm_bridge * drm_bridge_chain_get_first_bridge(struct drm_encoder *encoder) {
- return encoder->bridge;
- return list_first_entry_or_null(&encoder->bridge_chain,
struct drm_bridge, chain_node);}
bool drm_bridge_chain_mode_fixup(struct drm_bridge *bridge, diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h index f06164f44efe..9b3dde177c81 100644 --- a/include/drm/drm_encoder.h +++ b/include/drm/drm_encoder.h @@ -172,7 +172,14 @@ struct drm_encoder { * &drm_connector_state.crtc. */ struct drm_crtc *crtc;
- struct drm_bridge *bridge;
- /**
* @bridge_chain: Bridges attached to this encoder. The first entry of* this list is always &drm_encoder.bridge. It may be followed by other* bridge entities.*/- struct list_head bridge_chain;
- const struct drm_encoder_funcs *funcs; const struct drm_encoder_helper_funcs *helper_private;
};
Reviewed-by: Neil Armstrong narmstrong@baylibre.com
With these fixed, you can keep my Reviewed-by
Neil