Re: [PATCH] drm/atomic: protect crtc|connector->state with rcu

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

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@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  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@chris-wilson.co.uk>
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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx