On Thu, Nov 28, 2013 at 10:04:42AM +0200, Imre Deak wrote:
> So far we had a race during error capturing where a power domain could
> get disabled after we queried its state to be on. Fix this by protecting
> the power domain state tracking changes with a lock and holding this
> lock during error capturing.
>
> Note that this still allows the case where we are halfway in enabling a
> power domain and still report it as disabled. This isn't a problem as we
> only want to avoid register accesses while the domain is off and that is
> now guaranteed.
>
> Signed-off-by: Imre Deak <imre.deak@xxxxxxxxx>
>
> [ Applies on top of "drm/i915: add intel_display_power_enabled_sw() for
> use in atomic ctx" ]
Tbh I don't see the point for this - the error capture code is inherently
racy, we don't grab any of the other modeset locks. We also don't care and
furthermore we should try to grab as few locks as possible to increase the
chances that we can actually capture the error state successfully. Every
lock you add adds another depency and so more ways to end in tears and
deadlocks.
So if the racy error capture is the only reason I'll reject this.
-Daniel
> ---
> drivers/gpu/drm/i915/i915_drv.h | 1 +
> drivers/gpu/drm/i915/intel_display.c | 5 +++++
> drivers/gpu/drm/i915/intel_pm.c | 5 +++++
> 3 files changed, 11 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index cc06b9f..9e8d117 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -971,6 +971,7 @@ struct i915_power_domains {
> int power_well_count;
>
> struct mutex lock;
> + spinlock_t irq_lock; /* protects domain_use_count */
> int domain_use_count[POWER_DOMAIN_NUM];
> struct i915_power_well *power_wells;
> };
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 464ae66..c7feee4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11368,6 +11368,7 @@ intel_display_capture_error_state(struct drm_device *dev)
> {
> drm_i915_private_t *dev_priv = dev->dev_private;
> struct intel_display_error_state *error;
> + unsigned long flags;
> int transcoders[] = {
> TRANSCODER_A,
> TRANSCODER_B,
> @@ -11386,6 +11387,8 @@ intel_display_capture_error_state(struct drm_device *dev)
> if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> error->power_well_driver = I915_READ(HSW_PWR_WELL_DRIVER);
>
> + spin_lock_irqsave(&dev_priv->power_domains.irq_lock, flags);
> +
> for_each_pipe(i) {
> error->pipe[i].power_domain_on =
> intel_display_power_enabled_sw(dev, POWER_DOMAIN_PIPE(i));
> @@ -11441,6 +11444,8 @@ intel_display_capture_error_state(struct drm_device *dev)
> error->transcoder[i].vsync = I915_READ(VSYNC(cpu_transcoder));
> }
>
> + spin_unlock_irqrestore(&dev_priv->power_domains.irq_lock, flags);
> +
> return error;
> }
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 311199c..00f6b34 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5769,7 +5769,9 @@ void intel_display_power_get(struct drm_device *dev,
> for_each_power_well(i, power_well, BIT(domain), power_domains)
> __intel_power_well_get(dev, power_well);
>
> + spin_lock_irq(&power_domains->irq_lock);
> power_domains->domain_use_count[domain]++;
> + spin_unlock_irq(&power_domains->irq_lock);
>
> mutex_unlock(&power_domains->lock);
> }
> @@ -5787,7 +5789,9 @@ void intel_display_power_put(struct drm_device *dev,
> mutex_lock(&power_domains->lock);
>
> WARN_ON(!power_domains->domain_use_count[domain]);
> + spin_lock_irq(&power_domains->irq_lock);
> power_domains->domain_use_count[domain]--;
> + spin_unlock_irq(&power_domains->irq_lock);
>
> for_each_power_well_rev(i, power_well, BIT(domain), power_domains)
> __intel_power_well_put(dev, power_well);
> @@ -5871,6 +5875,7 @@ int intel_power_domains_init(struct drm_device *dev)
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct i915_power_domains *power_domains = &dev_priv->power_domains;
>
> + spin_lock_init(&power_domains->irq_lock);
> mutex_init(&power_domains->lock);
>
> /*
> --
> 1.8.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
