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

2014-09-01 Thread 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, , , 
> > )) {
> > type = intel_tv_detect_type(intel_tv, connector);
> > +   status = type < 0 ?
> > +   connector_status_disconnected :
> > +   connector_status_connected;
> > +
> > intel_release_load_detect_pipe(connector, );
> > } else
> > -   return connector_status_unknown;
> > +   status = connector_status_unknown;
> >  
> > drm_modeset_drop_locks();
> > drm_modeset_acquire_fini();
> > -   } 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(_priv->irq_lock, irqflags);
}
 
-   return type;
+   return status;
 }
 
 /*

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

2014-09-01 Thread 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.syrj...@linux.intel.com wrote:
> > From: Ville Syrjälä 
> > 
> > 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ä 
> >  Date:   Mon Aug 11 13:15:35 2014 +0300
> > 
> > drm/i915: Fix locking for intel_enable_pipe_a()
> > 
> > Cc: Tibor Billes 
> > Signed-off-by: Ville Syrjälä 
> > ---
> >  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, , , )) {
> > type = intel_tv_detect_type(intel_tv, connector);
> > intel_release_load_detect_pipe(connector, );
> > +   status = type < 0 ?
> > +   connector_status_disconnected :
> > +   connector_status_connected;
> > } else
> > -   return connector_status_unknown;
> > +   status = connector_status_unknown;
> >  
> > drm_modeset_drop_locks();
> > drm_modeset_acquire_fini();
> > -   } 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, , , )) 
> {
> type = intel_tv_detect_type(intel_tv, connector);
> +   status = type < 0 ?
> +   connector_status_disconnected :
> +   connector_status_connected;
> +
> intel_release_load_detect_pipe(connector, );
> } else
> -   return connector_status_unknown;
> +   status = connector_status_unknown;
>  
> drm_modeset_drop_locks();
> drm_modeset_acquire_fini();
> -   } 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2014-09-01 Thread Chris Wilson 
On Mon, Sep 01, 2014 at 11:09:46AM +0300, ville.syrj...@linux.intel.com wrote:
> From: Ville Syrjälä 
> 
> 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ä 
>  Date:   Mon Aug 11 13:15:35 2014 +0300
> 
> drm/i915: Fix locking for intel_enable_pipe_a()
> 
> Cc: Tibor Billes 
> Signed-off-by: Ville Syrjälä 
> ---
>  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, , , )) {
>   type = intel_tv_detect_type(intel_tv, connector);
>   intel_release_load_detect_pipe(connector, );
> + status = type < 0 ?
> + connector_status_disconnected :
> + connector_status_connected;
>   } else
> - return connector_status_unknown;
> + status = connector_status_unknown;
>  
>   drm_modeset_drop_locks();
>   drm_modeset_acquire_fini();
> - } 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, , , )) {
type = intel_tv_detect_type(intel_tv, connector);
+   status = type < 0 ?
+   connector_status_disconnected :
+   connector_status_connected;
+
intel_release_load_detect_pipe(connector, );
} else
-   return connector_status_unknown;
+   status = connector_status_unknown;
 
drm_modeset_drop_locks();
drm_modeset_acquire_fini();
-   } 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2014-09-01 Thread ville . syrjala 
From: Ville Syrjälä 

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ä 
 Date:   Mon Aug 11 13:15:35 2014 +0300

drm/i915: Fix locking for intel_enable_pipe_a()

Cc: Tibor Billes 
Signed-off-by: Ville Syrjälä 
---
 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, , , )) {
type = intel_tv_detect_type(intel_tv, connector);
intel_release_load_detect_pipe(connector, );
+   status = type < 0 ?
+   connector_status_disconnected :
+   connector_status_connected;
} else
-   return connector_status_unknown;
+   status = connector_status_unknown;
 
drm_modeset_drop_locks();
drm_modeset_acquire_fini();
-   } 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2014-09-01 Thread ville . syrjala 
From: Ville Syrjälä ville.syrj...@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.syrj...@linux.intel.com
 Date:   Mon Aug 11 13:15:35 2014 +0300

drm/i915: Fix locking for intel_enable_pipe_a()

Cc: Tibor Billes tbil...@gmx.com
Signed-off-by: Ville Syrjälä ville.syrj...@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

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2014-09-01 Thread Chris Wilson 
On Mon, Sep 01, 2014 at 11:09:46AM +0300, ville.syrj...@linux.intel.com wrote:
 From: Ville Syrjälä ville.syrj...@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.syrj...@linux.intel.com
  Date:   Mon Aug 11 13:15:35 2014 +0300
 
 drm/i915: Fix locking for intel_enable_pipe_a()
 
 Cc: Tibor Billes tbil...@gmx.com
 Signed-off-by: Ville Syrjälä ville.syrj...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2014-09-01 Thread 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.syrj...@linux.intel.com wrote:
  From: Ville Syrjälä ville.syrj...@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.syrj...@linux.intel.com
   Date:   Mon Aug 11 13:15:35 2014 +0300
  
  drm/i915: Fix locking for intel_enable_pipe_a()
  
  Cc: Tibor Billes tbil...@gmx.com
  Signed-off-by: Ville Syrjälä ville.syrj...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2014-09-01 Thread 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)