* [PATCH] drm: bridge: Allow daisy chaining of bridges
@ 2015-04-09 6:12 Archit Taneja
2015-04-09 7:24 ` Jani Nikula
2015-05-08 10:11 ` [PATCH v2] " Archit Taneja
0 siblings, 2 replies; 8+ messages in thread
From: Archit Taneja @ 2015-04-09 6:12 UTC (permalink / raw)
To: dri-devel; +Cc: daniel.vetter, airlied, andy.yan, ajaykumar.rs
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@codeaurora.org>
---
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);
+}
+
+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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] drm: bridge: Allow daisy chaining of bridges
2015-04-09 6:12 [PATCH] drm: bridge: Allow daisy chaining of bridges Archit Taneja
@ 2015-04-09 7:24 ` Jani Nikula
2015-04-09 8:08 ` Archit Taneja
2015-05-08 10:11 ` [PATCH v2] " Archit Taneja
1 sibling, 1 reply; 8+ messages in thread
From: Jani Nikula @ 2015-04-09 7:24 UTC (permalink / raw)
To: Archit Taneja, dri-devel; +Cc: daniel.vetter, andy.yan, ajaykumar.rs, airlied
On Thu, 09 Apr 2015, Archit Taneja <architt@codeaurora.org> 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@codeaurora.org>
> ---
> 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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drm: bridge: Allow daisy chaining of bridges
2015-04-09 7:24 ` Jani Nikula
@ 2015-04-09 8:08 ` Archit Taneja
0 siblings, 0 replies; 8+ messages in thread
From: Archit Taneja @ 2015-04-09 8:08 UTC (permalink / raw)
To: Jani Nikula, dri-devel; +Cc: daniel.vetter, andy.yan, ajaykumar.rs, airlied
Hi,
On 04/09/2015 12:54 PM, Jani Nikula wrote:
> On Thu, 09 Apr 2015, Archit Taneja <architt@codeaurora.org> 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@codeaurora.org>
>> ---
>> 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.
This looks like a mistake from my end. I'll fix this.
>
> How about EXPORT_SYMBOLs for the new functions?
I'll add this.
>
> Other than these, the patch LGTM.
Thanks for the review!
Archit
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2] drm: bridge: Allow daisy chaining of bridges
2015-04-09 6:12 [PATCH] drm: bridge: Allow daisy chaining of bridges Archit Taneja
2015-04-09 7:24 ` Jani Nikula
@ 2015-05-08 10:11 ` Archit Taneja
2015-05-08 13:03 ` Rob Clark
1 sibling, 1 reply; 8+ messages in thread
From: Archit Taneja @ 2015-05-08 10:11 UTC (permalink / raw)
To: dri-devel, jani.nikula; +Cc: daniel.vetter, airlied, andy.yan, ajaykumar.rs
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@codeaurora.org>
---
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 | 76 +++++++++++++++++++++++++++++++++++++
drivers/gpu/drm/drm_crtc_helper.c | 54 ++++++++++----------------
include/drm/drm_crtc.h | 14 +++++++
4 files changed, 120 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..98c39f6 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -66,6 +66,82 @@ 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);
+
+ ret = ret && drm_bridge_mode_fixup(bridge->next, mode, adjusted_mode);
+
+ return ret;
+}
+EXPORT_SYMBOL(drm_bridge_mode_fixup);
+
+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);
+
+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);
+
+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);
+
+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);
+
+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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] drm: bridge: Allow daisy chaining of bridges
2015-05-08 10:11 ` [PATCH v2] " Archit Taneja
@ 2015-05-08 13:03 ` Rob Clark
2015-05-08 13:54 ` Daniel Vetter
0 siblings, 1 reply; 8+ messages in thread
From: Rob Clark @ 2015-05-08 13:03 UTC (permalink / raw)
To: Archit Taneja; +Cc: Daniel Vetter, dri-devel, airlied, Andy Yan, Ajay Kumar
On Fri, May 8, 2015 at 6:11 AM, Archit Taneja <architt@codeaurora.org> 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@codeaurora.org>
Looks good. But I guess we probably should have some headerdoc for the
new fxns, and somewhere a comment about not calling the bridge vfuncs
directly but using the new helper funcs which handle the chaining. I
guess it isn't *as* critical as it would be if these were things
intended to be called by drivers, rather than just from core, but all
the same I think it would be a good idea. With that,
Reviewed-by: Rob Clark <robdclark@gmail.com>
> ---
> 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 | 76 +++++++++++++++++++++++++++++++++++++
> drivers/gpu/drm/drm_crtc_helper.c | 54 ++++++++++----------------
> include/drm/drm_crtc.h | 14 +++++++
> 4 files changed, 120 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..98c39f6 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -66,6 +66,82 @@ 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);
> +
> + ret = ret && drm_bridge_mode_fixup(bridge->next, mode, adjusted_mode);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(drm_bridge_mode_fixup);
> +
> +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);
> +
> +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);
> +
> +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);
> +
> +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);
> +
> +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
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] drm: bridge: Allow daisy chaining of bridges
2015-05-08 13:03 ` Rob Clark
@ 2015-05-08 13:54 ` Daniel Vetter
2015-05-08 14:30 ` Rob Clark
0 siblings, 1 reply; 8+ messages in thread
From: Daniel Vetter @ 2015-05-08 13:54 UTC (permalink / raw)
To: Rob Clark; +Cc: Daniel Vetter, dri-devel, airlied, Andy Yan, Ajay Kumar
On Fri, May 08, 2015 at 09:03:19AM -0400, Rob Clark wrote:
> On Fri, May 8, 2015 at 6:11 AM, Archit Taneja <architt@codeaurora.org> 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@codeaurora.org>
>
> Looks good. But I guess we probably should have some headerdoc for the
> new fxns, and somewhere a comment about not calling the bridge vfuncs
> directly but using the new helper funcs which handle the chaining. I
> guess it isn't *as* critical as it would be if these were things
> intended to be called by drivers, rather than just from core, but all
> the same I think it would be a good idea. With that,
Yeah, at least for patches that I'll take in through drm-misc I really
want kerneldoc. Also shouldnt' we do a cocci-run over all the drm drivers
to make sure they use the new helpers now?
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] drm: bridge: Allow daisy chaining of bridges
2015-05-08 13:54 ` Daniel Vetter
@ 2015-05-08 14:30 ` Rob Clark
2015-05-12 7:04 ` Archit Taneja
0 siblings, 1 reply; 8+ messages in thread
From: Rob Clark @ 2015-05-08 14:30 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Daniel Vetter, dri-devel, airlied, Andy Yan, Ajay Kumar
On Fri, May 8, 2015 at 9:54 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Fri, May 08, 2015 at 09:03:19AM -0400, Rob Clark wrote:
>> On Fri, May 8, 2015 at 6:11 AM, Archit Taneja <architt@codeaurora.org> 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@codeaurora.org>
>>
>> Looks good. But I guess we probably should have some headerdoc for the
>> new fxns, and somewhere a comment about not calling the bridge vfuncs
>> directly but using the new helper funcs which handle the chaining. I
>> guess it isn't *as* critical as it would be if these were things
>> intended to be called by drivers, rather than just from core, but all
>> the same I think it would be a good idea. With that,
>
> Yeah, at least for patches that I'll take in through drm-misc I really
> want kerneldoc. Also shouldnt' we do a cocci-run over all the drm drivers
> to make sure they use the new helpers now?
cocci might have been the more clever way to do it, but I did have a
quick check on the call-sites for bridge vfunc's with this patch
applied and didn't see any calls from drivers or unconverted call
sites in core..
BR,
-R
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] drm: bridge: Allow daisy chaining of bridges
2015-05-08 14:30 ` Rob Clark
@ 2015-05-12 7:04 ` Archit Taneja
0 siblings, 0 replies; 8+ messages in thread
From: Archit Taneja @ 2015-05-12 7:04 UTC (permalink / raw)
To: Rob Clark, Daniel Vetter
Cc: Daniel Vetter, dri-devel, airlied, Andy Yan, Ajay Kumar
On 05/08/2015 08:00 PM, Rob Clark wrote:
> On Fri, May 8, 2015 at 9:54 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Fri, May 08, 2015 at 09:03:19AM -0400, Rob Clark wrote:
>>> On Fri, May 8, 2015 at 6:11 AM, Archit Taneja <architt@codeaurora.org> 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@codeaurora.org>
>>>
>>> Looks good. But I guess we probably should have some headerdoc for the
>>> new fxns, and somewhere a comment about not calling the bridge vfuncs
>>> directly but using the new helper funcs which handle the chaining. I
>>> guess it isn't *as* critical as it would be if these were things
>>> intended to be called by drivers, rather than just from core, but all
>>> the same I think it would be a good idea. With that,
>>
>> Yeah, at least for patches that I'll take in through drm-misc I really
>> want kerneldoc. Also shouldnt' we do a cocci-run over all the drm drivers
>> to make sure they use the new helpers now?
>
> cocci might have been the more clever way to do it, but I did have a
> quick check on the call-sites for bridge vfunc's with this patch
> applied and didn't see any calls from drivers or unconverted call
> sites in core..
I'll repost with headerdoc for the new functions.
Thanks,
Archit
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2015-05-12 7:04 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-04-09 6:12 [PATCH] drm: bridge: Allow daisy chaining of bridges Archit Taneja
2015-04-09 7:24 ` Jani Nikula
2015-04-09 8:08 ` Archit Taneja
2015-05-08 10:11 ` [PATCH v2] " Archit Taneja
2015-05-08 13:03 ` Rob Clark
2015-05-08 13:54 ` Daniel Vetter
2015-05-08 14:30 ` Rob Clark
2015-05-12 7:04 ` Archit Taneja
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.