[PATCH 1/3] drm/bridge: Move the struct drm_bridge member kerneldoc inline.

[PATCH 1/3] drm/bridge: Move the struct drm_bridge member kerneldoc inline.

[Eric Anholt]

This makes it more likely that the docs stay updated with the code.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 include/drm/drm_bridge.h | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 70131ab57e8f..bd850747ce54 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -270,27 +270,29 @@ struct drm_bridge_timings {
 
 /**
  * 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
- * @timings: the timing specification for the bridge, if any (may
- * be NULL)
- * @funcs: control functions
- * @driver_private: pointer to the bridge driver's internal context
  */
 struct drm_bridge {
+	/** @dev: DRM device this bridge belongs to */
 	struct drm_device *dev;
+	/** @encoder: encoder to which this bridge is connected */
 	struct drm_encoder *encoder;
+	/** @next: the next bridge in the encoder chain */
 	struct drm_bridge *next;
 #ifdef CONFIG_OF
+	/** @of_node: device node pointer to the bridge */
 	struct device_node *of_node;
 #endif
+	/** @list: to keep track of all added bridges */
 	struct list_head list;
+	/**
+	 * @timings:
+	 *
+	 * the timing specification for the bridge, if any (may be NULL)
+	 */
 	const struct drm_bridge_timings *timings;
-
+	/** @funcs: control functions */
 	const struct drm_bridge_funcs *funcs;
+	/** @driver_private: pointer to the bridge driver's internal context */
 	void *driver_private;
 };
 
-- 
2.17.0

[PATCH 1/3] drm/bridge: Move the struct drm_bridge member kerneldoc inline.

[Eric Anholt]

This makes it more likely that the docs stay updated with the code.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 include/drm/drm_bridge.h | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 70131ab57e8f..bd850747ce54 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -270,27 +270,29 @@ struct drm_bridge_timings {
 
 /**
  * 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
- * @timings: the timing specification for the bridge, if any (may
- * be NULL)
- * @funcs: control functions
- * @driver_private: pointer to the bridge driver's internal context
  */
 struct drm_bridge {
+	/** @dev: DRM device this bridge belongs to */
 	struct drm_device *dev;
+	/** @encoder: encoder to which this bridge is connected */
 	struct drm_encoder *encoder;
+	/** @next: the next bridge in the encoder chain */
 	struct drm_bridge *next;
 #ifdef CONFIG_OF
+	/** @of_node: device node pointer to the bridge */
 	struct device_node *of_node;
 #endif
+	/** @list: to keep track of all added bridges */
 	struct list_head list;
+	/**
+	 * @timings:
+	 *
+	 * the timing specification for the bridge, if any (may be NULL)
+	 */
 	const struct drm_bridge_timings *timings;
-
+	/** @funcs: control functions */
 	const struct drm_bridge_funcs *funcs;
+	/** @driver_private: pointer to the bridge driver's internal context */
 	void *driver_private;
 };
 
-- 
2.17.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/3] drm/bridge: Allow DSI encoders to decide when to call bridge funcs.

[Eric Anholt]

For DSI, we want to pre_enable once the DSI link is up but before
video packets are being sent, and similarly we want to be able to
post_disable while the link is still up but after video has stopped.

Given that with drm_panels we've had issues with drivers missing calls
to one of the hooks, include some checks in the atomic helpers that
the driver got it right.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/gpu/drm/drm_atomic_helper.c | 32 +++++++++++++++++++++++++----
 drivers/gpu/drm/drm_bridge.c        | 12 +++++++++++
 include/drm/drm_bridge.h            | 21 +++++++++++++++++++
 3 files changed, 61 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 130da5195f3b..2950ddcc9013 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -950,7 +950,13 @@ 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.
 		 */
-		drm_bridge_disable(encoder->bridge);
+		if (encoder->bridge) {
+			encoder->bridge->post_disable_called = false;
+			encoder->bridge->disable_called = false;
+
+			if (!encoder->bridge->disable_midlayer_calls)
+				drm_bridge_disable(encoder->bridge);
+		}
 
 		/* Right function depends upon target state. */
 		if (funcs) {
@@ -962,7 +968,13 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
 				funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
 		}
 
-		drm_bridge_post_disable(encoder->bridge);
+		if (encoder->bridge) {
+			if (!encoder->bridge->disable_midlayer_calls)
+				drm_bridge_post_disable(encoder->bridge);
+
+			WARN_ON(!encoder->bridge->post_disable_called);
+			WARN_ON(!encoder->bridge->disable_called);
+		}
 	}
 
 	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
@@ -1240,7 +1252,13 @@ 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.
 		 */
-		drm_bridge_pre_enable(encoder->bridge);
+		if (encoder->bridge) {
+			encoder->bridge->pre_enable_called = false;
+			encoder->bridge->enable_called = false;
+
+			if (!encoder->bridge->disable_midlayer_calls)
+				drm_bridge_pre_enable(encoder->bridge);
+		}
 
 		if (funcs) {
 			if (funcs->enable)
@@ -1249,7 +1267,13 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
 				funcs->commit(encoder);
 		}
 
-		drm_bridge_enable(encoder->bridge);
+		if (encoder->bridge) {
+			if (!encoder->bridge->disable_midlayer_calls)
+				drm_bridge_enable(encoder->bridge);
+
+			WARN_ON(!encoder->bridge->pre_enable_called);
+			WARN_ON(!encoder->bridge->enable_called);
+		}
 	}
 }
 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 1638bfe9627c..847c4209da60 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -153,6 +153,8 @@ void drm_bridge_detach(struct drm_bridge *bridge)
 	if (bridge->funcs->detach)
 		bridge->funcs->detach(bridge);
 
+	bridge->disable_midlayer_calls = false;
+
 	bridge->dev = NULL;
 }
 
@@ -248,6 +250,8 @@ void drm_bridge_disable(struct drm_bridge *bridge)
 	if (!bridge)
 		return;
 
+	bridge->disable_called = true;
+
 	drm_bridge_disable(bridge->next);
 
 	if (bridge->funcs->disable)
@@ -270,6 +274,9 @@ void drm_bridge_post_disable(struct drm_bridge *bridge)
 	if (!bridge)
 		return;
 
+	WARN_ON(!bridge->disable_called);
+	bridge->post_disable_called = true;
+
 	if (bridge->funcs->post_disable)
 		bridge->funcs->post_disable(bridge);
 
@@ -319,6 +326,8 @@ void drm_bridge_pre_enable(struct drm_bridge *bridge)
 	if (!bridge)
 		return;
 
+	bridge->pre_enable_called = true;
+
 	drm_bridge_pre_enable(bridge->next);
 
 	if (bridge->funcs->pre_enable)
@@ -341,6 +350,9 @@ void drm_bridge_enable(struct drm_bridge *bridge)
 	if (!bridge)
 		return;
 
+	WARN_ON(!bridge->pre_enable_called);
+	bridge->enable_called = true;
+
 	if (bridge->funcs->enable)
 		bridge->funcs->enable(bridge);
 
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index bd850747ce54..ba0a227c96b7 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -294,6 +294,27 @@ struct drm_bridge {
 	const struct drm_bridge_funcs *funcs;
 	/** @driver_private: pointer to the bridge driver's internal context */
 	void *driver_private;
+
+	/**
+	 * @disable_midlayer_calls:
+	 *
+	 * disables the calls from the DRM atomic helpers into the
+	 * bridge, leaving them to be performed by the driver.  This
+	 * may be useful for encoders such as DSI, where the
+	 * pre_enable/post_disable hooks want to be called while the
+	 * bus is still enabled but before/after video packets are
+	 * being sent.
+	 */
+	bool disable_midlayer_calls : 1;
+
+	/* private: flags for atomic helpers to check that the driver
+	 * called the bridge functions properly if
+	 * @disable_midlayer_calls was set.
+	 */
+	bool pre_enable_called : 1;
+	bool enable_called : 1;
+	bool disable_called : 1;
+	bool post_disable_called : 1;
 };
 
 void drm_bridge_add(struct drm_bridge *bridge);
-- 
2.17.0

[PATCH 2/3] drm/bridge: Allow DSI encoders to decide when to call bridge funcs.

[Eric Anholt]

For DSI, we want to pre_enable once the DSI link is up but before
video packets are being sent, and similarly we want to be able to
post_disable while the link is still up but after video has stopped.

Given that with drm_panels we've had issues with drivers missing calls
to one of the hooks, include some checks in the atomic helpers that
the driver got it right.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/gpu/drm/drm_atomic_helper.c | 32 +++++++++++++++++++++++++----
 drivers/gpu/drm/drm_bridge.c        | 12 +++++++++++
 include/drm/drm_bridge.h            | 21 +++++++++++++++++++
 3 files changed, 61 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 130da5195f3b..2950ddcc9013 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -950,7 +950,13 @@ 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.
 		 */
-		drm_bridge_disable(encoder->bridge);
+		if (encoder->bridge) {
+			encoder->bridge->post_disable_called = false;
+			encoder->bridge->disable_called = false;
+
+			if (!encoder->bridge->disable_midlayer_calls)
+				drm_bridge_disable(encoder->bridge);
+		}
 
 		/* Right function depends upon target state. */
 		if (funcs) {
@@ -962,7 +968,13 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
 				funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
 		}
 
-		drm_bridge_post_disable(encoder->bridge);
+		if (encoder->bridge) {
+			if (!encoder->bridge->disable_midlayer_calls)
+				drm_bridge_post_disable(encoder->bridge);
+
+			WARN_ON(!encoder->bridge->post_disable_called);
+			WARN_ON(!encoder->bridge->disable_called);
+		}
 	}
 
 	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
@@ -1240,7 +1252,13 @@ 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.
 		 */
-		drm_bridge_pre_enable(encoder->bridge);
+		if (encoder->bridge) {
+			encoder->bridge->pre_enable_called = false;
+			encoder->bridge->enable_called = false;
+
+			if (!encoder->bridge->disable_midlayer_calls)
+				drm_bridge_pre_enable(encoder->bridge);
+		}
 
 		if (funcs) {
 			if (funcs->enable)
@@ -1249,7 +1267,13 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
 				funcs->commit(encoder);
 		}
 
-		drm_bridge_enable(encoder->bridge);
+		if (encoder->bridge) {
+			if (!encoder->bridge->disable_midlayer_calls)
+				drm_bridge_enable(encoder->bridge);
+
+			WARN_ON(!encoder->bridge->pre_enable_called);
+			WARN_ON(!encoder->bridge->enable_called);
+		}
 	}
 }
 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 1638bfe9627c..847c4209da60 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -153,6 +153,8 @@ void drm_bridge_detach(struct drm_bridge *bridge)
 	if (bridge->funcs->detach)
 		bridge->funcs->detach(bridge);
 
+	bridge->disable_midlayer_calls = false;
+
 	bridge->dev = NULL;
 }
 
@@ -248,6 +250,8 @@ void drm_bridge_disable(struct drm_bridge *bridge)
 	if (!bridge)
 		return;
 
+	bridge->disable_called = true;
+
 	drm_bridge_disable(bridge->next);
 
 	if (bridge->funcs->disable)
@@ -270,6 +274,9 @@ void drm_bridge_post_disable(struct drm_bridge *bridge)
 	if (!bridge)
 		return;
 
+	WARN_ON(!bridge->disable_called);
+	bridge->post_disable_called = true;
+
 	if (bridge->funcs->post_disable)
 		bridge->funcs->post_disable(bridge);
 
@@ -319,6 +326,8 @@ void drm_bridge_pre_enable(struct drm_bridge *bridge)
 	if (!bridge)
 		return;
 
+	bridge->pre_enable_called = true;
+
 	drm_bridge_pre_enable(bridge->next);
 
 	if (bridge->funcs->pre_enable)
@@ -341,6 +350,9 @@ void drm_bridge_enable(struct drm_bridge *bridge)
 	if (!bridge)
 		return;
 
+	WARN_ON(!bridge->pre_enable_called);
+	bridge->enable_called = true;
+
 	if (bridge->funcs->enable)
 		bridge->funcs->enable(bridge);
 
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index bd850747ce54..ba0a227c96b7 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -294,6 +294,27 @@ struct drm_bridge {
 	const struct drm_bridge_funcs *funcs;
 	/** @driver_private: pointer to the bridge driver's internal context */
 	void *driver_private;
+
+	/**
+	 * @disable_midlayer_calls:
+	 *
+	 * disables the calls from the DRM atomic helpers into the
+	 * bridge, leaving them to be performed by the driver.  This
+	 * may be useful for encoders such as DSI, where the
+	 * pre_enable/post_disable hooks want to be called while the
+	 * bus is still enabled but before/after video packets are
+	 * being sent.
+	 */
+	bool disable_midlayer_calls : 1;
+
+	/* private: flags for atomic helpers to check that the driver
+	 * called the bridge functions properly if
+	 * @disable_midlayer_calls was set.
+	 */
+	bool pre_enable_called : 1;
+	bool enable_called : 1;
+	bool disable_called : 1;
+	bool post_disable_called : 1;
 };
 
 void drm_bridge_add(struct drm_bridge *bridge);
-- 
2.17.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/3] drm/bridge: Allow DSI encoders to decide when to call bridge funcs.

[Andrzej Hajda]

On 06.06.2018 21:04, Eric Anholt wrote:
> For DSI, we want to pre_enable once the DSI link is up but before
> video packets are being sent, and similarly we want to be able to
> post_disable while the link is still up but after video has stopped.
>
> Given that with drm_panels we've had issues with drivers missing calls
> to one of the hooks, include some checks in the atomic helpers that
> the driver got it right.

There is no explicit description of disable_midlayer_calls.

Anyway I have few general comments I put them here to simplify discussion:
1. disable_midlayer_calls is used only by drm core on only 1st bridge in
the chain, in such case the flag should be placed rather in drm_encoder.
On the other hand to support this case it would be enough to use private
drm_bridge pointer instead of drm_encoder->bridge. I guess it should be
enough to solve your case.
2. Similar issue can appear not only between encoder->bridge pair, but
also in bridge->bridge pair (for longer display chains), it would be
good to cover such case, so maybe instead of disable_midlayer_calls
there should be flag disable_recursive_calls/disable_chain_calls in
drm_bridge, but again it can be solved also by not using
drm_bridge->next member.
3. There are 4 bools to control state machine of the bridge (it makes 16
possibilities, most of them is invalid), but it has only 3 states: off,
prepared, enabled. I think it would be clearer if we define
enum with these three states and use it.
4. WARN in atomics checks only 1st bridge in the chain, in most cases it
is enough but in case of longer chains it will not check next
bridges/panels - this is definitely only partial solution, maybe
skipping it wouldn't be bad idea, but it is just my suggestion.

Regards
Andrzej

>
> Signed-off-by: Eric Anholt <eric@anholt.net>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 32 +++++++++++++++++++++++++----
>  drivers/gpu/drm/drm_bridge.c        | 12 +++++++++++
>  include/drm/drm_bridge.h            | 21 +++++++++++++++++++
>  3 files changed, 61 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 130da5195f3b..2950ddcc9013 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -950,7 +950,13 @@ 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.
>  		 */
> -		drm_bridge_disable(encoder->bridge);
> +		if (encoder->bridge) {
> +			encoder->bridge->post_disable_called = false;
> +			encoder->bridge->disable_called = false;
> +
> +			if (!encoder->bridge->disable_midlayer_calls)
> +				drm_bridge_disable(encoder->bridge);
> +		}
>  
>  		/* Right function depends upon target state. */
>  		if (funcs) {
> @@ -962,7 +968,13 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
>  				funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
>  		}
>  
> -		drm_bridge_post_disable(encoder->bridge);
> +		if (encoder->bridge) {
> +			if (!encoder->bridge->disable_midlayer_calls)
> +				drm_bridge_post_disable(encoder->bridge);
> +
> +			WARN_ON(!encoder->bridge->post_disable_called);
> +			WARN_ON(!encoder->bridge->disable_called);
> +		}
>  	}
>  
>  	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
> @@ -1240,7 +1252,13 @@ 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.
>  		 */
> -		drm_bridge_pre_enable(encoder->bridge);
> +		if (encoder->bridge) {
> +			encoder->bridge->pre_enable_called = false;
> +			encoder->bridge->enable_called = false;
> +
> +			if (!encoder->bridge->disable_midlayer_calls)
> +				drm_bridge_pre_enable(encoder->bridge);
> +		}
>  
>  		if (funcs) {
>  			if (funcs->enable)
> @@ -1249,7 +1267,13 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
>  				funcs->commit(encoder);
>  		}
>  
> -		drm_bridge_enable(encoder->bridge);
> +		if (encoder->bridge) {
> +			if (!encoder->bridge->disable_midlayer_calls)
> +				drm_bridge_enable(encoder->bridge);
> +
> +			WARN_ON(!encoder->bridge->pre_enable_called);
> +			WARN_ON(!encoder->bridge->enable_called);
> +		}
>  	}
>  }
>  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 1638bfe9627c..847c4209da60 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -153,6 +153,8 @@ void drm_bridge_detach(struct drm_bridge *bridge)
>  	if (bridge->funcs->detach)
>  		bridge->funcs->detach(bridge);
>  
> +	bridge->disable_midlayer_calls = false;
> +
>  	bridge->dev = NULL;
>  }
>  
> @@ -248,6 +250,8 @@ void drm_bridge_disable(struct drm_bridge *bridge)
>  	if (!bridge)
>  		return;
>  
> +	bridge->disable_called = true;
> +
>  	drm_bridge_disable(bridge->next);
>  
>  	if (bridge->funcs->disable)
> @@ -270,6 +274,9 @@ void drm_bridge_post_disable(struct drm_bridge *bridge)
>  	if (!bridge)
>  		return;
>  
> +	WARN_ON(!bridge->disable_called);
> +	bridge->post_disable_called = true;
> +
>  	if (bridge->funcs->post_disable)
>  		bridge->funcs->post_disable(bridge);
>  
> @@ -319,6 +326,8 @@ void drm_bridge_pre_enable(struct drm_bridge *bridge)
>  	if (!bridge)
>  		return;
>  
> +	bridge->pre_enable_called = true;
> +
>  	drm_bridge_pre_enable(bridge->next);
>  
>  	if (bridge->funcs->pre_enable)
> @@ -341,6 +350,9 @@ void drm_bridge_enable(struct drm_bridge *bridge)
>  	if (!bridge)
>  		return;
>  
> +	WARN_ON(!bridge->pre_enable_called);
> +	bridge->enable_called = true;
> +
>  	if (bridge->funcs->enable)
>  		bridge->funcs->enable(bridge);
>  
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index bd850747ce54..ba0a227c96b7 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -294,6 +294,27 @@ struct drm_bridge {
>  	const struct drm_bridge_funcs *funcs;
>  	/** @driver_private: pointer to the bridge driver's internal context */
>  	void *driver_private;
> +
> +	/**
> +	 * @disable_midlayer_calls:
> +	 *
> +	 * disables the calls from the DRM atomic helpers into the
> +	 * bridge, leaving them to be performed by the driver.  This
> +	 * may be useful for encoders such as DSI, where the
> +	 * pre_enable/post_disable hooks want to be called while the
> +	 * bus is still enabled but before/after video packets are
> +	 * being sent.
> +	 */
> +	bool disable_midlayer_calls : 1;
> +
> +	/* private: flags for atomic helpers to check that the driver
> +	 * called the bridge functions properly if
> +	 * @disable_midlayer_calls was set.
> +	 */
> +	bool pre_enable_called : 1;
> +	bool enable_called : 1;
> +	bool disable_called : 1;
> +	bool post_disable_called : 1;
>  };
>  
>  void drm_bridge_add(struct drm_bridge *bridge);

Re: [PATCH 2/3] drm/bridge: Allow DSI encoders to decide when to call bridge funcs.

[Andrzej Hajda]

On 06.06.2018 21:04, Eric Anholt wrote:
> For DSI, we want to pre_enable once the DSI link is up but before
> video packets are being sent, and similarly we want to be able to
> post_disable while the link is still up but after video has stopped.
>
> Given that with drm_panels we've had issues with drivers missing calls
> to one of the hooks, include some checks in the atomic helpers that
> the driver got it right.

There is no explicit description of disable_midlayer_calls.

Anyway I have few general comments I put them here to simplify discussion:
1. disable_midlayer_calls is used only by drm core on only 1st bridge in
the chain, in such case the flag should be placed rather in drm_encoder.
On the other hand to support this case it would be enough to use private
drm_bridge pointer instead of drm_encoder->bridge. I guess it should be
enough to solve your case.
2. Similar issue can appear not only between encoder->bridge pair, but
also in bridge->bridge pair (for longer display chains), it would be
good to cover such case, so maybe instead of disable_midlayer_calls
there should be flag disable_recursive_calls/disable_chain_calls in
drm_bridge, but again it can be solved also by not using
drm_bridge->next member.
3. There are 4 bools to control state machine of the bridge (it makes 16
possibilities, most of them is invalid), but it has only 3 states: off,
prepared, enabled. I think it would be clearer if we define
enum with these three states and use it.
4. WARN in atomics checks only 1st bridge in the chain, in most cases it
is enough but in case of longer chains it will not check next
bridges/panels - this is definitely only partial solution, maybe
skipping it wouldn't be bad idea, but it is just my suggestion.

Regards
Andrzej

>
> Signed-off-by: Eric Anholt <eric@anholt.net>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 32 +++++++++++++++++++++++++----
>  drivers/gpu/drm/drm_bridge.c        | 12 +++++++++++
>  include/drm/drm_bridge.h            | 21 +++++++++++++++++++
>  3 files changed, 61 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 130da5195f3b..2950ddcc9013 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -950,7 +950,13 @@ 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.
>  		 */
> -		drm_bridge_disable(encoder->bridge);
> +		if (encoder->bridge) {
> +			encoder->bridge->post_disable_called = false;
> +			encoder->bridge->disable_called = false;
> +
> +			if (!encoder->bridge->disable_midlayer_calls)
> +				drm_bridge_disable(encoder->bridge);
> +		}
>  
>  		/* Right function depends upon target state. */
>  		if (funcs) {
> @@ -962,7 +968,13 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
>  				funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
>  		}
>  
> -		drm_bridge_post_disable(encoder->bridge);
> +		if (encoder->bridge) {
> +			if (!encoder->bridge->disable_midlayer_calls)
> +				drm_bridge_post_disable(encoder->bridge);
> +
> +			WARN_ON(!encoder->bridge->post_disable_called);
> +			WARN_ON(!encoder->bridge->disable_called);
> +		}
>  	}
>  
>  	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
> @@ -1240,7 +1252,13 @@ 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.
>  		 */
> -		drm_bridge_pre_enable(encoder->bridge);
> +		if (encoder->bridge) {
> +			encoder->bridge->pre_enable_called = false;
> +			encoder->bridge->enable_called = false;
> +
> +			if (!encoder->bridge->disable_midlayer_calls)
> +				drm_bridge_pre_enable(encoder->bridge);
> +		}
>  
>  		if (funcs) {
>  			if (funcs->enable)
> @@ -1249,7 +1267,13 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
>  				funcs->commit(encoder);
>  		}
>  
> -		drm_bridge_enable(encoder->bridge);
> +		if (encoder->bridge) {
> +			if (!encoder->bridge->disable_midlayer_calls)
> +				drm_bridge_enable(encoder->bridge);
> +
> +			WARN_ON(!encoder->bridge->pre_enable_called);
> +			WARN_ON(!encoder->bridge->enable_called);
> +		}
>  	}
>  }
>  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 1638bfe9627c..847c4209da60 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -153,6 +153,8 @@ void drm_bridge_detach(struct drm_bridge *bridge)
>  	if (bridge->funcs->detach)
>  		bridge->funcs->detach(bridge);
>  
> +	bridge->disable_midlayer_calls = false;
> +
>  	bridge->dev = NULL;
>  }
>  
> @@ -248,6 +250,8 @@ void drm_bridge_disable(struct drm_bridge *bridge)
>  	if (!bridge)
>  		return;
>  
> +	bridge->disable_called = true;
> +
>  	drm_bridge_disable(bridge->next);
>  
>  	if (bridge->funcs->disable)
> @@ -270,6 +274,9 @@ void drm_bridge_post_disable(struct drm_bridge *bridge)
>  	if (!bridge)
>  		return;
>  
> +	WARN_ON(!bridge->disable_called);
> +	bridge->post_disable_called = true;
> +
>  	if (bridge->funcs->post_disable)
>  		bridge->funcs->post_disable(bridge);
>  
> @@ -319,6 +326,8 @@ void drm_bridge_pre_enable(struct drm_bridge *bridge)
>  	if (!bridge)
>  		return;
>  
> +	bridge->pre_enable_called = true;
> +
>  	drm_bridge_pre_enable(bridge->next);
>  
>  	if (bridge->funcs->pre_enable)
> @@ -341,6 +350,9 @@ void drm_bridge_enable(struct drm_bridge *bridge)
>  	if (!bridge)
>  		return;
>  
> +	WARN_ON(!bridge->pre_enable_called);
> +	bridge->enable_called = true;
> +
>  	if (bridge->funcs->enable)
>  		bridge->funcs->enable(bridge);
>  
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index bd850747ce54..ba0a227c96b7 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -294,6 +294,27 @@ struct drm_bridge {
>  	const struct drm_bridge_funcs *funcs;
>  	/** @driver_private: pointer to the bridge driver's internal context */
>  	void *driver_private;
> +
> +	/**
> +	 * @disable_midlayer_calls:
> +	 *
> +	 * disables the calls from the DRM atomic helpers into the
> +	 * bridge, leaving them to be performed by the driver.  This
> +	 * may be useful for encoders such as DSI, where the
> +	 * pre_enable/post_disable hooks want to be called while the
> +	 * bus is still enabled but before/after video packets are
> +	 * being sent.
> +	 */
> +	bool disable_midlayer_calls : 1;
> +
> +	/* private: flags for atomic helpers to check that the driver
> +	 * called the bridge functions properly if
> +	 * @disable_midlayer_calls was set.
> +	 */
> +	bool pre_enable_called : 1;
> +	bool enable_called : 1;
> +	bool disable_called : 1;
> +	bool post_disable_called : 1;
>  };
>  
>  void drm_bridge_add(struct drm_bridge *bridge);


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/3] drm/vc4: Make DSI call into the bridge after the DSI link is enabled.

[Eric Anholt]

This allows panels or bridges that need to send DSI commands during
pre_enable() to successfully send them.  We delay DISP0 (aka the
actual display) enabling until after pre_enable so that pixels aren't
streaming before then.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/gpu/drm/vc4/vc4_dsi.c | 37 +++++++++++++++++++++--------------
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
index 8aa897835118..02cbfcc3cc4b 100644
--- a/drivers/gpu/drm/vc4/vc4_dsi.c
+++ b/drivers/gpu/drm/vc4/vc4_dsi.c
@@ -814,7 +814,9 @@ static void vc4_dsi_encoder_disable(struct drm_encoder *encoder)
 	struct vc4_dsi *dsi = vc4_encoder->dsi;
 	struct device *dev = &dsi->pdev->dev;
 
+	drm_bridge_disable(dsi->bridge);
 	vc4_dsi_ulps(dsi, true);
+	drm_bridge_post_disable(dsi->bridge);
 
 	clk_disable_unprepare(dsi->pll_phy_clock);
 	clk_disable_unprepare(dsi->escape_clock);
@@ -1089,21 +1091,6 @@ static void vc4_dsi_encoder_enable(struct drm_encoder *encoder)
 	/* Display reset sequence timeout */
 	DSI_PORT_WRITE(PR_TO_CNT, 100000);
 
-	if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO) {
-		DSI_PORT_WRITE(DISP0_CTRL,
-			       VC4_SET_FIELD(dsi->divider,
-					     DSI_DISP0_PIX_CLK_DIV) |
-			       VC4_SET_FIELD(dsi->format, DSI_DISP0_PFORMAT) |
-			       VC4_SET_FIELD(DSI_DISP0_LP_STOP_PERFRAME,
-					     DSI_DISP0_LP_STOP_CTRL) |
-			       DSI_DISP0_ST_END |
-			       DSI_DISP0_ENABLE);
-	} else {
-		DSI_PORT_WRITE(DISP0_CTRL,
-			       DSI_DISP0_COMMAND_MODE |
-			       DSI_DISP0_ENABLE);
-	}
-
 	/* Set up DISP1 for transferring long command payloads through
 	 * the pixfifo.
 	 */
@@ -1128,6 +1115,25 @@ static void vc4_dsi_encoder_enable(struct drm_encoder *encoder)
 
 	vc4_dsi_ulps(dsi, false);
 
+	drm_bridge_pre_enable(dsi->bridge);
+
+	if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO) {
+		DSI_PORT_WRITE(DISP0_CTRL,
+			       VC4_SET_FIELD(dsi->divider,
+					     DSI_DISP0_PIX_CLK_DIV) |
+			       VC4_SET_FIELD(dsi->format, DSI_DISP0_PFORMAT) |
+			       VC4_SET_FIELD(DSI_DISP0_LP_STOP_PERFRAME,
+					     DSI_DISP0_LP_STOP_CTRL) |
+			       DSI_DISP0_ST_END |
+			       DSI_DISP0_ENABLE);
+	} else {
+		DSI_PORT_WRITE(DISP0_CTRL,
+			       DSI_DISP0_COMMAND_MODE |
+			       DSI_DISP0_ENABLE);
+	}
+
+	drm_bridge_enable(dsi->bridge);
+
 	if (debug_dump_regs) {
 		DRM_INFO("DSI regs after:\n");
 		vc4_dsi_dump_regs(dsi);
@@ -1639,6 +1645,7 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data)
 		dev_err(dev, "bridge attach failed: %d\n", ret);
 		return ret;
 	}
+	dsi->bridge->disable_midlayer_calls = true;
 
 	pm_runtime_enable(dev);
 
-- 
2.17.0

[PATCH 3/3] drm/vc4: Make DSI call into the bridge after the DSI link is enabled.

[Eric Anholt]

This allows panels or bridges that need to send DSI commands during
pre_enable() to successfully send them.  We delay DISP0 (aka the
actual display) enabling until after pre_enable so that pixels aren't
streaming before then.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/gpu/drm/vc4/vc4_dsi.c | 37 +++++++++++++++++++++--------------
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_dsi.c b/drivers/gpu/drm/vc4/vc4_dsi.c
index 8aa897835118..02cbfcc3cc4b 100644
--- a/drivers/gpu/drm/vc4/vc4_dsi.c
+++ b/drivers/gpu/drm/vc4/vc4_dsi.c
@@ -814,7 +814,9 @@ static void vc4_dsi_encoder_disable(struct drm_encoder *encoder)
 	struct vc4_dsi *dsi = vc4_encoder->dsi;
 	struct device *dev = &dsi->pdev->dev;
 
+	drm_bridge_disable(dsi->bridge);
 	vc4_dsi_ulps(dsi, true);
+	drm_bridge_post_disable(dsi->bridge);
 
 	clk_disable_unprepare(dsi->pll_phy_clock);
 	clk_disable_unprepare(dsi->escape_clock);
@@ -1089,21 +1091,6 @@ static void vc4_dsi_encoder_enable(struct drm_encoder *encoder)
 	/* Display reset sequence timeout */
 	DSI_PORT_WRITE(PR_TO_CNT, 100000);
 
-	if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO) {
-		DSI_PORT_WRITE(DISP0_CTRL,
-			       VC4_SET_FIELD(dsi->divider,
-					     DSI_DISP0_PIX_CLK_DIV) |
-			       VC4_SET_FIELD(dsi->format, DSI_DISP0_PFORMAT) |
-			       VC4_SET_FIELD(DSI_DISP0_LP_STOP_PERFRAME,
-					     DSI_DISP0_LP_STOP_CTRL) |
-			       DSI_DISP0_ST_END |
-			       DSI_DISP0_ENABLE);
-	} else {
-		DSI_PORT_WRITE(DISP0_CTRL,
-			       DSI_DISP0_COMMAND_MODE |
-			       DSI_DISP0_ENABLE);
-	}
-
 	/* Set up DISP1 for transferring long command payloads through
 	 * the pixfifo.
 	 */
@@ -1128,6 +1115,25 @@ static void vc4_dsi_encoder_enable(struct drm_encoder *encoder)
 
 	vc4_dsi_ulps(dsi, false);
 
+	drm_bridge_pre_enable(dsi->bridge);
+
+	if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO) {
+		DSI_PORT_WRITE(DISP0_CTRL,
+			       VC4_SET_FIELD(dsi->divider,
+					     DSI_DISP0_PIX_CLK_DIV) |
+			       VC4_SET_FIELD(dsi->format, DSI_DISP0_PFORMAT) |
+			       VC4_SET_FIELD(DSI_DISP0_LP_STOP_PERFRAME,
+					     DSI_DISP0_LP_STOP_CTRL) |
+			       DSI_DISP0_ST_END |
+			       DSI_DISP0_ENABLE);
+	} else {
+		DSI_PORT_WRITE(DISP0_CTRL,
+			       DSI_DISP0_COMMAND_MODE |
+			       DSI_DISP0_ENABLE);
+	}
+
+	drm_bridge_enable(dsi->bridge);
+
 	if (debug_dump_regs) {
 		DRM_INFO("DSI regs after:\n");
 		vc4_dsi_dump_regs(dsi);
@@ -1639,6 +1645,7 @@ static int vc4_dsi_bind(struct device *dev, struct device *master, void *data)
 		dev_err(dev, "bridge attach failed: %d\n", ret);
 		return ret;
 	}
+	dsi->bridge->disable_midlayer_calls = true;
 
 	pm_runtime_enable(dev);
 
-- 
2.17.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] drm/bridge: Move the struct drm_bridge member kerneldoc inline.

[Andrzej Hajda]

On 06.06.2018 21:04, Eric Anholt wrote:
> This makes it more likely that the docs stay updated with the code.
>
> Signed-off-by: Eric Anholt <eric@anholt.net>
Reviewed-by: Andrzej Hajda <a.hajda@samsung.com>

 --
Regards
Andrzej
> ---
>  include/drm/drm_bridge.h | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 70131ab57e8f..bd850747ce54 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -270,27 +270,29 @@ struct drm_bridge_timings {
>  
>  /**
>   * 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
> - * @timings: the timing specification for the bridge, if any (may
> - * be NULL)
> - * @funcs: control functions
> - * @driver_private: pointer to the bridge driver's internal context
>   */
>  struct drm_bridge {
> +	/** @dev: DRM device this bridge belongs to */
>  	struct drm_device *dev;
> +	/** @encoder: encoder to which this bridge is connected */
>  	struct drm_encoder *encoder;
> +	/** @next: the next bridge in the encoder chain */
>  	struct drm_bridge *next;
>  #ifdef CONFIG_OF
> +	/** @of_node: device node pointer to the bridge */
>  	struct device_node *of_node;
>  #endif
> +	/** @list: to keep track of all added bridges */
>  	struct list_head list;
> +	/**
> +	 * @timings:
> +	 *
> +	 * the timing specification for the bridge, if any (may be NULL)
> +	 */
>  	const struct drm_bridge_timings *timings;
> -
> +	/** @funcs: control functions */
>  	const struct drm_bridge_funcs *funcs;
> +	/** @driver_private: pointer to the bridge driver's internal context */
>  	void *driver_private;
>  };
>