On Thu, 22 Aug 2019 03:12:07 +0300 Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
Hi Boris,
Thank you for the patch.
On Thu, Aug 08, 2019 at 05:11:43PM +0200, Boris Brezillon wrote:
So that bridge drivers have a way to check/reject an atomic operation. The drm_atomic_bridge_chain_check() (which is just a wrapper around the ->atomic_check() hook) is called in place of drm_bridge_chain_mode_fixup() (when ->atomic_check() is not implemented, the core falls back to ->mode_fixup(), so the behavior should stay the same for existing bridge drivers).
Signed-off-by: Boris Brezillon boris.brezillon@collabora.com
drivers/gpu/drm/drm_atomic_helper.c | 11 +++--- drivers/gpu/drm/drm_bridge.c | 59 +++++++++++++++++++++++++++++ include/drm/drm_bridge.h | 23 +++++++++++ 3 files changed, 87 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index e76ec9648b6f..3fadde38ead7 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -445,12 +445,11 @@ mode_fixup(struct drm_atomic_state *state) encoder = new_conn_state->best_encoder; funcs = encoder->helper_private;
ret = drm_bridge_chain_mode_fixup(encoder,&new_crtc_state->mode,&new_crtc_state->adjusted_mode);if (!ret) {DRM_DEBUG_ATOMIC("Bridge fixup failed\n");return -EINVAL;
ret = drm_atomic_bridge_chain_check(encoder, new_crtc_state,new_conn_state);if (ret) {DRM_DEBUG_ATOMIC("Bridge atomic check failed\n");return ret;}
if (funcs && funcs->atomic_check) {
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index 0528ca941855..dcad661daa74 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -583,6 +583,65 @@ void drm_atomic_bridge_chain_enable(struct drm_encoder *encoder, } EXPORT_SYMBOL(drm_atomic_bridge_chain_enable);
+static int drm_atomic_bridge_check(struct drm_bridge *bridge,
struct drm_crtc_state *crtc_state,struct drm_connector_state *conn_state)+{
- if (bridge->funcs->atomic_check) {
struct drm_bridge_state *bridge_state;int ret;bridge_state = drm_atomic_get_new_bridge_state(crtc_state->state,bridge);if (WARN_ON(!bridge_state))return -EINVAL;ret = bridge->funcs->atomic_check(bridge, bridge_state,crtc_state, conn_state);if (ret)return ret;- } else if (bridge->funcs->mode_fixup) {
if (!bridge->funcs->mode_fixup(bridge, &crtc_state->mode,&crtc_state->adjusted_mode))return -EINVAL;- }
- return 0;
+}
+/**
- drm_atomic_bridge_chain_check() - Do an atomic check on the bridge chain
- @encoder: encoder object
- @crtc_state: new CRTC state
- @conn_state: new connector state
- Calls &drm_bridge_funcs.atomic_check() (falls back on
- &drm_bridge_funcs.mode_fixup()) op for all the bridges in the encoder chain,
- starting from the last bridge to the first. These are called before calling
- &drm_encoder_helper_funcs.atomic_check()
- RETURNS:
- 0 on success, a negative error code on failure
- */
+int drm_atomic_bridge_chain_check(struct drm_encoder *encoder,
struct drm_crtc_state *crtc_state,struct drm_connector_state *conn_state)+{
- struct drm_bridge *bridge;
- list_for_each_entry_reverse(bridge, &encoder->bridge_chain,
chain_node) {int ret;ret = drm_atomic_bridge_check(bridge, crtc_state, conn_state);if (ret)return ret;- }
- return 0;
+} +EXPORT_SYMBOL(drm_atomic_bridge_chain_check);
/**
- drm_atomic_helper_init_bridge_state() - Initializes a bridge state
- @bridge this state is referring to
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h index 9383b4e4b853..5d8fe3709bde 100644 --- a/include/drm/drm_bridge.h +++ b/include/drm/drm_bridge.h @@ -379,6 +379,26 @@ struct drm_bridge_funcs { */ void (*atomic_destroy_state)(struct drm_bridge *bridge, struct drm_bridge_state *state);
- /**
* @atomic_check:** This method is responsible for checking bridge state correctness.* It can also check the state of the surrounding components in chain* to make sure the whole pipeline can work properly.As this is meant to replace .mode_fixup() you should document that here and in the .mode_fixup() operation.
There's also the additional question of what parameters can be modified. Is a bridge .atomic_check() allowed to modify the CRTC or connector state ? Of so, what are the restrictions on how those can be modified ? For instance .mode_fixup() can change the pixel clock but isn't allowed to modify the h/v active values. Those restrictions were not clearly documented for .mode_fixup() and have led to grey areas. I don't want to repeat the same mistake for bridges, we need to be very explicit here.
Any recommendation? I didn't face this problem myself, so I'd need inputs from those who did to take a decision.
In the list of open questions, what if a bridge changes the pixel clock to a value it supports (as .mode_fixup() does), and then the previous bridge in the chain (the next one in traversal order for the drm_atomic_bridge_chain_check() function) also changes the pixel clock, to a value that the current bridge doesn't support ? I think we really need to think about the semantic of this operation.
Don't we have the exact same problem right now with the ->mode_fixup() hook when bridges are chained? Not sure why it suddenly becomes a priority to clarify that, but if we quickly settle on this new semantic, I'm fine integrating those changes in my patchset.
As the goal of your series is to implement configuration of bus formats between bridges, shouldn't a bridge .atomic_check() also receive the state of the next bridge in the chain ?
I thought we had enough arguments passed to the atomic_check() hook and decided to let the driver retrieve the next state. I can pass the next bridge state if you prefer.
** &drm_bridge_funcs.atomic_check() hooks are called in reverse* order (from the last to the first bridge).** This method is optional.** RETURNS:* zero if the check passed, a negative error code otherwise.*/- int (*atomic_check)(struct drm_bridge *bridge,
struct drm_bridge_state *bridge_state,struct drm_crtc_state *crtc_state,struct drm_connector_state *conn_state);};
/** @@ -479,6 +499,9 @@ void drm_bridge_chain_mode_set(struct drm_encoder *encoder, void drm_bridge_chain_pre_enable(struct drm_encoder *encoder); void drm_bridge_chain_enable(struct drm_encoder *encoder);
+int drm_atomic_bridge_chain_check(struct drm_encoder *encoder,
struct drm_crtc_state *crtc_state,struct drm_connector_state *conn_state);void drm_atomic_bridge_chain_disable(struct drm_encoder *encoder, struct drm_atomic_state *state); void drm_atomic_bridge_chain_post_disable(struct drm_encoder *encoder,