| Message ID | 1414934370-11924-16-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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_ */ >
On Mon, Nov 03, 2014 at 02:45:28PM +0000, Daniel Thompson wrote: > > 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). Indeed, will shuffle around. > > +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)? kfree is a nop when the argument is NULL, which is a crucial property of this - memset would oops on driver load. Even neglecting this a memset imo doesn't blow up loudly enough if the driver subclasses the state structs (by adding more of it's driver private state at the end). Whereas underallocating tends to anger the slab poisoning code badly. Finally it's really not just a memset, but a free + realloc. See the plane state, which also needs to drop a potential fb reference. Imo the explicit kfree+realloc makes that more obvious. -Daniel
On 03/11/14 14:53, Daniel Vetter wrote: > On Mon, Nov 03, 2014 at 02:45:28PM +0000, Daniel Thompson wrote: >>> 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). > > Indeed, will shuffle around. > >>> +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)? > > kfree is a nop when the argument is NULL, which is a crucial property of > this - memset would oops on driver load. Oops. Missed that (I think I misread who as assuming there was always obj->state in the patch header). Do you fancy making the comment "by freeing the state pointer and allocating a new..." into "by freeing the state pointer (which may be NULL) and allocating a new...". If nothing else that means the documentation is richer than the code... > Even neglecting this a memset imo doesn't blow up loudly enough if the > driver subclasses the state structs (by adding more of it's driver private > state at the end). Whereas underallocating tends to anger the slab > poisoning code badly. > > Finally it's really not just a memset, but a free + realloc. See the plane > state, which also needs to drop a potential fb reference. Imo the explicit > kfree+realloc makes that more obvious.
On Mon, Nov 03, 2014 at 03:06:07PM +0000, Daniel Thompson wrote: > > kfree is a nop when the argument is NULL, which is a crucial property of > > this - memset would oops on driver load. > > Oops. Missed that (I think I misread who as assuming there was always > obj->state in the patch header). > > Do you fancy making the comment "by freeing the state pointer and > allocating a new..." into "by freeing the state pointer (which may be > NULL) and allocating a new...". > > If nothing else that means the documentation is richer than the code... Yeah, sounds like a good idea. I'll also mention that this is for driver load mostly (if the driver doesn't decide to throw away the state for some reason over suspend/resume). -Daniel
On Sun, Nov 02, 2014 at 02:19:28PM +0100, 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> Minus the fixup nits that danielt pointed out Reviewed-by: Sean Paul <seanpaul@chromium.org> > --- > 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 > * @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 > * @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 > * @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); > +} > +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); > +} > +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); > +} > +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_ */ > -- > 2.1.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Nov 06, 2014 at 12:43:43PM -0500, Sean Paul wrote: > On Sun, Nov 02, 2014 at 02:19:28PM +0100, 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> > > Minus the fixup nits that danielt pointed out Yeah already fixed them but forgotten to resend the patch. Done now. > > Reviewed-by: Sean Paul <seanpaul@chromium.org> Can you please double check that the new version is ok? Thanks, Daniel
On Thu, Nov 6, 2014 at 2:57 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Thu, Nov 06, 2014 at 12:43:43PM -0500, Sean Paul wrote: >> On Sun, Nov 02, 2014 at 02:19:28PM +0100, 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> >> >> Minus the fixup nits that danielt pointed out > > Yeah already fixed them but forgotten to resend the patch. Done now. >> >> Reviewed-by: Sean Paul <seanpaul@chromium.org> > > Can you please double check that the new version is ok? > Reviewed-by: Sean Paul <seanpaul@chromium.org> > Thanks, Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
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 * @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 * @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 * @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); +} +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); +} +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); +} +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_ */
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(-)