[PATCH] drm/i915: grab struct_mutex at intel_edp_psr_enable()

[PATCH] drm/i915: grab struct_mutex at intel_edp_psr_enable()

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Otherwise we will hit "lockdep_assert_held(&dev->struct_mutex)"
from intel_edp_psr_match_conditions(). This happens all the time on my
BDW machine: just boot it and you'll get it.

In this patch I just grab struct_mutex in the deepest possible place
(around psr.lock, otherwise lockdep complains).

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 87d0489..a91142c 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1876,11 +1876,11 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp)
 		return;
 	}
 
+	mutex_lock(&dev->struct_mutex);
 	mutex_lock(&dev_priv->psr.lock);
 	if (dev_priv->psr.enabled) {
 		DRM_DEBUG_KMS("PSR already in use\n");
-		mutex_unlock(&dev_priv->psr.lock);
-		return;
+		goto out;
 	}
 
 	dev_priv->psr.busy_frontbuffer_bits = 0;
@@ -1890,7 +1890,10 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp)
 
 	if (intel_edp_psr_match_conditions(intel_dp))
 		dev_priv->psr.enabled = intel_dp;
+
+out:
 	mutex_unlock(&dev_priv->psr.lock);
+	mutex_unlock(&dev->struct_mutex);
 }
 
 void intel_edp_psr_disable(struct intel_dp *intel_dp)
-- 
2.0.1

[PATCH] drm/i915: Don't require dev->struct_mutex in psr_match_conditions

[Daniel Vetter]

Since I've reworked psr support to no longer require x-tiling we don't
check any state protected by the Giant GEM Lock. So drop that check.

Also boo for lockdep_assert_held for not yelling when lockdep is
disabled.

Cc: Paulo Zanoni <przanoni@gmail.com>
Reported-by: Paulo Zanoni <przanoni@gmail.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_dp.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 3e6100ea7295..6dbe0f84d455 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1773,7 +1773,6 @@ static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp)
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 
 	lockdep_assert_held(&dev_priv->psr.lock);
-	lockdep_assert_held(&dev->struct_mutex);
 	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
 	WARN_ON(!drm_modeset_is_locked(&crtc->mutex));
 
-- 
2.0.1

Re: [PATCH] drm/i915: Don't require dev->struct_mutex in psr_match_conditions

[Paulo Zanoni]

2014-08-04 5:46 GMT-03:00 Daniel Vetter <daniel.vetter@ffwll.ch>:
> Since I've reworked psr support to no longer require x-tiling we don't
> check any state protected by the Giant GEM Lock. So drop that check.
>
> Also boo for lockdep_assert_held for not yelling when lockdep is
> disabled.
>
> Cc: Paulo Zanoni <przanoni@gmail.com>
> Reported-by: Paulo Zanoni <przanoni@gmail.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

I was going to start reviewing it, but then I realized it's already merged.

Do we have any doc explaining all our locks/mutexes and what each one
is supposed to protect?

Anyway, the patch looks fine.


> ---
>  drivers/gpu/drm/i915/intel_dp.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 3e6100ea7295..6dbe0f84d455 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1773,7 +1773,6 @@ static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp)
>         struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>
>         lockdep_assert_held(&dev_priv->psr.lock);
> -       lockdep_assert_held(&dev->struct_mutex);
>         WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
>         WARN_ON(!drm_modeset_is_locked(&crtc->mutex));
>
> --
> 2.0.1
>



-- 
Paulo Zanoni

Re: [PATCH] drm/i915: Don't require dev->struct_mutex in psr_match_conditions

[Daniel Vetter]

On Mon, Aug 04, 2014 at 11:16:10AM -0300, Paulo Zanoni wrote:
> 2014-08-04 5:46 GMT-03:00 Daniel Vetter <daniel.vetter@ffwll.ch>:
> > Since I've reworked psr support to no longer require x-tiling we don't
> > check any state protected by the Giant GEM Lock. So drop that check.
> >
> > Also boo for lockdep_assert_held for not yelling when lockdep is
> > disabled.
> >
> > Cc: Paulo Zanoni <przanoni@gmail.com>
> > Reported-by: Paulo Zanoni <przanoni@gmail.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> I was going to start reviewing it, but then I realized it's already merged.

Oh, I've figured I'll sneak this one by the "danvet must have reviewed-by
too" rule ;-) But I'll drop such patches asap if anyone spots something
with them ofc.

> Do we have any doc explaining all our locks/mutexes and what each one
> is supposed to protect?

Unfortunately not. It's also constantly changing (e.g. the recent
introduction of the connection_mutex) and rather shockingly often not
quite correct. Atm you need to dig through git history and for drm core
locks through all drm drivers to figure this out :(

> Anyway, the patch looks fine.

I'll count this as an ack and added it, thanks.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch