On Wed, Nov 16, 2016 at 05:11:31PM +0100, Maarten Lankhorst wrote:
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..
Yeah, just bring it on. -Daniel
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