Hi,
On 05/19/2015 03:04 PM, Daniel Vetter wrote:
On Tue, May 19, 2015 at 02:05:04PM +0530, Archit Taneja wrote:
Allow drm_bridge objects to link to each other in order to form an encoder
chain. The requirement for creating a chain of bridges comes because the
MSM drm driver uses up its encoder and bridge objects for blocks within
the SoC itself. There isn't anything left to use if the SoC display output
is connected to an external encoder IC. Having an additional bridge
connected to the existing bridge helps here. In general, it is possible for
platforms to have multiple devices between the encoder and the
connector/panel that require some sort of configuration.
We create drm bridge helper functions corresponding to each op in
'drm_bridge_funcs'. These helpers call the corresponding
'drm_bridge_funcs' op for the entire chain of bridges. These helpers are
used internally by drm_atomic_helper.c and drm_crtc_helper.c.
The drm_bridge_enable/pre_enable helpers execute enable/pre_enable ops of
the bridge closet to the encoder, and proceed until the last bridge in the
chain is enabled. The same holds for drm_bridge_mode_set/mode_fixup
helpers. The drm_bridge_disable/post_disable helpers disable the last
bridge in the chain first, and proceed until the first bridge in the chain
is disabled.
drm_bridge_attach() remains the same. As before, the driver calling this
function should make sure it has set the links correctly. The order in
which the bridges are connected to each other determines the order in which
the calls are made. One requirement is that every bridge in the chain
should point the parent encoder object. This is required since bridge
drivers expect a valid encoder pointer in drm_bridge. For example, consider
a chain where an encoder's output is connected to bridge1, and bridge1's
output is connected to bridge2:
/* Like before, attach bridge to an encoder */
bridge1->encoder = encoder;
ret = drm_bridge_attach(dev, bridge1);
..
/*
* set the first bridge's 'next' bridge to bridge2, set its encoder
* as bridge1's encoder
*/
bridge1->next = bridge2
bridge2->encoder = bridge1->encoder;
ret = drm_bridge_attach(dev, bridge2);
...
...
This method of bridge chaining isn't intrusive and existing drivers that
use drm_bridge will behave the same way as before. The bridge helpers also
cleans up the atomic and crtc helper files a bit.
Reviewed-by: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
Reviewed-by: Rob Clark <robdclark@xxxxxxxxx>
Signed-off-by: Archit Taneja <architt@xxxxxxxxxxxxxx>
Two comments below about the nesting of callers. lgtm otherwise.
-Daniel
---
v3:
- Add headerdocs for the new functions
v2:
- Add EXPORT_SYMBOL for the new functions
- Fix logic issue in mode_fixup()
drivers/gpu/drm/drm_atomic_helper.c | 29 +++-----
drivers/gpu/drm/drm_bridge.c | 144 ++++++++++++++++++++++++++++++++++++
drivers/gpu/drm/drm_crtc_helper.c | 54 +++++---------
include/drm/drm_crtc.h | 14 ++++
4 files changed, 188 insertions(+), 53 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 1d2ca52..d6c85c0 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -281,14 +281,11 @@ mode_fixup(struct drm_atomic_state *state)
encoder = conn_state->best_encoder;
funcs = encoder->helper_private;
- if (encoder->bridge && encoder->bridge->funcs->mode_fixup) {
- ret = encoder->bridge->funcs->mode_fixup(
- encoder->bridge, &crtc_state->mode,
- &crtc_state->adjusted_mode);
- if (!ret) {
- DRM_DEBUG_ATOMIC("Bridge fixup failed\n");
- return -EINVAL;
- }
+ ret = drm_bridge_mode_fixup(encoder->bridge, &crtc_state->mode,
+ &crtc_state->adjusted_mode);
+ if (!ret) {
+ DRM_DEBUG_ATOMIC("Bridge fixup failed\n");
+ return -EINVAL;
}
if (funcs->atomic_check) {
@@ -578,8 +575,7 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
* Each encoder has at most one connector (since we always steal
* it away), so we won't call disable hooks twice.
*/
- if (encoder->bridge)
- encoder->bridge->funcs->disable(encoder->bridge);
+ drm_bridge_disable(encoder->bridge);
/* Right function depends upon target state. */
if (connector->state->crtc && funcs->prepare)
@@ -589,8 +585,7 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
else
funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
- if (encoder->bridge)
- encoder->bridge->funcs->post_disable(encoder->bridge);
+ drm_bridge_post_disable(encoder->bridge);
}
for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
@@ -713,9 +708,7 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state)
if (funcs->mode_set)
funcs->mode_set(encoder, mode, adjusted_mode);
- if (encoder->bridge && encoder->bridge->funcs->mode_set)
- encoder->bridge->funcs->mode_set(encoder->bridge,
- mode, adjusted_mode);
+ drm_bridge_mode_set(encoder->bridge, mode, adjusted_mode);
}
}
@@ -809,16 +802,14 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
* Each encoder has at most one connector (since we always steal
* it away), so we won't call enable hooks twice.
*/
- if (encoder->bridge)
- encoder->bridge->funcs->pre_enable(encoder->bridge);
+ drm_bridge_pre_enable(encoder->bridge);
if (funcs->enable)
funcs->enable(encoder);
else
funcs->commit(encoder);
- if (encoder->bridge)
- encoder->bridge->funcs->enable(encoder->bridge);
+ drm_bridge_enable(encoder->bridge);
}
}
EXPORT_SYMBOL(drm_atomic_helper_commit_modeset_enables);
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index eaa5790..f70e617 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -66,6 +66,150 @@ int drm_bridge_attach(struct drm_device *dev, struct drm_bridge *bridge)
}
EXPORT_SYMBOL(drm_bridge_attach);
+/**
+ * drm_bridge_mode_fixup - fixup proposed mode for all bridges in the
+ * encoder chain
+ * @bridge: bridge control structure
+ * @mode: desired mode to be set for the bridge
+ * @adjusted_mode: updated mode that works for this bridge
+ *
+ * Calls 'mode_fixup' drm_bridge_funcs op for all the bridges in the
+ * encoder chain, starting from the first bridge to the last.
+ *
+ * Note: the bridge passed should be the one closest to the encoder
+ */
+bool drm_bridge_mode_fixup(struct drm_bridge *bridge,
+ const struct drm_display_mode *mode,
+ struct drm_display_mode *adjusted_mode)
+{
+ bool ret = true;
+
+ if (!bridge)
+ return true;
+
+ if (bridge->funcs->mode_fixup)
+ ret = bridge->funcs->mode_fixup(bridge, mode, adjusted_mode);
+
+ ret = ret && drm_bridge_mode_fixup(bridge->next, mode, adjusted_mode);
+
+ return ret;
+}
+EXPORT_SYMBOL(drm_bridge_mode_fixup);
+
+/**
+ * drm_bridge_disable - calls 'disable' drm_bridge_funcs op for all
+ * bridges in the encoder chain.
+ * @bridge: bridge control structure
+ *
+ * Calls 'disable' drm_bridge_funcs op for all the bridges in the encoder
+ * chain, starting from the last bridge to the first. These are called before
+ * calling the encoder's prepare op.
+ *
+ * Note: the bridge passed should be the one closest to the encoder
+ */
+void drm_bridge_disable(struct drm_bridge *bridge)
+{
+ if (!bridge)
+ return;
+
+ drm_bridge_disable(bridge->next);
+
+ bridge->funcs->disable(bridge);
+}
+EXPORT_SYMBOL(drm_bridge_disable);
+
+/**
+ * drm_bridge_post_disable - calls 'post_disable' drm_bridge_funcs op for
+ * all bridges in the encoder chain.
+ * @bridge: bridge control structure
+ *
+ * Calls 'post_disable' drm_bridge_funcs op for all the bridges in the
+ * encoder chain, starting from the last bridge to the first. These are called
+ * after completing the encoder's prepare op.
+ *
+ * Note: the bridge passed should be the one closest to the encoder
+ */
+void drm_bridge_post_disable(struct drm_bridge *bridge)
+{
+ if (!bridge)
+ return;
+
+ drm_bridge_post_disable(bridge->next);
+
+ bridge->funcs->post_disable(bridge);
For symmetry I'd call the post_disable hook for the next brigde _after_
this one. Otherwise we break abstraction. E.g. with 1 bridge we'd have
bridgeA_disable();
encoder_disable();
bridgeA_post_disable();
But if on some other part bridge A is connected to bridge B (i.e. we
replace the encoder with a combo of other_encoder+bridgeA) with your code
the post_disable is suddenly interleaved with the preceeding stages in the
pipeline:
bridgeA_disable();
bridgeB_disable();
other_encoder_disable();
bridgeA_post_disable();
bridgeB_post_disable();
Which means from the pov of bridgeA there's a difference between whether
it's connected to "encoder" or "other_encoder+bridgeB". But if we reorder
the post_disable sequence like I suggest we'll get:
bridgeA_disable();
bridgeB_disable();
other_encoder_disable();
bridgeB_post_disable();
bridgeA_post_disable();
Which means that "encoder" and "other_encoder+bridgeB" look the same for
bridgeA. To avoid confusion the two pipelines in hw are:
encoder ---> bridgeA
other_encoder ---> bridgeB ---> bridgeA
I agree with this, thanks for the explanation. Although, I'm not sure
about the pre_enable part below.
<snip>
+void drm_bridge_pre_enable(struct drm_bridge *bridge)
+{
+ if (!bridge)
+ return;
+
+ bridge->funcs->pre_enable(bridge);
+
+ drm_bridge_pre_enable(bridge->next);
Same as with post_disable, pre_enable for bridge->next should be called
_before_ the pre_enable for this bridge.
The order of nesting suggested by you gives the sequence:
bridgeA_pre_enable();
bridgeB_pre_enable();
other_encoder_enable();
bridgeB_enable();
bridgeA_enable();
This won't work if the bridge A relies on bridge B to be enabled first.
This happens in the encoder path I'm working on:
msm mdp5 encoder -> dsi bridge -> adv7511 dsi to hdmi bridge chip
The adv chip relies on dsi's clock for it to function. If dsi's
pre_enable() isn't called first, then adv's pre_enable would fail.
We could modify the call order in drm_bridge_enable() instead to achieve:
bridgeB_pre_enable();
bridgeA_pre_enable();
other_encoder_enable();
bridgeA_enable();
bridgeB_enable();
This fixes the sequence for pre_enable() calls, but assumes that the
configurations in the enable() don't need to follow a specific sequence
to ensure proper behavior.
pre_enable() should ideally represent things to be done before we enable
the next encoder in the chain. So the sequence right above seems to be
better.
Archit
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
