On Thursday, 3 of April 2008, Jesse Barnes wrote:
> On Tuesday, April 01, 2008 5:09 pm Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rjw@xxxxxxx>
> >
> > Introduce 'struct pm_ops' and 'struct pm_ext_ops' ('ext' meaning
> > 'extended') representing suspend and hibernation operations for bus
> > types, device classes, device types and device drivers.
> >
> > Modify the PM core to use 'struct pm_ops' and 'struct pm_ext_ops'
> > objects, if defined, instead of the ->suspend() and ->resume(),
> > ->suspend_late(), and ->resume_early() callbacks (the old callbacks
> > will be considered as legacy and gradually phased out).
> >
> > The main purpose of doing this is to separate suspend (aka S2RAM and
> > standby) callbacks from hibernation callbacks in such a way that the
> > new callbacks won't take arguments and the semantics of each of them
> > will be clearly specified. This has been requested for multiple
> > times by many people, including Linus himself, and the reason is that
> > within the current scheme if ->resume() is called, for example, it's
> > difficult to say why it's been called (ie. is it a resume from RAM or
> > from hibernation or a suspend/hibernation failure etc.?).
>
> I like the new ops much better; their purpose is clearer and better separated
> than before.
Well, that's the idea. :-)
> I think the i915 changes should look something like this?
Basically, yes, but with one comment (below).
> Also, what about class devices? Right now, they just have suspend & resume
> callbacks, not full pm_ops structures.
They just haven't been modified yet, but that's going to happen.
> But maybe they're not really necessary anyway,
IIRC, there are some device classes that may need them. Like leds etc.
> I could set the pm_ops.prepare & complete callbacks to DRM core routines in
> order to suspend & resume DRM client requests...
That would be the way to go, IMHO.
> Also, it looks like the PCI bits I had in i915 aren't really necessary?
Well, I think some of them are.
> diff --git a/drivers/char/drm/i915_drv.c b/drivers/char/drm/i915_drv.c
> index b2b451d..ec6356a 100644
> --- a/drivers/char/drm/i915_drv.c
> +++ b/drivers/char/drm/i915_drv.c
> @@ -239,8 +239,9 @@ static void i915_restore_vga(struct drm_device *dev)
>
> }
>
> -static int i915_suspend(struct drm_device *dev, pm_message_t state)
> +static int i915_save(struct device *device)
> {
> + struct drm_device *dev = container_of(device, struct drm_device, dev);
> struct drm_i915_private *dev_priv = dev->dev_private;
> int i;
>
> @@ -250,10 +251,6 @@ static int i915_suspend(struct drm_device *dev,
> pm_message_t state)
> return -ENODEV;
> }
>
> - if (state.event == PM_EVENT_PRETHAW)
> - return 0;
> -
> - pci_save_state(dev->pdev);
> pci_read_config_byte(dev->pdev, LBB, &dev_priv->saveLBB);
>
> /* Pipe & plane A info */
> @@ -367,24 +364,16 @@ static int i915_suspend(struct drm_device *dev,
> pm_message_t state)
>
> i915_save_vga(dev);
>
> - if (state.event == PM_EVENT_SUSPEND) {
> - /* Shut down the device */
> - pci_disable_device(dev->pdev);
> - pci_set_power_state(dev->pdev, PCI_D3hot);
> - }
> -
> return 0;
> }
>
> -static int i915_resume(struct drm_device *dev)
> +static int i915_restore(struct device *device)
> {
> + struct drm_device *dev = container_of(device, struct drm_device, dev);
> struct drm_i915_private *dev_priv = dev->dev_private;
> int i;
>
> pci_set_power_state(dev->pdev, PCI_D0);
> - pci_restore_state(dev->pdev);
> - if (pci_enable_device(dev->pdev))
> - return -1;
>
> pci_write_config_byte(dev->pdev, LBB, dev_priv->saveLBB);
>
> @@ -527,6 +516,23 @@ static int i915_resume(struct drm_device *dev)
> return 0;
> }
>
> +static int i915_poweroff(struct device *dev)
> +{
> + /* Shut down the device */
> + pci_disable_device(dev->pdev);
> + pci_set_power_state(dev->pdev, PCI_D3hot);
I think you may need to do that in ->suspend() too, as opposed to ->freeze(),
...
> +}
> +
> +static struct pm_ops i915_pm_ops = {
> + .prepare = NULL, /* DRM core should prevent any new ioctls? */
> + .complete = NULL, /* required to re-enable DRM client requests */
> + .suspend = i915_save,
> + .resume = i915_restore,
> + .freeze = i915_save,
... so perhaps define ->suspend() as ->save() + ->poweroff()?
> + .restore = i915_restore,
> + .poweroff = i915_poweroff,
> +};
> +
> static struct drm_driver driver = {
> /* don't use mtrr's here, the Xserver or user space app should
> * deal with them for intel hardware.
> @@ -539,8 +545,6 @@ static struct drm_driver driver = {
> .unload = i915_driver_unload,
> .lastclose = i915_driver_lastclose,
> .preclose = i915_driver_preclose,
> - .suspend = i915_suspend,
> - .resume = i915_resume,
> .device_is_agp = i915_driver_device_is_agp,
> .vblank_wait = i915_driver_vblank_wait,
> .vblank_wait2 = i915_driver_vblank_wait2,
> @@ -581,6 +585,7 @@ static struct drm_driver driver = {
> static int __init i915_init(void)
> {
> driver.num_ioctls = i915_max_ioctl;
> + driver->dev.pm_ops = &i915_pm_ops;
> return drm_init(&driver);
> }
Well, I see I should push the patches to Greg ... ;-)
Thanks,
Rafael
_______________________________________________
linux-pm mailing list
linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
