On Thu, May 02, 2019 at 03:49:45PM -0400, 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
>
> Cc: Daniel Vetter <daniel@xxxxxxxx>
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Signed-off-by: Sean Paul <seanpaul@xxxxxxxxxxxx>
> ---
> drivers/gpu/drm/drm_atomic_helper.c | 8 +-
> drivers/gpu/drm/drm_bridge.c | 110 +++++++++++++++++++++++++++
> include/drm/drm_bridge.h | 114 ++++++++++++++++++++++++++++
> 3 files changed, 228 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 1f81ca8daad7..9d9e47276839 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..accccb586adf 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
> + * the encoder's prepare op.
Since this is for atomic, where &drm_encoder_helper_funcs.atomic_disable
is the recommened one, would be good to change that. Plus use the
hyperlinked form. Same for all the below.
> + *
> + * 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
> + * the encoder's prepare op.
> + *
> + * 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_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.pre_enable (falls back on
atomic_pre_enable (I think all the others you've adjusted correctly).
> + * &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
> + * the encoder's commit op.
&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
> + * the encoder's commit op.
> + *
> + * 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..86f436895c7b 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -237,6 +237,111 @@ 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,
> + * &drm_encoder_helper_funcs.enable, &drm_encoder_helper_funcs.commit or
> + * &drm_encoder_helper_funcs.dpms hook.
I woulnd't mention the depracated (for atomic drivers) hooks anymore, i.e.
drop commit, dpms, prepare in all these texts. Also makes it more
readable.
> + *
> + * 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,
> + * &drm_encoder_helper_funcs.enable, &drm_encoder_helper_funcs.commit or
> + * &drm_encoder_helper_funcs.dpms 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,
> + * &drm_encoder_helper_funcs.disable, &drm_encoder_helper_funcs.prepare
> + * or &drm_encoder_helper_funcs.dpms 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,
> + * &drm_encoder_helper_funcs.disable, &drm_encoder_helper_funcs.prepare
> + * or &drm_encoder_helper_funcs.dpms hook.
> + *
> + * The bridge must assume that the display pipe (i.e. clocks and timing
> + * singals) 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 +419,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
With the doc nits:
Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
