On Fri, Nov 21, 2014 at 03:28:31PM -0500, Rob Clark wrote:
> Chasing plane->state->crtc of planes that are *not* part of the same
> atomic update is racy, making it incredibly awkward (or impossible) to
> do something simple like iterate over all planes and figure out which
> ones are attached to a crtc.
>
> Solve this by adding a bitmask of currently attached planes in the
> crtc-state.
>
> Note that the transitional helpers do not maintain the plane_mask. But
> they only support the legacy ioctls, which have sufficient brute-force
> locking around plane updates that they can continue to loop over all
> planes to see what is attached to a crtc the old way.
>
> Signed-off-by: Rob Clark <robdclark at gmail.com>
> ---
> drivers/gpu/drm/drm_atomic.c | 25 ++++++++++++++++++++-----
> drivers/gpu/drm/drm_atomic_helper.c | 8 ++++----
> include/drm/drm_atomic.h | 4 ++--
> include/drm/drm_crtc.h | 14 +++++++++++---
> 4 files changed, 37 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index d3b4674..8effbba 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -350,7 +350,8 @@ EXPORT_SYMBOL(drm_atomic_get_connector_state);
>
> /**
> * drm_atomic_set_crtc_for_plane - set crtc for plane
> - * @plane_state: atomic state object for the plane
> + * @state: the incoming atomic state
> + * @plane: the plane whose incoming state to update
> * @crtc: crtc to use for the plane
> *
> * Changing the assigned crtc for a plane requires us to grab the lock and
> state
> @@ -363,20 +364,34 @@ EXPORT_SYMBOL(drm_atomic_get_connector_state);
> * sequence must be restarted. All other errors are fatal.
> */
> int
> -drm_atomic_set_crtc_for_plane(struct drm_plane_state *plane_state,
> - struct drm_crtc *crtc)
> +drm_atomic_set_crtc_for_plane(struct drm_atomic_state *state,
> + struct drm_plane *plane, struct drm_crtc *crtc)
> {
> + struct drm_plane_state *plane_state =
> + drm_atomic_get_plane_state(state, plane);
> struct drm_crtc_state *crtc_state;
>
> - if (crtc) {
> + /* acquire outgoing crtc lock, and clear bit in outgoing crtc mask: */
Ok still don't like these comments since at least for the old crtc it's
wrong: We already must both hold the lock and copied the state. So I've
dropped them (kerneldoc already mentions this too).
> + if (plane_state->crtc) {
> crtc_state = drm_atomic_get_crtc_state(plane_state->state,
> - crtc);
> + plane_state->crtc);
> if (IS_ERR(crtc_state))
And changed this to WARN_ON too to make sure this never fails. The pulled
it into my atomic-fixes branch.
-Daniel
> return PTR_ERR(crtc_state);
> +
> + crtc_state->plane_mask &= ~(1 << drm_plane_index(plane));
> }
>
> plane_state->crtc = crtc;
>
> + /* acquire incoming crtc lock, and set bit in incoming crtc mask: */
> + if (crtc) {
> + crtc_state = drm_atomic_get_crtc_state(plane_state->state,
> + crtc);
> + if (IS_ERR(crtc_state))
> + return PTR_ERR(crtc_state);
> + crtc_state->plane_mask |= (1 << drm_plane_index(plane));
> + }
> +
> if (crtc)
> DRM_DEBUG_KMS("Link plane state %p to [CRTC:%d]\n",
> plane_state, crtc->base.id);
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c
> index a17b8e9..d765d37 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1187,7 +1187,7 @@ retry:
> goto fail;
> }
>
> - ret = drm_atomic_set_crtc_for_plane(plane_state, crtc);
> + ret = drm_atomic_set_crtc_for_plane(state, plane, crtc);
> if (ret != 0)
> goto fail;
> drm_atomic_set_fb_for_plane(plane_state, fb);
> @@ -1255,7 +1255,7 @@ retry:
> goto fail;
> }
>
> - ret = drm_atomic_set_crtc_for_plane(plane_state, NULL);
> + ret = drm_atomic_set_crtc_for_plane(state, plane, NULL);
> if (ret != 0)
> goto fail;
> drm_atomic_set_fb_for_plane(plane_state, NULL);
> @@ -1426,7 +1426,7 @@ retry:
> goto fail;
> }
>
> - ret = drm_atomic_set_crtc_for_plane(primary_state, crtc);
> + ret = drm_atomic_set_crtc_for_plane(state, crtc->primary, crtc);
> if (ret != 0)
> goto fail;
> drm_atomic_set_fb_for_plane(primary_state, set->fb);
> @@ -1698,7 +1698,7 @@ retry:
> goto fail;
> }
>
> - ret = drm_atomic_set_crtc_for_plane(plane_state, crtc);
> + ret = drm_atomic_set_crtc_for_plane(state, plane, crtc);
> if (ret != 0)
> goto fail;
> drm_atomic_set_fb_for_plane(plane_state, fb);
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 9d91916..6ff8775 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -44,8 +44,8 @@ drm_atomic_get_connector_state(struct drm_atomic_state
> *state,
> struct drm_connector *connector);
>
> int __must_check
> -drm_atomic_set_crtc_for_plane(struct drm_plane_state *plane_state,
> - struct drm_crtc *crtc);
> +drm_atomic_set_crtc_for_plane(struct drm_atomic_state *state,
> + struct drm_plane *plane, struct drm_crtc *crtc);
> void drm_atomic_set_fb_for_plane(struct drm_plane_state *plane_state,
> struct drm_framebuffer *fb);
> int __must_check
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index b459e8f..4cf6905 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -231,6 +231,7 @@ struct drm_atomic_state;
> * struct drm_crtc_state - mutable CRTC state
> * @enable: whether the CRTC should be enabled, gates all other state
> * @mode_changed: for use by helpers and drivers when computing state updates
> + * @plane_mask: bitmask of (1 << drm_plane_index(plane)) of attached planes
> * @last_vblank_count: for helpers and drivers to capture the vblank of the
> * update to ensure framebuffer cleanup isn't done too early
> * @planes_changed: for use by helpers and drivers when computing state
> updates
> @@ -247,6 +248,13 @@ struct drm_crtc_state {
> bool planes_changed : 1;
> bool mode_changed : 1;
>
> + /* attached planes bitmask:
> + * WARNING: transitional helpers do not maintain plane_mask so
> + * drivers not converted over to atomic helpers should not rely
> + * on plane_mask being accurate!
> + */
> + u32 plane_mask;
> +
> /* last_vblank_count: for vblank waits before cleanup */
> u32 last_vblank_count;
>
> @@ -438,7 +446,7 @@ struct drm_crtc {
> * @state: backpointer to global drm_atomic_state
> */
> struct drm_connector_state {
> - struct drm_crtc *crtc;
> + struct drm_crtc *crtc; /* do not write directly, use
> drm_atomic_set_crtc_for_connector() */
>
> struct drm_encoder *best_encoder;
>
> @@ -673,8 +681,8 @@ struct drm_connector {
> * @state: backpointer to global drm_atomic_state
> */
> struct drm_plane_state {
> - struct drm_crtc *crtc;
> - struct drm_framebuffer *fb;
> + struct drm_crtc *crtc; /* do not write directly, use
> drm_atomic_set_crtc_for_plane() */
> + struct drm_framebuffer *fb; /* do not write directly, use
> drm_atomic_set_fb_for_plane() */
> struct fence *fence;
>
> /* Signed dest location allows it to be partially off screen */
> --
> 1.9.3
>
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
