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

[Matt Roper <matthew.d.roper@intel.com>]

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

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx