Onnkhorst> that would be the right thing to do
<danvet> chickens didn't want to audit 20+ drivers :-)
<mlankhorst> neither, could we just break them? :pTue, Aug 29, 2017 at
04:02:03PM +0200, Maarten Lankhorst wrote:
> Currently we neatly track the crtc state, but forget to look at
> plane/connector state.
>
> When doing a nonblocking modeset, immediately followed by a setprop
> before the modeset completes, the setprop will see the modesets new
> state as the old state and free it.
>
> This has to be solved by waiting for hw_done on the connector, even
> if it's not assigned to a crtc. When a connector is unbound we take
> the last crtc commit, and when it stays unbound we create a new
> crtc commit for the connector that gets signaled on hw_done.
>
> We wait for it the same way as we do for crtc's, which will make
> sure we never run into a use-after-free situation.
>
> Signed-off-by: Maarten Lankhorst <[email protected]>
> Testcase: kms_atomic_transition.plane-use-after-nonblocking-unbind*
> Cc: Laurent Pinchart <[email protected]>
> ---
> drivers/gpu/drm/drm_atomic_helper.c | 171
> ++++++++++++++++++++++++++++++++++--
> include/drm/drm_connector.h | 7 ++
> include/drm/drm_plane.h | 7 ++
> 3 files changed, 179 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c
> index 9c2888cb4081..a4fd500d6200 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1644,6 +1644,39 @@ static void release_crtc_commit(struct completion
> *completion)
> drm_crtc_commit_put(commit);
> }
>
> +static void init_commit(struct drm_crtc_commit *commit, struct drm_crtc
> *crtc)
> +{
> + init_completion(&commit->flip_done);
> + init_completion(&commit->hw_done);
> + init_completion(&commit->cleanup_done);
> + INIT_LIST_HEAD(&commit->commit_entry);
> + kref_init(&commit->ref);
> + commit->crtc = crtc;
> +}
> +
> +static struct drm_crtc_commit *
> +init_or_ref_crtc_commit(struct drm_atomic_state *state, struct drm_crtc
> *crtc)
> +{
> + struct drm_crtc_commit *commit;
> +
> + if (crtc) {
> + struct drm_crtc_state *new_crtc_state;
> +
> + new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> +
> + commit = new_crtc_state->commit;
> + drm_crtc_commit_get(commit);
> + } else {
> + commit = kzalloc(sizeof(*commit), GFP_KERNEL);
> + if (!commit)
> + return NULL;
> +
> + init_commit(commit, NULL);
> + }
> +
> + return commit;
> +}
> +
> /**
> * drm_atomic_helper_setup_commit - setup possibly nonblocking commit
> * @state: new modeset state to be committed
> @@ -1692,6 +1725,10 @@ int drm_atomic_helper_setup_commit(struct
> drm_atomic_state *state,
> {
> struct drm_crtc *crtc;
> struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> + struct drm_connector *conn;
> + struct drm_connector_state *old_conn_state, *new_conn_state;
> + struct drm_plane *plane;
> + struct drm_plane_state *old_plane_state, *new_plane_state;
> struct drm_crtc_commit *commit;
> int i, ret;
>
> @@ -1700,12 +1737,7 @@ int drm_atomic_helper_setup_commit(struct
> drm_atomic_state *state,
> if (!commit)
> return -ENOMEM;
>
> - init_completion(&commit->flip_done);
> - init_completion(&commit->hw_done);
> - init_completion(&commit->cleanup_done);
> - INIT_LIST_HEAD(&commit->commit_entry);
> - kref_init(&commit->ref);
> - commit->crtc = crtc;
> + init_commit(commit, crtc);
>
> new_crtc_state->commit = commit;
>
> @@ -1741,6 +1773,36 @@ int drm_atomic_helper_setup_commit(struct
> drm_atomic_state *state,
> drm_crtc_commit_get(commit);
> }
>
> + for_each_oldnew_connector_in_state(state, conn, old_conn_state,
> new_conn_state, i) {
> + if (new_conn_state->crtc)
> + continue;
> +
> + if (nonblock && old_conn_state->commit &&
> +
> !try_wait_for_completion(&old_conn_state->commit->flip_done))
> + return -EBUSY;
> +
> + commit = init_or_ref_crtc_commit(state, old_conn_state->crtc);
> + if (!commit)
> + return -ENOMEM;
> +
> + new_conn_state->commit = commit;
> + }
> +
> + for_each_oldnew_plane_in_state(state, plane, old_plane_state,
> new_plane_state, i) {
> + if (new_plane_state->crtc)
> + continue;
> +
> + if (nonblock && old_plane_state->commit &&
> +
> !try_wait_for_completion(&old_plane_state->commit->flip_done))
> + return -EBUSY;
> +
> + commit = init_or_ref_crtc_commit(state, old_plane_state->crtc);
> + if (!commit)
> + return -ENOMEM;
> +
> + new_plane_state->commit = commit;
> + }
> +
> return 0;
> }
Ok, I think this works, but it's a bit confusing to have a chain of
drm_crtc_commits only for when the plane/connector isn't connected to
anything. I agree that with your design something else is clearer.
I had something slightly different in mind, with slightly different
semantics:
- we add a ->disabling_commit to the crtc state
- in setup_commit we do the following
- if state->crtc != NULL, we do nothing, since the commit is tracked
through the crtc
- if old_state->crtc && !new_state->crtc, then we are disabling the
connector/plane in this commit. In that case we'll reuse the same
drm_crtc_commit for the current crtc commmit, i.e.
new_conn_state->disabling_commit =
new_crtc_state(old_conn_state->crtc)->commit;
drm_crtc_commit_get(new_conn_state->disabling_commit);
- if the connector is disabled and stays disabled, we check whether the
commit has completed already. If not, we copy it forward:
new_conn_state->disabling_commit = old_conn_state->disabling_commit;
drm_crtc_commit_get(new_conn_state->disabling_commit);
The last point is where your and my idea differ, by simply copying the
same commit forward we don't end up with a chain of somewhat confusing
fake commit objects. We also don't have to call complete_all on any
connector/plane commits (since they're always real drm_crtc_commit
trackers for a real crtc).
I think calling it ->disabling_commit also makes it more clear what
exactly is the special case and when we need it. Maybe even call it
last_disabling_commit for more clarity.
> EXPORT_SYMBOL(drm_atomic_helper_setup_commit);
> @@ -1761,6 +1823,10 @@ void drm_atomic_helper_wait_for_dependencies(struct
> drm_atomic_state *old_state)
> {
> struct drm_crtc *crtc;
> struct drm_crtc_state *old_crtc_state;
> + struct drm_plane *plane;
> + struct drm_plane_state *old_plane_state;
> + struct drm_connector *conn;
> + struct drm_connector_state *old_conn_state;
> struct drm_crtc_commit *commit;
> int i;
> long ret;
> @@ -1785,6 +1851,48 @@ void drm_atomic_helper_wait_for_dependencies(struct
> drm_atomic_state *old_state)
> DRM_ERROR("[CRTC:%d:%s] flip_done timed out\n",
> crtc->base.id, crtc->name);
> }
> +
> + for_each_old_connector_in_state(old_state, conn, old_conn_state, i) {
> + commit = old_conn_state->commit;
> +
> + if (!commit)
> + continue;
> +
> + ret = wait_for_completion_timeout(&commit->hw_done,
> + 10*HZ);
> + if (ret == 0)
> + DRM_ERROR("[CONNECTOR:%d:%s] hw_done timed out\n",
> + conn->base.id, conn->name);
> +
> + /* Currently no support for overwriting flips, hence
> + * stall for previous one to execute completely. */
> + ret = wait_for_completion_timeout(&commit->flip_done,
> + 10*HZ);
> + if (ret == 0)
> + DRM_ERROR("[CONNECTOR:%d:%s] flip_done timed out\n",
> + conn->base.id, conn->name);
> + }
> +
> + for_each_old_plane_in_state(old_state, plane, old_plane_state, i) {
> + commit = old_plane_state->commit;
> +
> + if (!commit)
> + continue;
> +
> + ret = wait_for_completion_timeout(&commit->hw_done,
> + 10*HZ);
> + if (ret == 0)
> + DRM_ERROR("[PLANE:%d:%s] hw_done timed out\n",
> + plane->base.id, plane->name);
> +
> + /* Currently no support for overwriting flips, hence
> + * stall for previous one to execute completely. */
> + ret = wait_for_completion_timeout(&commit->flip_done,
> + 10*HZ);
> + if (ret == 0)
> + DRM_ERROR("[PLANE:%d:%s] flip_done timed out\n",
> + plane->base.id, plane->name);
> + }
Why do you wait for flip_done and not hw_done? The general rule is that
you can signal flip_done when the hw stops scanning out stuff, which when
disabling, can be way before we've shut down the hw properly.
For CRTC we wait for both, I think the same must be done for
planes/connectors (yes more code).
Also, you're missing the corresponding code in swap_state. i915 might be
able to get away without that, but these helpers here aren't just for
i915.
> }
> EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
>
> @@ -1807,6 +1915,10 @@ void drm_atomic_helper_commit_hw_done(struct
> drm_atomic_state *old_state)
> {
> struct drm_crtc *crtc;
> struct drm_crtc_state *new_crtc_state;
> + struct drm_connector *conn;
> + struct drm_connector_state *new_conn_state;
> + struct drm_plane *plane;
> + struct drm_plane_state *new_plane_state;
> struct drm_crtc_commit *commit;
> int i;
>
> @@ -1819,6 +1931,23 @@ void drm_atomic_helper_commit_hw_done(struct
> drm_atomic_state *old_state)
> WARN_ON(new_crtc_state->event);
> complete_all(&commit->hw_done);
> }
> +
> + for_each_new_connector_in_state(old_state, conn, new_conn_state, i) {
> + commit = new_conn_state->commit;
> + if (commit && !commit->crtc) {
> + complete_all(&commit->hw_done);
> + complete_all(&commit->flip_done);
> + }
> + }
> +
> + for_each_new_plane_in_state(old_state, plane, new_plane_state, i) {
> + commit = new_plane_state->commit;
> + if (commit && !commit->crtc) {
> + complete_all(&commit->hw_done);
> + complete_all(&commit->flip_done);
> + }
> + }
> +
> }
> EXPORT_SYMBOL(drm_atomic_helper_commit_hw_done);
>
> @@ -2258,6 +2387,28 @@ int drm_atomic_helper_swap_state(struct
> drm_atomic_state *state,
> if (ret)
> return ret;
> }
> +
> + for_each_old_connector_in_state(state, connector,
> old_conn_state, i) {
> + commit = old_conn_state->commit;
> +
> + if (!commit)
> + continue;
> +
> + ret =
> wait_for_completion_interruptible(&commit->hw_done);
> + if (ret)
> + return ret;
> + }
> +
> + for_each_old_plane_in_state(state, plane, old_plane_state, i) {
> + commit = old_plane_state->commit;
> +
> + if (!commit)
> + continue;
> +
> + ret =
> wait_for_completion_interruptible(&commit->hw_done);
> + if (ret)
> + return ret;
> + }
> }
>
> for_each_oldnew_connector_in_state(state, connector, old_conn_state,
> new_conn_state, i) {
> @@ -3240,6 +3391,7 @@ void __drm_atomic_helper_plane_duplicate_state(struct
> drm_plane *plane,
> drm_framebuffer_get(state->fb);
>
> state->fence = NULL;
> + state->commit = NULL;
> }
> EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state);
>
> @@ -3281,6 +3433,9 @@ void __drm_atomic_helper_plane_destroy_state(struct
> drm_plane_state *state)
>
> if (state->fence)
> dma_fence_put(state->fence);
> +
> + if (state->commit)
> + drm_crtc_commit_put(state->commit);
> }
> EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
>
> @@ -3359,6 +3514,7 @@ __drm_atomic_helper_connector_duplicate_state(struct
> drm_connector *connector,
> memcpy(state, connector->state, sizeof(*state));
> if (state->crtc)
> drm_connector_get(connector);
> + state->commit = NULL;
> }
> EXPORT_SYMBOL(__drm_atomic_helper_connector_duplicate_state);
>
> @@ -3485,6 +3641,9 @@ __drm_atomic_helper_connector_destroy_state(struct
> drm_connector_state *state)
> {
> if (state->crtc)
> drm_connector_put(state->connector);
> +
> + if (state->commit)
> + drm_crtc_commit_put(state->commit);
> }
> EXPORT_SYMBOL(__drm_atomic_helper_connector_destroy_state);
>
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index ea8da401c93c..8837649d16e8 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -347,6 +347,13 @@ struct drm_connector_state {
>
> struct drm_atomic_state *state;
>
> + /**
> + * @commit: Tracks the pending commit to prevent use-after-free
> conditions.
For multiline member comments please do an empty line between the heading
and the real text. It's definitely the style guide, but it also might
confuse kerneldoc.
Cheers, Daniel
> + *
> + * Is only set when @crtc is NULL.
> + */
> + struct drm_crtc_commit *commit;
> +
> struct drm_tv_connector_state tv;
>
> /**
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 73f90f9d057f..7d96116fd4c4 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -123,6 +123,13 @@ struct drm_plane_state {
> */
> bool visible;
>
> + /**
> + * @commit: Tracks the pending commit to prevent use-after-free
> conditions.
> + *
> + * Is only set when @crtc is NULL.
> + */
> + struct drm_crtc_commit *commit;
> +
> struct drm_atomic_state *state;
> };
>
> --
> 2.11.0
>
> _______________________________________________
> Intel-gfx mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
