On Thu, Jun 08, 2017 at 10:51:34AM +0100, Chris Wilson wrote:
> Reduce acquisition of struct_mutex to the critical regions that must
> hold it; for KMS, we need struct_mutex currently only for the purpose of
> pinning/unpinning the framebuffer's VMA into the global GTT. This allows
> us to avoid taking the struct_mutex when disabling the CRTC (i.e. NULL
> framebuffer objects) before a reset. (Not yet achieving the full goal of
> avoiding the strut_mutex nesting, but good enough to break the first
> half of the reset deadlock.)
>
> Signed-off-by: Chris Wilson <[email protected]>
> ---
> drivers/gpu/drm/i915/intel_display.c | 85
> +++++++++++++++++-------------------
> 1 file changed, 41 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 25390dd8e08e..121fdd278fcd 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12771,14 +12771,7 @@ static int intel_atomic_prepare_commit(struct
> drm_device *dev,
> flush_workqueue(dev_priv->wq);
> }
>
> - ret = mutex_lock_interruptible(&dev->struct_mutex);
> - if (ret)
> - return ret;
> -
> - ret = drm_atomic_helper_prepare_planes(dev, state);
> - mutex_unlock(&dev->struct_mutex);
> -
> - return ret;
> + return drm_atomic_helper_prepare_planes(dev, state);
> }
>
> u32 intel_crtc_get_vblank_counter(struct intel_crtc *crtc)
> @@ -13141,9 +13134,7 @@ static void intel_atomic_commit_tail(struct
> drm_atomic_state *state)
> intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET);
> }
>
> - mutex_lock(&dev->struct_mutex);
> drm_atomic_helper_cleanup_planes(dev, state);
> - mutex_unlock(&dev->struct_mutex);
>
> drm_atomic_helper_commit_cleanup_done(state);
>
> @@ -13317,32 +13308,6 @@ intel_prepare_plane_fb(struct drm_plane *plane,
> struct drm_i915_gem_object *old_obj = intel_fb_obj(plane->state->fb);
> int ret;
>
> - if (obj) {
> - if (plane->type == DRM_PLANE_TYPE_CURSOR &&
> - INTEL_INFO(dev_priv)->cursor_needs_physical) {
> - const int align = intel_cursor_alignment(dev_priv);
> -
> - ret = i915_gem_object_attach_phys(obj, align);
> - if (ret) {
> - DRM_DEBUG_KMS("failed to attach phys object\n");
> - return ret;
> - }
> - } else {
> - struct i915_vma *vma;
> -
> - vma = intel_pin_and_fence_fb_obj(fb,
> new_state->rotation);
> - if (IS_ERR(vma)) {
> - DRM_DEBUG_KMS("failed to pin object\n");
> - return PTR_ERR(vma);
> - }
> -
> - to_intel_plane_state(new_state)->vma = vma;
> - }
> - }
> -
> - if (!obj && !old_obj)
> - return 0;
> -
> if (old_obj) {
> struct drm_crtc_state *crtc_state =
> drm_atomic_get_existing_crtc_state(new_state->state,
> @@ -13381,6 +13346,38 @@ intel_prepare_plane_fb(struct drm_plane *plane,
> if (!obj)
> return 0;
>
> + ret = i915_gem_object_pin_pages(obj);
> + if (ret)
> + return ret;
> +
> + ret = mutex_lock_interruptible(&dev_priv->drm.struct_mutex);
> + if (ret) {
> + i915_gem_object_unpin_pages(obj);
> + return ret;
> + }
> +
> + i915_gem_object_wait_priority(obj, 0, I915_PRIORITY_DISPLAY);
Does this guy need struct mutex? Maybe it could use a lockdep assert if
so.
> +
> + if (plane->type == DRM_PLANE_TYPE_CURSOR &&
> + INTEL_INFO(dev_priv)->cursor_needs_physical) {
> + const int align = intel_cursor_alignment(dev_priv);
> +
> + ret = i915_gem_object_attach_phys(obj, align);
Isn't this going to fail due to the i915_gem_object_pin_pages() above?
> + } else {
> + struct i915_vma *vma;
> +
> + vma = intel_pin_and_fence_fb_obj(fb, new_state->rotation);
> + if (!IS_ERR(vma))
> + to_intel_plane_state(new_state)->vma = vma;
> + else
> + ret = PTR_ERR(vma);
> + }
> +
> + mutex_unlock(&dev_priv->drm.struct_mutex);
> + i915_gem_object_unpin_pages(obj);
> + if (ret)
> + return ret;
> +
> if (!new_state->fence) { /* implicit fencing */
> ret =
> i915_sw_fence_await_reservation(&intel_state->commit_ready,
> obj->resv, NULL,
> @@ -13388,8 +13385,6 @@ intel_prepare_plane_fb(struct drm_plane *plane,
> GFP_KERNEL);
> if (ret < 0)
> return ret;
> -
> - i915_gem_object_wait_priority(obj, 0, I915_PRIORITY_DISPLAY);
> }
>
> return 0;
> @@ -13412,8 +13407,11 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
>
> /* Should only be called after a successful intel_prepare_plane_fb()! */
> vma = fetch_and_zero(&to_intel_plane_state(old_state)->vma);
> - if (vma)
> + if (vma) {
> + mutex_lock(&plane->dev->struct_mutex);
> intel_unpin_fb_vma(vma);
> + mutex_unlock(&plane->dev->struct_mutex);
> + }
> }
>
> int
> @@ -13583,7 +13581,7 @@ intel_legacy_cursor_update(struct drm_plane *plane,
> struct intel_plane *intel_plane = to_intel_plane(plane);
> struct drm_framebuffer *old_fb;
> struct drm_crtc_state *crtc_state = crtc->state;
> - struct i915_vma *old_vma;
> + struct i915_vma *old_vma, *vma;
>
> /*
> * When crtc is inactive or there is a modeset pending,
> @@ -13641,8 +13639,6 @@ intel_legacy_cursor_update(struct drm_plane *plane,
> goto out_unlock;
> }
> } else {
> - struct i915_vma *vma;
> -
> vma = intel_pin_and_fence_fb_obj(fb, new_plane_state->rotation);
> if (IS_ERR(vma)) {
> DRM_DEBUG_KMS("failed to pin object\n");
> @@ -13665,7 +13661,7 @@ intel_legacy_cursor_update(struct drm_plane *plane,
> *to_intel_plane_state(old_plane_state) =
> *to_intel_plane_state(new_plane_state);
> new_plane_state->fence = NULL;
> new_plane_state->fb = old_fb;
> - to_intel_plane_state(new_plane_state)->vma = old_vma;
> + to_intel_plane_state(new_plane_state)->vma = NULL;
>
> if (plane->state->visible) {
> trace_intel_update_plane(plane, to_intel_crtc(crtc));
> @@ -13677,7 +13673,8 @@ intel_legacy_cursor_update(struct drm_plane *plane,
> intel_plane->disable_plane(intel_plane, to_intel_crtc(crtc));
> }
>
> - intel_cleanup_plane_fb(plane, new_plane_state);
> + if (old_vma)
> + intel_unpin_fb_vma(old_vma);
>
> out_unlock:
> mutex_unlock(&dev_priv->drm.struct_mutex);
> --
> 2.11.0
>
> _______________________________________________
> Intel-gfx mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
