On Mon, Nov 21, 2016 at 05:52:57PM +0100, Daniel Vetter wrote:
> I totally butcherd the job on typing the kernel-doc for these, and no
> one realized. Noticed by Russell. Maarten has a more complete approach
> to this confusion, by making it more explicit what the new/old state
> is, instead of this magic switching behaviour.
>
> v2:
I feel a v3 coming soon :)
> - Liviu pointed out that wait_for_fences is even more magic. Leave
> that as @state, and document @pre_swap better.
> - While at it, patch in header for the reference section.
> - Fix spelling issues Russell noticed.
>
> Cc: Liviu Dudau <liviu.dudau@xxxxxxx>
> Reported-by: Russell King - ARM Linux <linux@xxxxxxxxxxxxxxx>
> Cc: Russell King - ARM Linux <linux@xxxxxxxxxxxxxxx>
> Fixes: 9f2a7950e77a ("drm/atomic-helper: nonblocking commit support")
> Cc: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxxxx>
> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> Cc: Tomeu Vizoso <tomeu.vizoso@xxxxxxxxx>
> Cc: Daniel Stone <daniels@xxxxxxxxxxxxx>
> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> ---
> Documentation/gpu/drm-kms-helpers.rst | 3 ++
> drivers/gpu/drm/drm_atomic_helper.c | 78 ++++++++++++++++++--------------
> include/drm/drm_modeset_helper_vtables.h | 12 +++--
> 3 files changed, 54 insertions(+), 39 deletions(-)
>
> diff --git a/Documentation/gpu/drm-kms-helpers.rst b/Documentation/gpu/drm-kms-helpers.rst
> index 4ca77f675967..03040aa14fe8 100644
> --- a/Documentation/gpu/drm-kms-helpers.rst
> +++ b/Documentation/gpu/drm-kms-helpers.rst
> @@ -63,6 +63,9 @@ Atomic State Reset and Initialization
> .. kernel-doc:: drivers/gpu/drm/drm_atomic_helper.c
> :doc: atomic state reset and initialization
>
> +Helper Functions Reference
> +--------------------------
> +
> .. kernel-doc:: include/drm/drm_atomic_helper.h
> :internal:
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 0b16587cdc62..86459554ef5f 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1006,13 +1006,21 @@ EXPORT_SYMBOL(drm_atomic_helper_commit_modeset_enables);
> * drm_atomic_helper_wait_for_fences - wait for fences stashed in plane state
> * @dev: DRM device
> * @state: atomic state object with old state structures
> - * @pre_swap: if true, do an interruptible wait
> + * @pre_swap: If true, do an interruptible wait, and @state is the new state.
> + * Otherwise @state is the old state.
> *
> * For implicit sync, driver should fish the exclusive fence out from the
> * incoming fb's and stash it in the drm_plane_state. This is called after
> * drm_atomic_helper_swap_state() so it uses the current plane state (and
> * just uses the atomic state to find the changed planes)
> *
> + * Note that @pre_swap is needed since we the point where we block for fences
confused about 'we the point' in there. Feels like you were trying to say something else?
> + * moves around depending upon whether an atomic commit is synchronous or
> + * asynchronous. For async commit all waiting needs to happen after
> + * drm_atomic_helper_swap_state() is called, but for synchronous commits we want
> + * to wait _before_ we do anything that can't be easily rolled back. And hence
s/And hence/That is/
Otherwise, it looks good to me.
Best regards,
Liviu
> + * before we call drm_atomic_helper_swap_state().
> + *
> * Returns zero if success or < 0 if dma_fence_wait() fails.
> */
> int drm_atomic_helper_wait_for_fences(struct drm_device *dev,
> @@ -1147,7 +1155,7 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks);
>
> /**
> * drm_atomic_helper_commit_tail - commit atomic update to hardware
> - * @state: new modeset state to be committed
> + * @old_state: atomic state object with old state structures
> *
> * This is the default implemenation for the ->atomic_commit_tail() hook of the
> * &drm_mode_config_helper_funcs vtable.
> @@ -1158,53 +1166,53 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks);
> *
> * For drivers supporting runtime PM the recommended sequence is instead ::
> *
> - * drm_atomic_helper_commit_modeset_disables(dev, state);
> + * drm_atomic_helper_commit_modeset_disables(dev, old_state);
> *
> - * drm_atomic_helper_commit_modeset_enables(dev, state);
> + * drm_atomic_helper_commit_modeset_enables(dev, old_state);
> *
> - * drm_atomic_helper_commit_planes(dev, state,
> + * drm_atomic_helper_commit_planes(dev, old_state,
> * DRM_PLANE_COMMIT_ACTIVE_ONLY);
> *
> * for committing the atomic update to hardware. See the kerneldoc entries for
> * these three functions for more details.
> */
> -void drm_atomic_helper_commit_tail(struct drm_atomic_state *state)
> +void drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state)
> {
> - struct drm_device *dev = state->dev;
> + struct drm_device *dev = old_state->dev;
>
> - drm_atomic_helper_commit_modeset_disables(dev, state);
> + drm_atomic_helper_commit_modeset_disables(dev, old_state);
>
> - drm_atomic_helper_commit_planes(dev, state, 0);
> + drm_atomic_helper_commit_planes(dev, old_state, 0);
>
> - drm_atomic_helper_commit_modeset_enables(dev, state);
> + drm_atomic_helper_commit_modeset_enables(dev, old_state);
>
> - drm_atomic_helper_commit_hw_done(state);
> + drm_atomic_helper_commit_hw_done(old_state);
>
> - drm_atomic_helper_wait_for_vblanks(dev, state);
> + drm_atomic_helper_wait_for_vblanks(dev, old_state);
>
> - drm_atomic_helper_cleanup_planes(dev, state);
> + drm_atomic_helper_cleanup_planes(dev, old_state);
> }
> EXPORT_SYMBOL(drm_atomic_helper_commit_tail);
>
> -static void commit_tail(struct drm_atomic_state *state)
> +static void commit_tail(struct drm_atomic_state *old_state)
> {
> - struct drm_device *dev = state->dev;
> + struct drm_device *dev = old_state->dev;
> struct drm_mode_config_helper_funcs *funcs;
>
> funcs = dev->mode_config.helper_private;
>
> - drm_atomic_helper_wait_for_fences(dev, state, false);
> + drm_atomic_helper_wait_for_fences(dev, old_state, false);
>
> - drm_atomic_helper_wait_for_dependencies(state);
> + drm_atomic_helper_wait_for_dependencies(old_state);
>
> if (funcs && funcs->atomic_commit_tail)
> - funcs->atomic_commit_tail(state);
> + funcs->atomic_commit_tail(old_state);
> else
> - drm_atomic_helper_commit_tail(state);
> + drm_atomic_helper_commit_tail(old_state);
>
> - drm_atomic_helper_commit_cleanup_done(state);
> + drm_atomic_helper_commit_cleanup_done(old_state);
>
> - drm_atomic_state_put(state);
> + drm_atomic_state_put(old_state);
> }
>
> static void commit_work(struct work_struct *work)
> @@ -1498,10 +1506,10 @@ static struct drm_crtc_commit *preceeding_commit(struct drm_crtc *crtc)
>
> /**
> * drm_atomic_helper_wait_for_dependencies - wait for required preceeding commits
> - * @state: new modeset state to be committed
> + * @old_state: atomic state object with old state structures
> *
> * This function waits for all preceeding commits that touch the same CRTC as
> - * @state to both be committed to the hardware (as signalled by
> + * @old_state to both be committed to the hardware (as signalled by
> * drm_atomic_helper_commit_hw_done) and executed by the hardware (as signalled
> * by calling drm_crtc_vblank_send_event on the event member of
> * &drm_crtc_state).
> @@ -1509,7 +1517,7 @@ static struct drm_crtc_commit *preceeding_commit(struct drm_crtc *crtc)
> * This is part of the atomic helper support for nonblocking commits, see
> * drm_atomic_helper_setup_commit() for an overview.
> */
> -void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *state)
> +void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *old_state)
> {
> struct drm_crtc *crtc;
> struct drm_crtc_state *crtc_state;
> @@ -1517,7 +1525,7 @@ void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state *state)
> int i;
> long ret;
>
> - for_each_crtc_in_state(state, crtc, crtc_state, i) {
> + for_each_crtc_in_state(old_state, crtc, crtc_state, i) {
> spin_lock(&crtc->commit_lock);
> commit = preceeding_commit(crtc);
> if (commit)
> @@ -1548,7 +1556,7 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
>
> /**
> * drm_atomic_helper_commit_hw_done - setup possible nonblocking commit
> - * @state: new modeset state to be committed
> + * @old_state: atomic state object with old state structures
> *
> * This function is used to signal completion of the hardware commit step. After
> * this step the driver is not allowed to read or change any permanent software
> @@ -1561,15 +1569,15 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
> * This is part of the atomic helper support for nonblocking commits, see
> * drm_atomic_helper_setup_commit() for an overview.
> */
> -void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *state)
> +void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *old_state)
> {
> struct drm_crtc *crtc;
> struct drm_crtc_state *crtc_state;
> struct drm_crtc_commit *commit;
> int i;
>
> - for_each_crtc_in_state(state, crtc, crtc_state, i) {
> - commit = state->crtcs[i].commit;
> + for_each_crtc_in_state(old_state, crtc, crtc_state, i) {
> + commit = old_state->crtcs[i].commit;
> if (!commit)
> continue;
>
> @@ -1584,16 +1592,16 @@ EXPORT_SYMBOL(drm_atomic_helper_commit_hw_done);
>
> /**
> * drm_atomic_helper_commit_cleanup_done - signal completion of commit
> - * @state: new modeset state to be committed
> + * @old_state: atomic state object with old state structures
> *
> - * This signals completion of the atomic update @state, including any cleanup
> - * work. If used, it must be called right before calling
> + * This signals completion of the atomic update @old_state, including any
> + * cleanup work. If used, it must be called right before calling
> * drm_atomic_state_put().
> *
> * This is part of the atomic helper support for nonblocking commits, see
> * drm_atomic_helper_setup_commit() for an overview.
> */
> -void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *state)
> +void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *old_state)
> {
> struct drm_crtc *crtc;
> struct drm_crtc_state *crtc_state;
> @@ -1601,8 +1609,8 @@ void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *state)
> int i;
> long ret;
>
> - for_each_crtc_in_state(state, crtc, crtc_state, i) {
> - commit = state->crtcs[i].commit;
> + for_each_crtc_in_state(old_state, crtc, crtc_state, i) {
> + commit = old_state->crtcs[i].commit;
> if (WARN_ON(!commit))
> continue;
>
> diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h
> index 72478cf82147..69c3974bf133 100644
> --- a/include/drm/drm_modeset_helper_vtables.h
> +++ b/include/drm/drm_modeset_helper_vtables.h
> @@ -999,10 +999,14 @@ struct drm_mode_config_helper_funcs {
> * to implement blocking and nonblocking commits easily. It is not used
> * by the atomic helpers
> *
> - * This hook should first commit the given atomic state to the hardware.
> - * But drivers can add more waiting calls at the start of their
> - * implementation, e.g. to wait for driver-internal request for implicit
> - * syncing, before starting to commit the update to the hardware.
> + * This function is called when the new atomic state has already been
> + * swapped into the various state pointers. The passed in state
> + * therefore contains copies of the old/previous state. This hook should
> + * commit the new state into hardware. Note that the helpers have
> + * already waited for preceeding atomic commits and fences, but drivers
> + * can add more waiting calls at the start of their implementation, e.g.
> + * to wait for driver-internal request for implicit syncing, before
> + * starting to commit the update to the hardware.
> *
> * After the atomic update is committed to the hardware this hook needs
> * to call drm_atomic_helper_commit_hw_done(). Then wait for the upate
> --
> 2.10.2
>
--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
