[PATCH 1/2] drm/i915: Add missing locking to primary plane handlers

[PATCH 1/2] drm/i915: Add missing locking to primary plane handlers

[Matt Roper]

intel_primary_plane_{setplane,disable} were lacking struct_mutex locking
around their GEM operations.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5a50ff9..6b25068 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11389,9 +11389,11 @@ intel_primary_plane_disable(struct drm_plane *plane)
 	intel_disable_primary_hw_plane(dev_priv, intel_plane->plane,
 				       intel_plane->pipe);
 disable_unpin:
+	mutex_lock(&dev->struct_mutex);
 	i915_gem_track_fb(intel_fb_obj(plane->fb), NULL,
 			  INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe));
 	intel_unpin_fb_obj(intel_fb_obj(plane->fb));
+	mutex_unlock(&dev->struct_mutex);
 	plane->fb = NULL;
 
 	return 0;
@@ -11448,6 +11450,8 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
 	 * turn on the display with all planes setup as desired.
 	 */
 	if (!crtc->enabled) {
+		mutex_lock(&dev->struct_mutex);
+
 		/*
 		 * If we already called setplane while the crtc was disabled,
 		 * we may have an fb pinned; unpin it.
@@ -11459,7 +11463,10 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
 				  INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe));
 
 		/* Pin and return without programming hardware */
-		return intel_pin_and_fence_fb_obj(dev, obj, NULL);
+		ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
+		mutex_unlock(&dev->struct_mutex);
+
+		return ret;
 	}
 
 	intel_crtc_wait_for_pending_flips(crtc);
@@ -11471,14 +11478,18 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
 	 * because plane->fb still gets set and pinned.
 	 */
 	if (!visible) {
+		mutex_lock(&dev->struct_mutex);
+
 		/*
 		 * Try to pin the new fb first so that we can bail out if we
 		 * fail.
 		 */
 		if (plane->fb != fb) {
 			ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
-			if (ret)
+			if (ret) {
+				mutex_unlock(&dev->struct_mutex);
 				return ret;
+			}
 		}
 
 		i915_gem_track_fb(old_obj, obj,
@@ -11494,6 +11505,8 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
 			if (plane->fb)
 				intel_unpin_fb_obj(old_obj);
 
+		mutex_unlock(&dev->struct_mutex);
+
 		return 0;
 	}
 
-- 
1.8.5.1

[PATCH 2/2] drm/i915: Add a couple WARN()'s to catch missing locks

[Matt Roper]

Add !mutex_is_locked() checks to intel_pin_and_fence_fb_obj() and
intel_unpin_fb_obj() to help catch failures to grab struct_mutex when
operating on fb objects.

Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 6b25068..6f2729a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2196,6 +2196,8 @@ intel_pin_and_fence_fb_obj(struct drm_device *dev,
 	u32 alignment;
 	int ret;
 
+	WARN_ON(!mutex_is_locked(&dev->struct_mutex));
+
 	switch (obj->tiling_mode) {
 	case I915_TILING_NONE:
 		if (IS_BROADWATER(dev) || IS_CRESTLINE(dev))
@@ -2252,6 +2254,8 @@ err_interruptible:
 
 void intel_unpin_fb_obj(struct drm_i915_gem_object *obj)
 {
+	WARN_ON(!mutex_is_locked(&obj->base.dev->struct_mutex));
+
 	i915_gem_object_unpin_fence(obj);
 	i915_gem_object_unpin_from_display_plane(obj);
 }
-- 
1.8.5.1

Re: [PATCH 1/2] drm/i915: Add missing locking to primary plane handlers

[Daniel Vetter]

On Wed, Jul 09, 2014 at 04:22:10PM -0700, Matt Roper wrote:
> intel_primary_plane_{setplane,disable} were lacking struct_mutex locking
> around their GEM operations.
> 
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>

Both merged with a reported-by: Damien added to this one here. Btw have
you checked that your primary plane tests hit this with the new WARN_ONs?
I'm missing the Testcase: tag ;-)
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 5a50ff9..6b25068 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11389,9 +11389,11 @@ intel_primary_plane_disable(struct drm_plane *plane)
>  	intel_disable_primary_hw_plane(dev_priv, intel_plane->plane,
>  				       intel_plane->pipe);
>  disable_unpin:
> +	mutex_lock(&dev->struct_mutex);
>  	i915_gem_track_fb(intel_fb_obj(plane->fb), NULL,
>  			  INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe));
>  	intel_unpin_fb_obj(intel_fb_obj(plane->fb));
> +	mutex_unlock(&dev->struct_mutex);
>  	plane->fb = NULL;
>  
>  	return 0;
> @@ -11448,6 +11450,8 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
>  	 * turn on the display with all planes setup as desired.
>  	 */
>  	if (!crtc->enabled) {
> +		mutex_lock(&dev->struct_mutex);
> +
>  		/*
>  		 * If we already called setplane while the crtc was disabled,
>  		 * we may have an fb pinned; unpin it.
> @@ -11459,7 +11463,10 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
>  				  INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe));
>  
>  		/* Pin and return without programming hardware */
> -		return intel_pin_and_fence_fb_obj(dev, obj, NULL);
> +		ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
> +		mutex_unlock(&dev->struct_mutex);
> +
> +		return ret;
>  	}
>  
>  	intel_crtc_wait_for_pending_flips(crtc);
> @@ -11471,14 +11478,18 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
>  	 * because plane->fb still gets set and pinned.
>  	 */
>  	if (!visible) {
> +		mutex_lock(&dev->struct_mutex);
> +
>  		/*
>  		 * Try to pin the new fb first so that we can bail out if we
>  		 * fail.
>  		 */
>  		if (plane->fb != fb) {
>  			ret = intel_pin_and_fence_fb_obj(dev, obj, NULL);
> -			if (ret)
> +			if (ret) {
> +				mutex_unlock(&dev->struct_mutex);
>  				return ret;
> +			}
>  		}
>  
>  		i915_gem_track_fb(old_obj, obj,
> @@ -11494,6 +11505,8 @@ intel_primary_plane_setplane(struct drm_plane *plane, struct drm_crtc *crtc,
>  			if (plane->fb)
>  				intel_unpin_fb_obj(old_obj);
>  
> +		mutex_unlock(&dev->struct_mutex);
> +
>  		return 0;
>  	}
>  
> -- 
> 1.8.5.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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