Op 16-03-17 om 16:52 schreef Daniel Vetter:
> The vblank code really wants to look at crtc->state without having to
> take a ww_mutex. One option might be to take one of the vblank locks
> right when assigning crtc->state, which would ensure that the vblank
> code doesn't race and access freed memory.
I'm not sure this is the right approach for vblank.
crtc->state might not be the same as the current state in case of a nonblocking
modeset that hasn't even disabled the old crtc yet.
> But userspace tends to poke the vblank_ioctl to query the current
> vblank timestamp rather often, and going all in with rcu would help a
> bit. We're not there yet, but also doesn't really hurt.
>
> v2: Maarten needs this to make connector properties atomic, so he can
> peek at state from the various ->mode_valid hooks.
>
> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> ---
> drivers/gpu/drm/drm_atomic.c | 26 +++++++++++++++++---------
> drivers/gpu/drm/drm_atomic_helper.c | 2 +-
> include/drm/drm_atomic.h | 5 +++++
> include/drm/drm_connector.h | 13 ++++++++++++-
> include/drm/drm_crtc.h | 9 ++++++++-
> 5 files changed, 43 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 9b892af7811a..ba11e31ff9ba 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -213,16 +213,10 @@ void drm_atomic_state_clear(struct drm_atomic_state *state)
> }
> EXPORT_SYMBOL(drm_atomic_state_clear);
>
> -/**
> - * __drm_atomic_state_free - free all memory for an atomic state
> - * @ref: This atomic state to deallocate
> - *
> - * This frees all memory associated with an atomic state, including all the
> - * per-object state for planes, crtcs and connectors.
> - */
> -void __drm_atomic_state_free(struct kref *ref)
> +void ___drm_atomic_state_free(struct rcu_head *rhead)
> {
> - struct drm_atomic_state *state = container_of(ref, typeof(*state), ref);
> + struct drm_atomic_state *state =
> + container_of(rhead, typeof(*state), rhead);
> struct drm_mode_config *config = &state->dev->mode_config;
>
> drm_atomic_state_clear(state);
> @@ -236,6 +230,20 @@ void __drm_atomic_state_free(struct kref *ref)
> kfree(state);
> }
> }
whatisRCU.txt:
"This function invokes func(head) after a grace period has elapsed.
This invocation might happen from either softirq or process context,
so the function is not permitted to block."
Looking at
commit 6f0f02dc56f18760b46dc1bf5b3f7386869d4162
Author: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Date: Mon Jan 23 21:29:39 2017 +0000
drm/i915: Move atomic state free from out of fence release
I would say that drm_atomic_state_free would definitely block..
The only thing that makes sense IMO is doing kfree_rcu on the object_states.
But I don't think RCU is the answer here, it won't protect you against using
the wrong crtc state.
I think I would try to use the crtc ww_mutex in the vblank code and serialize it to pending commits, if at all possible.
> +
> +/**
> + * __drm_atomic_state_free - free all memory for an atomic state
> + * @ref: This atomic state to deallocate
> + *
> + * This frees all memory associated with an atomic state, including all the
> + * per-object state for planes, crtcs and connectors.
> + */
> +void __drm_atomic_state_free(struct kref *ref)
> +{
> + struct drm_atomic_state *state = container_of(ref, typeof(*state), ref);
> +
> + call_rcu(&state->rhead, ___drm_atomic_state_free);
> +}
> EXPORT_SYMBOL(__drm_atomic_state_free);
>
> /**
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 1c015344d063..b6bb8bad36a8 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -2041,7 +2041,7 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
> new_crtc_state->state = NULL;
>
> state->crtcs[i].state = old_crtc_state;
> - crtc->state = new_crtc_state;
> + rcu_assign_pointer(crtc->state, new_crtc_state);
>
> if (state->crtcs[i].commit) {
> spin_lock(&crtc->commit_lock);
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 0147a047878d..65433eec270b 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -28,6 +28,8 @@
> #ifndef DRM_ATOMIC_H_
> #define DRM_ATOMIC_H_
>
> +#include <linux/rcupdate.h>
> +
> #include <drm/drm_crtc.h>
>
> /**
> @@ -188,6 +190,9 @@ struct drm_atomic_state {
> * commit without blocking.
> */
> struct work_struct commit_work;
> +
> + /* private: */
> + struct rcu_head rhead;
> };
>
> void __drm_crtc_commit_free(struct kref *kref);
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index fabb35aba5f6..8e476986a43e 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -603,7 +603,6 @@ struct drm_cmdline_mode {
> * @bad_edid_counter: track sinks that give us an EDID with invalid checksum
> * @edid_corrupt: indicates whether the last read EDID was corrupt
> * @debugfs_entry: debugfs directory for this connector
> - * @state: current atomic state for this connector
> * @has_tile: is this connector connected to a tiled monitor
> * @tile_group: tile group for the connected monitor
> * @tile_is_single_monitor: whether the tile is one monitor housing
> @@ -771,6 +770,18 @@ struct drm_connector {
>
> struct dentry *debugfs_entry;
>
> + /**
> + * @state:
> + *
> + * Current atomic state for this connector. Note that this is protected
> + * by @mutex, but also by RCU (for the mode validation code, which needs
> + * to peek at this with only hold &drm_mode_config.mutex).
> + *
> + * FIXME:
> + *
> + * This isn't annoted with __rcu because fixing up all the drivers is a
> + * massive amount of work.
> + */
> struct drm_connector_state *state;
>
> /* DisplayID bits */
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 24dcb121bad4..6470a0012e38 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -747,7 +747,14 @@ struct drm_crtc {
> /**
> * @state:
> *
> - * Current atomic state for this CRTC.
> + * Current atomic state for this CRTC. Note that this is protected by
> + * @mutex, but also by RCU (for the vblank code, which needs to peek at
> + * this from interrupt context).
> + *
> + * FIXME:
> + *
> + * This isn't annoted with __rcu because fixing up all the drivers is a
> + * massive amount of work.
> */
> struct drm_crtc_state *state;
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
