Re: Possible regression in 3.17-rc2 in i915 driver

Re: Possible regression in 3.17-rc2 in i915 driver

[Jani Nikula]

+intel-gfx

Ville, Daniel, any thoughts before we queue a revert?

BR,
Jani.


On Sun, 31 Aug 2014, Tibor Billes <tbilles@gmx.com> wrote:
> Hi!
>
> I tried to upgrade my kernel from 3.16 to 3.17-rc2 and I found that my laptop
> was unable to boot. The boot process hangs after 2-3 seconds (according to
> timestamps of log messages). The last kernel log line I usually see is the
> discovery of my touchpad: "input: SynPS/2 Synaptics TouchPad as
> /devices/platform/i8042/serio2/input/input11".
>
> I did a git bisect and it pointed me to the following commit:
> commit 208bf9fdcd3575aa4a5d48b3e0295f7cdaf6fc44
> Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Date:   Mon Aug 11 13:15:35 2014 +0300
>  
>     drm/i915: Fix locking for intel_enable_pipe_a()
>  
>     intel_enable_pipe_a() gets called with all the modeset locks already
>     held (by drm_modeset_lock_all()), so trying to grab the same
>     locks using another drm_modeset_acquire_ctx is going to fail miserably.
>      
>     Move most of the drm_modeset_acquire_ctx handling (init/drop/fini)
>     out from intel_{get,release}_load_detect_pipe() into the callers
>     (intel_{crt,tv}_detect()). Only the actual locking and backoff
>     handling is left in intel_get_load_detect_pipe(). And in
>     intel_enable_pipe_a() we just share the mode_config.acquire_ctx from
>     drm_modeset_lock_all() which is already holding all the relevant locks.
>      
>     It's perfectly legal to lock the same ww_mutex multiple times using the
>     same ww_acquire_ctx. drm_modeset_lock() will convert the returned
>     -EALREADY into 0, so the caller doesn't need to do antyhing special.
>      
>     Fixes a hang on resume on my 830.
>      
>     Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>     Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>     Cc: stable@vger.kernel.org
>     Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>  
> I tried booting with the above commit reverted on top of 3.17-rc2 and it
> booted successfully.
>  
> I also did a MagicSyrq-W (Display list of blocked (D state) tasks) after the
> boot process stopped (using plain 3.17-rc2, without reverting the above commit)
> and it showed that plymouthd was blocked with the following call trace (hand
> copied):
> irq_exit
> common_interrupt
> schedule_preemt_disabled
> __mutex_lock_slowpath
> mutex_lock
> drm_modeset_lock
> drm_getconnector
> drm_mode_getcrttc
> drm_ioctl
> ...
>  
> This may have something to do with the hang or it may not, I don't know but is
> related to drm locking so I thought it was a good idea to mention it.
>  
> My laptop is an old Fujitsu-Siemens Amilo M1450g, running Linux Mint 16.
>  
> Let me know if I can help further debug this issue.
>  
> Tibor Billes

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Fix lock dropping in intel_tv_detect()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

When intel_tv_detect() fails to do load detection it would forget to
drop the locks and clean up the acquire context. Fix it up.

This is a regression from:
 commit 208bf9fdcd3575aa4a5d48b3e0295f7cdaf6fc44
 Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
 Date:   Mon Aug 11 13:15:35 2014 +0300

    drm/i915: Fix locking for intel_enable_pipe_a()

Cc: Tibor Billes <tbilles@gmx.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_tv.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
index 32186a6..abbf0ea 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -1311,7 +1311,8 @@ intel_tv_detect(struct drm_connector *connector, bool force)
 {
 	struct drm_display_mode mode;
 	struct intel_tv *intel_tv = intel_attached_tv(connector);
-	int type;
+	enum drm_connector_status status = connector->status;
+	int type = -1;
 
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s] force=%d\n",
 		      connector->base.id, connector->name,
@@ -1328,21 +1329,23 @@ intel_tv_detect(struct drm_connector *connector, bool force)
 		if (intel_get_load_detect_pipe(connector, &mode, &tmp, &ctx)) {
 			type = intel_tv_detect_type(intel_tv, connector);
 			intel_release_load_detect_pipe(connector, &tmp);
+			status = type < 0 ?
+				connector_status_disconnected :
+				connector_status_connected;
 		} else
-			return connector_status_unknown;
+			status = connector_status_unknown;
 
 		drm_modeset_drop_locks(&ctx);
 		drm_modeset_acquire_fini(&ctx);
-	} else
-		return connector->status;
+	}
 
 	if (type < 0)
-		return connector_status_disconnected;
+		return status;
 
 	intel_tv->type = type;
 	intel_tv_find_better_format(connector);
 
-	return connector_status_connected;
+	return status;
 }
 
 static const struct input_res {
-- 
1.8.5.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Fix lock dropping in intel_tv_detect()

[Chris Wilson]

On Mon, Sep 01, 2014 at 11:09:46AM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> When intel_tv_detect() fails to do load detection it would forget to
> drop the locks and clean up the acquire context. Fix it up.
> 
> This is a regression from:
>  commit 208bf9fdcd3575aa4a5d48b3e0295f7cdaf6fc44
>  Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
>  Date:   Mon Aug 11 13:15:35 2014 +0300
> 
>     drm/i915: Fix locking for intel_enable_pipe_a()
> 
> Cc: Tibor Billes <tbilles@gmx.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_tv.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
> index 32186a6..abbf0ea 100644
> --- a/drivers/gpu/drm/i915/intel_tv.c
> +++ b/drivers/gpu/drm/i915/intel_tv.c
> @@ -1311,7 +1311,8 @@ intel_tv_detect(struct drm_connector *connector, bool force)
>  {
>  	struct drm_display_mode mode;
>  	struct intel_tv *intel_tv = intel_attached_tv(connector);
> -	int type;
> +	enum drm_connector_status status = connector->status;
> +	int type = -1;
>  
>  	DRM_DEBUG_KMS("[CONNECTOR:%d:%s] force=%d\n",
>  		      connector->base.id, connector->name,
> @@ -1328,21 +1329,23 @@ intel_tv_detect(struct drm_connector *connector, bool force)
>  		if (intel_get_load_detect_pipe(connector, &mode, &tmp, &ctx)) {
>  			type = intel_tv_detect_type(intel_tv, connector);
>  			intel_release_load_detect_pipe(connector, &tmp);
> +			status = type < 0 ?
> +				connector_status_disconnected :
> +				connector_status_connected;
>  		} else
> -			return connector_status_unknown;
> +			status = connector_status_unknown;
>  
>  		drm_modeset_drop_locks(&ctx);
>  		drm_modeset_acquire_fini(&ctx);
> -	} else
> -		return connector->status;
> +	}
>  
>  	if (type < 0)
> -		return connector_status_disconnected;
> +		return status;
>  
>  	intel_tv->type = type;
>  	intel_tv_find_better_format(connector);
>  
> -	return connector_status_connected;
> +	return status;

Hmm, this makes the code unclear. if type != -1, status should always be
connected.

I think:

if (status != connector_status_connected)
  return status;

if (WARN_ON(type < 0))
   return connector_status_disconnected;

intel_tv->type = type;
find_better_format();
return connector_status_connected;

And leave type unset to encourage gcc.



diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
index 32186a6..a109b7b 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -1311,6 +1311,7 @@ intel_tv_detect(struct drm_connector *connector, bool force)
 {
        struct drm_display_mode mode;
        struct intel_tv *intel_tv = intel_attached_tv(connector);
+       enum drm_connector_status status = connector->status;
        int type;
 
        DRM_DEBUG_KMS("[CONNECTOR:%d:%s] force=%d\n",
@@ -1327,17 +1328,20 @@ intel_tv_detect(struct drm_connector *connector, bool force)
 
                if (intel_get_load_detect_pipe(connector, &mode, &tmp, &ctx)) {
                        type = intel_tv_detect_type(intel_tv, connector);
+                       status = type < 0 ?
+                               connector_status_disconnected :
+                               connector_status_connected;
+
                        intel_release_load_detect_pipe(connector, &tmp);
                } else
-                       return connector_status_unknown;
+                       status = connector_status_unknown;
 
                drm_modeset_drop_locks(&ctx);
                drm_modeset_acquire_fini(&ctx);
-       } else
-               return connector->status;
+       }
 
-       if (type < 0)
-               return connector_status_disconnected;
+       if (status != connector_status_connected)
+               return status;
 
        intel_tv->type = type;
        intel_tv_find_better_format(connector);

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Fix lock dropping in intel_tv_detect()

[Ville Syrjälä]

On Mon, Sep 01, 2014 at 09:50:22AM +0100, Chris Wilson wrote:
> On Mon, Sep 01, 2014 at 11:09:46AM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > When intel_tv_detect() fails to do load detection it would forget to
> > drop the locks and clean up the acquire context. Fix it up.
> > 
> > This is a regression from:
> >  commit 208bf9fdcd3575aa4a5d48b3e0295f7cdaf6fc44
> >  Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >  Date:   Mon Aug 11 13:15:35 2014 +0300
> > 
> >     drm/i915: Fix locking for intel_enable_pipe_a()
> > 
> > Cc: Tibor Billes <tbilles@gmx.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_tv.c | 15 +++++++++------
> >  1 file changed, 9 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
> > index 32186a6..abbf0ea 100644
> > --- a/drivers/gpu/drm/i915/intel_tv.c
> > +++ b/drivers/gpu/drm/i915/intel_tv.c
> > @@ -1311,7 +1311,8 @@ intel_tv_detect(struct drm_connector *connector, bool force)
> >  {
> >  	struct drm_display_mode mode;
> >  	struct intel_tv *intel_tv = intel_attached_tv(connector);
> > -	int type;
> > +	enum drm_connector_status status = connector->status;
> > +	int type = -1;
> >  
> >  	DRM_DEBUG_KMS("[CONNECTOR:%d:%s] force=%d\n",
> >  		      connector->base.id, connector->name,
> > @@ -1328,21 +1329,23 @@ intel_tv_detect(struct drm_connector *connector, bool force)
> >  		if (intel_get_load_detect_pipe(connector, &mode, &tmp, &ctx)) {
> >  			type = intel_tv_detect_type(intel_tv, connector);
> >  			intel_release_load_detect_pipe(connector, &tmp);
> > +			status = type < 0 ?
> > +				connector_status_disconnected :
> > +				connector_status_connected;
> >  		} else
> > -			return connector_status_unknown;
> > +			status = connector_status_unknown;
> >  
> >  		drm_modeset_drop_locks(&ctx);
> >  		drm_modeset_acquire_fini(&ctx);
> > -	} else
> > -		return connector->status;
> > +	}
> >  
> >  	if (type < 0)
> > -		return connector_status_disconnected;
> > +		return status;
> >  
> >  	intel_tv->type = type;
> >  	intel_tv_find_better_format(connector);
> >  
> > -	return connector_status_connected;
> > +	return status;
> 
> Hmm, this makes the code unclear. if type != -1, status should always be
> connected.
> 
> I think:
> 
> if (status != connector_status_connected)
>   return status;
> 
> if (WARN_ON(type < 0))
>    return connector_status_disconnected;
> 
> intel_tv->type = type;
> find_better_format();
> return connector_status_connected;
> 
> And leave type unset to encourage gcc.
> 
> 
> 
> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
> index 32186a6..a109b7b 100644
> --- a/drivers/gpu/drm/i915/intel_tv.c
> +++ b/drivers/gpu/drm/i915/intel_tv.c
> @@ -1311,6 +1311,7 @@ intel_tv_detect(struct drm_connector *connector, bool force)
>  {
>         struct drm_display_mode mode;
>         struct intel_tv *intel_tv = intel_attached_tv(connector);
> +       enum drm_connector_status status = connector->status;
>         int type;
>  
>         DRM_DEBUG_KMS("[CONNECTOR:%d:%s] force=%d\n",
> @@ -1327,17 +1328,20 @@ intel_tv_detect(struct drm_connector *connector, bool force)
>  
>                 if (intel_get_load_detect_pipe(connector, &mode, &tmp, &ctx)) {
>                         type = intel_tv_detect_type(intel_tv, connector);
> +                       status = type < 0 ?
> +                               connector_status_disconnected :
> +                               connector_status_connected;
> +
>                         intel_release_load_detect_pipe(connector, &tmp);
>                 } else
> -                       return connector_status_unknown;
> +                       status = connector_status_unknown;
>  
>                 drm_modeset_drop_locks(&ctx);
>                 drm_modeset_acquire_fini(&ctx);
> -       } else
> -               return connector->status;
> +       }
>  
> -       if (type < 0)
> -               return connector_status_disconnected;
> +       if (status != connector_status_connected)
> +               return status;
>  
>         intel_tv->type = type;
>         intel_tv_find_better_format(connector);

With this approach we're going to have to keep the !force else branch
to avoid populating intel_tv->type with stack garbage. But I suppose
the resulting code might still be a bit clearer.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm/i915: Fix lock dropping in intel_tv_detect()

[Chris Wilson]

On Mon, Sep 01, 2014 at 12:45:56PM +0300, Ville Syrjälä wrote:
> On Mon, Sep 01, 2014 at 09:50:22AM +0100, Chris Wilson wrote:
> > diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
> > index 32186a6..a109b7b 100644
> > --- a/drivers/gpu/drm/i915/intel_tv.c
> > +++ b/drivers/gpu/drm/i915/intel_tv.c
> > @@ -1311,6 +1311,7 @@ intel_tv_detect(struct drm_connector *connector, bool force)
> >  {
> >         struct drm_display_mode mode;
> >         struct intel_tv *intel_tv = intel_attached_tv(connector);
> > +       enum drm_connector_status status = connector->status;
> >         int type;
> >  
> >         DRM_DEBUG_KMS("[CONNECTOR:%d:%s] force=%d\n",
> > @@ -1327,17 +1328,20 @@ intel_tv_detect(struct drm_connector *connector, bool force)
> >  
> >                 if (intel_get_load_detect_pipe(connector, &mode, &tmp, &ctx)) {
> >                         type = intel_tv_detect_type(intel_tv, connector);
> > +                       status = type < 0 ?
> > +                               connector_status_disconnected :
> > +                               connector_status_connected;
> > +
> >                         intel_release_load_detect_pipe(connector, &tmp);
> >                 } else
> > -                       return connector_status_unknown;
> > +                       status = connector_status_unknown;
> >  
> >                 drm_modeset_drop_locks(&ctx);
> >                 drm_modeset_acquire_fini(&ctx);
> > -       } else
> > -               return connector->status;
> > +       }
> >  
> > -       if (type < 0)
> > -               return connector_status_disconnected;
> > +       if (status != connector_status_connected)
> > +               return status;
> >  
> >         intel_tv->type = type;
> >         intel_tv_find_better_format(connector);
> 
> With this approach we're going to have to keep the !force else branch
> to avoid populating intel_tv->type with stack garbage. But I suppose
> the resulting code might still be a bit clearer.

Or just set intel_tv->type directly inside

status = intel_tv_detect_type() ?

Hmm. Indeed, we should not be touching them at all for !forced, as type
is only set for forced, and so we can similarly ignore hpd of better
formats.

diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
index 32186a656816..e80eac5e5239 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -1169,7 +1169,7 @@ static const struct drm_display_mode reported_modes[] = {
  * \return true if TV is connected.
  * \return false if TV is disconnected.
  */
-static int
+static enum drm_connector_status
 intel_tv_detect_type(struct intel_tv *intel_tv,
                      struct drm_connector *connector)
 {
@@ -1178,10 +1178,10 @@ intel_tv_detect_type(struct intel_tv *intel_tv,
        struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
        struct drm_device *dev = encoder->dev;
        struct drm_i915_private *dev_priv = dev->dev_private;
+       enum drm_connector_status status;
        unsigned long irqflags;
        u32 tv_ctl, save_tv_ctl;
        u32 tv_dac, save_tv_dac;
-       int type;
 
        /* Disable TV interrupts around load detect or we'll recurse */
        if (connector->polled & DRM_CONNECTOR_POLL_HPD) {
@@ -1229,7 +1229,6 @@ intel_tv_detect_type(struct intel_tv *intel_tv,
        intel_wait_for_vblank(intel_tv->base.base.dev,
                              to_intel_crtc(intel_tv->base.base.crtc)->pipe);
 
-       type = -1;
        tv_dac = I915_READ(TV_DAC);
        DRM_DEBUG_KMS("TV detected: %x, %x\n", tv_ctl, tv_dac);
        /*
@@ -1238,18 +1237,19 @@ intel_tv_detect_type(struct intel_tv *intel_tv,
         *  1 0 X svideo
         *  0 0 0 Component
         */
+       status = connector_status_connected;
        if ((tv_dac & TVDAC_SENSE_MASK) == (TVDAC_B_SENSE | TVDAC_C_SENSE)) {
                DRM_DEBUG_KMS("Detected Composite TV connection\n");
-               type = DRM_MODE_CONNECTOR_Composite;
+               intel_tv->type = DRM_MODE_CONNECTOR_Composite;
        } else if ((tv_dac & (TVDAC_A_SENSE|TVDAC_B_SENSE)) == TVDAC_A_SENSE) {
                DRM_DEBUG_KMS("Detected S-Video TV connection\n");
-               type = DRM_MODE_CONNECTOR_SVIDEO;
+               intel_tv->type = DRM_MODE_CONNECTOR_SVIDEO;
        } else if ((tv_dac & TVDAC_SENSE_MASK) == 0) {
                DRM_DEBUG_KMS("Detected Component TV connection\n");
-               type = DRM_MODE_CONNECTOR_Component;
+               intel_tv->type = DRM_MODE_CONNECTOR_Component;
        } else {
                DRM_DEBUG_KMS("Unrecognised TV connection\n");
-               type = -1;
+               status = connector_status_disconnected;
        }
 
        I915_WRITE(TV_DAC, save_tv_dac & ~TVDAC_STATE_CHG_EN);
@@ -1269,7 +1269,7 @@ intel_tv_detect_type(struct intel_tv *intel_tv,
                spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
        }
 
-       return type;
+       return status;
 }
 
 /*
@@ -1309,39 +1309,33 @@ static void intel_tv_find_better_format(struct drm_connector *connector)
 static enum drm_connector_status
 intel_tv_detect(struct drm_connector *connector, bool force)
 {
-       struct drm_display_mode mode;
        struct intel_tv *intel_tv = intel_attached_tv(connector);
-       int type;
+       struct drm_display_mode mode = reported_modes[0];
+       struct intel_load_detect_pipe tmp;
+       struct drm_modeset_acquire_ctx ctx;
+       enum drm_connector_status status;
 
        DRM_DEBUG_KMS("[CONNECTOR:%d:%s] force=%d\n",
                      connector->base.id, connector->name,
                      force);
+       if (!force)
+               return connector->status;
 
-       mode = reported_modes[0];
-
-       if (force) {
-               struct intel_load_detect_pipe tmp;
-               struct drm_modeset_acquire_ctx ctx;
-
-               drm_modeset_acquire_init(&ctx, 0);
-
-               if (intel_get_load_detect_pipe(connector, &mode, &tmp, &ctx)) {
-                       type = intel_tv_detect_type(intel_tv, connector);
-                       intel_release_load_detect_pipe(connector, &tmp);
-               } else
-                       return connector_status_unknown;
+       drm_modeset_acquire_init(&ctx, 0);
 
-               drm_modeset_drop_locks(&ctx);
-               drm_modeset_acquire_fini(&ctx);
+       if (intel_get_load_detect_pipe(connector, &mode, &tmp, &ctx)) {
+               status = intel_tv_detect_type(intel_tv, connector);
+               intel_release_load_detect_pipe(connector, &tmp);
        } else
-               return connector->status;
+               status = connector_status_unknown;
 
-       if (type < 0)
-               return connector_status_disconnected;
+       drm_modeset_drop_locks(&ctx);
+       drm_modeset_acquire_fini(&ctx);
 
-       intel_tv->type = type;
-       intel_tv_find_better_format(connector);
+       if (status != connector
+               return status;
 
+       intel_tv_find_better_format(connector);
        return connector_status_connected;
 }


-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH v2] drm/i915: Fix lock dropping in intel_tv_detect()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

When intel_tv_detect() fails to do load detection it would forget to
drop the locks and clean up the acquire context. Fix it up.

This is a regression from:
 commit 208bf9fdcd3575aa4a5d48b3e0295f7cdaf6fc44
 Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
 Date:   Mon Aug 11 13:15:35 2014 +0300

    drm/i915: Fix locking for intel_enable_pipe_a()

v2: Make the code more readable (Chris)

Cc: Tibor Billes <tbilles@gmx.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_tv.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
index 32186a6..c6b00f20 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -1311,6 +1311,7 @@ intel_tv_detect(struct drm_connector *connector, bool force)
 {
 	struct drm_display_mode mode;
 	struct intel_tv *intel_tv = intel_attached_tv(connector);
+	enum drm_connector_status status;
 	int type;
 
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s] force=%d\n",
@@ -1328,15 +1329,21 @@ intel_tv_detect(struct drm_connector *connector, bool force)
 		if (intel_get_load_detect_pipe(connector, &mode, &tmp, &ctx)) {
 			type = intel_tv_detect_type(intel_tv, connector);
 			intel_release_load_detect_pipe(connector, &tmp);
+			status = type < 0 ?
+				connector_status_disconnected :
+				connector_status_connected;
 		} else
-			return connector_status_unknown;
+			status = connector_status_unknown;
 
 		drm_modeset_drop_locks(&ctx);
 		drm_modeset_acquire_fini(&ctx);
 	} else
 		return connector->status;
 
-	if (type < 0)
+	if (status != connector_status_connected)
+		return status;
+
+	if (WARN_ON(type < 0))
 		return connector_status_disconnected;
 
 	intel_tv->type = type;
-- 
1.8.5.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Fix lock dropping in intel_tv_detect()

[Chris Wilson]

On Mon, Sep 01, 2014 at 01:07:40PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> When intel_tv_detect() fails to do load detection it would forget to
> drop the locks and clean up the acquire context. Fix it up.
> 
> This is a regression from:
>  commit 208bf9fdcd3575aa4a5d48b3e0295f7cdaf6fc44
>  Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
>  Date:   Mon Aug 11 13:15:35 2014 +0300
> 
>     drm/i915: Fix locking for intel_enable_pipe_a()
> 
> v2: Make the code more readable (Chris)
> 
> Cc: Tibor Billes <tbilles@gmx.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Hmm, if we use WARN_ON() you should init type.

Otherwise,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chrsi

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v2] drm/i915: Fix lock dropping in intel_tv_detect()

[Ville Syrjälä]

On Mon, Sep 01, 2014 at 11:20:09AM +0100, Chris Wilson wrote:
> On Mon, Sep 01, 2014 at 01:07:40PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > When intel_tv_detect() fails to do load detection it would forget to
> > drop the locks and clean up the acquire context. Fix it up.
> > 
> > This is a regression from:
> >  commit 208bf9fdcd3575aa4a5d48b3e0295f7cdaf6fc44
> >  Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >  Date:   Mon Aug 11 13:15:35 2014 +0300
> > 
> >     drm/i915: Fix locking for intel_enable_pipe_a()
> > 
> > v2: Make the code more readable (Chris)
> > 
> > Cc: Tibor Billes <tbilles@gmx.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Hmm, if we use WARN_ON() you should init type.

type is always set in the branch that sets status=connected.

> 
> Otherwise,
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> -Chrsi
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v2] drm/i915: Fix lock dropping in intel_tv_detect()

[Chris Wilson]

On Mon, Sep 01, 2014 at 01:36:37PM +0300, Ville Syrjälä wrote:
> On Mon, Sep 01, 2014 at 11:20:09AM +0100, Chris Wilson wrote:
> > On Mon, Sep 01, 2014 at 01:07:40PM +0300, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > When intel_tv_detect() fails to do load detection it would forget to
> > > drop the locks and clean up the acquire context. Fix it up.
> > > 
> > > This is a regression from:
> > >  commit 208bf9fdcd3575aa4a5d48b3e0295f7cdaf6fc44
> > >  Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >  Date:   Mon Aug 11 13:15:35 2014 +0300
> > > 
> > >     drm/i915: Fix locking for intel_enable_pipe_a()
> > > 
> > > v2: Make the code more readable (Chris)
> > > 
> > > Cc: Tibor Billes <tbilles@gmx.com>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Hmm, if we use WARN_ON() you should init type.
> 
> type is always set in the branch that sets status=connected.

Back to thinking about readability and making sure that the WARN_ON
never happens with just a glance. Otherwise, the WARN_ON would be better
as WARN_ON(unsigned)type >= last_tv_type); Or something. Anway, take
your pick and slap my r-b on it. :)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH v2] drm/i915: Fix lock dropping in intel_tv_detect()

[Jani Nikula]

On Mon, 01 Sep 2014, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Mon, Sep 01, 2014 at 01:36:37PM +0300, Ville Syrjälä wrote:
>> On Mon, Sep 01, 2014 at 11:20:09AM +0100, Chris Wilson wrote:
>> > On Mon, Sep 01, 2014 at 01:07:40PM +0300, ville.syrjala@linux.intel.com wrote:
>> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > > 
>> > > When intel_tv_detect() fails to do load detection it would forget to
>> > > drop the locks and clean up the acquire context. Fix it up.
>> > > 
>> > > This is a regression from:
>> > >  commit 208bf9fdcd3575aa4a5d48b3e0295f7cdaf6fc44
>> > >  Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > >  Date:   Mon Aug 11 13:15:35 2014 +0300
>> > > 
>> > >     drm/i915: Fix locking for intel_enable_pipe_a()
>> > > 
>> > > v2: Make the code more readable (Chris)
>> > > 
>> > > Cc: Tibor Billes <tbilles@gmx.com>
>> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > 
>> > Hmm, if we use WARN_ON() you should init type.
>> 
>> type is always set in the branch that sets status=connected.
>
> Back to thinking about readability and making sure that the WARN_ON
> never happens with just a glance. Otherwise, the WARN_ON would be better
> as WARN_ON(unsigned)type >= last_tv_type); Or something. Anway, take
> your pick and slap my r-b on it. :)

Ville?

J.


> -Chris
>
> -- 
> Chris Wilson, Intel Open Source Technology Centre

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Fix lock dropping in intel_tv_detect()

[Ville Syrjälä]

On Tue, Sep 02, 2014 at 10:41:05AM +0300, Jani Nikula wrote:
> On Mon, 01 Sep 2014, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Mon, Sep 01, 2014 at 01:36:37PM +0300, Ville Syrjälä wrote:
> >> On Mon, Sep 01, 2014 at 11:20:09AM +0100, Chris Wilson wrote:
> >> > On Mon, Sep 01, 2014 at 01:07:40PM +0300, ville.syrjala@linux.intel.com wrote:
> >> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> > > 
> >> > > When intel_tv_detect() fails to do load detection it would forget to
> >> > > drop the locks and clean up the acquire context. Fix it up.
> >> > > 
> >> > > This is a regression from:
> >> > >  commit 208bf9fdcd3575aa4a5d48b3e0295f7cdaf6fc44
> >> > >  Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> > >  Date:   Mon Aug 11 13:15:35 2014 +0300
> >> > > 
> >> > >     drm/i915: Fix locking for intel_enable_pipe_a()
> >> > > 
> >> > > v2: Make the code more readable (Chris)
> >> > > 
> >> > > Cc: Tibor Billes <tbilles@gmx.com>
> >> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> > 
> >> > Hmm, if we use WARN_ON() you should init type.
> >> 
> >> type is always set in the branch that sets status=connected.
> >
> > Back to thinking about readability and making sure that the WARN_ON
> > never happens with just a glance. Otherwise, the WARN_ON would be better
> > as WARN_ON(unsigned)type >= last_tv_type); Or something. Anway, take
> > your pick and slap my r-b on it. :)
> 
> Ville?

I don't know anymore. Just kill the WARN_ON() if it makes things
confusing?

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v2] drm/i915: Fix lock dropping in intel_tv_detect()

[Chris Wilson]

On Tue, Sep 02, 2014 at 11:16:12AM +0300, Ville Syrjälä wrote:
> On Tue, Sep 02, 2014 at 10:41:05AM +0300, Jani Nikula wrote:
> > On Mon, 01 Sep 2014, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > On Mon, Sep 01, 2014 at 01:36:37PM +0300, Ville Syrjälä wrote:
> > >> On Mon, Sep 01, 2014 at 11:20:09AM +0100, Chris Wilson wrote:
> > >> > On Mon, Sep 01, 2014 at 01:07:40PM +0300, ville.syrjala@linux.intel.com wrote:
> > >> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >> > > 
> > >> > > When intel_tv_detect() fails to do load detection it would forget to
> > >> > > drop the locks and clean up the acquire context. Fix it up.
> > >> > > 
> > >> > > This is a regression from:
> > >> > >  commit 208bf9fdcd3575aa4a5d48b3e0295f7cdaf6fc44
> > >> > >  Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >> > >  Date:   Mon Aug 11 13:15:35 2014 +0300
> > >> > > 
> > >> > >     drm/i915: Fix locking for intel_enable_pipe_a()
> > >> > > 
> > >> > > v2: Make the code more readable (Chris)
> > >> > > 
> > >> > > Cc: Tibor Billes <tbilles@gmx.com>
> > >> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >> > 
> > >> > Hmm, if we use WARN_ON() you should init type.
> > >> 
> > >> type is always set in the branch that sets status=connected.
> > >
> > > Back to thinking about readability and making sure that the WARN_ON
> > > never happens with just a glance. Otherwise, the WARN_ON would be better
> > > as WARN_ON(unsigned)type >= last_tv_type); Or something. Anway, take
> > > your pick and slap my r-b on it. :)
> > 
> > Ville?
> 
> I don't know anymore. Just kill the WARN_ON() if it makes things
> confusing?

Just drop the WARN_ON. I prefer the if() using the status rather than
type, as that seems more idiomatic (when looking at our other detection
routines).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH v3] drm/i915: Fix lock dropping in intel_tv_detect()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

When intel_tv_detect() fails to do load detection it would forget to
drop the locks and clean up the acquire context. Fix it up.

This is a regression from:
 commit 208bf9fdcd3575aa4a5d48b3e0295f7cdaf6fc44
 Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
 Date:   Mon Aug 11 13:15:35 2014 +0300

    drm/i915: Fix locking for intel_enable_pipe_a()

v2: Make the code more readable (Chris)
v3: Drop WARN_ON(type < 0) (Chris)

Cc: stable@vger.kernel.org
Cc: Tibor Billes <tbilles@gmx.com>
Reported-by: Tibor Billes <tbilles@gmx.com>
Tested-by: Tibor Billes <tbilles@gmx.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_tv.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
index 32186a6..c69d3ce 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -1311,6 +1311,7 @@ intel_tv_detect(struct drm_connector *connector, bool force)
 {
 	struct drm_display_mode mode;
 	struct intel_tv *intel_tv = intel_attached_tv(connector);
+	enum drm_connector_status status;
 	int type;
 
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s] force=%d\n",
@@ -1328,16 +1329,19 @@ intel_tv_detect(struct drm_connector *connector, bool force)
 		if (intel_get_load_detect_pipe(connector, &mode, &tmp, &ctx)) {
 			type = intel_tv_detect_type(intel_tv, connector);
 			intel_release_load_detect_pipe(connector, &tmp);
+			status = type < 0 ?
+				connector_status_disconnected :
+				connector_status_connected;
 		} else
-			return connector_status_unknown;
+			status = connector_status_unknown;
 
 		drm_modeset_drop_locks(&ctx);
 		drm_modeset_acquire_fini(&ctx);
 	} else
 		return connector->status;
 
-	if (type < 0)
-		return connector_status_disconnected;
+	if (status != connector_status_connected)
+		return status;
 
 	intel_tv->type = type;
 	intel_tv_find_better_format(connector);
-- 
1.8.5.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915: Fix lock dropping in intel_tv_detect()

[Jani Nikula]

On Tue, 02 Sep 2014, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> When intel_tv_detect() fails to do load detection it would forget to
> drop the locks and clean up the acquire context. Fix it up.
>
> This is a regression from:
>  commit 208bf9fdcd3575aa4a5d48b3e0295f7cdaf6fc44
>  Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
>  Date:   Mon Aug 11 13:15:35 2014 +0300
>
>     drm/i915: Fix locking for intel_enable_pipe_a()
>
> v2: Make the code more readable (Chris)
> v3: Drop WARN_ON(type < 0) (Chris)
>
> Cc: stable@vger.kernel.org
> Cc: Tibor Billes <tbilles@gmx.com>
> Reported-by: Tibor Billes <tbilles@gmx.com>
> Tested-by: Tibor Billes <tbilles@gmx.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Pushed to drm-intel-fixes, thanks for the patch, review, and testing.

BR,
Jani.


> ---
>  drivers/gpu/drm/i915/intel_tv.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
> index 32186a6..c69d3ce 100644
> --- a/drivers/gpu/drm/i915/intel_tv.c
> +++ b/drivers/gpu/drm/i915/intel_tv.c
> @@ -1311,6 +1311,7 @@ intel_tv_detect(struct drm_connector *connector, bool force)
>  {
>  	struct drm_display_mode mode;
>  	struct intel_tv *intel_tv = intel_attached_tv(connector);
> +	enum drm_connector_status status;
>  	int type;
>  
>  	DRM_DEBUG_KMS("[CONNECTOR:%d:%s] force=%d\n",
> @@ -1328,16 +1329,19 @@ intel_tv_detect(struct drm_connector *connector, bool force)
>  		if (intel_get_load_detect_pipe(connector, &mode, &tmp, &ctx)) {
>  			type = intel_tv_detect_type(intel_tv, connector);
>  			intel_release_load_detect_pipe(connector, &tmp);
> +			status = type < 0 ?
> +				connector_status_disconnected :
> +				connector_status_connected;
>  		} else
> -			return connector_status_unknown;
> +			status = connector_status_unknown;
>  
>  		drm_modeset_drop_locks(&ctx);
>  		drm_modeset_acquire_fini(&ctx);
>  	} else
>  		return connector->status;
>  
> -	if (type < 0)
> -		return connector_status_disconnected;
> +	if (status != connector_status_connected)
> +		return status;
>  
>  	intel_tv->type = type;
>  	intel_tv_find_better_format(connector);
> -- 
> 1.8.5.5
>

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2] drm/i915: Fix lock dropping in intel_tv_detect()

[Tibor Billes]

Hi!

From: ville.syrjala@linux.intel.com Sent: Monday, September 01, 2014 at 12:07 PM

> When intel_tv_detect() fails to do load detection it would forget to
> drop the locks and clean up the acquire context. Fix it up.
> 
> This is a regression from:
> commit 208bf9fdcd3575aa4a5d48b3e0295f7cdaf6fc44
> Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Date: Mon Aug 11 13:15:35 2014 +0300
> 
> drm/i915: Fix locking for intel_enable_pipe_a()
> 
> v2: Make the code more readable (Chris)
> 
> Cc: Tibor Billes <tbilles@gmx.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

I tested the posted patch, applying it on top of 3.17-rc2 did solve my boot problems. Just to make sure I also tried a clean 3.17-rc3, which was unable to boot. With this patch applied, it again booted successfully.

I'm not quite sure how tags work, but you may add my following tags:
Reported-by: Tibor Billes <tbilles@gmx.com>
Tested-by: Tibor Billes <tbilles@gmx.com>

Thank you all for the quick responses and the patch, nice work! :)

Tibor