Re: [Intel-gfx] [PATCH] drm/i915: Avoid double mutex lock applying pipe A quirk during sanitize_crtc()

2014-06-10 Thread Daniel Vetter 
On Mon, Jun 09, 2014 at 07:47:10AM +0100, Chris Wilson wrote:
 Thomas found that his machine would deadlock reloading the i915.ko
 module after resume. He identified that this was caused by the
 reacquisition of the connection mutex inside intel_enable_pipe_a()
 during the CRTC sanitization routine. This will only affect machines
 that quirk PIPE A, i.e. the original 830m chipsets.
 
 This patch move the locking into a wrapper function so that
 intel_enable_pipe_a() can bypass the locking knowing that it already
 holds the correct locks.
 
 Reported-by: Thomas Richter rich...@rus.uni-stuttgart.de
 Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
 Cc: Thomas Richter rich...@rus.uni-stuttgart.de
 Cc: Daniel Vetter dan...@ffwll.ch
 Cc: Ville Syrjälä ville.syrj...@linux.intel.com
 ---
  drivers/gpu/drm/i915/intel_display.c |   54 
 ++
  1 file changed, 35 insertions(+), 19 deletions(-)
 
 diff --git a/drivers/gpu/drm/i915/intel_display.c 
 b/drivers/gpu/drm/i915/intel_display.c
 index c1f79a1..26d3424 100644
 --- a/drivers/gpu/drm/i915/intel_display.c
 +++ b/drivers/gpu/drm/i915/intel_display.c
 @@ -8387,9 +8387,10 @@ mode_fits_in_fbdev(struct drm_device *dev,
  #endif
  }
  
 -bool intel_get_load_detect_pipe(struct drm_connector *connector,
 - struct drm_display_mode *mode,
 - struct intel_load_detect_pipe *old)
 +static bool
 +__intel_get_load_detect_pipe(struct drm_connector *connector,
 +  struct drm_display_mode *mode,
 +  struct intel_load_detect_pipe *old)
  {
   struct intel_crtc *intel_crtc;
   struct intel_encoder *intel_encoder =
 @@ -8405,7 +8406,7 @@ bool intel_get_load_detect_pipe(struct drm_connector 
 *connector,
 connector-base.id, connector-name,
 encoder-base.id, encoder-name);
  
 - mutex_lock(dev-mode_config.connection_mutex);
 + lockdep_assert_held(dev-mode_config.connection_mutex);
  
   /*
* Algorithm gets a little messy:
 @@ -8449,7 +8450,7 @@ bool intel_get_load_detect_pipe(struct drm_connector 
 *connector,
*/
   if (!crtc) {
   DRM_DEBUG_KMS(no pipe available for load-detect\n);
 - goto fail_unlock_connector;
 + return false;
   }
  
   mutex_lock(crtc-mutex);
 @@ -8503,14 +8504,13 @@ bool intel_get_load_detect_pipe(struct drm_connector 
 *connector,
   else
   intel_crtc-new_config = NULL;
   mutex_unlock(crtc-mutex);
 -fail_unlock_connector:
 - mutex_unlock(dev-mode_config.connection_mutex);
  
   return false;
  }
  
 -void intel_release_load_detect_pipe(struct drm_connector *connector,
 - struct intel_load_detect_pipe *old)
 +static void
 +__intel_release_load_detect_pipe(struct drm_connector *connector,
 +  struct intel_load_detect_pipe *old)
  {
   struct intel_encoder *intel_encoder =
   intel_attached_encoder(connector);
 @@ -8518,6 +8518,8 @@ void intel_release_load_detect_pipe(struct 
 drm_connector *connector,
   struct drm_crtc *crtc = encoder-crtc;
   struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
  
 + lockdep_assert_held(connector-dev-mode_config.connection_mutex);
 +
   DRM_DEBUG_KMS([CONNECTOR:%d:%s], [ENCODER:%d:%s]\n,
 connector-base.id, connector-name,
 encoder-base.id, encoder-name);
 @@ -8533,17 +8535,32 @@ void intel_release_load_detect_pipe(struct 
 drm_connector *connector,
   drm_framebuffer_unregister_private(old-release_fb);
   drm_framebuffer_unreference(old-release_fb);
   }
 + } else {
 + /* Switch crtc and encoder back off if necessary */
 + if (old-dpms_mode != DRM_MODE_DPMS_ON)
 + connector-funcs-dpms(connector, old-dpms_mode);
 + }

This part looks like an unrelated change?
-Daniel

 + mutex_unlock(crtc-mutex);
 +}
  
 - mutex_unlock(crtc-mutex);
 +bool intel_get_load_detect_pipe(struct drm_connector *connector,
 + struct drm_display_mode *mode,
 + struct intel_load_detect_pipe *old)
 +{
 + mutex_lock(connector-dev-mode_config.connection_mutex);
 + if (!__intel_get_load_detect_pipe(connector, mode, old)) {
   mutex_unlock(connector-dev-mode_config.connection_mutex);
 - return;
 + return false;
   }
  
 - /* Switch crtc and encoder back off if necessary */
 - if (old-dpms_mode != DRM_MODE_DPMS_ON)
 - connector-funcs-dpms(connector, old-dpms_mode);
 + /* lock will be released by intel_release_load_detect_pipe() */
 + return true;
 +}
  
 - mutex_unlock(crtc-mutex);
 +void intel_release_load_detect_pipe(struct drm_connector *connector,
 + struct

Re: [Intel-gfx] [PATCH] drm/i915: Avoid double mutex lock applying pipe A quirk during sanitize_crtc()

2014-06-10 Thread Daniel Vetter 
On Mon, Jun 09, 2014 at 11:30:26AM +0300, Ville Syrjälä wrote:
 On Mon, Jun 09, 2014 at 07:47:10AM +0100, Chris Wilson wrote:
  Thomas found that his machine would deadlock reloading the i915.ko
  module after resume. He identified that this was caused by the
  reacquisition of the connection mutex inside intel_enable_pipe_a()
  during the CRTC sanitization routine. This will only affect machines
  that quirk PIPE A, i.e. the original 830m chipsets.
  
  This patch move the locking into a wrapper function so that
  intel_enable_pipe_a() can bypass the locking knowing that it already
  holds the correct locks.
 
 It can still try to grab crtc-mutex twice. Looks like Danial undid my
 fix to not take all the modeset locks around
 intel_modeset_setup_hw_state().

Hm, I didn't find anything in git logs and we've had places where we used
modeset_lock_all since a long time. And I don't see how Chris' patch
wouldn't address this here. Can you please explain?

Thanks, Daniel

 
  
  Reported-by: Thomas Richter rich...@rus.uni-stuttgart.de
  Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
  Cc: Thomas Richter rich...@rus.uni-stuttgart.de
  Cc: Daniel Vetter dan...@ffwll.ch
  Cc: Ville Syrjälä ville.syrj...@linux.intel.com
  ---
   drivers/gpu/drm/i915/intel_display.c |   54 
  ++
   1 file changed, 35 insertions(+), 19 deletions(-)
  
  diff --git a/drivers/gpu/drm/i915/intel_display.c 
  b/drivers/gpu/drm/i915/intel_display.c
  index c1f79a1..26d3424 100644
  --- a/drivers/gpu/drm/i915/intel_display.c
  +++ b/drivers/gpu/drm/i915/intel_display.c
  @@ -8387,9 +8387,10 @@ mode_fits_in_fbdev(struct drm_device *dev,
   #endif
   }
   
  -bool intel_get_load_detect_pipe(struct drm_connector *connector,
  -   struct drm_display_mode *mode,
  -   struct intel_load_detect_pipe *old)
  +static bool
  +__intel_get_load_detect_pipe(struct drm_connector *connector,
  +struct drm_display_mode *mode,
  +struct intel_load_detect_pipe *old)
   {
  struct intel_crtc *intel_crtc;
  struct intel_encoder *intel_encoder =
  @@ -8405,7 +8406,7 @@ bool intel_get_load_detect_pipe(struct drm_connector 
  *connector,
connector-base.id, connector-name,
encoder-base.id, encoder-name);
   
  -   mutex_lock(dev-mode_config.connection_mutex);
  +   lockdep_assert_held(dev-mode_config.connection_mutex);
   
  /*
   * Algorithm gets a little messy:
  @@ -8449,7 +8450,7 @@ bool intel_get_load_detect_pipe(struct drm_connector 
  *connector,
   */
  if (!crtc) {
  DRM_DEBUG_KMS(no pipe available for load-detect\n);
  -   goto fail_unlock_connector;
  +   return false;
  }
   
  mutex_lock(crtc-mutex);
  @@ -8503,14 +8504,13 @@ bool intel_get_load_detect_pipe(struct 
  drm_connector *connector,
  else
  intel_crtc-new_config = NULL;
  mutex_unlock(crtc-mutex);
  -fail_unlock_connector:
  -   mutex_unlock(dev-mode_config.connection_mutex);
   
  return false;
   }
   
  -void intel_release_load_detect_pipe(struct drm_connector *connector,
  -   struct intel_load_detect_pipe *old)
  +static void
  +__intel_release_load_detect_pipe(struct drm_connector *connector,
  +struct intel_load_detect_pipe *old)
   {
  struct intel_encoder *intel_encoder =
  intel_attached_encoder(connector);
  @@ -8518,6 +8518,8 @@ void intel_release_load_detect_pipe(struct 
  drm_connector *connector,
  struct drm_crtc *crtc = encoder-crtc;
  struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
   
  +   lockdep_assert_held(connector-dev-mode_config.connection_mutex);
  +
  DRM_DEBUG_KMS([CONNECTOR:%d:%s], [ENCODER:%d:%s]\n,
connector-base.id, connector-name,
encoder-base.id, encoder-name);
  @@ -8533,17 +8535,32 @@ void intel_release_load_detect_pipe(struct 
  drm_connector *connector,
  drm_framebuffer_unregister_private(old-release_fb);
  drm_framebuffer_unreference(old-release_fb);
  }
  +   } else {
  +   /* Switch crtc and encoder back off if necessary */
  +   if (old-dpms_mode != DRM_MODE_DPMS_ON)
  +   connector-funcs-dpms(connector, old-dpms_mode);
  +   }
  +   mutex_unlock(crtc-mutex);
  +}
   
  -   mutex_unlock(crtc-mutex);
  +bool intel_get_load_detect_pipe(struct drm_connector *connector,
  +   struct drm_display_mode *mode,
  +   struct intel_load_detect_pipe *old)
  +{
  +   mutex_lock(connector-dev-mode_config.connection_mutex);
  +   if (!__intel_get_load_detect_pipe(connector, mode, old)) {
  mutex_unlock(connector-dev-mode_config.connection_mutex);
  -   return;
  +   return false;
  }
   
  -   /* Switch crtc and

Re: [Intel-gfx] [PATCH] drm/i915: Avoid double mutex lock applying pipe A quirk during sanitize_crtc()

2014-06-10 Thread Chris Wilson 
On Tue, Jun 10, 2014 at 08:59:42AM +0200, Daniel Vetter wrote:
 On Mon, Jun 09, 2014 at 07:47:10AM +0100, Chris Wilson wrote:
  +   } else {
  +   /* Switch crtc and encoder back off if necessary */
  +   if (old-dpms_mode != DRM_MODE_DPMS_ON)
  +   connector-funcs-dpms(connector, old-dpms_mode);
  +   }
 
 This part looks like an unrelated change?

It's just diff. The change was to rejig the braces to remove the
duplicated unlock + return.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Avoid double mutex lock applying pipe A quirk during sanitize_crtc()

2014-06-10 Thread Ville Syrjälä 
On Tue, Jun 10, 2014 at 09:02:07AM +0200, Daniel Vetter wrote:
 On Mon, Jun 09, 2014 at 11:30:26AM +0300, Ville Syrjälä wrote:
  On Mon, Jun 09, 2014 at 07:47:10AM +0100, Chris Wilson wrote:
   Thomas found that his machine would deadlock reloading the i915.ko
   module after resume. He identified that this was caused by the
   reacquisition of the connection mutex inside intel_enable_pipe_a()
   during the CRTC sanitization routine. This will only affect machines
   that quirk PIPE A, i.e. the original 830m chipsets.
   
   This patch move the locking into a wrapper function so that
   intel_enable_pipe_a() can bypass the locking knowing that it already
   holds the correct locks.
  
  It can still try to grab crtc-mutex twice. Looks like Danial undid my
  fix to not take all the modeset locks around
  intel_modeset_setup_hw_state().
 
 Hm, I didn't find anything in git logs and we've had places where we used
 modeset_lock_all since a long time. And I don't see how Chris' patch
 wouldn't address this here. Can you please explain?

I added the modeset_all locking here:
 commit 027476642811f8559cbe00ef6cc54db230e48a20
 Author: Ville Syrjälä ville.syrj...@linux.intel.com
 Date:   Mon Dec 2 11:08:06 2013 +0200

drm/i915: Take modeset locks around intel_modeset_setup_hw_state()

and then had to reduce it to just the mode_config.mutex precisely due to
the pipe A quirk here:
 commit 7ad228b11ec26a820291c9f5a1168d6176580dc1
 Author: Ville Syrjälä ville.syrj...@linux.intel.com
 Date:   Tue Jan 7 16:15:36 2014 +0200

drm/i915: Don't grab crtc mutexes in intel_modeset_gem_init()

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Avoid double mutex lock applying pipe A quirk during sanitize_crtc()

2014-06-10 Thread Daniel Vetter 
On Tue, Jun 10, 2014 at 11:53:51AM +0300, Ville Syrjälä wrote:
 On Tue, Jun 10, 2014 at 09:02:07AM +0200, Daniel Vetter wrote:
  On Mon, Jun 09, 2014 at 11:30:26AM +0300, Ville Syrjälä wrote:
   On Mon, Jun 09, 2014 at 07:47:10AM +0100, Chris Wilson wrote:
Thomas found that his machine would deadlock reloading the i915.ko
module after resume. He identified that this was caused by the
reacquisition of the connection mutex inside intel_enable_pipe_a()
during the CRTC sanitization routine. This will only affect machines
that quirk PIPE A, i.e. the original 830m chipsets.

This patch move the locking into a wrapper function so that
intel_enable_pipe_a() can bypass the locking knowing that it already
holds the correct locks.
   
   It can still try to grab crtc-mutex twice. Looks like Danial undid my
   fix to not take all the modeset locks around
   intel_modeset_setup_hw_state().
  
  Hm, I didn't find anything in git logs and we've had places where we used
  modeset_lock_all since a long time. And I don't see how Chris' patch
  wouldn't address this here. Can you please explain?
 
 I added the modeset_all locking here:
  commit 027476642811f8559cbe00ef6cc54db230e48a20
  Author: Ville Syrjälä ville.syrj...@linux.intel.com
  Date:   Mon Dec 2 11:08:06 2013 +0200
 
 drm/i915: Take modeset locks around intel_modeset_setup_hw_state()
 
 and then had to reduce it to just the mode_config.mutex precisely due to
 the pipe A quirk here:
  commit 7ad228b11ec26a820291c9f5a1168d6176580dc1
  Author: Ville Syrjälä ville.syrj...@linux.intel.com
  Date:   Tue Jan 7 16:15:36 2014 +0200
 
 drm/i915: Don't grab crtc mutexes in intel_modeset_gem_init()

Hm, indeed missed this since we have a bunch of other places that still do
the full modeset_lock_all, but probably not possible to hit the pipe A
quirk there. The ww mutex code makes this a bit more annoying so I guess
Chris' patch with having a lock-less pipe A quirk is the way to go.

But that one needs to be updated for latest drm-next/-nightly.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915: Avoid double mutex lock applying pipe A quirk during sanitize_crtc()

2014-06-09 Thread Chris Wilson 
Thomas found that his machine would deadlock reloading the i915.ko
module after resume. He identified that this was caused by the
reacquisition of the connection mutex inside intel_enable_pipe_a()
during the CRTC sanitization routine. This will only affect machines
that quirk PIPE A, i.e. the original 830m chipsets.

This patch move the locking into a wrapper function so that
intel_enable_pipe_a() can bypass the locking knowing that it already
holds the correct locks.

Reported-by: Thomas Richter rich...@rus.uni-stuttgart.de
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
Cc: Thomas Richter rich...@rus.uni-stuttgart.de
Cc: Daniel Vetter dan...@ffwll.ch
Cc: Ville Syrjälä ville.syrj...@linux.intel.com
---
 drivers/gpu/drm/i915/intel_display.c |   54 ++
 1 file changed, 35 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index c1f79a1..26d3424 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8387,9 +8387,10 @@ mode_fits_in_fbdev(struct drm_device *dev,
 #endif
 }
 
-bool intel_get_load_detect_pipe(struct drm_connector *connector,
-   struct drm_display_mode *mode,
-   struct intel_load_detect_pipe *old)
+static bool
+__intel_get_load_detect_pipe(struct drm_connector *connector,
+struct drm_display_mode *mode,
+struct intel_load_detect_pipe *old)
 {
struct intel_crtc *intel_crtc;
struct intel_encoder *intel_encoder =
@@ -8405,7 +8406,7 @@ bool intel_get_load_detect_pipe(struct drm_connector 
*connector,
  connector-base.id, connector-name,
  encoder-base.id, encoder-name);
 
-   mutex_lock(dev-mode_config.connection_mutex);
+   lockdep_assert_held(dev-mode_config.connection_mutex);
 
/*
 * Algorithm gets a little messy:
@@ -8449,7 +8450,7 @@ bool intel_get_load_detect_pipe(struct drm_connector 
*connector,
 */
if (!crtc) {
DRM_DEBUG_KMS(no pipe available for load-detect\n);
-   goto fail_unlock_connector;
+   return false;
}
 
mutex_lock(crtc-mutex);
@@ -8503,14 +8504,13 @@ bool intel_get_load_detect_pipe(struct drm_connector 
*connector,
else
intel_crtc-new_config = NULL;
mutex_unlock(crtc-mutex);
-fail_unlock_connector:
-   mutex_unlock(dev-mode_config.connection_mutex);
 
return false;
 }
 
-void intel_release_load_detect_pipe(struct drm_connector *connector,
-   struct intel_load_detect_pipe *old)
+static void
+__intel_release_load_detect_pipe(struct drm_connector *connector,
+struct intel_load_detect_pipe *old)
 {
struct intel_encoder *intel_encoder =
intel_attached_encoder(connector);
@@ -8518,6 +8518,8 @@ void intel_release_load_detect_pipe(struct drm_connector 
*connector,
struct drm_crtc *crtc = encoder-crtc;
struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 
+   lockdep_assert_held(connector-dev-mode_config.connection_mutex);
+
DRM_DEBUG_KMS([CONNECTOR:%d:%s], [ENCODER:%d:%s]\n,
  connector-base.id, connector-name,
  encoder-base.id, encoder-name);
@@ -8533,17 +8535,32 @@ void intel_release_load_detect_pipe(struct 
drm_connector *connector,
drm_framebuffer_unregister_private(old-release_fb);
drm_framebuffer_unreference(old-release_fb);
}
+   } else {
+   /* Switch crtc and encoder back off if necessary */
+   if (old-dpms_mode != DRM_MODE_DPMS_ON)
+   connector-funcs-dpms(connector, old-dpms_mode);
+   }
+   mutex_unlock(crtc-mutex);
+}
 
-   mutex_unlock(crtc-mutex);
+bool intel_get_load_detect_pipe(struct drm_connector *connector,
+   struct drm_display_mode *mode,
+   struct intel_load_detect_pipe *old)
+{
+   mutex_lock(connector-dev-mode_config.connection_mutex);
+   if (!__intel_get_load_detect_pipe(connector, mode, old)) {
mutex_unlock(connector-dev-mode_config.connection_mutex);
-   return;
+   return false;
}
 
-   /* Switch crtc and encoder back off if necessary */
-   if (old-dpms_mode != DRM_MODE_DPMS_ON)
-   connector-funcs-dpms(connector, old-dpms_mode);
+   /* lock will be released by intel_release_load_detect_pipe() */
+   return true;
+}
 
-   mutex_unlock(crtc-mutex);
+void intel_release_load_detect_pipe(struct drm_connector *connector,
+   struct intel_load_detect_pipe *old)
+{
+   __intel_release_load_detect_pipe(connector, old);

Re: [Intel-gfx] [PATCH] drm/i915: Avoid double mutex lock applying pipe A quirk during sanitize_crtc()

2014-06-09 Thread Ville Syrjälä 
On Mon, Jun 09, 2014 at 07:47:10AM +0100, Chris Wilson wrote:
 Thomas found that his machine would deadlock reloading the i915.ko
 module after resume. He identified that this was caused by the
 reacquisition of the connection mutex inside intel_enable_pipe_a()
 during the CRTC sanitization routine. This will only affect machines
 that quirk PIPE A, i.e. the original 830m chipsets.
 
 This patch move the locking into a wrapper function so that
 intel_enable_pipe_a() can bypass the locking knowing that it already
 holds the correct locks.

It can still try to grab crtc-mutex twice. Looks like Danial undid my
fix to not take all the modeset locks around
intel_modeset_setup_hw_state().

 
 Reported-by: Thomas Richter rich...@rus.uni-stuttgart.de
 Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
 Cc: Thomas Richter rich...@rus.uni-stuttgart.de
 Cc: Daniel Vetter dan...@ffwll.ch
 Cc: Ville Syrjälä ville.syrj...@linux.intel.com
 ---
  drivers/gpu/drm/i915/intel_display.c |   54 
 ++
  1 file changed, 35 insertions(+), 19 deletions(-)
 
 diff --git a/drivers/gpu/drm/i915/intel_display.c 
 b/drivers/gpu/drm/i915/intel_display.c
 index c1f79a1..26d3424 100644
 --- a/drivers/gpu/drm/i915/intel_display.c
 +++ b/drivers/gpu/drm/i915/intel_display.c
 @@ -8387,9 +8387,10 @@ mode_fits_in_fbdev(struct drm_device *dev,
  #endif
  }
  
 -bool intel_get_load_detect_pipe(struct drm_connector *connector,
 - struct drm_display_mode *mode,
 - struct intel_load_detect_pipe *old)
 +static bool
 +__intel_get_load_detect_pipe(struct drm_connector *connector,
 +  struct drm_display_mode *mode,
 +  struct intel_load_detect_pipe *old)
  {
   struct intel_crtc *intel_crtc;
   struct intel_encoder *intel_encoder =
 @@ -8405,7 +8406,7 @@ bool intel_get_load_detect_pipe(struct drm_connector 
 *connector,
 connector-base.id, connector-name,
 encoder-base.id, encoder-name);
  
 - mutex_lock(dev-mode_config.connection_mutex);
 + lockdep_assert_held(dev-mode_config.connection_mutex);
  
   /*
* Algorithm gets a little messy:
 @@ -8449,7 +8450,7 @@ bool intel_get_load_detect_pipe(struct drm_connector 
 *connector,
*/
   if (!crtc) {
   DRM_DEBUG_KMS(no pipe available for load-detect\n);
 - goto fail_unlock_connector;
 + return false;
   }
  
   mutex_lock(crtc-mutex);
 @@ -8503,14 +8504,13 @@ bool intel_get_load_detect_pipe(struct drm_connector 
 *connector,
   else
   intel_crtc-new_config = NULL;
   mutex_unlock(crtc-mutex);
 -fail_unlock_connector:
 - mutex_unlock(dev-mode_config.connection_mutex);
  
   return false;
  }
  
 -void intel_release_load_detect_pipe(struct drm_connector *connector,
 - struct intel_load_detect_pipe *old)
 +static void
 +__intel_release_load_detect_pipe(struct drm_connector *connector,
 +  struct intel_load_detect_pipe *old)
  {
   struct intel_encoder *intel_encoder =
   intel_attached_encoder(connector);
 @@ -8518,6 +8518,8 @@ void intel_release_load_detect_pipe(struct 
 drm_connector *connector,
   struct drm_crtc *crtc = encoder-crtc;
   struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
  
 + lockdep_assert_held(connector-dev-mode_config.connection_mutex);
 +
   DRM_DEBUG_KMS([CONNECTOR:%d:%s], [ENCODER:%d:%s]\n,
 connector-base.id, connector-name,
 encoder-base.id, encoder-name);
 @@ -8533,17 +8535,32 @@ void intel_release_load_detect_pipe(struct 
 drm_connector *connector,
   drm_framebuffer_unregister_private(old-release_fb);
   drm_framebuffer_unreference(old-release_fb);
   }
 + } else {
 + /* Switch crtc and encoder back off if necessary */
 + if (old-dpms_mode != DRM_MODE_DPMS_ON)
 + connector-funcs-dpms(connector, old-dpms_mode);
 + }
 + mutex_unlock(crtc-mutex);
 +}
  
 - mutex_unlock(crtc-mutex);
 +bool intel_get_load_detect_pipe(struct drm_connector *connector,
 + struct drm_display_mode *mode,
 + struct intel_load_detect_pipe *old)
 +{
 + mutex_lock(connector-dev-mode_config.connection_mutex);
 + if (!__intel_get_load_detect_pipe(connector, mode, old)) {
   mutex_unlock(connector-dev-mode_config.connection_mutex);
 - return;
 + return false;
   }
  
 - /* Switch crtc and encoder back off if necessary */
 - if (old-dpms_mode != DRM_MODE_DPMS_ON)
 - connector-funcs-dpms(connector, old-dpms_mode);
 + /* lock will be released by intel_release_load_detect_pipe() */
 + return true;
 +}
  
 - mutex_unlock(crtc-mutex);
 +void

Re: [Intel-gfx] [PATCH] drm/i915: Avoid double mutex lock applying pipe A quirk during sanitize_crtc()

2014-06-09 Thread Chris Wilson 
On Mon, Jun 09, 2014 at 11:30:26AM +0300, Ville Syrjälä wrote:
 On Mon, Jun 09, 2014 at 07:47:10AM +0100, Chris Wilson wrote:
  Thomas found that his machine would deadlock reloading the i915.ko
  module after resume. He identified that this was caused by the
  reacquisition of the connection mutex inside intel_enable_pipe_a()
  during the CRTC sanitization routine. This will only affect machines
  that quirk PIPE A, i.e. the original 830m chipsets.
  
  This patch move the locking into a wrapper function so that
  intel_enable_pipe_a() can bypass the locking knowing that it already
  holds the correct locks.
 
 It can still try to grab crtc-mutex twice. Looks like Danial undid my
 fix to not take all the modeset locks around
 intel_modeset_setup_hw_state().

Oh well, I only considered the santize_crtc path as I thought we would
have caught the modeset sequence deadlocking earlier.

Anyway, the locking is in flux due to the conversion to ww_mutex.
Have fun ;-)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx