On Thu, 09 Apr 2015, Archit Taneja <architt@xxxxxxxxxxxxxx> 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.
>
> Signed-off-by: Archit Taneja <architt@xxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/drm_atomic_helper.c | 29 ++++++----------
> drivers/gpu/drm/drm_bridge.c | 68 +++++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/drm_crtc_helper.c | 54 +++++++++++------------------
> include/drm/drm_crtc.h | 14 ++++++++
> 4 files changed, 112 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 7e3a52b..0b4574e 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -287,14 +287,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_KMS("Bridge fixup failed\n");
> - return -EINVAL;
> - }
> + ret = drm_bridge_mode_fixup(encoder->bridge, &crtc_state->mode,
> + &crtc_state->adjusted_mode);
> + if (!ret) {
> + DRM_DEBUG_KMS("Bridge fixup failed\n");
> + return -EINVAL;
> }
>
> if (funcs->atomic_check) {
> @@ -607,8 +604,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 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)
> @@ -618,8 +614,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 (i = 0; i < ncrtcs; i++) {
> @@ -761,9 +756,7 @@ crtc_set_mode(struct drm_device *dev, struct drm_atomic_state *old_state)
> */
> 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);
> }
> }
>
> @@ -849,16 +842,14 @@ void drm_atomic_helper_commit_post_planes(struct drm_device *dev,
> * Each encoder has at most one connector (since we always steal
> * it away), so we won't call 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_post_planes);
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index d1187e5..b52c387 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -66,6 +66,74 @@ extern int drm_bridge_attach(struct drm_device *dev, struct drm_bridge *bridge)
> }
> EXPORT_SYMBOL(drm_bridge_attach);
>
> +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);
> +
> + return ret & drm_bridge_mode_fixup(bridge->next, mode, adjusted_mode);
Is that an intentional bitwise rather than logical AND? If it is, please
don't combine this into the return statement.
How about EXPORT_SYMBOLs for the new functions?
Other than these, the patch LGTM.
BR,
Jani.
> +}
> +
> +void drm_bridge_disable(struct drm_bridge *bridge)
> +{
> + if (!bridge)
> + return;
> +
> + drm_bridge_disable(bridge->next);
> +
> + bridge->funcs->disable(bridge);
> +}
> +
> +void drm_bridge_post_disable(struct drm_bridge *bridge)
> +{
> + if (!bridge)
> + return;
> +
> + drm_bridge_post_disable(bridge->next);
> +
> + bridge->funcs->post_disable(bridge);
> +}
> +
> +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);
> +}
> +
> +void drm_bridge_pre_enable(struct drm_bridge *bridge)
> +{
> + if (!bridge)
> + return;
> +
> + bridge->funcs->pre_enable(bridge);
> +
> + drm_bridge_pre_enable(bridge->next);
> +}
> +
> +void drm_bridge_enable(struct drm_bridge *bridge)
> +{
> + if (!bridge)
> + return;
> +
> + bridge->funcs->enable(bridge);
> +
> + drm_bridge_enable(bridge->next);
> +}
> +
> #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 b1979e7..a199f49 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)
> {
> 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)
> @@ -311,13 +309,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;
> @@ -340,15 +336,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);
> @@ -373,9 +367,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. */
> @@ -386,14 +378,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);
> }
>
> /* Store real post-adjustment hardware mode. */
> @@ -734,23 +724,19 @@ static void drm_helper_encoder_dpms(struct drm_encoder *encoder, int mode)
> struct drm_bridge *bridge = encoder->bridge;
> 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 920e21a..8fced1d 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -893,6 +893,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
> @@ -902,6 +904,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
> @@ -1225,6 +1228,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
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
