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 <maarten.lankhorst@xxxxxxxxxxxxxxx>
> Testcase: kms_atomic_transition.plane-use-after-nonblocking-unbind*
> Cc: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> ---
> 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
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
