Re: [PATCH 1/6] drm/i915: kill dev_priv->pm.regsave

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



2014-03-20 9:58 GMT-03:00 Imre Deak <imre.deak@xxxxxxxxx>:
> On Fri, 2014-03-07 at 20:12 -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
>>
>> Now that we don't keep the hotplug interrupts enabled anymore, we can
>> kill the regsave struct and just cal the normal IRQ preinstall,
>> postinstall and uninstall functions. This makes it easier to add
>> runtime PM support to non-HSW platforms.
>>
>> The only downside is in case we get a request to update interrupts
>> while they are disabled, won't be able to update the regsave struct.
>> But this should never happen anyway, so we're not losing too much.
>>
>> v2: - Rebase.
>> v3: - Rebase.
>> v4: - Rebase.
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h      | 12 +-----
>>  drivers/gpu/drm/i915/i915_irq.c      | 79 +++++-------------------------------
>>  drivers/gpu/drm/i915/intel_display.c |  4 +-
>>  drivers/gpu/drm/i915/intel_drv.h     |  4 +-
>>  4 files changed, 15 insertions(+), 84 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 70feb61..493579d 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1351,23 +1351,13 @@ struct ilk_wm_values {
>>   * goes back to false exactly before we reenable the IRQs. We use this variable
>>   * to check if someone is trying to enable/disable IRQs while they're supposed
>>   * to be disabled. This shouldn't happen and we'll print some error messages in
>> - * case it happens, but if it actually happens we'll also update the variables
>> - * inside struct regsave so when we restore the IRQs they will contain the
>> - * latest expected values.
>> + * case it happens.
>>   *
>>   * For more, read the Documentation/power/runtime_pm.txt.
>>   */
>>  struct i915_runtime_pm {
>>       bool suspended;
>>       bool irqs_disabled;
>> -
>> -     struct {
>> -             uint32_t deimr;
>> -             uint32_t sdeimr;
>> -             uint32_t gtimr;
>> -             uint32_t gtier;
>> -             uint32_t gen6_pmimr;
>> -     } regsave;
>>  };
>>
>>  enum intel_pipe_crc_source {
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
>> index dee3a3b..2aefb94 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -131,11 +131,8 @@ ironlake_enable_display_irq(drm_i915_private_t *dev_priv, u32 mask)
>>  {
>>       assert_spin_locked(&dev_priv->irq_lock);
>>
>> -     if (dev_priv->pm.irqs_disabled) {
>> -             WARN(1, "IRQs disabled\n");
>> -             dev_priv->pm.regsave.deimr &= ~mask;
>> +     if (WARN_ON(dev_priv->pm.irqs_disabled))
>>               return;
>> -     }
>>
>>       if ((dev_priv->irq_mask & mask) != 0) {
>>               dev_priv->irq_mask &= ~mask;
>> @@ -149,11 +146,8 @@ ironlake_disable_display_irq(drm_i915_private_t *dev_priv, u32 mask)
>>  {
>>       assert_spin_locked(&dev_priv->irq_lock);
>>
>> -     if (dev_priv->pm.irqs_disabled) {
>> -             WARN(1, "IRQs disabled\n");
>> -             dev_priv->pm.regsave.deimr |= mask;
>> +     if (WARN_ON(dev_priv->pm.irqs_disabled))
>>               return;
>> -     }
>>
>>       if ((dev_priv->irq_mask & mask) != mask) {
>>               dev_priv->irq_mask |= mask;
>> @@ -174,13 +168,8 @@ static void ilk_update_gt_irq(struct drm_i915_private *dev_priv,
>>  {
>>       assert_spin_locked(&dev_priv->irq_lock);
>>
>> -     if (dev_priv->pm.irqs_disabled) {
>> -             WARN(1, "IRQs disabled\n");
>> -             dev_priv->pm.regsave.gtimr &= ~interrupt_mask;
>> -             dev_priv->pm.regsave.gtimr |= (~enabled_irq_mask &
>> -                                             interrupt_mask);
>> +     if (WARN_ON(dev_priv->pm.irqs_disabled))
>>               return;
>> -     }
>>
>>       dev_priv->gt_irq_mask &= ~interrupt_mask;
>>       dev_priv->gt_irq_mask |= (~enabled_irq_mask & interrupt_mask);
>> @@ -212,13 +201,8 @@ static void snb_update_pm_irq(struct drm_i915_private *dev_priv,
>>
>>       assert_spin_locked(&dev_priv->irq_lock);
>>
>> -     if (dev_priv->pm.irqs_disabled) {
>> -             WARN(1, "IRQs disabled\n");
>> -             dev_priv->pm.regsave.gen6_pmimr &= ~interrupt_mask;
>> -             dev_priv->pm.regsave.gen6_pmimr |= (~enabled_irq_mask &
>> -                                                  interrupt_mask);
>> +     if (WARN_ON(dev_priv->pm.irqs_disabled))
>>               return;
>> -     }
>>
>>       new_val = dev_priv->pm_irq_mask;
>>       new_val &= ~interrupt_mask;
>> @@ -358,14 +342,8 @@ static void ibx_display_interrupt_update(struct drm_i915_private *dev_priv,
>>
>>       assert_spin_locked(&dev_priv->irq_lock);
>>
>> -     if (dev_priv->pm.irqs_disabled &&
>> -         (interrupt_mask & SDE_HOTPLUG_MASK_CPT)) {
>> -             WARN(1, "IRQs disabled\n");
>> -             dev_priv->pm.regsave.sdeimr &= ~interrupt_mask;
>> -             dev_priv->pm.regsave.sdeimr |= (~enabled_irq_mask &
>> -                                              interrupt_mask);
>> +     if (WARN_ON(dev_priv->pm.irqs_disabled))
>>               return;
>> -     }
>>
>>       I915_WRITE(SDEIMR, sdeimr);
>>       POSTING_READ(SDEIMR);
>> @@ -4091,57 +4069,20 @@ void intel_hpd_init(struct drm_device *dev)
>>  }
>>
>>  /* Disable interrupts so we can allow runtime PM. */
>> -void hsw_runtime_pm_disable_interrupts(struct drm_device *dev)
>> +void intel_runtime_pm_disable_interrupts(struct drm_device *dev)
>>  {
>>       struct drm_i915_private *dev_priv = dev->dev_private;
>> -     unsigned long irqflags;
>> -
>> -     spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
>> -
>> -     dev_priv->pm.regsave.deimr = I915_READ(DEIMR);
>> -     dev_priv->pm.regsave.sdeimr = I915_READ(SDEIMR);
>> -     dev_priv->pm.regsave.gtimr = I915_READ(GTIMR);
>> -     dev_priv->pm.regsave.gtier = I915_READ(GTIER);
>> -     dev_priv->pm.regsave.gen6_pmimr = I915_READ(GEN6_PMIMR);
>> -
>> -     ironlake_disable_display_irq(dev_priv, 0xffffffff);
>> -     ibx_disable_display_interrupt(dev_priv, 0xffffffff);
>> -     ilk_disable_gt_irq(dev_priv, 0xffffffff);
>> -     snb_disable_pm_irq(dev_priv, 0xffffffff);
>>
>> +     dev->driver->irq_uninstall(dev);
>>       dev_priv->pm.irqs_disabled = true;
>> -
>> -     spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>
> It made me think whether removing the locking here is ok. It seems to be
> ok, as we get here in an idle state where no interrupts should happen. A
> note about this change in the commit message would have been nice.
> Otherwise the patch looks ok:
>
> Reviewed-by: Imre Deak <imre.deak@xxxxxxxxx>

I wrote this patch many months ago, something in my head was telling
me there was a reason behind the removal, but I can't remember, and I
added it back and it seems to work just fine... I guess I'll keep it
there and continue testing.

>
>
>>  }
>>
>>  /* Restore interrupts so we can recover from runtime PM. */
>> -void hsw_runtime_pm_restore_interrupts(struct drm_device *dev)
>> +void intel_runtime_pm_restore_interrupts(struct drm_device *dev)
>>  {
>>       struct drm_i915_private *dev_priv = dev->dev_private;
>> -     unsigned long irqflags;
>> -     uint32_t val;
>> -
>> -     spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
>> -
>> -     val = I915_READ(DEIMR);
>> -     WARN(val != 0xffffffff, "DEIMR is 0x%08x\n", val);
>> -
>> -     val = I915_READ(SDEIMR);
>> -     WARN(val != 0xffffffff, "SDEIMR is 0x%08x\n", val);
>> -
>> -     val = I915_READ(GTIMR);
>> -     WARN(val != 0xffffffff, "GTIMR is 0x%08x\n", val);
>> -
>> -     val = I915_READ(GEN6_PMIMR);
>> -     WARN(val != 0xffffffff, "GEN6_PMIMR is 0x%08x\n", val);
>>
>>       dev_priv->pm.irqs_disabled = false;
>> -
>> -     ironlake_enable_display_irq(dev_priv, ~dev_priv->pm.regsave.deimr);
>> -     ibx_enable_display_interrupt(dev_priv, ~dev_priv->pm.regsave.sdeimr);
>> -     ilk_enable_gt_irq(dev_priv, ~dev_priv->pm.regsave.gtimr);
>> -     snb_enable_pm_irq(dev_priv, ~dev_priv->pm.regsave.gen6_pmimr);
>> -     I915_WRITE(GTIER, dev_priv->pm.regsave.gtier);
>> -
>> -     spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>> +     dev->driver->irq_preinstall(dev);
>> +     dev->driver->irq_postinstall(dev);
>>  }
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index ed9233e..df69866 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -6829,7 +6829,7 @@ void hsw_enable_pc8(struct drm_i915_private *dev_priv)
>>       }
>>
>>       lpt_disable_clkout_dp(dev);
>> -     hsw_runtime_pm_disable_interrupts(dev);
>> +     intel_runtime_pm_disable_interrupts(dev);
>>       hsw_disable_lcpll(dev_priv, true, true);
>>  }
>>
>> @@ -6843,7 +6843,7 @@ void hsw_disable_pc8(struct drm_i915_private *dev_priv)
>>       DRM_DEBUG_KMS("Disabling package C8+\n");
>>
>>       hsw_restore_lcpll(dev_priv);
>> -     hsw_runtime_pm_restore_interrupts(dev);
>> +     intel_runtime_pm_restore_interrupts(dev);
>>       lpt_init_pch_refclk(dev);
>>
>>       if (dev_priv->pch_id == INTEL_PCH_LPT_LP_DEVICE_ID_TYPE) {
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 9a7db84..0dfb6e9 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -618,8 +618,8 @@ void ilk_enable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask);
>>  void ilk_disable_gt_irq(struct drm_i915_private *dev_priv, uint32_t mask);
>>  void snb_enable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask);
>>  void snb_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask);
>> -void hsw_runtime_pm_disable_interrupts(struct drm_device *dev);
>> -void hsw_runtime_pm_restore_interrupts(struct drm_device *dev);
>> +void intel_runtime_pm_disable_interrupts(struct drm_device *dev);
>> +void intel_runtime_pm_restore_interrupts(struct drm_device *dev);
>>
>>
>>  /* intel_crt.c */
>



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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux