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

[Daniel Vetter]

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch