diff mbox

[01/10] drm/i915: Grab struct_mutex around all FBC updates

Message ID 1383771745-22463-2-git-send-email-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ville Syrjälä Nov. 6, 2013, 9:02 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

We need some protection for the FBC state, and since struct_mutex
is it currently in most places, make sure all FBC update/disable
calles are protected by it.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Rodrigo Vivi Nov. 20, 2013, 10:39 p.m. UTC | #1 
On Wed, Nov 06, 2013 at 11:02:16PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> We need some protection for the FBC state, and since struct_mutex
> is it currently in most places, make sure all FBC update/disable
> calles are protected by it.

Why don't you create a new mutex only for fbc update?

> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 12cf362..bce6e07 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3576,8 +3576,10 @@ static void haswell_crtc_disable_planes(struct drm_crtc *crtc)
>  	drm_vblank_off(dev, pipe);
>  
>  	/* FBC must be disabled before disabling the plane on HSW. */
> +	mutex_lock(&dev->struct_mutex);
>  	if (dev_priv->fbc.plane == plane)
>  		intel_disable_fbc(dev);
> +	mutex_unlock(&dev->struct_mutex);
>  
>  	hsw_disable_ips(intel_crtc);
>  
> @@ -3717,8 +3719,10 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc)
>  	intel_crtc_wait_for_pending_flips(crtc);
>  	drm_vblank_off(dev, pipe);
>  
> +	mutex_lock(&dev->struct_mutex);
>  	if (dev_priv->fbc.plane == plane)
>  		intel_disable_fbc(dev);
> +	mutex_unlock(&dev->struct_mutex);
>  
>  	intel_crtc_update_cursor(crtc, false);
>  	intel_disable_planes(crtc);
> @@ -4102,7 +4106,9 @@ static void valleyview_crtc_enable(struct drm_crtc *crtc)
>  	intel_enable_planes(crtc);
>  	intel_crtc_update_cursor(crtc, true);
>  
> +	mutex_lock(&dev->struct_mutex);
>  	intel_update_fbc(dev);
> +	mutex_unlock(&dev->struct_mutex);
>  
>  	for_each_encoder_on_crtc(dev, crtc, encoder)
>  		encoder->enable(encoder);
> @@ -4146,7 +4152,9 @@ static void i9xx_crtc_enable(struct drm_crtc *crtc)
>  	/* Give the overlay scaler a chance to enable if it's on this pipe */
>  	intel_crtc_dpms_overlay(intel_crtc, true);
>  
> +	mutex_lock(&dev->struct_mutex);
>  	intel_update_fbc(dev);
> +	mutex_unlock(&dev->struct_mutex);
>  
>  	for_each_encoder_on_crtc(dev, crtc, encoder)
>  		encoder->enable(encoder);
> @@ -4210,7 +4218,9 @@ static void i9xx_crtc_disable(struct drm_crtc *crtc)
>  	intel_crtc->active = false;
>  	intel_update_watermarks(crtc);
>  
> +	mutex_lock(&dev->struct_mutex);
>  	intel_update_fbc(dev);
> +	mutex_unlock(&dev->struct_mutex);
>  }
>  
>  static void i9xx_crtc_off(struct drm_crtc *crtc)
> -- 
> 1.8.1.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Daniel Vetter Nov. 21, 2013, 8:22 a.m. UTC | #2 
On Wed, Nov 20, 2013 at 11:39 PM, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
> On Wed, Nov 06, 2013 at 11:02:16PM +0200, ville.syrjala@linux.intel.com wrote:
>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>
>> We need some protection for the FBC state, and since struct_mutex
>> is it currently in most places, make sure all FBC update/disable
>> calles are protected by it.
>
> Why don't you create a new mutex only for fbc update?

Yeah, if it's not core gem state please don't spread the usage of
dev->struct_mutex. Eventually we need to slash that one into pieces,
but until that happens making things worse doesn't help.
-Daniel
Ville Syrjälä Nov. 21, 2013, 10:49 a.m. UTC | #3 
On Thu, Nov 21, 2013 at 09:22:43AM +0100, Daniel Vetter wrote:
> On Wed, Nov 20, 2013 at 11:39 PM, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
> > On Wed, Nov 06, 2013 at 11:02:16PM +0200, ville.syrjala@linux.intel.com wrote:
> >> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>
> >> We need some protection for the FBC state, and since struct_mutex
> >> is it currently in most places, make sure all FBC update/disable
> >> calles are protected by it.
> >
> > Why don't you create a new mutex only for fbc update?
> 
> Yeah, if it's not core gem state please don't spread the usage of
> dev->struct_mutex. Eventually we need to slash that one into pieces,
> but until that happens making things worse doesn't help.

I don't think I'm making things worse. struct_mutex is the lock that
protects the fbc state currently, except we forgot to grab it in a some
places. I'd rather fix this first, and then as a followup someone can
start to think about using a new lock for fbc.
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 12cf362..bce6e07 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3576,8 +3576,10 @@  static void haswell_crtc_disable_planes(struct drm_crtc *crtc)
 	drm_vblank_off(dev, pipe);
 
 	/* FBC must be disabled before disabling the plane on HSW. */
+	mutex_lock(&dev->struct_mutex);
 	if (dev_priv->fbc.plane == plane)
 		intel_disable_fbc(dev);
+	mutex_unlock(&dev->struct_mutex);
 
 	hsw_disable_ips(intel_crtc);
 
@@ -3717,8 +3719,10 @@  static void ironlake_crtc_disable(struct drm_crtc *crtc)
 	intel_crtc_wait_for_pending_flips(crtc);
 	drm_vblank_off(dev, pipe);
 
+	mutex_lock(&dev->struct_mutex);
 	if (dev_priv->fbc.plane == plane)
 		intel_disable_fbc(dev);
+	mutex_unlock(&dev->struct_mutex);
 
 	intel_crtc_update_cursor(crtc, false);
 	intel_disable_planes(crtc);
@@ -4102,7 +4106,9 @@  static void valleyview_crtc_enable(struct drm_crtc *crtc)
 	intel_enable_planes(crtc);
 	intel_crtc_update_cursor(crtc, true);
 
+	mutex_lock(&dev->struct_mutex);
 	intel_update_fbc(dev);
+	mutex_unlock(&dev->struct_mutex);
 
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		encoder->enable(encoder);
@@ -4146,7 +4152,9 @@  static void i9xx_crtc_enable(struct drm_crtc *crtc)
 	/* Give the overlay scaler a chance to enable if it's on this pipe */
 	intel_crtc_dpms_overlay(intel_crtc, true);
 
+	mutex_lock(&dev->struct_mutex);
 	intel_update_fbc(dev);
+	mutex_unlock(&dev->struct_mutex);
 
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		encoder->enable(encoder);
@@ -4210,7 +4218,9 @@  static void i9xx_crtc_disable(struct drm_crtc *crtc)
 	intel_crtc->active = false;
 	intel_update_watermarks(crtc);
 
+	mutex_lock(&dev->struct_mutex);
 	intel_update_fbc(dev);
+	mutex_unlock(&dev->struct_mutex);
 }
 
 static void i9xx_crtc_off(struct drm_crtc *crtc)