[PATCH v2 0/4] drm/atomic: Add accessor macros for all atomic state.

[Maarten Lankhorst]

Op 16-11-16 om 15:18 schreef Daniel Vetter:
> On Wed, Nov 16, 2016 at 02:58:04PM +0100, Maarten Lankhorst wrote:
>> Second approach. Instead of trying to convert all drivers straight away,
>> implement all macros that are required to get state working.
>>
>> Current situation:
>> Use obj->state, which can refer to old or new state.
>> Use drm_atomic_get_(existing_)obj_state, which can refer to new or old state.
>> Use for_each_obj_in_state, which refers to new or old state.
>>
>> New situation:
>> When doing some dereferencing outside atomic_state, use
>> drm_atomic_get_current_obj_state which has locking checks, instead of
>> obj->state.
>>
>> During atomic check:
>> - Use drm_atomic_get_obj_state to add a object to the atomic state,
>>   or get the new state.
>> - Use drm_atomic_get_(old/new)_obj_state to peek at the new/old state,
>>   without adding the object if it's not part of the state. For planes
>>   and connectors the relevant crtc_state is added, so this will work to
>>   get the crtc_state from foo_state->crtc too, saves some error handling. :)
> Hm, this needs to check looking, somehow. Otherwise everyone just randomly
> peeks at state and all hell breaks loose once more. Or why do you want to
> avoid adding state for CRTCs?
We don't avoid adding state for crtc's, we always add them as required.

Some ->check_plane callbacks do things like:

if (plane_state->crtc) {
crtc_state = get_crtc_state(plane_state->crtc);
ret = PTR_ERR_OR_ZERO(crtc_state);
if (ret)
return ret;
}

which can be simplified to

if (plane_state->crtc) crtc_state = get_crtc_state(plane_state->crtc); /* No need to null check */

Same for grabbing crtc_state from connector_state.

No changes in locking required. In fact when called from atomic_commit_tail all locks may have been dropped already.
>> During atomic commit:
>> - Do not use drm_atomic_get_obj_state, obj->state or drm_atomic_get_(existing_)obj_state
>>   any more, replace with drm_atomic_get_old/new_obj_state calls as required.
> Wild idea, can we enforce this? E.g. with a drm_mode_config->in_atomic_check
> atomic counter that we inc/dec around the atomic_check call, and then a
> WARN_ON(!dev->mode_config.in_atomic_check); It will have some false
> positives when concurrent atomic commits happen, but for testing it should
> catch all offenders.
>
> Of course this won't catch obj->state access, but we can fix that by
> renaming to obj->_state ...
drm_atomic_get_existing is allowed for now, but should be converted. Patch 4 adds WARNS if used after swap_state,
which is probably where most offenders are.

Relatedly.. can we revive the ww_acquire_done patch again? Needs a fix for i915 load detect though..


>> During both:
>> - Use for_each_(new,old,oldnew)_obj_in_state to get the old or new state as needed.
>>   oldnew will be renamed to for_each_obj_in_state after all callers are converted
>>   to the new api.
>>
>> This will give the correct state regardless of swapping.
> Otherwise sounds like a reasonable plan I think.
Ok good. :)

~Maarten