| Message ID | 1432024505-4139-1-git-send-email-architt@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@linux.intel.com> > Reviewed-by: Rob Clark <robdclark@gmail.com> > Signed-off-by: Archit Taneja <architt@codeaurora.org> 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 > +} > +EXPORT_SYMBOL(drm_bridge_post_disable); > + > +/** > + * drm_bridge_mode_set - set 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_set' 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 > + */ > +void drm_bridge_mode_set(struct drm_bridge *bridge, > + struct drm_display_mode *mode, > + struct drm_display_mode *adjusted_mode) > +{ > + if (!bridge) > + return; > + > + if (bridge->funcs->mode_set) > + bridge->funcs->mode_set(bridge, mode, adjusted_mode); > + > + drm_bridge_mode_set(bridge->next, mode, adjusted_mode); > +} > +EXPORT_SYMBOL(drm_bridge_mode_set); > + > +/** > + * drm_bridge_pre_enable - calls 'pre_enable' drm_bridge_funcs op for all > + * bridges in the encoder chain. > + * @bridge: bridge control structure > + * > + * Calls 'pre_enable' drm_bridge_funcs op for all the bridges in the encoder > + * chain, starting from the first bridge to the last. These are called > + * before calling the encoder's commit op. > + * > + * Note: the bridge passed should be the one closest to the encoder > + */ > +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. > +} > +EXPORT_SYMBOL(drm_bridge_pre_enable); > + > +/** > + * drm_bridge_enable - calls 'enable' drm_bridge_funcs op for all bridges > + * in the encoder chain. > + * @bridge: bridge control structure > + * > + * Calls 'enable' drm_bridge_funcs op for all the bridges in the encoder > + * chain, starting from the first bridge to the last. These are called > + * after completing the encoder's commit op. > + * > + * Note that the bridge passed should be the one closest to the encoder > + */ > +void drm_bridge_enable(struct drm_bridge *bridge) > +{ > + if (!bridge) > + return; > + > + bridge->funcs->enable(bridge); > + > + drm_bridge_enable(bridge->next); > +} > +EXPORT_SYMBOL(drm_bridge_enable); > + > #ifdef CONFIG_OF > struct drm_bridge *of_drm_find_bridge(struct device_node *np) > { > diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c > index ab00286..ff9b338 100644 > --- a/drivers/gpu/drm/drm_crtc_helper.c > +++ b/drivers/gpu/drm/drm_crtc_helper.c > @@ -163,16 +163,14 @@ drm_encoder_disable(struct drm_encoder *encoder) > { > const struct drm_encoder_helper_funcs *encoder_funcs = encoder->helper_private; > > - if (encoder->bridge) > - encoder->bridge->funcs->disable(encoder->bridge); > + drm_bridge_disable(encoder->bridge); > > if (encoder_funcs->disable) > (*encoder_funcs->disable)(encoder); > else > (*encoder_funcs->dpms)(encoder, DRM_MODE_DPMS_OFF); > > - if (encoder->bridge) > - encoder->bridge->funcs->post_disable(encoder->bridge); > + drm_bridge_post_disable(encoder->bridge); > } > > static void __drm_helper_disable_unused_functions(struct drm_device *dev) > @@ -312,13 +310,11 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, > if (encoder->crtc != crtc) > continue; > > - if (encoder->bridge && encoder->bridge->funcs->mode_fixup) { > - ret = encoder->bridge->funcs->mode_fixup( > - encoder->bridge, mode, adjusted_mode); > - if (!ret) { > - DRM_DEBUG_KMS("Bridge fixup failed\n"); > - goto done; > - } > + ret = drm_bridge_mode_fixup(encoder->bridge, > + mode, adjusted_mode); > + if (!ret) { > + DRM_DEBUG_KMS("Bridge fixup failed\n"); > + goto done; > } > > encoder_funcs = encoder->helper_private; > @@ -343,15 +339,13 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, > if (encoder->crtc != crtc) > continue; > > - if (encoder->bridge) > - encoder->bridge->funcs->disable(encoder->bridge); > + drm_bridge_disable(encoder->bridge); > > encoder_funcs = encoder->helper_private; > /* Disable the encoders as the first thing we do. */ > encoder_funcs->prepare(encoder); > > - if (encoder->bridge) > - encoder->bridge->funcs->post_disable(encoder->bridge); > + drm_bridge_post_disable(encoder->bridge); > } > > drm_crtc_prepare_encoders(dev); > @@ -376,9 +370,7 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, > encoder_funcs = encoder->helper_private; > encoder_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); > } > > /* Now enable the clocks, plane, pipe, and connectors that we set up. */ > @@ -389,14 +381,12 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, > if (encoder->crtc != crtc) > continue; > > - if (encoder->bridge) > - encoder->bridge->funcs->pre_enable(encoder->bridge); > + drm_bridge_pre_enable(encoder->bridge); > > encoder_funcs = encoder->helper_private; > encoder_funcs->commit(encoder); > > - if (encoder->bridge) > - encoder->bridge->funcs->enable(encoder->bridge); > + drm_bridge_enable(encoder->bridge); > } > > /* Calculate and store various constants which > @@ -735,23 +725,19 @@ static void drm_helper_encoder_dpms(struct drm_encoder *encoder, int mode) > struct drm_bridge *bridge = encoder->bridge; > const struct drm_encoder_helper_funcs *encoder_funcs; > > - if (bridge) { > - if (mode == DRM_MODE_DPMS_ON) > - bridge->funcs->pre_enable(bridge); > - else > - bridge->funcs->disable(bridge); > - } > + if (mode == DRM_MODE_DPMS_ON) > + drm_bridge_pre_enable(bridge); > + else > + drm_bridge_disable(bridge); > > encoder_funcs = encoder->helper_private; > if (encoder_funcs->dpms) > encoder_funcs->dpms(encoder, mode); > > - if (bridge) { > - if (mode == DRM_MODE_DPMS_ON) > - bridge->funcs->enable(bridge); > - else > - bridge->funcs->post_disable(bridge); > - } > + if (mode == DRM_MODE_DPMS_ON) > + drm_bridge_enable(bridge); > + else > + drm_bridge_post_disable(bridge); > } > > static int drm_helper_choose_crtc_dpms(struct drm_crtc *crtc) > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index ca71c03..db830f4 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -895,6 +895,8 @@ struct drm_bridge_funcs { > /** > * struct drm_bridge - central DRM bridge control structure > * @dev: DRM device this bridge belongs to > + * @encoder: encoder to which this bridge is connected > + * @next: the next bridge in the encoder chain > * @of_node: device node pointer to the bridge > * @list: to keep track of all added bridges > * @base: base mode object > @@ -904,6 +906,7 @@ struct drm_bridge_funcs { > struct drm_bridge { > struct drm_device *dev; > struct drm_encoder *encoder; > + struct drm_bridge *next; > #ifdef CONFIG_OF > struct device_node *of_node; > #endif > @@ -1230,6 +1233,17 @@ extern void drm_bridge_remove(struct drm_bridge *bridge); > extern struct drm_bridge *of_drm_find_bridge(struct device_node *np); > extern int drm_bridge_attach(struct drm_device *dev, struct drm_bridge *bridge); > > +bool drm_bridge_mode_fixup(struct drm_bridge *bridge, > + const struct drm_display_mode *mode, > + struct drm_display_mode *adjusted_mode); > +void drm_bridge_disable(struct drm_bridge *bridge); > +void drm_bridge_post_disable(struct drm_bridge *bridge); > +void drm_bridge_mode_set(struct drm_bridge *bridge, > + struct drm_display_mode *mode, > + struct drm_display_mode *adjusted_mode); > +void drm_bridge_pre_enable(struct drm_bridge *bridge); > +void drm_bridge_enable(struct drm_bridge *bridge); > + > extern int drm_encoder_init(struct drm_device *dev, > struct drm_encoder *encoder, > const struct drm_encoder_funcs *funcs, > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > hosted by The Linux Foundation >
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@linux.intel.com> >> Reviewed-by: Rob Clark <robdclark@gmail.com> >> Signed-off-by: Archit Taneja <architt@codeaurora.org> > > 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
On Tue, May 19, 2015 at 04:37:44PM +0530, Archit Taneja wrote: > On 05/19/2015 03:04 PM, Daniel Vetter wrote: > >On Tue, May 19, 2015 at 02:05:04PM +0530, Archit Taneja wrote: > >>+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. If you need the clock, then imo that's code which should be in the enable hook, and not in the pre-enable hook. At least thus far the rules have roughly been: - pre_enable: anything that needs to be done _before_ clock+timings are enabled by the source for this bridge. - enable: anything that needs clock+timings to be on from the source for this bridge. Similar for the disable side: - disable still has clocks+timing on. - post_disable should be called after clocks are off. In i915 we once had a callback in between clock enabling and timing enabling, but we've removed that again after some restructuring. Maybe we need such a hook bridges? But that also means we need to split up encoder/crtc callbacks, and that will get nasty real fast. > 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. Nah, that's even more backwards imo. From bridgeA's pov it really should make no difference at all whether the input is directly from an encoder or whether it's "other_encoder+bridgeB". If we leak this detail down the bridge chain, then the abstraction is leaky and bridge drivers will be impossible to be reused. -Daniel
On 05/19/2015 09:00 PM, Daniel Vetter wrote: > On Tue, May 19, 2015 at 04:37:44PM +0530, Archit Taneja wrote: >> On 05/19/2015 03:04 PM, Daniel Vetter wrote: >>> On Tue, May 19, 2015 at 02:05:04PM +0530, Archit Taneja wrote: >>>> +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. > > If you need the clock, then imo that's code which should be in the enable > hook, and not in the pre-enable hook. At least thus far the rules have > roughly been: > - pre_enable: anything that needs to be done _before_ clock+timings are > enabled by the source for this bridge. > - enable: anything that needs clock+timings to be on from the source for > this bridge. Okay. I think I got the rules wrong. I assumed that pre_enable() should do the heavy enabling. This happened because I was looking at the bridges(dsi/hdmi/edp) in msm as reference. Those were actually a special case, where the bridge feeds back the pixel clock to the mdp encoder. That's why everything was stuffed within pre_enable. They can afford to not comply to the rules since they are tightly coupled with the msm driver. > > Similar for the disable side: > - disable still has clocks+timing on. > - post_disable should be called after clocks are off. > > In i915 we once had a callback in between clock enabling and timing > enabling, but we've removed that again after some restructuring. Maybe we > need such a hook bridges? But that also means we need to split up > encoder/crtc callbacks, and that will get nasty real fast. Yeah, let's stay away from that! > >> 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. > > Nah, that's even more backwards imo. From bridgeA's pov it really should > make no difference at all whether the input is directly from an encoder or > whether it's "other_encoder+bridgeB". If we leak this detail down the > bridge chain, then the abstraction is leaky and bridge drivers will be > impossible to be reused. Right. That makes sense. I came up with that to under the assumption that the bridge's source should be ready with clocks and timings, which was wrong. I'll stick to the order you mentioned. Thanks, Archit
On Wed, May 20, 2015 at 10:49:38AM +0530, Archit Taneja wrote: > On 05/19/2015 09:00 PM, Daniel Vetter wrote: > >On Tue, May 19, 2015 at 04:37:44PM +0530, Archit Taneja wrote: > >>On 05/19/2015 03:04 PM, Daniel Vetter wrote: > >>>On Tue, May 19, 2015 at 02:05:04PM +0530, Archit Taneja wrote: > >>>>+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. > > > >If you need the clock, then imo that's code which should be in the enable > >hook, and not in the pre-enable hook. At least thus far the rules have > >roughly been: > >- pre_enable: anything that needs to be done _before_ clock+timings are > > enabled by the source for this bridge. > >- enable: anything that needs clock+timings to be on from the source for > > this bridge. > > Okay. I think I got the rules wrong. I assumed that pre_enable() should do > the heavy enabling. This happened because I was looking at the > bridges(dsi/hdmi/edp) in msm as reference. Those were actually a special > case, where the bridge feeds back the pixel clock to the mdp encoder. That's > why everything was stuffed within pre_enable. They can afford to not comply > to the rules since they are tightly coupled with the msm driver. > > > > >Similar for the disable side: > >- disable still has clocks+timing on. > >- post_disable should be called after clocks are off. > > > >In i915 we once had a callback in between clock enabling and timing > >enabling, but we've removed that again after some restructuring. Maybe we > >need such a hook bridges? But that also means we need to split up > >encoder/crtc callbacks, and that will get nasty real fast. > > Yeah, let's stay away from that! > > > > >>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. > > > >Nah, that's even more backwards imo. From bridgeA's pov it really should > >make no difference at all whether the input is directly from an encoder or > >whether it's "other_encoder+bridgeB". If we leak this detail down the > >bridge chain, then the abstraction is leaky and bridge drivers will be > >impossible to be reused. > > Right. That makes sense. I came up with that to under the assumption that > the bridge's source should be ready with clocks and timings, which was > wrong. I'll stick to the order you mentioned. Since this (how pre/post are wrapped around clock+timing enabling from the source) caused confusion I think we should document this in the DOC kerneldoc for drm_bridge. Can you please create a small patch for that? Maybe even do a new DOC: section with it's own subtitle like "Default sequence for bridge callbacks" -Daniel
On 05/20/2015 12:05 PM, Daniel Vetter wrote: > On Wed, May 20, 2015 at 10:49:38AM +0530, Archit Taneja wrote: >> On 05/19/2015 09:00 PM, Daniel Vetter wrote: >>> On Tue, May 19, 2015 at 04:37:44PM +0530, Archit Taneja wrote: >>>> On 05/19/2015 03:04 PM, Daniel Vetter wrote: >>>>> On Tue, May 19, 2015 at 02:05:04PM +0530, Archit Taneja wrote: >>>>>> +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. >>> >>> If you need the clock, then imo that's code which should be in the enable >>> hook, and not in the pre-enable hook. At least thus far the rules have >>> roughly been: >>> - pre_enable: anything that needs to be done _before_ clock+timings are >>> enabled by the source for this bridge. >>> - enable: anything that needs clock+timings to be on from the source for >>> this bridge. >> >> Okay. I think I got the rules wrong. I assumed that pre_enable() should do >> the heavy enabling. This happened because I was looking at the >> bridges(dsi/hdmi/edp) in msm as reference. Those were actually a special >> case, where the bridge feeds back the pixel clock to the mdp encoder. That's >> why everything was stuffed within pre_enable. They can afford to not comply >> to the rules since they are tightly coupled with the msm driver. >> >>> >>> Similar for the disable side: >>> - disable still has clocks+timing on. >>> - post_disable should be called after clocks are off. >>> >>> In i915 we once had a callback in between clock enabling and timing >>> enabling, but we've removed that again after some restructuring. Maybe we >>> need such a hook bridges? But that also means we need to split up >>> encoder/crtc callbacks, and that will get nasty real fast. >> >> Yeah, let's stay away from that! >> >>> >>>> 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. >>> >>> Nah, that's even more backwards imo. From bridgeA's pov it really should >>> make no difference at all whether the input is directly from an encoder or >>> whether it's "other_encoder+bridgeB". If we leak this detail down the >>> bridge chain, then the abstraction is leaky and bridge drivers will be >>> impossible to be reused. >> >> Right. That makes sense. I came up with that to under the assumption that >> the bridge's source should be ready with clocks and timings, which was >> wrong. I'll stick to the order you mentioned. > > Since this (how pre/post are wrapped around clock+timing enabling from the > source) caused confusion I think we should document this in the DOC > kerneldoc for drm_bridge. Can you please create a small patch for that? > Maybe even do a new DOC: section with it's own subtitle like "Default > sequence for bridge callbacks" Yes, I was planning to add this in the second patch itself. Where do you think drm_bridge documentation fits? I was considering putting it under 'KMS initialization and setup', while pointing out that it isn't exactly a drm_mode_object entity like crtcs or encoders etc. Archit
On Wed, May 20, 2015 at 12:43:40PM +0530, Archit Taneja wrote: > Where do you think drm_bridge documentation fits? I was considering putting > it under 'KMS initialization and setup', while pointing out that it isn't > exactly a drm_mode_object entity like crtcs or encoders etc. Since drm_bridge is not exposed to userspace it's not part of core drm and imo should be a helper library - it's only used by drivers internally to help them implement modesetting. New chapter under "Mode Setting Helper Functions" sounds best to me. Btw drm_bridge being a core part of drm is another thing we probably should fix up eventually. I looked into doing that a while ago, but it wasn't quite that simple. And I don't have a good arm build-test setup, so hard to make sure I've caught all drivers and made sure they depend upon DRM_KMS_HELPER properly. Cheers, Daniel
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); +} +EXPORT_SYMBOL(drm_bridge_post_disable); + +/** + * drm_bridge_mode_set - set 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_set' 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 + */ +void drm_bridge_mode_set(struct drm_bridge *bridge, + struct drm_display_mode *mode, + struct drm_display_mode *adjusted_mode) +{ + if (!bridge) + return; + + if (bridge->funcs->mode_set) + bridge->funcs->mode_set(bridge, mode, adjusted_mode); + + drm_bridge_mode_set(bridge->next, mode, adjusted_mode); +} +EXPORT_SYMBOL(drm_bridge_mode_set); + +/** + * drm_bridge_pre_enable - calls 'pre_enable' drm_bridge_funcs op for all + * bridges in the encoder chain. + * @bridge: bridge control structure + * + * Calls 'pre_enable' drm_bridge_funcs op for all the bridges in the encoder + * chain, starting from the first bridge to the last. These are called + * before calling the encoder's commit op. + * + * Note: the bridge passed should be the one closest to the encoder + */ +void drm_bridge_pre_enable(struct drm_bridge *bridge) +{ + if (!bridge) + return; + + bridge->funcs->pre_enable(bridge); + + drm_bridge_pre_enable(bridge->next); +} +EXPORT_SYMBOL(drm_bridge_pre_enable); + +/** + * drm_bridge_enable - calls 'enable' drm_bridge_funcs op for all bridges + * in the encoder chain. + * @bridge: bridge control structure + * + * Calls 'enable' drm_bridge_funcs op for all the bridges in the encoder + * chain, starting from the first bridge to the last. These are called + * after completing the encoder's commit op. + * + * Note that the bridge passed should be the one closest to the encoder + */ +void drm_bridge_enable(struct drm_bridge *bridge) +{ + if (!bridge) + return; + + bridge->funcs->enable(bridge); + + drm_bridge_enable(bridge->next); +} +EXPORT_SYMBOL(drm_bridge_enable); + #ifdef CONFIG_OF struct drm_bridge *of_drm_find_bridge(struct device_node *np) { diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index ab00286..ff9b338 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -163,16 +163,14 @@ drm_encoder_disable(struct drm_encoder *encoder) { const struct drm_encoder_helper_funcs *encoder_funcs = encoder->helper_private; - if (encoder->bridge) - encoder->bridge->funcs->disable(encoder->bridge); + drm_bridge_disable(encoder->bridge); if (encoder_funcs->disable) (*encoder_funcs->disable)(encoder); else (*encoder_funcs->dpms)(encoder, DRM_MODE_DPMS_OFF); - if (encoder->bridge) - encoder->bridge->funcs->post_disable(encoder->bridge); + drm_bridge_post_disable(encoder->bridge); } static void __drm_helper_disable_unused_functions(struct drm_device *dev) @@ -312,13 +310,11 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, if (encoder->crtc != crtc) continue; - if (encoder->bridge && encoder->bridge->funcs->mode_fixup) { - ret = encoder->bridge->funcs->mode_fixup( - encoder->bridge, mode, adjusted_mode); - if (!ret) { - DRM_DEBUG_KMS("Bridge fixup failed\n"); - goto done; - } + ret = drm_bridge_mode_fixup(encoder->bridge, + mode, adjusted_mode); + if (!ret) { + DRM_DEBUG_KMS("Bridge fixup failed\n"); + goto done; } encoder_funcs = encoder->helper_private; @@ -343,15 +339,13 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, if (encoder->crtc != crtc) continue; - if (encoder->bridge) - encoder->bridge->funcs->disable(encoder->bridge); + drm_bridge_disable(encoder->bridge); encoder_funcs = encoder->helper_private; /* Disable the encoders as the first thing we do. */ encoder_funcs->prepare(encoder); - if (encoder->bridge) - encoder->bridge->funcs->post_disable(encoder->bridge); + drm_bridge_post_disable(encoder->bridge); } drm_crtc_prepare_encoders(dev); @@ -376,9 +370,7 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, encoder_funcs = encoder->helper_private; encoder_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); } /* Now enable the clocks, plane, pipe, and connectors that we set up. */ @@ -389,14 +381,12 @@ bool drm_crtc_helper_set_mode(struct drm_crtc *crtc, if (encoder->crtc != crtc) continue; - if (encoder->bridge) - encoder->bridge->funcs->pre_enable(encoder->bridge); + drm_bridge_pre_enable(encoder->bridge); encoder_funcs = encoder->helper_private; encoder_funcs->commit(encoder); - if (encoder->bridge) - encoder->bridge->funcs->enable(encoder->bridge); + drm_bridge_enable(encoder->bridge); } /* Calculate and store various constants which @@ -735,23 +725,19 @@ static void drm_helper_encoder_dpms(struct drm_encoder *encoder, int mode) struct drm_bridge *bridge = encoder->bridge; const struct drm_encoder_helper_funcs *encoder_funcs; - if (bridge) { - if (mode == DRM_MODE_DPMS_ON) - bridge->funcs->pre_enable(bridge); - else - bridge->funcs->disable(bridge); - } + if (mode == DRM_MODE_DPMS_ON) + drm_bridge_pre_enable(bridge); + else + drm_bridge_disable(bridge); encoder_funcs = encoder->helper_private; if (encoder_funcs->dpms) encoder_funcs->dpms(encoder, mode); - if (bridge) { - if (mode == DRM_MODE_DPMS_ON) - bridge->funcs->enable(bridge); - else - bridge->funcs->post_disable(bridge); - } + if (mode == DRM_MODE_DPMS_ON) + drm_bridge_enable(bridge); + else + drm_bridge_post_disable(bridge); } static int drm_helper_choose_crtc_dpms(struct drm_crtc *crtc) diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index ca71c03..db830f4 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -895,6 +895,8 @@ struct drm_bridge_funcs { /** * struct drm_bridge - central DRM bridge control structure * @dev: DRM device this bridge belongs to + * @encoder: encoder to which this bridge is connected + * @next: the next bridge in the encoder chain * @of_node: device node pointer to the bridge * @list: to keep track of all added bridges * @base: base mode object @@ -904,6 +906,7 @@ struct drm_bridge_funcs { struct drm_bridge { struct drm_device *dev; struct drm_encoder *encoder; + struct drm_bridge *next; #ifdef CONFIG_OF struct device_node *of_node; #endif @@ -1230,6 +1233,17 @@ extern void drm_bridge_remove(struct drm_bridge *bridge); extern struct drm_bridge *of_drm_find_bridge(struct device_node *np); extern int drm_bridge_attach(struct drm_device *dev, struct drm_bridge *bridge); +bool drm_bridge_mode_fixup(struct drm_bridge *bridge, + const struct drm_display_mode *mode, + struct drm_display_mode *adjusted_mode); +void drm_bridge_disable(struct drm_bridge *bridge); +void drm_bridge_post_disable(struct drm_bridge *bridge); +void drm_bridge_mode_set(struct drm_bridge *bridge, + struct drm_display_mode *mode, + struct drm_display_mode *adjusted_mode); +void drm_bridge_pre_enable(struct drm_bridge *bridge); +void drm_bridge_enable(struct drm_bridge *bridge); + extern int drm_encoder_init(struct drm_device *dev, struct drm_encoder *encoder, const struct drm_encoder_funcs *funcs,