[PATCH 1/8] drm: Introduce an atomic_commit_setup function
Thomas Zimmermann
tzimmermann at suse.de
Fri Nov 20 08:38:59 UTC 2020
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 at ffwll.ch>
>>> Signed-off-by: Maxime Ripard <maxime at cerno.tech>
>>> ---
>>> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_0x680DC11D530B7A23.asc
Type: application/pgp-keys
Size: 7435 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20201120/ae754e2b/attachment.key>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20201120/ae754e2b/attachment.sig>
More information about the dri-devel
mailing list