Hi Am 19.11.20 um 16:32 schrieb Daniel Vetter:
On Thu, Nov 19, 2020 at 10:59:42AM +0100, Thomas Zimmermann wrote:Hi Am 13.11.20 um 16:29 schrieb Maxime Ripard:Private objects storing a state shared across all CRTCs need to be carefully handled to avoid a use-after-free issue. The proper way to do this to track all the commits using that shared state and wait for the previous commits to be done before going on with the current one to avoid the reordering of commits that could occur. However, this commit setup needs to be done after drm_atomic_helper_setup_commit(), because before the CRTC commit structure hasn't been allocated before, and before the workqueue is scheduled, because we would be potentially reordered already otherwise. That means that drivers currently have to roll their own drm_atomic_helper_commit() function, even though it would be identical if not for the commit setup. Let's introduce a hook to do so that would be called as part of drm_atomic_helper_commit, allowing us to reuse the atomic helpers. Suggested-by: Daniel Vetter <daniel.vetter@xxxxxxxx> Signed-off-by: Maxime Ripard <maxime@xxxxxxxxxx> --- drivers/gpu/drm/drm_atomic_helper.c | 6 ++++++ include/drm/drm_modeset_helper_vtables.h | 18 ++++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index ddd0e3239150..7d69c7844dfc 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -2083,8 +2083,11 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state, struct drm_plane *plane; struct drm_plane_state *old_plane_state, *new_plane_state; struct drm_crtc_commit *commit; + const struct drm_mode_config_helper_funcs *funcs; int i, ret; + funcs = state->dev->mode_config.helper_private; + for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) { commit = kzalloc(sizeof(*commit), GFP_KERNEL); if (!commit) @@ -2169,6 +2172,9 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state, new_plane_state->commit = drm_crtc_commit_get(commit); } + if (funcs && funcs->atomic_commit_setup) + return funcs->atomic_commit_setup(state); + return 0; } EXPORT_SYMBOL(drm_atomic_helper_setup_commit); diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h index f2de050085be..56470baf0513 100644 --- a/include/drm/drm_modeset_helper_vtables.h +++ b/include/drm/drm_modeset_helper_vtables.h @@ -1396,6 +1396,24 @@ struct drm_mode_config_helper_funcs { * drm_atomic_helper_commit_tail(). */ void (*atomic_commit_tail)(struct drm_atomic_state *state); + + /** + * @atomic_commit_setup: + * + * This hook is used by the default atomic_commit() hook implemented in + * drm_atomic_helper_commit() together with the nonblocking helpers (see + * drm_atomic_helper_setup_commit()) to extend the DRM commit setup. It + * is not used by the atomic helpers. + * + * This function is called at the end of + * drm_atomic_helper_setup_commit(), so once the commit has been + * properly setup across the generic DRM object states. It allows + * drivers to do some additional commit tracking that isn't related to a + * CRTC, plane or connector, typically a private object. + * + * This hook is optional. + */ + int (*atomic_commit_setup)(struct drm_atomic_state *state);It feels hacky and screwed-on to me. I'd suggest to add an atomic_commit_prepare callback that is called by drm_atomic_helper where it currently calls drm_atomic_helper_setup_commit(). The default implementation would include setup_commit and prepare_planes. Some example code for drm_atomic_helper.c static int commit_prepare(state) { drm_atomic_helper_setup_commit(state) drm_atomic_helper_prepare_planes(state) } int drm_atomic_helper_commit() { if (async_update) { ... } if (funcs->atomic_commit_prepare) funcs->atomic_commit_prepare(state) else commit_prepare(state) /* the rest of the current function below */ INIT_WORK(&state->commit_work, commit_work); ... } Drivers that implement atomic_commit_prepare would be expected to call drm_atomic_helper_setup_commit() and drm_atomic_helper_prepare_planes() or their own implementation of them. The whole construct mimics how commit tails work.Yeah what I suggested is a bit much midlayer, but we've done what you suggested above with all drivers rolling their own atomic_commit. It wasn't pretty. There's still drivers like vc4 which haven't switched, and which have their own locking and synchronization. Hence why I think the callback approach here is better, gives drivers less excuses to roll their own and make a mess.
But it sounds like you'll regret this. As soon as a driver has to do additional stuff at the beginning of the setup function, another helper will be required, and so on. Maybe rather go with the commit_prepare helper and push driver authors to not use it.
Best regards Thomas
-Daniel
-- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer
Attachment:
OpenPGP_0x680DC11D530B7A23.asc
Description: application/pgp-keys
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
