diff mbox

[4/5] drm/i915: Change locking for struct_mutex.

Message ID 1443007632-5573-5-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Maarten Lankhorst Sept. 23, 2015, 11:27 a.m. UTC
Only acquire the struct_mutex once, and interruptibly.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

Comments

Matt Roper Oct. 28, 2015, 10:48 p.m. UTC | #1
On Wed, Sep 23, 2015 at 01:27:11PM +0200, Maarten Lankhorst wrote:
> Only acquire the struct_mutex once, and interruptibly.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Your headline/commit message seem a bit sparse here...you may want to
make it clear that this refers to framebuffer preparation/cleanup for
atomic commits.  The i915 driver in general still has a bunch of of
struct_mutex usage in other places that isn't touched by this patch.

Effectively you're changing logic from:

        loop {
                grab mutex()
                stuff
                drop mutex()
        }

to

        grab mutex()
        loop {
                stuff
        }
        drop mutex()

The code change itself looks fine to me, so with an updated commit
message you can consider this

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>


Matt

> ---
>  drivers/gpu/drm/i915/intel_display.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index cd651ff6c15b..2f046134cc9a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13008,8 +13008,13 @@ static int intel_atomic_prepare_commit(struct drm_device *dev,
>  			return ret;
>  	}
>  
> +	ret = i915_mutex_lock_interruptible(dev);
> +	if (ret)
> +		return ret;
> +
>  	ret = drm_atomic_helper_prepare_planes(dev, state);
>  
> +	mutex_unlock(&dev->struct_mutex);
>  	return ret;
>  }
>  
> @@ -13108,7 +13113,10 @@ static int intel_atomic_commit(struct drm_device *dev,
>  	/* FIXME: add subpixel order */
>  
>  	drm_atomic_helper_wait_for_vblanks(dev, state);
> +
> +	mutex_lock(&dev->struct_mutex);
>  	drm_atomic_helper_cleanup_planes(dev, state);
> +	mutex_unlock(&dev->struct_mutex);
>  
>  	if (any_ms)
>  		intel_modeset_check_state(dev, state);
> @@ -13295,10 +13303,6 @@ intel_prepare_plane_fb(struct drm_plane *plane,
>  	if (!obj && !old_obj)
>  		return 0;
>  
> -	ret = i915_mutex_lock_interruptible(dev);
> -	if (ret)
> -		return ret;
> -
>  	if (old_obj) {
>  		struct drm_crtc_state *crtc_state =
>  			drm_atomic_get_existing_crtc_state(new_state->state, plane->state->crtc);
> @@ -13319,7 +13323,7 @@ intel_prepare_plane_fb(struct drm_plane *plane,
>  
>  		/* Swallow -EIO errors to allow updates during hw lockup. */
>  		if (ret && ret != -EIO)
> -			goto out;
> +			return ret;
>  	}
>  
>  	if (!obj) {
> @@ -13337,9 +13341,6 @@ intel_prepare_plane_fb(struct drm_plane *plane,
>  	if (ret == 0)
>  		i915_gem_track_fb(old_obj, obj, intel_plane->frontbuffer_bit);
>  
> -out:
> -	mutex_unlock(&dev->struct_mutex);
> -
>  	return ret;
>  }
>  
> @@ -13362,7 +13363,6 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
>  	if (!obj && !old_obj)
>  		return;
>  
> -	mutex_lock(&dev->struct_mutex);
>  	if (old_obj && (plane->type != DRM_PLANE_TYPE_CURSOR ||
>  	    !INTEL_INFO(dev)->cursor_needs_physical))
>  		intel_unpin_fb_obj(old_state->fb, old_state);
> @@ -13371,7 +13371,6 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
>  	if ((old_obj && (old_obj->frontbuffer_bits & intel_plane->frontbuffer_bit)) ||
>  	    (obj && !(obj->frontbuffer_bits & intel_plane->frontbuffer_bit)))
>  		i915_gem_track_fb(old_obj, obj, intel_plane->frontbuffer_bit);
> -	mutex_unlock(&dev->struct_mutex);
>  }
>  
>  int
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index cd651ff6c15b..2f046134cc9a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13008,8 +13008,13 @@  static int intel_atomic_prepare_commit(struct drm_device *dev,
 			return ret;
 	}
 
+	ret = i915_mutex_lock_interruptible(dev);
+	if (ret)
+		return ret;
+
 	ret = drm_atomic_helper_prepare_planes(dev, state);
 
+	mutex_unlock(&dev->struct_mutex);
 	return ret;
 }
 
@@ -13108,7 +13113,10 @@  static int intel_atomic_commit(struct drm_device *dev,
 	/* FIXME: add subpixel order */
 
 	drm_atomic_helper_wait_for_vblanks(dev, state);
+
+	mutex_lock(&dev->struct_mutex);
 	drm_atomic_helper_cleanup_planes(dev, state);
+	mutex_unlock(&dev->struct_mutex);
 
 	if (any_ms)
 		intel_modeset_check_state(dev, state);
@@ -13295,10 +13303,6 @@  intel_prepare_plane_fb(struct drm_plane *plane,
 	if (!obj && !old_obj)
 		return 0;
 
-	ret = i915_mutex_lock_interruptible(dev);
-	if (ret)
-		return ret;
-
 	if (old_obj) {
 		struct drm_crtc_state *crtc_state =
 			drm_atomic_get_existing_crtc_state(new_state->state, plane->state->crtc);
@@ -13319,7 +13323,7 @@  intel_prepare_plane_fb(struct drm_plane *plane,
 
 		/* Swallow -EIO errors to allow updates during hw lockup. */
 		if (ret && ret != -EIO)
-			goto out;
+			return ret;
 	}
 
 	if (!obj) {
@@ -13337,9 +13341,6 @@  intel_prepare_plane_fb(struct drm_plane *plane,
 	if (ret == 0)
 		i915_gem_track_fb(old_obj, obj, intel_plane->frontbuffer_bit);
 
-out:
-	mutex_unlock(&dev->struct_mutex);
-
 	return ret;
 }
 
@@ -13362,7 +13363,6 @@  intel_cleanup_plane_fb(struct drm_plane *plane,
 	if (!obj && !old_obj)
 		return;
 
-	mutex_lock(&dev->struct_mutex);
 	if (old_obj && (plane->type != DRM_PLANE_TYPE_CURSOR ||
 	    !INTEL_INFO(dev)->cursor_needs_physical))
 		intel_unpin_fb_obj(old_state->fb, old_state);
@@ -13371,7 +13371,6 @@  intel_cleanup_plane_fb(struct drm_plane *plane,
 	if ((old_obj && (old_obj->frontbuffer_bits & intel_plane->frontbuffer_bit)) ||
 	    (obj && !(obj->frontbuffer_bits & intel_plane->frontbuffer_bit)))
 		i915_gem_track_fb(old_obj, obj, intel_plane->frontbuffer_bit);
-	mutex_unlock(&dev->struct_mutex);
 }
 
 int