On 02/11/14 13:19, Daniel Vetter wrote:> The atomic users and helpers assume that there is always a obj->state
structure around. Which means drivers need to somehow create that at driver load time. Also it should obviously reset hardware state, so needs to be reset upon resume.
Finally the destroy/duplicate_state functions are an awful lot of boilerplate if the driver doesn't need anything beyond the default state objects.
So add helper functions for all of this.
v2: Somehow the plane/connector versions got lost in the first version.
v3: Add kerneldoc.
v4: Make duplicate_state functions a bit more robust, which is useful for debugging state tracking issues when transitioning to atomic.
v5: Clear temporary variables in the crtc state when duplicating it, like ->mode_changed or ->planes_changed. If we don't do this stale values for these might pollute the next atomic modeset.
v6: Also clear crtc_state->event in case the driver didn't (yet) clear this out.
Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch
drivers/gpu/drm/drm_atomic_helper.c | 154
+++++++++++++++++++++++++++++++++++-
include/drm/drm_atomic_helper.h | 19 +++++ 2 files changed, 170 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
index 70bd67cf86e3..bd38df3cbe55 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1429,7 +1429,7 @@ EXPORT_SYMBOL(drm_atomic_helper_set_config); /**
- drm_atomic_helper_crtc_set_property - helper for crtc prorties
- @crtc: DRM crtc
- @prorty: DRM property
- @property: DRM property
This looks like a bad fixup (should be in patch 11).
- @val: value of property
- Provides a default plane disablle handler using the atomic driver
interface.
@@ -1493,7 +1493,7 @@ EXPORT_SYMBOL(drm_atomic_helper_crtc_set_property); /**
- drm_atomic_helper_plane_set_property - helper for plane prorties
- @plane: DRM plane
- @prorty: DRM property
- @property: DRM property
+1
- @val: value of property
- Provides a default plane disable handler using the atomic driver
interface.
@@ -1557,7 +1557,7 @@ EXPORT_SYMBOL(drm_atomic_helper_plane_set_property); /**
- drm_atomic_helper_connector_set_property - helper for connector
prorties
- @connector: DRM connector
- @prorty: DRM property
- @property: DRM property
+1
- @val: value of property
- Provides a default plane disablle handler using the atomic driver
interface.
@@ -1707,3 +1707,151 @@ backoff: goto retry; } EXPORT_SYMBOL(drm_atomic_helper_page_flip);
+/**
- drm_atomic_helper_crtc_reset - default ->reset hook for CRTCs
- @crtc: drm CRTC
- Resets the atomic state for @crtc by freeing the state pointer and
allocating
- a new empty state object.
- */
+void drm_atomic_helper_crtc_reset(struct drm_crtc *crtc) +{
- kfree(crtc->state);
- crtc->state = kzalloc(sizeof(*crtc->state), GFP_KERNEL);
This code looks semantically equivalent to a memset() although it may result in a change to the pointer value. Is this code trying to flush out uses-after-free?
I can't find this free/alloc pattern in delivered code anywhere else in the drm code base. Should this need to be replaced with memset() before merging (or at least commenting)?
+} +EXPORT_SYMBOL(drm_atomic_helper_crtc_reset);
+/**
- drm_atomic_helper_crtc_duplicate_state - default state duplicate hook
- @crtc: drm CRTC
- Default CRTC state duplicate hook for drivers which don't have
their own
- subclassed CRTC state structure.
- */
+struct drm_crtc_state * +drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc) +{
- struct drm_crtc_state *state;
- if (WARN_ON(!crtc->state))
return NULL;- state = kmemdup(crtc->state, sizeof(*crtc->state), GFP_KERNEL);
- if (state) {
state->mode_changed = false;state->planes_changed = false;state->event = NULL;- }
- return state;
+} +EXPORT_SYMBOL(drm_atomic_helper_crtc_duplicate_state);
+/**
- drm_atomic_helper_crtc_destroy_state - default state destroy hook
- @crtc: drm CRTC
- @state: CRTC state object to release
- Default CRTC state destroy hook for drivers which don't have their own
- subclassed CRTC state structure.
- */
+void drm_atomic_helper_crtc_destroy_state(struct drm_crtc *crtc,
struct drm_crtc_state *state)+{
- kfree(state);
+} +EXPORT_SYMBOL(drm_atomic_helper_crtc_destroy_state);
+/**
- drm_atomic_helper_plane_reset - default ->reset hook for planes
- @plane: drm plane
- Resets the atomic state for @plane by freeing the state pointer and
- allocating a new empty state object.
- */
+void drm_atomic_helper_plane_reset(struct drm_plane *plane) +{
- kfree(plane->state);
- plane->state = kzalloc(sizeof(*plane->state), GFP_KERNEL);
+1
+} +EXPORT_SYMBOL(drm_atomic_helper_plane_reset);
+/**
- drm_atomic_helper_plane_duplicate_state - default state duplicate hook
- @plane: drm plane
- Default plane state duplicate hook for drivers which don't have
their own
- subclassed plane state structure.
- */
+struct drm_plane_state * +drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane) +{
- if (WARN_ON(!plane->state))
return NULL;- return kmemdup(plane->state, sizeof(*plane->state), GFP_KERNEL);
+} +EXPORT_SYMBOL(drm_atomic_helper_plane_duplicate_state);
+/**
- drm_atomic_helper_plane_destroy_state - default state destroy hook
- @plane: drm plane
- @state: plane state object to release
- Default plane state destroy hook for drivers which don't have
their own
- subclassed plane state structure.
- */
+void drm_atomic_helper_plane_destroy_state(struct drm_plane *plane,
struct drm_plane_state *state)+{
- kfree(state);
+} +EXPORT_SYMBOL(drm_atomic_helper_plane_destroy_state);
+/**
- drm_atomic_helper_connector_reset - default ->reset hook for
connectors
- @connector: drm connector
- Resets the atomic state for @connector by freeing the state
pointer and
- allocating a new empty state object.
- */
+void drm_atomic_helper_connector_reset(struct drm_connector *connector) +{
- kfree(connector->state);
- connector->state = kzalloc(sizeof(*connector->state), GFP_KERNEL);
+1
+} +EXPORT_SYMBOL(drm_atomic_helper_connector_reset);
+/**
- drm_atomic_helper_connector_duplicate_state - default state
duplicate hook
- @connector: drm connector
- Default connector state duplicate hook for drivers which don't
have their own
- subclassed connector state structure.
- */
+struct drm_connector_state * +drm_atomic_helper_connector_duplicate_state(struct drm_connector
*connector)
+{
- if (WARN_ON(!connector->state))
return NULL;- return kmemdup(connector->state, sizeof(*connector->state), GFP_KERNEL);
+} +EXPORT_SYMBOL(drm_atomic_helper_connector_duplicate_state);
+/**
- drm_atomic_helper_connector_destroy_state - default state destroy hook
- @connector: drm connector
- @state: connector state object to release
- Default connector state destroy hook for drivers which don't have
their own
- subclassed connector state structure.
- */
+void drm_atomic_helper_connector_destroy_state(struct drm_connector
*connector,
struct drm_connector_state *state)+{
- kfree(state);
+} +EXPORT_SYMBOL(drm_atomic_helper_connector_destroy_state); diff --git a/include/drm/drm_atomic_helper.h
b/include/drm/drm_atomic_helper.h
index 28a2f3a815fd..67e3c4645ae0 100644 --- a/include/drm/drm_atomic_helper.h +++ b/include/drm/drm_atomic_helper.h @@ -74,5 +74,24 @@ int drm_atomic_helper_page_flip(struct drm_crtc *crtc, struct drm_pending_vblank_event *event, uint32_t flags);
+/* default implementations for state handling */ +void drm_atomic_helper_crtc_reset(struct drm_crtc *crtc); +struct drm_crtc_state * +drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc); +void drm_atomic_helper_crtc_destroy_state(struct drm_crtc *crtc,
struct drm_crtc_state *state);+void drm_atomic_helper_plane_reset(struct drm_plane *plane); +struct drm_plane_state * +drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane); +void drm_atomic_helper_plane_destroy_state(struct drm_plane *plane,
struct drm_plane_state *state);+void drm_atomic_helper_connector_reset(struct drm_connector *connector); +struct drm_connector_state * +drm_atomic_helper_connector_duplicate_state(struct drm_connector
*connector);
+void drm_atomic_helper_connector_destroy_state(struct drm_connector
*connector,
struct drm_connector_state *state);#endif /* DRM_ATOMIC_HELPER_H_ */