[PATCH] drm/i915: Fix locking in DRRS flush/invalidate hooks

[PATCH] drm/i915: Fix locking in DRRS flush/invalidate hooks

[Daniel Vetter]

We must acquire the mutex before we can check drrs.dp, otherwise
someone might sneak in with a modeset, clear the pointer after we've
checked it and then the code will Oops.

This issue has been introduced in

commit a93fad0f7fb8a3ff12e8814b630648f6d187954c
Author: Vandana Kannan <vandana.kannan@intel.com>
Date:   Sat Jan 10 02:25:59 2015 +0530

    drm/i915: DRRS calls based on frontbuffer

Cc: Ramalingam C <ramalingam.c@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Vandana Kannan <vandana.kannan@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 1b87969536ff..e265a0dee479 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -5170,7 +5170,6 @@ static void intel_edp_drrs_downclock_work(struct work_struct *work)
 			downclock_mode->vrefresh);
 
 unlock:
-
 	mutex_unlock(&dev_priv->drrs.mutex);
 }
 
@@ -5192,12 +5191,14 @@ void intel_edp_drrs_invalidate(struct drm_device *dev,
 	struct drm_crtc *crtc;
 	enum pipe pipe;
 
-	if (!dev_priv->drrs.dp)
-		return;
-
 	cancel_delayed_work_sync(&dev_priv->drrs.work);
 
 	mutex_lock(&dev_priv->drrs.mutex);
+	if (!dev_priv->drrs.dp) {
+		mutex_unlock(&dev_priv->drrs.mutex);
+		return;
+	}
+
 	crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc;
 	pipe = to_intel_crtc(crtc)->pipe;
 
@@ -5231,12 +5232,14 @@ void intel_edp_drrs_flush(struct drm_device *dev,
 	struct drm_crtc *crtc;
 	enum pipe pipe;
 
-	if (!dev_priv->drrs.dp)
-		return;
-
 	cancel_delayed_work_sync(&dev_priv->drrs.work);
 
 	mutex_lock(&dev_priv->drrs.mutex);
+	if (!dev_priv->drrs.dp) {
+		mutex_unlock(&dev_priv->drrs.mutex);
+		return;
+	}
+
 	crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc;
 	pipe = to_intel_crtc(crtc)->pipe;
 	dev_priv->drrs.busy_frontbuffer_bits &= ~frontbuffer_bits;
-- 
2.1.4

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

Re: [PATCH] drm/i915: Fix locking in DRRS flush/invalidate hooks

[Jani Nikula]

On Thu, 09 Apr 2015, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> We must acquire the mutex before we can check drrs.dp, otherwise
> someone might sneak in with a modeset, clear the pointer after we've
> checked it and then the code will Oops.
>
> This issue has been introduced in
>
> commit a93fad0f7fb8a3ff12e8814b630648f6d187954c
> Author: Vandana Kannan <vandana.kannan@intel.com>
> Date:   Sat Jan 10 02:25:59 2015 +0530
>
>     drm/i915: DRRS calls based on frontbuffer
>
> Cc: Ramalingam C <ramalingam.c@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Vandana Kannan <vandana.kannan@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Missed 4.0 so this is

Cc: stable@vger.kernel.org


> ---
>  drivers/gpu/drm/i915/intel_dp.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 1b87969536ff..e265a0dee479 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -5170,7 +5170,6 @@ static void intel_edp_drrs_downclock_work(struct work_struct *work)
>  			downclock_mode->vrefresh);
>  
>  unlock:
> -
>  	mutex_unlock(&dev_priv->drrs.mutex);
>  }
>  
> @@ -5192,12 +5191,14 @@ void intel_edp_drrs_invalidate(struct drm_device *dev,
>  	struct drm_crtc *crtc;
>  	enum pipe pipe;
>  
> -	if (!dev_priv->drrs.dp)
> -		return;
> -
>  	cancel_delayed_work_sync(&dev_priv->drrs.work);
>  
>  	mutex_lock(&dev_priv->drrs.mutex);
> +	if (!dev_priv->drrs.dp) {
> +		mutex_unlock(&dev_priv->drrs.mutex);
> +		return;
> +	}
> +
>  	crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc;
>  	pipe = to_intel_crtc(crtc)->pipe;
>  
> @@ -5231,12 +5232,14 @@ void intel_edp_drrs_flush(struct drm_device *dev,
>  	struct drm_crtc *crtc;
>  	enum pipe pipe;
>  
> -	if (!dev_priv->drrs.dp)
> -		return;
> -
>  	cancel_delayed_work_sync(&dev_priv->drrs.work);
>  
>  	mutex_lock(&dev_priv->drrs.mutex);
> +	if (!dev_priv->drrs.dp) {
> +		mutex_unlock(&dev_priv->drrs.mutex);
> +		return;
> +	}
> +
>  	crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc;
>  	pipe = to_intel_crtc(crtc)->pipe;
>  	dev_priv->drrs.busy_frontbuffer_bits &= ~frontbuffer_bits;
> -- 
> 2.1.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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] drm/i915: Fix locking in DRRS flush/invalidate hooks

[Ville Syrjälä]

On Thu, Apr 09, 2015 at 12:52:19PM +0300, Jani Nikula wrote:
> On Thu, 09 Apr 2015, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > We must acquire the mutex before we can check drrs.dp, otherwise
> > someone might sneak in with a modeset, clear the pointer after we've
> > checked it and then the code will Oops.
> >
> > This issue has been introduced in
> >
> > commit a93fad0f7fb8a3ff12e8814b630648f6d187954c
> > Author: Vandana Kannan <vandana.kannan@intel.com>
> > Date:   Sat Jan 10 02:25:59 2015 +0530
> >
> >     drm/i915: DRRS calls based on frontbuffer
> >
> > Cc: Ramalingam C <ramalingam.c@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Vandana Kannan <vandana.kannan@intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> 
> Missed 4.0 so this is

Fortunately so since it makes my BSW deadlock on module load. And Mika
says his IVB is showing similar symptoms.

> 
> Cc: stable@vger.kernel.org
> 
> 
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 17 ++++++++++-------
> >  1 file changed, 10 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 1b87969536ff..e265a0dee479 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -5170,7 +5170,6 @@ static void intel_edp_drrs_downclock_work(struct work_struct *work)
> >  			downclock_mode->vrefresh);
> >  
> >  unlock:
> > -
> >  	mutex_unlock(&dev_priv->drrs.mutex);
> >  }
> >  
> > @@ -5192,12 +5191,14 @@ void intel_edp_drrs_invalidate(struct drm_device *dev,
> >  	struct drm_crtc *crtc;
> >  	enum pipe pipe;
> >  
> > -	if (!dev_priv->drrs.dp)
> > -		return;
> > -
> >  	cancel_delayed_work_sync(&dev_priv->drrs.work);
> >  
> >  	mutex_lock(&dev_priv->drrs.mutex);
> > +	if (!dev_priv->drrs.dp) {
> > +		mutex_unlock(&dev_priv->drrs.mutex);
> > +		return;
> > +	}
> > +
> >  	crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc;
> >  	pipe = to_intel_crtc(crtc)->pipe;
> >  
> > @@ -5231,12 +5232,14 @@ void intel_edp_drrs_flush(struct drm_device *dev,
> >  	struct drm_crtc *crtc;
> >  	enum pipe pipe;
> >  
> > -	if (!dev_priv->drrs.dp)
> > -		return;
> > -
> >  	cancel_delayed_work_sync(&dev_priv->drrs.work);
> >  
> >  	mutex_lock(&dev_priv->drrs.mutex);
> > +	if (!dev_priv->drrs.dp) {
> > +		mutex_unlock(&dev_priv->drrs.mutex);
> > +		return;
> > +	}
> > +
> >  	crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc;
> >  	pipe = to_intel_crtc(crtc)->pipe;
> >  	dev_priv->drrs.busy_frontbuffer_bits &= ~frontbuffer_bits;
> > -- 
> > 2.1.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[PATCH 1/2] drm/i915: Fix locking in DRRS flush/invalidate hooks

[Daniel Vetter]

We must acquire the mutex before we can check drrs.dp, otherwise
someone might sneak in with a modeset, clear the pointer after we've
checked it and then the code will Oops.

This issue has been introduced in

commit a93fad0f7fb8a3ff12e8814b630648f6d187954c
Author: Vandana Kannan <vandana.kannan@intel.com>
Date:   Sat Jan 10 02:25:59 2015 +0530

    drm/i915: DRRS calls based on frontbuffer

v2: Don't blow up on uninitialized mutex and work item by checking
whether DRRS is support or not first. Also unconditionally initialize
the mutex/work item to avoid future trouble.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Ramalingam C <ramalingam.c@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Vandana Kannan <vandana.kannan@intel.com>
Cc: stable@vger.kernel.org (4.0+ only)
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 1b87969536ff..d846738365cb 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -5170,7 +5170,6 @@ static void intel_edp_drrs_downclock_work(struct work_struct *work)
 			downclock_mode->vrefresh);
 
 unlock:
-
 	mutex_unlock(&dev_priv->drrs.mutex);
 }
 
@@ -5192,12 +5191,17 @@ void intel_edp_drrs_invalidate(struct drm_device *dev,
 	struct drm_crtc *crtc;
 	enum pipe pipe;
 
-	if (!dev_priv->drrs.dp)
+	if (dev_priv->drrs.type == DRRS_NOT_SUPPORTED)
 		return;
 
 	cancel_delayed_work_sync(&dev_priv->drrs.work);
 
 	mutex_lock(&dev_priv->drrs.mutex);
+	if (!dev_priv->drrs.dp) {
+		mutex_unlock(&dev_priv->drrs.mutex);
+		return;
+	}
+
 	crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc;
 	pipe = to_intel_crtc(crtc)->pipe;
 
@@ -5231,12 +5235,17 @@ void intel_edp_drrs_flush(struct drm_device *dev,
 	struct drm_crtc *crtc;
 	enum pipe pipe;
 
-	if (!dev_priv->drrs.dp)
+	if (dev_priv->drrs.type == DRRS_NOT_SUPPORTED)
 		return;
 
 	cancel_delayed_work_sync(&dev_priv->drrs.work);
 
 	mutex_lock(&dev_priv->drrs.mutex);
+	if (!dev_priv->drrs.dp) {
+		mutex_unlock(&dev_priv->drrs.mutex);
+		return;
+	}
+
 	crtc = dp_to_dig_port(dev_priv->drrs.dp)->base.base.crtc;
 	pipe = to_intel_crtc(crtc)->pipe;
 	dev_priv->drrs.busy_frontbuffer_bits &= ~frontbuffer_bits;
@@ -5307,6 +5316,9 @@ intel_dp_drrs_init(struct intel_connector *intel_connector,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_display_mode *downclock_mode = NULL;
 
+	INIT_DELAYED_WORK(&dev_priv->drrs.work, intel_edp_drrs_downclock_work);
+	mutex_init(&dev_priv->drrs.mutex);
+
 	if (INTEL_INFO(dev)->gen <= 6) {
 		DRM_DEBUG_KMS("DRRS supported for Gen7 and above\n");
 		return NULL;
@@ -5325,10 +5337,6 @@ intel_dp_drrs_init(struct intel_connector *intel_connector,
 		return NULL;
 	}
 
-	INIT_DELAYED_WORK(&dev_priv->drrs.work, intel_edp_drrs_downclock_work);
-
-	mutex_init(&dev_priv->drrs.mutex);
-
 	dev_priv->drrs.type = dev_priv->vbt.drrs_type;
 
 	dev_priv->drrs.refresh_rate_type = DRRS_HIGH_RR;
-- 
2.1.0

[PATCH 2/2] drm/i915: Don't cancel DRRS worker synchronously for flush/invalidate

[Daniel Vetter]

It's not needed since the worker rechecks that it didn't race. We only
need to cancel synchronously after disabling drrs to make sure the
worker really is gone (e.g. for driver unload). But for normal
operation the stall is just wasted time.

Reported-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Ramalingam C <ramalingam.c@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Vandana Kannan <vandana.kannan@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index d846738365cb..b8afa2af0523 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -5194,7 +5194,7 @@ void intel_edp_drrs_invalidate(struct drm_device *dev,
 	if (dev_priv->drrs.type == DRRS_NOT_SUPPORTED)
 		return;
 
-	cancel_delayed_work_sync(&dev_priv->drrs.work);
+	cancel_delayed_work(&dev_priv->drrs.work);
 
 	mutex_lock(&dev_priv->drrs.mutex);
 	if (!dev_priv->drrs.dp) {
@@ -5238,7 +5238,7 @@ void intel_edp_drrs_flush(struct drm_device *dev,
 	if (dev_priv->drrs.type == DRRS_NOT_SUPPORTED)
 		return;
 
-	cancel_delayed_work_sync(&dev_priv->drrs.work);
+	cancel_delayed_work(&dev_priv->drrs.work);
 
 	mutex_lock(&dev_priv->drrs.mutex);
 	if (!dev_priv->drrs.dp) {
-- 
2.1.0

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

Re: [PATCH 2/2] drm/i915: Don't cancel DRRS worker synchronously for flush/invalidate

[Chris Wilson]

On Thu, Apr 09, 2015 at 04:44:16PM +0200, Daniel Vetter wrote:
> It's not needed since the worker rechecks that it didn't race. We only
> need to cancel synchronously after disabling drrs to make sure the
> worker really is gone (e.g. for driver unload). But for normal
> operation the stall is just wasted time.
> 
> Reported-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Ramalingam C <ramalingam.c@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Vandana Kannan <vandana.kannan@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-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: [PATCH 1/2] drm/i915: Fix locking in DRRS flush/invalidate hooks

[Chris Wilson]

On Thu, Apr 09, 2015 at 04:44:15PM +0200, Daniel Vetter wrote:
> We must acquire the mutex before we can check drrs.dp, otherwise
> someone might sneak in with a modeset, clear the pointer after we've
> checked it and then the code will Oops.
> 
> This issue has been introduced in
> 
> commit a93fad0f7fb8a3ff12e8814b630648f6d187954c
> Author: Vandana Kannan <vandana.kannan@intel.com>
> Date:   Sat Jan 10 02:25:59 2015 +0530
> 
>     drm/i915: DRRS calls based on frontbuffer
> 
> v2: Don't blow up on uninitialized mutex and work item by checking
> whether DRRS is support or not first. Also unconditionally initialize
> the mutex/work item to avoid future trouble.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Ramalingam C <ramalingam.c@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Vandana Kannan <vandana.kannan@intel.com>
> Cc: stable@vger.kernel.org (4.0+ only)
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Happier,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Tested-by: Chris Wilson <chris@chris-wilson.co.uk>
-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 1/2] drm/i915: Fix locking in DRRS flush/invalidate hooks

[Jani Nikula]

On Thu, 09 Apr 2015, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Thu, Apr 09, 2015 at 04:44:15PM +0200, Daniel Vetter wrote:
>> We must acquire the mutex before we can check drrs.dp, otherwise
>> someone might sneak in with a modeset, clear the pointer after we've
>> checked it and then the code will Oops.
>> 
>> This issue has been introduced in
>> 
>> commit a93fad0f7fb8a3ff12e8814b630648f6d187954c
>> Author: Vandana Kannan <vandana.kannan@intel.com>
>> Date:   Sat Jan 10 02:25:59 2015 +0530
>> 
>>     drm/i915: DRRS calls based on frontbuffer
>> 
>> v2: Don't blow up on uninitialized mutex and work item by checking
>> whether DRRS is support or not first. Also unconditionally initialize
>> the mutex/work item to avoid future trouble.
>> 
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Cc: Ramalingam C <ramalingam.c@intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Cc: Vandana Kannan <vandana.kannan@intel.com>
>> Cc: stable@vger.kernel.org (4.0+ only)
>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>
> Happier,
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> Tested-by: Chris Wilson <chris@chris-wilson.co.uk>

Both pushed to drm-intel-next-fixes, thanks for the patch and review.

BR,
Jani.

> -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

-- 
Jani Nikula, Intel Open Source Technology Center