On Mon, Nov 03, 2014 at 12:07:02AM +0100, Daniel Vetter wrote:
> Turned out to be much simpler on top of my latest atomic stuff than
> what I've feared. Some details:
>
> - Drop the modeset_lock_all snakeoil in drm_plane_init. Same
> justification as for the equivalent change in drm_crtc_init done in
>
> commit d0fa1af40e784aaf7ebb7ba8a17b229bb3fa4c21
> Author: Daniel Vetter <daniel.vet...@ffwll.ch>
> Date: Mon Sep 8 09:02:49 2014 +0200
>
> drm: Drop modeset locking from crtc init function
>
> Without these the drm_modeset_lock_init would fall over the exact
> same way.
>
> - Since the atomic core code wraps the locking switching it to
> per-plane locks was a one-line change.
>
> - For the legacy ioctls add a plane argument to the locking helper so
> that we can grab the right plane lock (cursor or primary). Since the
> universal cursor plane might not be there, or someone really crazy
> might forgoe the primary plane even accept NULL.
>
> - Add some locking WARN_ON to the atomic helpers for good paranoid
> measure and to check that it all works out.
>
> Tested on my exynos atomic hackfest with full lockdep checks and ww
> backoff injection.
Were you planning to convert setplane over to this?
>
> Signed-off-by: Daniel Vetter <daniel.vet...@intel.com>
> ---
> drivers/gpu/drm/drm_atomic.c | 2 +-
> drivers/gpu/drm/drm_atomic_helper.c | 4 ++++
> drivers/gpu/drm/drm_crtc.c | 9 ++++----
> drivers/gpu/drm/drm_modeset_lock.c | 43
> ++++++++++++++++++++++++++++---------
> include/drm/drm_crtc.h | 9 ++------
> include/drm/drm_modeset_lock.h | 4 +++-
> 6 files changed, 47 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index af34321b675d..d8d294f2a4a6 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -244,7 +244,7 @@ drm_atomic_get_plane_state(struct drm_atomic_state *state,
> * grab all crtc locks. Once we have per-plane locks we must update this
> * to only take the plane mutex.
> */
> - ret = drm_modeset_lock_all_crtcs(state->dev, state->acquire_ctx);
> + ret = drm_modeset_lock(&plane->mutex, state->acquire_ctx);
> if (ret)
> return ERR_PTR(ret);
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c
> index a5de60faedff..48931c95f180 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -987,6 +987,8 @@ void drm_atomic_helper_commit_planes(struct drm_device
> *dev,
> if (!crtc)
> continue;
>
> + WARN_ON(!drm_modeset_is_locked(&crtc->mutex));
> +
> funcs = crtc->helper_private;
>
> if (!funcs || !funcs->atomic_begin)
> @@ -1002,6 +1004,8 @@ void drm_atomic_helper_commit_planes(struct drm_device
> *dev,
> if (!plane)
> continue;
>
> + WARN_ON(!drm_modeset_is_locked(&plane->mutex));
> +
> funcs = plane->helper_private;
>
> if (!funcs || !funcs->atomic_update)
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index d01239db4042..a05f9652dcc4 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -1150,12 +1150,12 @@ int drm_universal_plane_init(struct drm_device *dev,
> struct drm_plane *plane,
> {
> int ret;
>
> - drm_modeset_lock_all(dev);
> -
> ret = drm_mode_object_get(dev, &plane->base, DRM_MODE_OBJECT_PLANE);
> if (ret)
> goto out;
>
> + drm_modeset_lock_init(&plane->mutex);
> +
> plane->base.properties = &plane->properties;
> plane->dev = dev;
> plane->funcs = funcs;
> @@ -1183,7 +1183,6 @@ int drm_universal_plane_init(struct drm_device *dev,
> struct drm_plane *plane,
> plane->type);
>
> out:
> - drm_modeset_unlock_all(dev);
>
> return ret;
> }
> @@ -2795,7 +2794,7 @@ static int drm_mode_cursor_common(struct drm_device
> *dev,
> if (crtc->cursor)
> return drm_mode_cursor_universal(crtc, req, file_priv);
>
> - drm_modeset_lock_crtc(crtc);
> + drm_modeset_lock_crtc(crtc, crtc->cursor);
> if (req->flags & DRM_MODE_CURSOR_BO) {
> if (!crtc->funcs->cursor_set && !crtc->funcs->cursor_set2) {
> ret = -ENXIO;
> @@ -4571,7 +4570,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> if (!crtc)
> return -ENOENT;
>
> - drm_modeset_lock_crtc(crtc);
> + drm_modeset_lock_crtc(crtc, crtc->primary);
> if (crtc->primary->fb == NULL) {
> /* The framebuffer is currently unbound, presumably
> * due to a hotplug event, that userspace has not
> diff --git a/drivers/gpu/drm/drm_modeset_lock.c
> b/drivers/gpu/drm/drm_modeset_lock.c
> index 474e4d12a2d8..51cc47d827d8 100644
> --- a/drivers/gpu/drm/drm_modeset_lock.c
> +++ b/drivers/gpu/drm/drm_modeset_lock.c
> @@ -157,14 +157,20 @@ void drm_modeset_unlock_all(struct drm_device *dev)
> EXPORT_SYMBOL(drm_modeset_unlock_all);
>
> /**
> - * drm_modeset_lock_crtc - lock crtc with hidden acquire ctx
> - * @crtc: drm crtc
> + * drm_modeset_lock_crtc - lock crtc with hidden acquire ctx for a plane
> update
> + * @crtc: DRM CRTC
> + * @plane: DRM plane to be updated on @crtc
> + *
> + * This function locks the given crtc and plane (which should be either the
> + * primary or cursor plane) using a hidden acquire context. This is
> necessary so
> + * that drivers internally using the atomic interfaces can grab further locks
> + * with the lock acquire context.
> *
> - * This function locks the given crtc using a hidden acquire context. This is
> - * necessary so that drivers internally using the atomic interfaces can grab
> - * further locks with the lock acquire context.
> + * Note that @plane can be NULL, e.g. when the cursor support hasn't yet been
> + * converted to universal planes yet.
> */
> -void drm_modeset_lock_crtc(struct drm_crtc *crtc)
> +void drm_modeset_lock_crtc(struct drm_crtc *crtc,
> + struct drm_plane *plane)
> {
> struct drm_modeset_acquire_ctx *ctx;
> int ret;
> @@ -180,6 +186,18 @@ retry:
> if (ret)
> goto fail;
>
> + if (plane) {
> + ret = drm_modeset_lock(&plane->mutex, ctx);
> + if (ret)
> + goto fail;
> +
> + if (plane->crtc) {
> + ret = drm_modeset_lock(&plane->crtc->mutex, ctx);
> + if (ret)
> + goto fail;
> + }
> + }
> +
> WARN_ON(crtc->acquire_ctx);
>
> /* now we hold the locks, so now that it is safe, stash the
> @@ -437,15 +455,14 @@ void drm_modeset_unlock(struct drm_modeset_lock *lock)
> }
> EXPORT_SYMBOL(drm_modeset_unlock);
>
> -/* Temporary.. until we have sufficiently fine grained locking, there
> - * are a couple scenarios where it is convenient to grab all crtc locks.
> - * It is planned to remove this:
> - */
> +/* In some legacy codepaths it's convenient to just grab all the crtc and
> plane
> + * related locks. */
> int drm_modeset_lock_all_crtcs(struct drm_device *dev,
> struct drm_modeset_acquire_ctx *ctx)
> {
> struct drm_mode_config *config = &dev->mode_config;
> struct drm_crtc *crtc;
> + struct drm_plane *plane;
> int ret = 0;
>
> list_for_each_entry(crtc, &config->crtc_list, head) {
> @@ -454,6 +471,12 @@ int drm_modeset_lock_all_crtcs(struct drm_device *dev,
> return ret;
> }
>
> + list_for_each_entry(plane, &config->plane_list, head) {
> + ret = drm_modeset_lock(&plane->mutex, ctx);
> + if (ret)
> + return ret;
> + }
> +
> return 0;
> }
> EXPORT_SYMBOL(drm_modeset_lock_all_crtcs);
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index f26b943fab37..e9776b191826 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -376,13 +376,6 @@ struct drm_crtc {
> struct device_node *port;
> struct list_head head;
>
> - /*
> - * crtc mutex
> - *
> - * This provides a read lock for the overall crtc state (mode, dpms
> - * state, ...) and a write lock for everything which can be update
> - * without a full modeset (fb, cursor data, ...)
> - */
> struct drm_modeset_lock mutex;
>
> struct drm_mode_object base;
> @@ -772,6 +765,8 @@ struct drm_plane {
> struct drm_device *dev;
> struct list_head head;
>
> + struct drm_modeset_lock mutex;
> +
> struct drm_mode_object base;
>
> uint32_t possible_crtcs;
> diff --git a/include/drm/drm_modeset_lock.h b/include/drm/drm_modeset_lock.h
> index 28931a23d96c..70595ff565ba 100644
> --- a/include/drm/drm_modeset_lock.h
> +++ b/include/drm/drm_modeset_lock.h
> @@ -127,11 +127,13 @@ void drm_modeset_unlock(struct drm_modeset_lock *lock);
>
> struct drm_device;
> struct drm_crtc;
> +struct drm_plane;
>
> void drm_modeset_lock_all(struct drm_device *dev);
> int __drm_modeset_lock_all(struct drm_device *dev, bool trylock);
> void drm_modeset_unlock_all(struct drm_device *dev);
> -void drm_modeset_lock_crtc(struct drm_crtc *crtc);
> +void drm_modeset_lock_crtc(struct drm_crtc *crtc,
> + struct drm_plane *plane);
> void drm_modeset_unlock_crtc(struct drm_crtc *crtc);
> void drm_warn_on_modeset_not_all_locked(struct drm_device *dev);
> struct drm_modeset_acquire_ctx *
> --
> 2.1.1
>
> _______________________________________________
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
