| Message ID | 1394209951-9963-15-git-send-email-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2014-03-07 13:32 GMT-03:00 <ville.syrjala@linux.intel.com>: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Add a mutex to protect most of the watermark state. Will be useful when > we start to update watermarks asynchronously from plane updates, or > when we get finer grained locking for planes. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 11 ++++++++++- > drivers/gpu/drm/i915/intel_drv.h | 5 ++++- > drivers/gpu/drm/i915/intel_pm.c | 21 +++++++++++++++++++-- > 3 files changed, 33 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 29da39f..0b19723 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1595,8 +1595,17 @@ typedef struct drm_i915_private { > /* cursor */ > uint16_t cur_latency[5]; > > - /* current hardware state */ > + /* > + * current hardware state > + * protected by dev_priv->wm.mutex > + */ > struct ilk_wm_values hw; > + > + /* > + * protects some dev_priv->wm and intel_crtc->wm > + * state as well as the actual hardware registers > + */ > + struct mutex mutex; > } wm; > > struct i915_package_c8 pc8; > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index f022a78..8e32d69 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -384,7 +384,10 @@ struct intel_crtc { > > /* per-pipe watermark state */ > struct { > - /* watermarks currently being used */ > + /* > + * watermarks currently being used > + * protected by dev_priv->wm.mutex > + */ > struct intel_pipe_wm active; > } wm; > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 3f5c1dc..d8adcb3 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -2692,8 +2692,13 @@ static void ilk_write_wm_values(struct drm_i915_private *dev_priv, > static bool ilk_disable_lp_wm(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > + bool changed; > > - return _ilk_disable_lp_wm(dev_priv, WM_DIRTY_LP_ALL); > + mutex_lock(&dev_priv->wm.mutex); > + changed = _ilk_disable_lp_wm(dev_priv, WM_DIRTY_LP_ALL); > + mutex_unlock(&dev_priv->wm.mutex); > + > + return changed; > } > > static void ilk_program_watermarks(struct drm_device *dev) > @@ -2733,6 +2738,7 @@ static void ilk_update_wm(struct drm_crtc *crtc) > { > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > struct drm_device *dev = crtc->dev; > + struct drm_i915_private *dev_priv = dev->dev_private; > struct ilk_pipe_wm_parameters params = {}; > struct intel_pipe_wm pipe_wm = {}; > > @@ -2740,12 +2746,17 @@ static void ilk_update_wm(struct drm_crtc *crtc) > > intel_compute_pipe_wm(crtc, ¶ms, &pipe_wm); > > + mutex_lock(&dev_priv->wm.mutex); > + > if (!memcmp(&intel_crtc->wm.active, &pipe_wm, sizeof(pipe_wm))) > - return; > + goto unlock; > > intel_crtc->wm.active = pipe_wm; > > ilk_program_watermarks(dev); > + > + unlock: > + mutex_unlock(&dev_priv->wm.mutex); > } > > static void ilk_update_sprite_wm(struct drm_plane *plane, > @@ -2817,10 +2828,14 @@ void ilk_wm_get_hw_state(struct drm_device *dev) > struct drm_i915_private *dev_priv = dev->dev_private; > struct drm_crtc *crtc; > > + mutex_lock(&dev_priv->wm.mutex); > + > _ilk_wm_get_hw_state(dev, &dev_priv->wm.hw); > > list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) > _ilk_pipe_wm_hw_to_sw(crtc); > + > + mutex_unlock(&dev_priv->wm.mutex); > } > > /** > @@ -5760,6 +5775,8 @@ void intel_init_pm(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > > + mutex_init(&dev_priv->wm.mutex); > + I think you should probably init this at intel_pm_setup(), which is called way earlier. Just to be safe. With that fixed: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>. > if (HAS_FBC(dev)) { > if (INTEL_INFO(dev)->gen >= 7) { > dev_priv->display.fbc_enabled = ironlake_fbc_enabled; > -- > 1.8.3.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Apr 23, 2014 at 06:47:04PM -0300, Paulo Zanoni wrote: > 2014-03-07 13:32 GMT-03:00 <ville.syrjala@linux.intel.com>: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > Add a mutex to protect most of the watermark state. Will be useful when > > we start to update watermarks asynchronously from plane updates, or > > when we get finer grained locking for planes. > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/i915_drv.h | 11 ++++++++++- > > drivers/gpu/drm/i915/intel_drv.h | 5 ++++- > > drivers/gpu/drm/i915/intel_pm.c | 21 +++++++++++++++++++-- > > 3 files changed, 33 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > > index 29da39f..0b19723 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -1595,8 +1595,17 @@ typedef struct drm_i915_private { > > /* cursor */ > > uint16_t cur_latency[5]; > > > > - /* current hardware state */ > > + /* > > + * current hardware state > > + * protected by dev_priv->wm.mutex > > + */ > > struct ilk_wm_values hw; > > + > > + /* > > + * protects some dev_priv->wm and intel_crtc->wm > > + * state as well as the actual hardware registers > > + */ > > + struct mutex mutex; > > } wm; > > > > struct i915_package_c8 pc8; > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > > index f022a78..8e32d69 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -384,7 +384,10 @@ struct intel_crtc { > > > > /* per-pipe watermark state */ > > struct { > > - /* watermarks currently being used */ > > + /* > > + * watermarks currently being used > > + * protected by dev_priv->wm.mutex > > + */ > > struct intel_pipe_wm active; > > } wm; > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > > index 3f5c1dc..d8adcb3 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -2692,8 +2692,13 @@ static void ilk_write_wm_values(struct drm_i915_private *dev_priv, > > static bool ilk_disable_lp_wm(struct drm_device *dev) > > { > > struct drm_i915_private *dev_priv = dev->dev_private; > > + bool changed; > > > > - return _ilk_disable_lp_wm(dev_priv, WM_DIRTY_LP_ALL); > > + mutex_lock(&dev_priv->wm.mutex); > > + changed = _ilk_disable_lp_wm(dev_priv, WM_DIRTY_LP_ALL); > > + mutex_unlock(&dev_priv->wm.mutex); > > + > > + return changed; > > } > > > > static void ilk_program_watermarks(struct drm_device *dev) > > @@ -2733,6 +2738,7 @@ static void ilk_update_wm(struct drm_crtc *crtc) > > { > > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > > struct drm_device *dev = crtc->dev; > > + struct drm_i915_private *dev_priv = dev->dev_private; > > struct ilk_pipe_wm_parameters params = {}; > > struct intel_pipe_wm pipe_wm = {}; > > > > @@ -2740,12 +2746,17 @@ static void ilk_update_wm(struct drm_crtc *crtc) > > > > intel_compute_pipe_wm(crtc, ¶ms, &pipe_wm); > > > > + mutex_lock(&dev_priv->wm.mutex); > > + > > if (!memcmp(&intel_crtc->wm.active, &pipe_wm, sizeof(pipe_wm))) > > - return; > > + goto unlock; > > > > intel_crtc->wm.active = pipe_wm; > > > > ilk_program_watermarks(dev); > > + > > + unlock: > > + mutex_unlock(&dev_priv->wm.mutex); > > } > > > > static void ilk_update_sprite_wm(struct drm_plane *plane, > > @@ -2817,10 +2828,14 @@ void ilk_wm_get_hw_state(struct drm_device *dev) > > struct drm_i915_private *dev_priv = dev->dev_private; > > struct drm_crtc *crtc; > > > > + mutex_lock(&dev_priv->wm.mutex); > > + > > _ilk_wm_get_hw_state(dev, &dev_priv->wm.hw); > > > > list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) > > _ilk_pipe_wm_hw_to_sw(crtc); > > + > > + mutex_unlock(&dev_priv->wm.mutex); > > } > > > > /** > > @@ -5760,6 +5775,8 @@ void intel_init_pm(struct drm_device *dev) > > { > > struct drm_i915_private *dev_priv = dev->dev_private; > > > > + mutex_init(&dev_priv->wm.mutex); > > + > > I think you should probably init this at intel_pm_setup(), which is > called way earlier. Just to be safe. If we call some WM function before intel_init_pm() we have a serious bug. This is where we set up the WM function pointers and extract the latency values from SSKPD/MLTR. > > With that fixed: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>. > > > if (HAS_FBC(dev)) { > > if (INTEL_INFO(dev)->gen >= 7) { > > dev_priv->display.fbc_enabled = ironlake_fbc_enabled; > > -- > > 1.8.3.2 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > Paulo Zanoni
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 29da39f..0b19723 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1595,8 +1595,17 @@ typedef struct drm_i915_private { /* cursor */ uint16_t cur_latency[5]; - /* current hardware state */ + /* + * current hardware state + * protected by dev_priv->wm.mutex + */ struct ilk_wm_values hw; + + /* + * protects some dev_priv->wm and intel_crtc->wm + * state as well as the actual hardware registers + */ + struct mutex mutex; } wm; struct i915_package_c8 pc8; diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index f022a78..8e32d69 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -384,7 +384,10 @@ struct intel_crtc { /* per-pipe watermark state */ struct { - /* watermarks currently being used */ + /* + * watermarks currently being used + * protected by dev_priv->wm.mutex + */ struct intel_pipe_wm active; } wm; diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 3f5c1dc..d8adcb3 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -2692,8 +2692,13 @@ static void ilk_write_wm_values(struct drm_i915_private *dev_priv, static bool ilk_disable_lp_wm(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; + bool changed; - return _ilk_disable_lp_wm(dev_priv, WM_DIRTY_LP_ALL); + mutex_lock(&dev_priv->wm.mutex); + changed = _ilk_disable_lp_wm(dev_priv, WM_DIRTY_LP_ALL); + mutex_unlock(&dev_priv->wm.mutex); + + return changed; } static void ilk_program_watermarks(struct drm_device *dev) @@ -2733,6 +2738,7 @@ static void ilk_update_wm(struct drm_crtc *crtc) { struct intel_crtc *intel_crtc = to_intel_crtc(crtc); struct drm_device *dev = crtc->dev; + struct drm_i915_private *dev_priv = dev->dev_private; struct ilk_pipe_wm_parameters params = {}; struct intel_pipe_wm pipe_wm = {}; @@ -2740,12 +2746,17 @@ static void ilk_update_wm(struct drm_crtc *crtc) intel_compute_pipe_wm(crtc, ¶ms, &pipe_wm); + mutex_lock(&dev_priv->wm.mutex); + if (!memcmp(&intel_crtc->wm.active, &pipe_wm, sizeof(pipe_wm))) - return; + goto unlock; intel_crtc->wm.active = pipe_wm; ilk_program_watermarks(dev); + + unlock: + mutex_unlock(&dev_priv->wm.mutex); } static void ilk_update_sprite_wm(struct drm_plane *plane, @@ -2817,10 +2828,14 @@ void ilk_wm_get_hw_state(struct drm_device *dev) struct drm_i915_private *dev_priv = dev->dev_private; struct drm_crtc *crtc; + mutex_lock(&dev_priv->wm.mutex); + _ilk_wm_get_hw_state(dev, &dev_priv->wm.hw); list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) _ilk_pipe_wm_hw_to_sw(crtc); + + mutex_unlock(&dev_priv->wm.mutex); } /** @@ -5760,6 +5775,8 @@ void intel_init_pm(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; + mutex_init(&dev_priv->wm.mutex); + if (HAS_FBC(dev)) { if (INTEL_INFO(dev)->gen >= 7) { dev_priv->display.fbc_enabled = ironlake_fbc_enabled;