Re: [PATCH] drm: bridge: Allow daisy chaining of bridges

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On 04/09/2015 12:54 PM, Jani Nikula wrote:
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.

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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel





[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux