Re: [PATCH 15/17] drm/atomic-helpers: functions for state duplicate/destroy/reset

[Daniel Thompson <daniel.thompson@linaro.org>]

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_ */
>


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel