| Message ID | 1439293065-15941-1-git-send-email-thierry.reding@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Aug 11, 2015 at 01:37:43PM +0200, Thierry Reding wrote: > From: Thierry Reding <treding@nvidia.com> > > This function can be used to duplicate an atomic state object. This is > useful for example to implement suspend/resume, where the state before > suspend can be saved and restored upon resume. > > Signed-off-by: Thierry Reding <treding@nvidia.com> > --- > drivers/gpu/drm/drm_atomic_helper.c | 90 +++++++++++++++++++++++++++++++++++++ > include/drm/drm_atomic_helper.h | 2 + > 2 files changed, 92 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index fd06595c2ee5..f842499ca79e 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -2358,6 +2358,96 @@ drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector) > EXPORT_SYMBOL(drm_atomic_helper_connector_duplicate_state); > > /** > + * drm_atomic_helper_duplicate_state - duplicate an atomic state object > + * @dev: DRM device > + * > + * Makes a copy of the current atomic state by looping over all objects and > + * duplicating their respective states. This needs a big warning that drivers better know what they're doing for two reasons: It promises that atomic states can be treated as persistent bits (e.g. if a driver has an update-counter somewhere that won't work). And it drops locks again so will race if that's not otherwise excluded (like in suspend/resume where nothing else is allowed to run if you shut down fbcon with fbdev_set_suspend() before calling this function). Maybe needs some rewording, but all of the above (including details like fbdev_set_suspend) must be mentioned. > + * > + * Returns: A pointer to the copy of the atomic state object on success or an > + * ERR_PTR()-encoded error code on failure. > + */ > +struct drm_atomic_state * > +drm_atomic_helper_duplicate_state(struct drm_device *dev) > +{ > + struct drm_modeset_acquire_ctx ctx; > + struct drm_atomic_state *state; > + struct drm_connector *conn; > + struct drm_plane *plane; > + struct drm_crtc *crtc; > + int err; > + > + state = drm_atomic_state_alloc(dev); > + if (!state) > + return ERR_PTR(-ENOMEM); > + > + drm_modeset_acquire_init(&ctx, 0); > + state->acquire_ctx = &ctx; > + > + err = drm_modeset_lock(&dev->mode_config.connection_mutex, &ctx); > + if (err < 0) > + goto free; Just use drm_modeset_lock_all. You need them all anyway and for suspend/resume there won't be any contention anyway. Otherwise lgtm. -Daniel > + > + drm_for_each_crtc(crtc, dev) { > + unsigned int index = drm_crtc_index(crtc); > + struct drm_crtc_state *crtc_state; > + > + crtc_state = crtc->funcs->atomic_duplicate_state(crtc); > + if (!crtc_state) { > + err = -ENOMEM; > + goto unlock; > + } > + > + state->crtc_states[index] = crtc_state; > + state->crtcs[index] = crtc; > + crtc_state->state = state; > + } > + > + drm_for_each_plane(plane, dev) { > + unsigned int index = drm_plane_index(plane); > + struct drm_plane_state *plane_state; > + > + plane_state = plane->funcs->atomic_duplicate_state(plane); > + if (!plane_state) { > + err = -ENOMEM; > + goto unlock; > + } > + > + state->plane_states[index] = plane_state; > + state->planes[index] = plane; > + plane_state->state = state; > + } > + > + drm_for_each_connector(conn, dev) { > + unsigned int index = drm_connector_index(conn); > + struct drm_connector_state *conn_state; > + > + conn_state = conn->funcs->atomic_duplicate_state(conn); > + if (!conn_state) { > + err = -ENOMEM; > + goto unlock; > + } > + > + state->connector_states[index] = conn_state; > + state->connectors[index] = conn; > + conn_state->state = state; > + } > + > +unlock: > + drm_modeset_drop_locks(&ctx); > +free: > + drm_modeset_acquire_fini(&ctx); > + > + if (err < 0) { > + drm_atomic_state_free(state); > + state = ERR_PTR(err); > + } > + > + return state; > +} > +EXPORT_SYMBOL(drm_atomic_helper_duplicate_state); > + > +/** > * __drm_atomic_helper_connector_destroy_state - release connector state > * @connector: connector object > * @state: connector state object to release > diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h > index 4ffe9dca07c4..5d27612997ce 100644 > --- a/include/drm/drm_atomic_helper.h > +++ b/include/drm/drm_atomic_helper.h > @@ -118,6 +118,8 @@ __drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector, > struct drm_connector_state *state); > struct drm_connector_state * > drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector); > +struct drm_atomic_state * > +drm_atomic_helper_duplicate_state(struct drm_device *dev); > void > __drm_atomic_helper_connector_destroy_state(struct drm_connector *connector, > struct drm_connector_state *state); > -- > 2.4.5 >
Hey, This patch series would be less complicated if you assume the caller locked with drm_modeset_lock_all and use the global acquire_ctx in each function. You're locking all state here after all.. Also you should use drm_atomic_get_*_state instead of calling those duplicate_state functions directly. Op 11-08-15 om 13:37 schreef Thierry Reding: > From: Thierry Reding <treding@nvidia.com> > > This function can be used to duplicate an atomic state object. This is > useful for example to implement suspend/resume, where the state before > suspend can be saved and restored upon resume. > > Signed-off-by: Thierry Reding <treding@nvidia.com> > --- > drivers/gpu/drm/drm_atomic_helper.c | 90 +++++++++++++++++++++++++++++++++++++ > include/drm/drm_atomic_helper.h | 2 + > 2 files changed, 92 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index fd06595c2ee5..f842499ca79e 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -2358,6 +2358,96 @@ drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector) > EXPORT_SYMBOL(drm_atomic_helper_connector_duplicate_state); > > /** > + * drm_atomic_helper_duplicate_state - duplicate an atomic state object > + * @dev: DRM device > + * > + * Makes a copy of the current atomic state by looping over all objects and > + * duplicating their respective states. > + * > + * Returns: A pointer to the copy of the atomic state object on success or an > + * ERR_PTR()-encoded error code on failure. > + */ > +struct drm_atomic_state * > +drm_atomic_helper_duplicate_state(struct drm_device *dev) > +{ > + struct drm_modeset_acquire_ctx ctx; > + struct drm_atomic_state *state; > + struct drm_connector *conn; > + struct drm_plane *plane; > + struct drm_crtc *crtc; > + int err; > + > + state = drm_atomic_state_alloc(dev); > + if (!state) > + return ERR_PTR(-ENOMEM); > + > + drm_modeset_acquire_init(&ctx, 0); > + state->acquire_ctx = &ctx; > + > + err = drm_modeset_lock(&dev->mode_config.connection_mutex, &ctx); > + if (err < 0) > + goto free; > + > + drm_for_each_crtc(crtc, dev) { > + unsigned int index = drm_crtc_index(crtc); > + struct drm_crtc_state *crtc_state; > + > + crtc_state = crtc->funcs->atomic_duplicate_state(crtc); > + if (!crtc_state) { > + err = -ENOMEM; > + goto unlock; > + } > + > + state->crtc_states[index] = crtc_state; > + state->crtcs[index] = crtc; > + crtc_state->state = state; > + } > + > + drm_for_each_plane(plane, dev) { > + unsigned int index = drm_plane_index(plane); > + struct drm_plane_state *plane_state; > + > + plane_state = plane->funcs->atomic_duplicate_state(plane); > + if (!plane_state) { > + err = -ENOMEM; > + goto unlock; > + } > + > + state->plane_states[index] = plane_state; > + state->planes[index] = plane; > + plane_state->state = state; > + } > + > + drm_for_each_connector(conn, dev) { > + unsigned int index = drm_connector_index(conn); > + struct drm_connector_state *conn_state; > + > + conn_state = conn->funcs->atomic_duplicate_state(conn); > + if (!conn_state) { > + err = -ENOMEM; > + goto unlock; > + } > + > + state->connector_states[index] = conn_state; > + state->connectors[index] = conn; > + conn_state->state = state; > + } > + > +unlock: > + drm_modeset_drop_locks(&ctx); > +free: > + drm_modeset_acquire_fini(&ctx); > + > + if (err < 0) { > + drm_atomic_state_free(state); > + state = ERR_PTR(err); > + } > + > + return state; > +} > +EXPORT_SYMBOL(drm_atomic_helper_duplicate_state); > + > +/** > * __drm_atomic_helper_connector_destroy_state - release connector state > * @connector: connector object > * @state: connector state object to release > diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h > index 4ffe9dca07c4..5d27612997ce 100644 > --- a/include/drm/drm_atomic_helper.h > +++ b/include/drm/drm_atomic_helper.h > @@ -118,6 +118,8 @@ __drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector, > struct drm_connector_state *state); > struct drm_connector_state * > drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector); > +struct drm_atomic_state * > +drm_atomic_helper_duplicate_state(struct drm_device *dev); > void > __drm_atomic_helper_connector_destroy_state(struct drm_connector *connector, > struct drm_connector_state *state);
On Tue, Aug 11, 2015 at 01:37:43PM +0200, Thierry Reding wrote: > From: Thierry Reding <treding@nvidia.com> > > This function can be used to duplicate an atomic state object. This is > useful for example to implement suspend/resume, where the state before > suspend can be saved and restored upon resume. > > Signed-off-by: Thierry Reding <treding@nvidia.com> > --- > drivers/gpu/drm/drm_atomic_helper.c | 90 +++++++++++++++++++++++++++++++++++++ > include/drm/drm_atomic_helper.h | 2 + > 2 files changed, 92 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c > index fd06595c2ee5..f842499ca79e 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -2358,6 +2358,96 @@ drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector) > EXPORT_SYMBOL(drm_atomic_helper_connector_duplicate_state); > > /** > + * drm_atomic_helper_duplicate_state - duplicate an atomic state object > + * @dev: DRM device > + * > + * Makes a copy of the current atomic state by looping over all objects and > + * duplicating their respective states. > + * > + * Returns: A pointer to the copy of the atomic state object on success or an > + * ERR_PTR()-encoded error code on failure. > + */ > +struct drm_atomic_state * > +drm_atomic_helper_duplicate_state(struct drm_device *dev) > +{ > + struct drm_modeset_acquire_ctx ctx; > + struct drm_atomic_state *state; > + struct drm_connector *conn; > + struct drm_plane *plane; > + struct drm_crtc *crtc; > + int err; > + > + state = drm_atomic_state_alloc(dev); > + if (!state) > + return ERR_PTR(-ENOMEM); > + > + drm_modeset_acquire_init(&ctx, 0); > + state->acquire_ctx = &ctx; > + > + err = drm_modeset_lock(&dev->mode_config.connection_mutex, &ctx); > + if (err < 0) > + goto free; > + > + drm_for_each_crtc(crtc, dev) { > + unsigned int index = drm_crtc_index(crtc); > + struct drm_crtc_state *crtc_state; > + > + crtc_state = crtc->funcs->atomic_duplicate_state(crtc); > + if (!crtc_state) { > + err = -ENOMEM; > + goto unlock; > + } > + > + state->crtc_states[index] = crtc_state; > + state->crtcs[index] = crtc; > + crtc_state->state = state; > + } > + > + drm_for_each_plane(plane, dev) { > + unsigned int index = drm_plane_index(plane); > + struct drm_plane_state *plane_state; > + > + plane_state = plane->funcs->atomic_duplicate_state(plane); > + if (!plane_state) { > + err = -ENOMEM; > + goto unlock; > + } > + > + state->plane_states[index] = plane_state; > + state->planes[index] = plane; > + plane_state->state = state; > + } > + > + drm_for_each_connector(conn, dev) { > + unsigned int index = drm_connector_index(conn); > + struct drm_connector_state *conn_state; > + > + conn_state = conn->funcs->atomic_duplicate_state(conn); > + if (!conn_state) { > + err = -ENOMEM; > + goto unlock; > + } > + > + state->connector_states[index] = conn_state; > + state->connectors[index] = conn; > + conn_state->state = state; > + } > + > +unlock: > + drm_modeset_drop_locks(&ctx); > +free: > + drm_modeset_acquire_fini(&ctx); One more: I think we should clear state->acquire_ctx here, just to make sure it's not accidentally reused. -Daniel > + > + if (err < 0) { > + drm_atomic_state_free(state); > + state = ERR_PTR(err); > + } > + > + return state; > +} > +EXPORT_SYMBOL(drm_atomic_helper_duplicate_state); > + > +/** > * __drm_atomic_helper_connector_destroy_state - release connector state > * @connector: connector object > * @state: connector state object to release > diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h > index 4ffe9dca07c4..5d27612997ce 100644 > --- a/include/drm/drm_atomic_helper.h > +++ b/include/drm/drm_atomic_helper.h > @@ -118,6 +118,8 @@ __drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector, > struct drm_connector_state *state); > struct drm_connector_state * > drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector); > +struct drm_atomic_state * > +drm_atomic_helper_duplicate_state(struct drm_device *dev); > void > __drm_atomic_helper_connector_destroy_state(struct drm_connector *connector, > struct drm_connector_state *state); > -- > 2.4.5 >
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index fd06595c2ee5..f842499ca79e 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -2358,6 +2358,96 @@ drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector) EXPORT_SYMBOL(drm_atomic_helper_connector_duplicate_state); /** + * drm_atomic_helper_duplicate_state - duplicate an atomic state object + * @dev: DRM device + * + * Makes a copy of the current atomic state by looping over all objects and + * duplicating their respective states. + * + * Returns: A pointer to the copy of the atomic state object on success or an + * ERR_PTR()-encoded error code on failure. + */ +struct drm_atomic_state * +drm_atomic_helper_duplicate_state(struct drm_device *dev) +{ + struct drm_modeset_acquire_ctx ctx; + struct drm_atomic_state *state; + struct drm_connector *conn; + struct drm_plane *plane; + struct drm_crtc *crtc; + int err; + + state = drm_atomic_state_alloc(dev); + if (!state) + return ERR_PTR(-ENOMEM); + + drm_modeset_acquire_init(&ctx, 0); + state->acquire_ctx = &ctx; + + err = drm_modeset_lock(&dev->mode_config.connection_mutex, &ctx); + if (err < 0) + goto free; + + drm_for_each_crtc(crtc, dev) { + unsigned int index = drm_crtc_index(crtc); + struct drm_crtc_state *crtc_state; + + crtc_state = crtc->funcs->atomic_duplicate_state(crtc); + if (!crtc_state) { + err = -ENOMEM; + goto unlock; + } + + state->crtc_states[index] = crtc_state; + state->crtcs[index] = crtc; + crtc_state->state = state; + } + + drm_for_each_plane(plane, dev) { + unsigned int index = drm_plane_index(plane); + struct drm_plane_state *plane_state; + + plane_state = plane->funcs->atomic_duplicate_state(plane); + if (!plane_state) { + err = -ENOMEM; + goto unlock; + } + + state->plane_states[index] = plane_state; + state->planes[index] = plane; + plane_state->state = state; + } + + drm_for_each_connector(conn, dev) { + unsigned int index = drm_connector_index(conn); + struct drm_connector_state *conn_state; + + conn_state = conn->funcs->atomic_duplicate_state(conn); + if (!conn_state) { + err = -ENOMEM; + goto unlock; + } + + state->connector_states[index] = conn_state; + state->connectors[index] = conn; + conn_state->state = state; + } + +unlock: + drm_modeset_drop_locks(&ctx); +free: + drm_modeset_acquire_fini(&ctx); + + if (err < 0) { + drm_atomic_state_free(state); + state = ERR_PTR(err); + } + + return state; +} +EXPORT_SYMBOL(drm_atomic_helper_duplicate_state); + +/** * __drm_atomic_helper_connector_destroy_state - release connector state * @connector: connector object * @state: connector state object to release diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h index 4ffe9dca07c4..5d27612997ce 100644 --- a/include/drm/drm_atomic_helper.h +++ b/include/drm/drm_atomic_helper.h @@ -118,6 +118,8 @@ __drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector, struct drm_connector_state *state); struct drm_connector_state * drm_atomic_helper_connector_duplicate_state(struct drm_connector *connector); +struct drm_atomic_state * +drm_atomic_helper_duplicate_state(struct drm_device *dev); void __drm_atomic_helper_connector_destroy_state(struct drm_connector *connector, struct drm_connector_state *state);