[Intel-gfx] [RFC][PATCH] drm: Insane but more fine grained locking for planes

[Daniel Vetter]

On Wed, Apr 17, 2013 at 08:04:52PM +0300, ville.syrjala at linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala at 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 at linux.intel.com>

Since the insanity is around plane->crtc_mutex, why not just add a plane
mutex which _only_ protects that?

That way we could partially resurect the old semantics by simply first
grabbing the old crtc mutex, removing the fb, then grabbing the new crtc
mutex and displaying it there. Whoever shows up with hw which can do that
in one atomic step gets to fix the resulting mess then ;-)
-Daniel

> ---
>  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;
> -- 
> 1.8.1.5
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch