Re: [RFC 1/6] drm/i915: add initial Runtime PM functions

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

 



2013/10/28 Imre Deak <imre.deak@xxxxxxxxx>:
> On Tue, 2013-10-22 at 17:30 -0200, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
>>
>> This patch adds the initial infrastructure to allow a Runtime PM
>> implementation that sets the device to its D3 state. The patch just
>> adds the necessary callbacks and the initial infrastructure.
>>
>> We still don't have any platform that actually uses this
>> infrastructure, we still don't call get/put in all the places we need
>> to, and we don't have any function to save/restore the state of the
>> registers. This is not a problem since no platform uses the code added
>> by this patch. We have a few people simultaneously working on runtime
>> PM, so this initial code could help everybody make their plans.
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
>> ---
>>  drivers/gpu/drm/i915/i915_dma.c     | 38 +++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/i915/i915_drv.c     | 42 +++++++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/i915/i915_drv.h     |  7 +++++++
>>  drivers/gpu/drm/i915/intel_drv.h    |  2 ++
>>  drivers/gpu/drm/i915/intel_pm.c     | 26 +++++++++++++++++++++++
>>  drivers/gpu/drm/i915/intel_uncore.c |  9 ++++++++
>>  6 files changed, 124 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
>> index fd848ef..6aa044e 100644
>> --- a/drivers/gpu/drm/i915/i915_dma.c
>> +++ b/drivers/gpu/drm/i915/i915_dma.c
>> @@ -42,6 +42,8 @@
>>  #include <linux/vga_switcheroo.h>
>>  #include <linux/slab.h>
>>  #include <acpi/video.h>
>> +#include <linux/pm.h>
>> +#include <linux/pm_runtime.h>
>>
>>  #define LP_RING(d) (&((struct drm_i915_private *)(d))->ring[RCS])
>>
>> @@ -1449,6 +1451,38 @@ static void i915_dump_device_info(struct drm_i915_private *dev_priv)
>>  #undef SEP_COMMA
>>  }
>>
>> +static void i915_init_runtime_pm(struct drm_i915_private *dev_priv)
>> +{
>> +     struct drm_device *dev = dev_priv->dev;
>> +     struct device *device = &dev->pdev->dev;
>> +
>> +     dev_priv->pm.suspended = false;
>> +
>> +     if (!HAS_RUNTIME_PM(dev))
>> +             return;
>> +
>> +     pm_runtime_set_active(device);
>> +     pm_runtime_enable(device);
>> +
>> +     pm_runtime_set_autosuspend_delay(device, 10000); /* 10s */
>> +     pm_runtime_mark_last_busy(device);
>> +     pm_runtime_use_autosuspend(device);
>> +     pm_runtime_allow(device);
>
> This shouldn't be needed as we get here already with an allowed state.
> It's not a problem as it's just a nop here, but imo it's confusing that
> we don't have the corresponding pm_runtime_forbid() in
> i915_fini_runtime_pm().

If we don't call this, when we boot the machine the "power/control"
sysfs file will be "on", which means runtime PM is disabled. We have
to manually "echo auto > control" to enable runtime PM then. But I
guess leaving runtime PM disabled by default might be what we want, so
I'll remove the call here.


>
>> +}
>> +
>> +static void i915_fini_runtime_pm(struct drm_i915_private *dev_priv)
>> +{
>> +     struct drm_device *dev = dev_priv->dev;
>> +     struct device *device = &dev->pdev->dev;
>> +
>> +     if (!HAS_RUNTIME_PM(dev))
>> +             return;
>> +
>> +     /* Make sure we're not suspended first. */
>> +     pm_runtime_get_sync(device);
>> +     pm_runtime_disable(device);
>> +}
>> +
>
> Could we have the above functions in intel_pm.c?

Done.

>
>>  /**
>>   * i915_driver_load - setup chip and create an initial config
>>   * @dev: DRM device
>> @@ -1664,6 +1698,8 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>>       if (IS_GEN5(dev))
>>               intel_gpu_ips_init(dev_priv);
>>
>> +     i915_init_runtime_pm(dev_priv);
>> +
>>       return 0;
>>
>>  out_power_well:
>> @@ -1704,6 +1740,8 @@ int i915_driver_unload(struct drm_device *dev)
>>       struct drm_i915_private *dev_priv = dev->dev_private;
>>       int ret;
>>
>> +     i915_fini_runtime_pm(dev_priv);
>> +
>>       intel_gpu_ips_teardown();
>>
>>       if (HAS_POWER_WELL(dev)) {
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index 85ae0dc..08fc7ea 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -469,6 +469,8 @@ static int i915_drm_freeze(struct drm_device *dev)
>>       struct drm_i915_private *dev_priv = dev->dev_private;
>>       struct drm_crtc *crtc;
>>
>> +     intel_runtime_pm_get(dev_priv);
>> +
>>       /* ignore lid events during suspend */
>>       mutex_lock(&dev_priv->modeset_restore_lock);
>>       dev_priv->modeset_restore = MODESET_SUSPENDED;
>> @@ -653,6 +655,8 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
>>       mutex_lock(&dev_priv->modeset_restore_lock);
>>       dev_priv->modeset_restore = MODESET_DONE;
>>       mutex_unlock(&dev_priv->modeset_restore_lock);
>> +
>> +     intel_runtime_pm_put(dev_priv);
>>       return error;
>>  }
>>
>> @@ -880,6 +884,42 @@ static int i915_pm_poweroff(struct device *dev)
>>       return i915_drm_freeze(drm_dev);
>>  }
>>
>> +static int i915_runtime_suspend(struct device *device)
>> +{
>> +     struct pci_dev *pdev = to_pci_dev(device);
>> +     struct drm_device *dev = pci_get_drvdata(pdev);
>> +     struct drm_i915_private *dev_priv = dev->dev_private;
>> +
>> +     WARN_ON(!HAS_RUNTIME_PM(dev));
>> +
>> +     DRM_DEBUG_KMS("Suspending device\n");
>> +
>> +     dev_priv->pm.suspended = true;
>> +
>> +     pci_save_state(pdev);
>> +     pci_set_power_state(pdev, PCI_D3cold);
>> +
>> +     return 0;
>> +}
>> +
>> +static int i915_runtime_resume(struct device *device)
>> +{
>> +     struct pci_dev *pdev = to_pci_dev(device);
>> +     struct drm_device *dev = pci_get_drvdata(pdev);
>> +     struct drm_i915_private *dev_priv = dev->dev_private;
>> +
>> +     WARN_ON(!HAS_RUNTIME_PM(dev));
>> +
>> +     DRM_DEBUG_KMS("Resuming device\n");
>> +
>> +     pci_set_power_state(pdev, PCI_D0);
>> +     pci_restore_state(pdev);
>> +
>> +     dev_priv->pm.suspended = false;
>> +
>> +     return 0;
>> +}
>> +
>>  static const struct dev_pm_ops i915_pm_ops = {
>>       .suspend = i915_pm_suspend,
>>       .resume = i915_pm_resume,
>> @@ -887,6 +927,8 @@ static const struct dev_pm_ops i915_pm_ops = {
>>       .thaw = i915_pm_thaw,
>>       .poweroff = i915_pm_poweroff,
>>       .restore = i915_pm_resume,
>> +     .runtime_suspend = i915_runtime_suspend,
>> +     .runtime_resume = i915_runtime_resume,
>>  };
>>
>>  static const struct vm_operations_struct i915_gem_vm_ops = {
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 80957ca..74f2b5d 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1230,6 +1230,10 @@ struct i915_package_c8 {
>>       } regsave;
>>  };
>>
>> +struct i915_runtime_pm {
>> +     bool suspended;
>> +};
>> +
>>  enum intel_pipe_crc_source {
>>       INTEL_PIPE_CRC_SOURCE_NONE,
>>       INTEL_PIPE_CRC_SOURCE_PLANE1,
>> @@ -1462,6 +1466,8 @@ typedef struct drm_i915_private {
>>
>>       struct i915_package_c8 pc8;
>>
>> +     struct i915_runtime_pm pm;
>> +
>>       /* Old dri1 support infrastructure, beware the dragons ya fools entering
>>        * here! */
>>       struct i915_dri1_state dri1;
>> @@ -1779,6 +1785,7 @@ struct drm_i915_file_private {
>>  #define HAS_POWER_WELL(dev)  (IS_HASWELL(dev))
>>  #define HAS_FPGA_DBG_UNCLAIMED(dev)  (INTEL_INFO(dev)->has_fpga_dbg)
>>  #define HAS_PSR(dev)         (IS_HASWELL(dev))
>> +#define HAS_RUNTIME_PM(dev)  false
>>
>>  #define INTEL_PCH_DEVICE_ID_MASK             0xff00
>>  #define INTEL_PCH_IBX_DEVICE_ID_TYPE         0x3b00
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index af1553c..80b5a80 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -840,6 +840,8 @@ void gen6_rps_idle(struct drm_i915_private *dev_priv);
>>  void gen6_rps_boost(struct drm_i915_private *dev_priv);
>>  void intel_aux_display_runtime_get(struct drm_i915_private *dev_priv);
>>  void intel_aux_display_runtime_put(struct drm_i915_private *dev_priv);
>> +void intel_runtime_pm_get(struct drm_i915_private *dev_priv);
>> +void intel_runtime_pm_put(struct drm_i915_private *dev_priv);
>>  void ilk_wm_get_hw_state(struct drm_device *dev);
>>
>>
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index 3e140ab..a973f35 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -31,6 +31,7 @@
>>  #include "../../../platform/x86/intel_ips.h"
>>  #include <linux/module.h>
>>  #include <drm/i915_powerwell.h>
>> +#include <linux/pm_runtime.h>
>>
>>  /**
>>   * RC6 is a special power stage which allows the GPU to enter an very
>> @@ -5779,6 +5780,31 @@ void intel_aux_display_runtime_put(struct drm_i915_private *dev_priv)
>>       hsw_enable_package_c8(dev_priv);
>>  }
>>
>> +void intel_runtime_pm_get(struct drm_i915_private *dev_priv)
>> +{
>> +     struct drm_device *dev = dev_priv->dev;
>> +     struct device *device = &dev->pdev->dev;
>> +
>> +     if (!HAS_RUNTIME_PM(dev))
>> +             return;
>> +
>> +     pm_runtime_mark_last_busy(device);
>
> What's the point of calling this here? We call pm_runtime_get below,
> which will prevent an auto suspend anyway and we call
> pm_runtime_mark_last_busy() then intel_runtime_pm_put().
>
>> +     pm_runtime_get(device);
>> +     pm_runtime_barrier(device);
>
> Why not using pm_runtime_get_sync() instead of the above 2 calls?

Done. Also added a WARN just to make sure it does what we want (I had
some problems with this while debugging).

Thanks for the review!

>
> --Imre
>
>> +}
>> +
>> +void intel_runtime_pm_put(struct drm_i915_private *dev_priv)
>> +{
>> +     struct drm_device *dev = dev_priv->dev;
>> +     struct device *device = &dev->pdev->dev;
>> +
>> +     if (!HAS_RUNTIME_PM(dev))
>> +             return;
>> +
>> +     pm_runtime_mark_last_busy(device);
>> +     pm_runtime_put_autosuspend(device);
>> +}
>> +
>>  /* Set up chip specific power management-related functions */
>>  void intel_init_pm(struct drm_device *dev)
>>  {
>> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
>> index f6fae35..f5a2a6d 100644
>> --- a/drivers/gpu/drm/i915/intel_uncore.c
>> +++ b/drivers/gpu/drm/i915/intel_uncore.c
>> @@ -343,6 +343,13 @@ hsw_unclaimed_reg_check(struct drm_i915_private *dev_priv, u32 reg)
>>       }
>>  }
>>
>> +static void
>> +assert_device_not_suspended(struct drm_i915_private *dev_priv)
>> +{
>> +     WARN(HAS_RUNTIME_PM(dev_priv->dev) && dev_priv->pm.suspended,
>> +          "Device suspended\n");
>> +}
>> +
>>  #define REG_READ_HEADER(x) \
>>       unsigned long irqflags; \
>>       u##x val = 0; \
>> @@ -435,6 +442,7 @@ gen6_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace
>>       if (NEEDS_FORCE_WAKE((dev_priv), (reg))) { \
>>               __fifo_ret = __gen6_gt_wait_for_fifo(dev_priv); \
>>       } \
>> +     assert_device_not_suspended(dev_priv); \
>>       __raw_i915_write##x(dev_priv, reg, val); \
>>       if (unlikely(__fifo_ret)) { \
>>               gen6_gt_check_fifodbg(dev_priv); \
>> @@ -450,6 +458,7 @@ hsw_write##x(struct drm_i915_private *dev_priv, off_t reg, u##x val, bool trace)
>>       if (NEEDS_FORCE_WAKE((dev_priv), (reg))) { \
>>               __fifo_ret = __gen6_gt_wait_for_fifo(dev_priv); \
>>       } \
>> +     assert_device_not_suspended(dev_priv); \
>>       hsw_unclaimed_reg_clear(dev_priv, reg); \
>>       __raw_i915_write##x(dev_priv, reg, val); \
>>       if (unlikely(__fifo_ret)) { \
>



-- 
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