| Message ID | 20161121165257.22710-1-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@arm.com> > Reported-by: Russell King - ARM Linux <linux@armlinux.org.uk> > Cc: Russell King - ARM Linux <linux@armlinux.org.uk> > Fixes: 9f2a7950e77a ("drm/atomic-helper: nonblocking commit support") > Cc: Gustavo Padovan <gustavo.padovan@collabora.co.uk> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Tomeu Vizoso <tomeu.vizoso@gmail.com> > Cc: Daniel Stone <daniels@collabora.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > 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 >
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 + * 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 + * 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
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: - 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@arm.com> Reported-by: Russell King - ARM Linux <linux@armlinux.org.uk> Cc: Russell King - ARM Linux <linux@armlinux.org.uk> Fixes: 9f2a7950e77a ("drm/atomic-helper: nonblocking commit support") Cc: Gustavo Padovan <gustavo.padovan@collabora.co.uk> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Tomeu Vizoso <tomeu.vizoso@gmail.com> Cc: Daniel Stone <daniels@collabora.com> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- 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(-)