On Tue, May 21, 2019 at 10:58:06AM +0200, Andrzej Hajda wrote: > On 08.05.2019 18:09, Sean Paul wrote: > > From: Sean Paul <seanpaul@xxxxxxxxxxxx> > > > > This patch adds atomic variants for all of > > pre_enable/enable/disable/post_disable bridge functions. These will be > > called from the appropriate atomic helper functions. If the bridge > > driver doesn't implement the atomic version of the function, we will > > fall back to the vanilla implementation. > > > > Note that some drivers call drm_bridge_disable directly, and these cases > > are not covered. It's up to the driver to decide whether to implement > > both atomic_disable and disable, or if it's not necessary. > > > > Changes in v3: > > - Added to the patchset > > Changes in v4: > > - Fix up docbook references (Daniel) > > > > Link to v3: https://patchwork.freedesktop.org/patch/msgid/20190502194956.218441-4-sean@xxxxxxxxxx > > > > Cc: Daniel Vetter <daniel@xxxxxxxx> > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> > > Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx> > > Signed-off-by: Sean Paul <seanpaul@xxxxxxxxxxxx> > > > In general patch looks OK, ie. it correctly mimics old behavior with > added atomic state. > > However I have mixed feelings about it, because of the whole idea that > order of enabling/disabling bridges in the chain is the same for all > bridge chains is just incorrect. It depends on link types between > bridges, there are also multi-in/out bridges which does not fit at all > in this model. Due to this lack of flexibility developers either > explicitly avoids drm_bridge->next chaining in favor of custom > solutions, or worse they forcibly try to fit into this model by either > violating hw specs, either by introducing hacks into the drivers. > > The simplest solution for this would be to just let parent bridge calls > children's callbacks explicitly (as exynos_drm_dsi and vc4_dsi do). Yeah, I completely agree that the bridge chaining is worn out. I also agree that letting the parent call the child would alleviate some of this. However I do worry that relying on the driver to order the calls correctly might be too much to ask. I think the pattern for handling this type of thing is to add bridge helper functions that allow drivers to override the default behavior when a tricky situation arises. This would allow us to break down the bridge midlayer and more tightly integrate it into the atomic world. > > But since it is not a blocker I will left it up to you :) : That said, agree that it's out of scope for this project. If you like the above, I can add a TODO item to the list in a follow-up patch. > > Reviewed-by: Andrzej Hajda <a.hajda@xxxxxxxxxxx> Thank you! Sean > > > Regards > > Andrzej > > > > --- > > drivers/gpu/drm/drm_atomic_helper.c | 8 +- > > drivers/gpu/drm/drm_bridge.c | 110 ++++++++++++++++++++++++++++ > > include/drm/drm_bridge.h | 106 +++++++++++++++++++++++++++ > > 3 files changed, 220 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > > index ccf01831f265..e8b7187a8494 100644 > > --- a/drivers/gpu/drm/drm_atomic_helper.c > > +++ b/drivers/gpu/drm/drm_atomic_helper.c > > @@ -995,7 +995,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. > > */ > > - drm_bridge_disable(encoder->bridge); > > + drm_atomic_bridge_disable(encoder->bridge, old_state); > > > > /* Right function depends upon target state. */ > > if (funcs) { > > @@ -1009,7 +1009,7 @@ 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); > > + drm_atomic_bridge_post_disable(encoder->bridge, old_state); > > } > > > > for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) { > > @@ -1308,7 +1308,7 @@ 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); > > + drm_atomic_bridge_pre_enable(encoder->bridge, old_state); > > > > if (funcs) { > > if (funcs->atomic_enable) > > @@ -1319,7 +1319,7 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev, > > funcs->commit(encoder); > > } > > > > - drm_bridge_enable(encoder->bridge); > > + drm_atomic_bridge_enable(encoder->bridge, old_state); > > } > > > > drm_atomic_helper_commit_writebacks(dev, old_state); > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c > > index 138b2711d389..cba537c99e43 100644 > > --- a/drivers/gpu/drm/drm_bridge.c > > +++ b/drivers/gpu/drm/drm_bridge.c > > @@ -352,6 +352,116 @@ void drm_bridge_enable(struct drm_bridge *bridge) > > } > > EXPORT_SYMBOL(drm_bridge_enable); > > > > +/** > > + * drm_atomic_bridge_disable - disables all bridges in the encoder chain > > + * @bridge: bridge control structure > > + * @state: atomic state being committed > > + * > > + * Calls &drm_bridge_funcs.atomic_disable (falls back on > > + * &drm_bridge_funcs.disable) 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_disable > > + * > > + * Note: the bridge passed should be the one closest to the encoder > > + */ > > +void drm_atomic_bridge_disable(struct drm_bridge *bridge, > > + struct drm_atomic_state *state) > > +{ > > + if (!bridge) > > + return; > > + > > + drm_atomic_bridge_disable(bridge->next, state); > > + > > + if (bridge->funcs->atomic_disable) > > + bridge->funcs->atomic_disable(bridge, state); > > + else if (bridge->funcs->disable) > > + bridge->funcs->disable(bridge); > > +} > > +EXPORT_SYMBOL(drm_atomic_bridge_disable); > > + > > +/** > > + * drm_atomic_bridge_post_disable - cleans up after disabling all bridges in the > > + * encoder chain > > + * @bridge: bridge control structure > > + * @state: atomic state being committed > > + * > > + * Calls &drm_bridge_funcs.atomic_post_disable (falls back on > > + * &drm_bridge_funcs.post_disable) op for all the bridges in the encoder chain, > > + * starting from the first bridge to the last. These are called after completing > > + * &drm_encoder_helper_funcs.atomic_disable > > + * > > + * Note: the bridge passed should be the one closest to the encoder > > + */ > > +void drm_atomic_bridge_post_disable(struct drm_bridge *bridge, > > + struct drm_atomic_state *state) > > +{ > > + if (!bridge) > > + return; > > + > > + if (bridge->funcs->atomic_post_disable) > > + bridge->funcs->atomic_post_disable(bridge, state); > > + else if (bridge->funcs->post_disable) > > + bridge->funcs->post_disable(bridge); > > + > > + drm_atomic_bridge_post_disable(bridge->next, state); > > +} > > +EXPORT_SYMBOL(drm_atomic_bridge_post_disable); > > + > > +/** > > + * drm_atomic_bridge_pre_enable - prepares for enabling all bridges in the > > + * encoder chain > > + * @bridge: bridge control structure > > + * @state: atomic state being committed > > + * > > + * Calls &drm_bridge_funcs.atomic_pre_enable (falls back on > > + * &drm_bridge_funcs.pre_enable) 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_enable > > + * > > + * Note: the bridge passed should be the one closest to the encoder > > + */ > > +void drm_atomic_bridge_pre_enable(struct drm_bridge *bridge, > > + struct drm_atomic_state *state) > > +{ > > + if (!bridge) > > + return; > > + > > + drm_atomic_bridge_pre_enable(bridge->next, state); > > + > > + if (bridge->funcs->atomic_pre_enable) > > + bridge->funcs->atomic_pre_enable(bridge, state); > > + else if (bridge->funcs->pre_enable) > > + bridge->funcs->pre_enable(bridge); > > +} > > +EXPORT_SYMBOL(drm_atomic_bridge_pre_enable); > > + > > +/** > > + * drm_atomic_bridge_enable - enables all bridges in the encoder chain > > + * @bridge: bridge control structure > > + * @state: atomic state being committed > > + * > > + * Calls &drm_bridge_funcs.atomic_enable (falls back on > > + * &drm_bridge_funcs.enable) op for all the bridges in the encoder chain, > > + * starting from the first bridge to the last. These are called after completing > > + * &drm_encoder_helper_funcs.atomic_enable > > + * > > + * Note: the bridge passed should be the one closest to the encoder > > + */ > > +void drm_atomic_bridge_enable(struct drm_bridge *bridge, > > + struct drm_atomic_state *state) > > +{ > > + if (!bridge) > > + return; > > + > > + if (bridge->funcs->atomic_enable) > > + bridge->funcs->atomic_enable(bridge, state); > > + else if (bridge->funcs->enable) > > + bridge->funcs->enable(bridge); > > + > > + drm_atomic_bridge_enable(bridge->next, state); > > +} > > +EXPORT_SYMBOL(drm_atomic_bridge_enable); > > + > > #ifdef CONFIG_OF > > /** > > * of_drm_find_bridge - find the bridge corresponding to the device node in > > diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h > > index d4428913a4e1..322801884814 100644 > > --- a/include/drm/drm_bridge.h > > +++ b/include/drm/drm_bridge.h > > @@ -237,6 +237,103 @@ struct drm_bridge_funcs { > > * The enable callback is optional. > > */ > > void (*enable)(struct drm_bridge *bridge); > > + > > + /** > > + * @atomic_pre_enable: > > + * > > + * This callback should enable the bridge. It is called right before > > + * the preceding element in the display pipe is enabled. If the > > + * preceding element is a bridge this means it's called before that > > + * bridge's @atomic_pre_enable or @pre_enable function. If the preceding > > + * element is a &drm_encoder it's called right before the encoder's > > + * &drm_encoder_helper_funcs.atomic_enable hook. > > + * > > + * The display pipe (i.e. clocks and timing signals) feeding this bridge > > + * will not yet be running when this callback is called. The bridge must > > + * not enable the display link feeding the next bridge in the chain (if > > + * there is one) when this callback is called. > > + * > > + * Note that this function will only be invoked in the context of an > > + * atomic commit. It will not be invoked from &drm_bridge_pre_enable. It > > + * would be prudent to also provide an implementation of @pre_enable if > > + * you are expecting driver calls into &drm_bridge_pre_enable. > > + * > > + * The @atomic_pre_enable callback is optional. > > + */ > > + void (*atomic_pre_enable)(struct drm_bridge *bridge, > > + struct drm_atomic_state *state); > > + > > + /** > > + * @atomic_enable: > > + * > > + * This callback should enable the bridge. It is called right after > > + * the preceding element in the display pipe is enabled. If the > > + * preceding element is a bridge this means it's called after that > > + * bridge's @atomic_enable or @enable function. If the preceding element > > + * is a &drm_encoder it's called right after the encoder's > > + * &drm_encoder_helper_funcs.atomic_enable hook. > > + * > > + * The bridge can assume that the display pipe (i.e. clocks and timing > > + * signals) feeding it is running when this callback is called. This > > + * callback must enable the display link feeding the next bridge in the > > + * chain if there is one. > > + * > > + * Note that this function will only be invoked in the context of an > > + * atomic commit. It will not be invoked from &drm_bridge_enable. It > > + * would be prudent to also provide an implementation of @enable if > > + * you are expecting driver calls into &drm_bridge_enable. > > + * > > + * The enable callback is optional. > > + */ > > + void (*atomic_enable)(struct drm_bridge *bridge, > > + struct drm_atomic_state *state); > > + /** > > + * @atomic_disable: > > + * > > + * This callback should disable the bridge. It is called right before > > + * the preceding element in the display pipe is disabled. If the > > + * preceding element is a bridge this means it's called before that > > + * bridge's @atomic_disable or @disable vfunc. If the preceding element > > + * is a &drm_encoder it's called right before the > > + * &drm_encoder_helper_funcs.atomic_disable hook. > > + * > > + * The bridge can assume that the display pipe (i.e. clocks and timing > > + * signals) feeding it is still running when this callback is called. > > + * > > + * Note that this function will only be invoked in the context of an > > + * atomic commit. It will not be invoked from &drm_bridge_disable. It > > + * would be prudent to also provide an implementation of @disable if > > + * you are expecting driver calls into &drm_bridge_disable. > > + * > > + * The disable callback is optional. > > + */ > > + void (*atomic_disable)(struct drm_bridge *bridge, > > + struct drm_atomic_state *state); > > + > > + /** > > + * @atomic_post_disable: > > + * > > + * This callback should disable the bridge. It is called right after the > > + * preceding element in the display pipe is disabled. If the preceding > > + * element is a bridge this means it's called after that bridge's > > + * @atomic_post_disable or @post_disable function. If the preceding > > + * element is a &drm_encoder it's called right after the encoder's > > + * &drm_encoder_helper_funcs.atomic_disable hook. > > + * > > + * The bridge must assume that the display pipe (i.e. clocks and timing > > + * signals) feeding it is no longer running when this callback is > > + * called. > > + * > > + * Note that this function will only be invoked in the context of an > > + * atomic commit. It will not be invoked from &drm_bridge_post_disable. > > + * It would be prudent to also provide an implementation of > > + * @post_disable if you are expecting driver calls into > > + * &drm_bridge_post_disable. > > + * > > + * The post_disable callback is optional. > > + */ > > + void (*atomic_post_disable)(struct drm_bridge *bridge, > > + struct drm_atomic_state *state); > > }; > > > > /** > > @@ -314,6 +411,15 @@ void drm_bridge_mode_set(struct drm_bridge *bridge, > > void drm_bridge_pre_enable(struct drm_bridge *bridge); > > void drm_bridge_enable(struct drm_bridge *bridge); > > > > +void drm_atomic_bridge_disable(struct drm_bridge *bridge, > > + struct drm_atomic_state *state); > > +void drm_atomic_bridge_post_disable(struct drm_bridge *bridge, > > + struct drm_atomic_state *state); > > +void drm_atomic_bridge_pre_enable(struct drm_bridge *bridge, > > + struct drm_atomic_state *state); > > +void drm_atomic_bridge_enable(struct drm_bridge *bridge, > > + struct drm_atomic_state *state); > > + > > #ifdef CONFIG_DRM_PANEL_BRIDGE > > struct drm_bridge *drm_panel_bridge_add(struct drm_panel *panel, > > u32 connector_type); > > -- Sean Paul, Software Engineer, Google / Chromium OS _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
