[Intel-gfx] [PATCH 2/3] drm/i915: add the FBC mutex
Daniel Vetter
daniel at ffwll.ch
Wed Jun 17 01:21:30 PDT 2015
On Tue, Jan 02, 2001 at 04:58:58AM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni at intel.com>
>
> Make sure we're not gonna have weird races in really weird cases where
> a lot of different CRTCs are doing rendering and modesets at the same
> time.
>
> v2:
> - Rebase (6 months later)
> - Also lock debugfs and stolen.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 8 ++++
> drivers/gpu/drm/i915/i915_drv.h | 1 +
> drivers/gpu/drm/i915/i915_gem_stolen.c | 7 ++++
> drivers/gpu/drm/i915/intel_fbc.c | 73 +++++++++++++++++++++++++++-------
> 4 files changed, 75 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index c49fe2a..c99be0e 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1593,6 +1593,7 @@ static int i915_fbc_status(struct seq_file *m, void *unused)
> }
>
> intel_runtime_pm_get(dev_priv);
> + mutex_lock(&dev_priv->fbc.lock);
>
> if (intel_fbc_enabled(dev))
> seq_puts(m, "FBC enabled\n");
> @@ -1605,6 +1606,7 @@ static int i915_fbc_status(struct seq_file *m, void *unused)
> yesno(I915_READ(FBC_STATUS2) &
> FBC_COMPRESSION_MASK));
>
> + mutex_unlock(&dev_priv->fbc.lock);
> intel_runtime_pm_put(dev_priv);
>
> return 0;
> @@ -1619,7 +1621,11 @@ static int i915_fbc_fc_get(void *data, u64 *val)
> return -ENODEV;
>
> drm_modeset_lock_all(dev);
Modeset locks aren't required any more here. That was just to prevent
concurrent modesets, but if we have a new unified fbc lock for all of this
it's not needed any more.
> + mutex_lock(&dev_priv->fbc.lock);
> +
> *val = dev_priv->fbc.false_color;
> +
> + mutex_unlock(&dev_priv->fbc.lock);
> drm_modeset_unlock_all(dev);
>
> return 0;
> @@ -1635,6 +1641,7 @@ static int i915_fbc_fc_set(void *data, u64 val)
> return -ENODEV;
>
> drm_modeset_lock_all(dev);
Same.
> + mutex_lock(&dev_priv->fbc.lock);
>
> reg = I915_READ(ILK_DPFC_CONTROL);
> dev_priv->fbc.false_color = val;
> @@ -1643,6 +1650,7 @@ static int i915_fbc_fc_set(void *data, u64 val)
> (reg | FBC_CTL_FALSE_COLOR) :
> (reg & ~FBC_CTL_FALSE_COLOR));
>
> + mutex_unlock(&dev_priv->fbc.lock);
> drm_modeset_unlock_all(dev);
> return 0;
> }
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 491ef0c..9ab04f7 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -891,6 +891,7 @@ enum fb_op_origin {
> };
>
> struct i915_fbc {
> + struct mutex lock;
> unsigned long uncompressed_size;
> unsigned threshold;
> unsigned int fb_id;
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index 348ed5a..947ddb4 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -250,6 +250,8 @@ int i915_gem_stolen_setup_compression(struct drm_device *dev, int size, int fb_c
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
>
> + lockdep_assert_held(&dev_priv->fbc.lock);
Please use WARN_ON(!mutex_locked). lockdep_assert_held is a no-op when
lockdep is compiled out. And the benefit of also checking that the current
task is the lock owner is imo negligible (it only matters when you have an
actual race, which almost never happens).
Also see my comment on the next patch, stolen memory management is protect
by dev->struct_mutex actually.
Locking looks good otherwise, but I guess we need to re-audit all this
once it's all settled down into the new design.
> +
> if (!drm_mm_initialized(&dev_priv->mm.stolen))
> return -ENODEV;
>
> @@ -266,6 +268,8 @@ void i915_gem_stolen_cleanup_compression(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
>
> + lockdep_assert_held(&dev_priv->fbc.lock);
> +
> if (dev_priv->fbc.uncompressed_size == 0)
> return;
>
> @@ -286,7 +290,10 @@ void i915_gem_cleanup_stolen(struct drm_device *dev)
> if (!drm_mm_initialized(&dev_priv->mm.stolen))
> return;
>
> + mutex_lock(&dev_priv->fbc.lock);
> i915_gem_stolen_cleanup_compression(dev);
> + mutex_unlock(&dev_priv->fbc.lock);
> +
> drm_mm_takedown(&dev_priv->mm.stolen);
> }
>
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index 9e55b9b..31ff88b 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -336,6 +336,7 @@ static void intel_fbc_work_fn(struct work_struct *__work)
> struct drm_i915_private *dev_priv = dev->dev_private;
>
> mutex_lock(&dev->struct_mutex);
> + mutex_lock(&dev_priv->fbc.lock);
> if (work == dev_priv->fbc.fbc_work) {
> /* Double check that we haven't switched fb without cancelling
> * the prior work.
> @@ -350,6 +351,7 @@ static void intel_fbc_work_fn(struct work_struct *__work)
>
> dev_priv->fbc.fbc_work = NULL;
> }
> + mutex_unlock(&dev_priv->fbc.lock);
> mutex_unlock(&dev->struct_mutex);
>
> kfree(work);
> @@ -357,6 +359,8 @@ static void intel_fbc_work_fn(struct work_struct *__work)
>
> static void intel_fbc_cancel_work(struct drm_i915_private *dev_priv)
> {
> + lockdep_assert_held(&dev_priv->fbc.lock);
Small note for the future: The async worker cancelling scheme used by fbc
doesn't match what psr/drrs use - they all cancel the work synchronously,
but outside of any locked critical section. We couldn't do that with fbc
because dev->struct_mutex is too big. But with the new locking it might
make sense to switch to the same design for consistency.
But we can only do that once fbc looks more like psr/drrs with dedicated
enable/disable, and that's still a way off.
Cheers, Daniel
> +
> if (dev_priv->fbc.fbc_work == NULL)
> return;
>
> @@ -384,6 +388,8 @@ static void intel_fbc_enable(struct drm_crtc *crtc)
> struct drm_device *dev = crtc->dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
>
> + lockdep_assert_held(&dev_priv->fbc.lock);
> +
> if (!dev_priv->display.enable_fbc)
> return;
>
> @@ -418,16 +424,12 @@ static void intel_fbc_enable(struct drm_crtc *crtc)
> schedule_delayed_work(&work->work, msecs_to_jiffies(50));
> }
>
> -/**
> - * intel_fbc_disable - disable FBC
> - * @dev: the drm_device
> - *
> - * This function disables FBC.
> - */
> -void intel_fbc_disable(struct drm_device *dev)
> +static void __intel_fbc_disable(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
>
> + lockdep_assert_held(&dev_priv->fbc.lock);
> +
> intel_fbc_cancel_work(dev_priv);
>
> if (!dev_priv->display.disable_fbc)
> @@ -437,6 +439,21 @@ void intel_fbc_disable(struct drm_device *dev)
> dev_priv->fbc.crtc = NULL;
> }
>
> +/**
> + * intel_fbc_disable - disable FBC
> + * @dev: the drm_device
> + *
> + * This function disables FBC.
> + */
> +void intel_fbc_disable(struct drm_device *dev)
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> +
> + mutex_lock(&dev_priv->fbc.lock);
> + __intel_fbc_disable(dev);
> + mutex_unlock(&dev_priv->fbc.lock);
> +}
> +
> const char *intel_no_fbc_reason_str(enum no_fbc_reason reason)
> {
> switch (reason) {
> @@ -516,7 +533,7 @@ static struct drm_crtc *intel_fbc_find_crtc(struct drm_i915_private *dev_priv)
> }
>
> /**
> - * intel_fbc_update - enable/disable FBC as needed
> + * __intel_fbc_update - enable/disable FBC as needed, unlocked
> * @dev: the drm_device
> *
> * Set up the framebuffer compression hardware at mode set time. We
> @@ -534,7 +551,7 @@ static struct drm_crtc *intel_fbc_find_crtc(struct drm_i915_private *dev_priv)
> *
> * We need to enable/disable FBC on a global basis.
> */
> -void intel_fbc_update(struct drm_device *dev)
> +static void __intel_fbc_update(struct drm_device *dev)
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct drm_crtc *crtc = NULL;
> @@ -544,6 +561,8 @@ void intel_fbc_update(struct drm_device *dev)
> const struct drm_display_mode *adjusted_mode;
> unsigned int max_width, max_height;
>
> + lockdep_assert_held(&dev_priv->fbc.lock);
> +
> if (!HAS_FBC(dev))
> return;
>
> @@ -665,7 +684,7 @@ void intel_fbc_update(struct drm_device *dev)
> * some point. And we wait before enabling FBC anyway.
> */
> DRM_DEBUG_KMS("disabling active FBC for update\n");
> - intel_fbc_disable(dev);
> + __intel_fbc_disable(dev);
> }
>
> intel_fbc_enable(crtc);
> @@ -676,11 +695,26 @@ out_disable:
> /* Multiple disables should be harmless */
> if (intel_fbc_enabled(dev)) {
> DRM_DEBUG_KMS("unsupported config, disabling FBC\n");
> - intel_fbc_disable(dev);
> + __intel_fbc_disable(dev);
> }
> i915_gem_stolen_cleanup_compression(dev);
> }
>
> +/*
> + * intel_fbc_update - enable/disable FBC as needed
> + * @dev: the drm_device
> + *
> + * This function reevaluates the overall state and enables or disables FBC.
> + */
> +void intel_fbc_update(struct drm_device *dev)
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> +
> + mutex_lock(&dev_priv->fbc.lock);
> + __intel_fbc_update(dev);
> + mutex_unlock(&dev_priv->fbc.lock);
> +}
> +
> void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
> unsigned int frontbuffer_bits,
> enum fb_op_origin origin)
> @@ -691,6 +725,8 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
> if (origin == ORIGIN_GTT)
> return;
>
> + mutex_lock(&dev_priv->fbc.lock);
> +
> if (dev_priv->fbc.enabled)
> fbc_bits = INTEL_FRONTBUFFER_PRIMARY(dev_priv->fbc.crtc->pipe);
> else if (dev_priv->fbc.fbc_work)
> @@ -702,7 +738,9 @@ void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
> dev_priv->fbc.busy_bits |= (fbc_bits & frontbuffer_bits);
>
> if (dev_priv->fbc.busy_bits)
> - intel_fbc_disable(dev);
> + __intel_fbc_disable(dev);
> +
> + mutex_unlock(&dev_priv->fbc.lock);
> }
>
> void intel_fbc_flush(struct drm_i915_private *dev_priv,
> @@ -710,13 +748,18 @@ void intel_fbc_flush(struct drm_i915_private *dev_priv,
> {
> struct drm_device *dev = dev_priv->dev;
>
> + mutex_lock(&dev_priv->fbc.lock);
> +
> if (!dev_priv->fbc.busy_bits)
> - return;
> + goto out;
>
> dev_priv->fbc.busy_bits &= ~frontbuffer_bits;
>
> if (!dev_priv->fbc.busy_bits)
> - intel_fbc_update(dev);
> + __intel_fbc_update(dev);
> +
> +out:
> + mutex_unlock(&dev_priv->fbc.lock);
> }
>
> /**
> @@ -729,6 +772,8 @@ void intel_fbc_init(struct drm_i915_private *dev_priv)
> {
> enum pipe pipe;
>
> + mutex_init(&dev_priv->fbc.lock);
> +
> if (!HAS_FBC(dev_priv)) {
> dev_priv->fbc.enabled = false;
> dev_priv->fbc.no_fbc_reason = FBC_UNSUPPORTED;
> --
> 2.1.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the Intel-gfx
mailing list