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

[Daniel Vetter <daniel@ffwll.ch>]

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?

> 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 ...

> 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.
-Daniel

> Maarten Lankhorst (4):
>   drm/atomic: Add new iterators over all state, v2.
>   drm/atomic: Add accessor macros for the current state.
>   drm/atomic: Add macros to access existing old/new state
>   drm/atomic: Add checks to ensure get_state is not called after
>     swapping.
> 
>  drivers/gpu/drm/drm_atomic.c         |  24 +++-
>  drivers/gpu/drm/drm_atomic_helper.c  |  47 ++++++-
>  drivers/gpu/drm/i915/intel_display.c |  11 +-
>  include/drm/drm_atomic.h             | 247 ++++++++++++++++++++++++++++++++++-
>  include/drm/drm_atomic_helper.h      |   2 +
>  include/drm/drm_modeset_lock.h       |  21 +++
>  6 files changed, 338 insertions(+), 14 deletions(-)
> 
> -- 
> 2.7.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel