From: Ville Syrjälä ville.syrjala@linux.intel.com
Instead of locking all modeset locks during plane updates, use just a single CRTC mutex. To make that work, track the CRTC that "owns" the plane currently. During enable/update that means the new CRTC, and during disable it means the old CRTC.
Since the plane state is no longer protected by a single lock, we need to sprinkle some additional memory barriers when relinquishing ownership. Otherwise the next CRTC might observe some stale state even though the crtc_mutex already got updated. drm_framebuffer_remove() doesn't need extra barriers since it already holds all CRTC locks, and thus no-one can be poking around at the same time. On the read side cmpxchg() already should have the necessary memory barriers.
This design implies that before a plane can be moved to another CRTC, it must be disabled first, even if the hardware would offer some kind of mechanism to move an active plane over directly. I believe everyone has agreed that this an acceptable compromise.
Signed-off-by: Ville Syrjälä ville.syrjala@linux.intel.com --- drivers/gpu/drm/drm_crtc.c | 43 +++++++++++++++++++++++++++++++++++++++---- include/drm/drm_crtc.h | 3 +++ 2 files changed, 42 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 957fb70..6f7385e 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -576,6 +576,7 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb) __drm_framebuffer_unreference(plane->fb); plane->fb = NULL; plane->crtc = NULL; + plane->crtc_mutex = NULL; } } drm_modeset_unlock_all(dev); @@ -1785,6 +1786,7 @@ int drm_mode_setplane(struct drm_device *dev, void *data, int ret = 0; unsigned int fb_width, fb_height; int i; + struct mutex *old_crtc_mutex;
if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EINVAL; @@ -1804,12 +1806,33 @@ int drm_mode_setplane(struct drm_device *dev, void *data,
/* No fb means shut it down */ if (!plane_req->fb_id) { - drm_modeset_lock_all(dev); + struct mutex *crtc_mutex; + + retry: + crtc_mutex = ACCESS_ONCE(plane->crtc_mutex); + + /* plane was already disabled? */ + if (!crtc_mutex) + return 0; + + mutex_lock(crtc_mutex); + + /* re-check that plane is still on the same crtc... */ + if (crtc_mutex != plane->crtc_mutex) { + mutex_unlock(crtc_mutex); + goto retry; + } + old_fb = plane->fb; plane->funcs->disable_plane(plane); plane->crtc = NULL; plane->fb = NULL; - drm_modeset_unlock_all(dev); + + smp_wmb(); + plane->crtc_mutex = NULL; + + mutex_unlock(crtc_mutex); + goto out; }
@@ -1875,7 +1898,15 @@ int drm_mode_setplane(struct drm_device *dev, void *data, goto out; }
- drm_modeset_lock_all(dev); + mutex_lock(&crtc->mutex); + + old_crtc_mutex = cmpxchg(&plane->crtc_mutex, NULL, &crtc->mutex); + if (old_crtc_mutex != NULL && old_crtc_mutex != &crtc->mutex) { + mutex_unlock(&crtc->mutex); + ret = -EBUSY; + goto out; + } + ret = plane->funcs->update_plane(plane, crtc, fb, plane_req->crtc_x, plane_req->crtc_y, plane_req->crtc_w, plane_req->crtc_h, @@ -1886,8 +1917,12 @@ int drm_mode_setplane(struct drm_device *dev, void *data, plane->crtc = crtc; plane->fb = fb; fb = NULL; + } else { + smp_wmb(); + plane->crtc_mutex = old_crtc_mutex; } - drm_modeset_unlock_all(dev); + + mutex_unlock(&crtc->mutex);
out: if (fb) diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 8c7846b..cc3779f 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -651,6 +651,7 @@ struct drm_plane_funcs { * @dev: DRM device this plane belongs to * @head: for list management * @base: base mode object + * @crtc_mutex: points to the mutex of the current "owner" CRTC * @possible_crtcs: pipes this plane can be bound to * @format_types: array of formats supported by this plane * @format_count: number of formats supported @@ -669,6 +670,8 @@ struct drm_plane {
struct drm_mode_object base;
+ struct mutex *crtc_mutex; + uint32_t possible_crtcs; uint32_t *format_types; uint32_t format_count;