Re: [PATCH 1/8] drm: Introduce an atomic_commit_setup function

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1.1: Type: text/plain, Size: 5804 bytes --]

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@ffwll.ch>
>>> Signed-off-by: Maxime Ripard <maxime@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

[-- Attachment #1.1.2: OpenPGP_0x680DC11D530B7A23.asc --]
[-- Type: application/pgp-keys, Size: 7535 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]