On Wed, Aug 05, 2020 at 03:19:01PM -0500, Bjorn Helgaas wrote:

On Wed, Aug 05, 2020 at 11:37:11PM +0530, Vaibhav Gupta wrote:

...

Drivers using legacy power management .suspen()/.resume() callbacks have to manage PCI states and device's PM states themselves. They also need to take care of standard configuration registers.

s/using legacy/using legacy PCI/ s/.suspen/.suspend/ (in all these patches)

Oh, that's a blunder. Since most of the drivers in my project need similar changes, I made a template for commit message. And by mistake I would have edited the template itself.

I wouldn't necessarily repost the whole series just for that (unless the maintainer wants it), but maybe update your branch so if you have occasion to repost for other reasons, this will be fixed.

This particular driver actually doesn't *do* any of the PCI state or device PM state management you mention. And I don't see the "single 'struct dev_pm_ops'" you mention below -- I thought that meant you would have a single struct shared between drivers (I think you did that for IDE?), but that's not what you're doing. This driver has gxfb_pm_ops, the next has lxfb_pm_ops, etc.

Yeah, the sentence sounds misleading. What I meant was that earlier there were two pointers for PM, .suspend and .resume. Whereas now there is a single "struct dev_pm_ops" variable inside pci_driver.

AFAICT the patches are fine, but the commit logs don't seem exactly accurate.

I am fixing it.

Thanks Vaibhav Gupta

...

Switch to generic power management framework using a single "struct dev_pm_ops" variable to take the unnecessary load from the driver. This also avoids the need for the driver to directly call most of the PCI helper functions and device power state control functions, as through the generic framework PCI Core takes care of the necessary operations, and drivers are required to do only device-specific jobs.

Signed-off-by: Vaibhav Gupta vaibhavgupta40@gmail.com

drivers/video/fbdev/geode/gxfb.h | 5 ---- drivers/video/fbdev/geode/gxfb_core.c | 36 ++++++++++++++------------ drivers/video/fbdev/geode/suspend_gx.c | 4 --- 3 files changed, 20 insertions(+), 25 deletions(-)

diff --git a/drivers/video/fbdev/geode/gxfb.h b/drivers/video/fbdev/geode/gxfb.h index d2e9c5c8e294..792c111c21e4 100644 --- a/drivers/video/fbdev/geode/gxfb.h +++ b/drivers/video/fbdev/geode/gxfb.h @@ -21,7 +21,6 @@ struct gxfb_par { void __iomem *dc_regs; void __iomem *vid_regs; void __iomem *gp_regs; -#ifdef CONFIG_PM int powered_down;

/* register state, for power management functionality */ @@ -36,7 +35,6 @@ struct gxfb_par { uint64_t fp[FP_REG_COUNT];

uint32_t pal[DC_PAL_COUNT]; -#endif };

unsigned int gx_frame_buffer_size(void); @@ -49,11 +47,8 @@ void gx_set_dclk_frequency(struct fb_info *info); void gx_configure_display(struct fb_info *info); int gx_blank_display(struct fb_info *info, int blank_mode);

-#ifdef CONFIG_PM int gx_powerdown(struct fb_info *info); int gx_powerup(struct fb_info *info); -#endif

/* Graphics Processor registers (table 6-23 from the data book) */ enum gp_registers { diff --git a/drivers/video/fbdev/geode/gxfb_core.c b/drivers/video/fbdev/geode/gxfb_core.c index d38a148d4746..44089b331f91 100644 --- a/drivers/video/fbdev/geode/gxfb_core.c +++ b/drivers/video/fbdev/geode/gxfb_core.c @@ -322,17 +322,14 @@ static struct fb_info *gxfb_init_fbinfo(struct device *dev) return info; }

-#ifdef CONFIG_PM -static int gxfb_suspend(struct pci_dev *pdev, pm_message_t state) +static int __maybe_unused gxfb_suspend(struct device *dev) {

• struct fb_info *info = pci_get_drvdata(pdev);

• struct fb_info *info = dev_get_drvdata(dev);

• if (state.event == PM_EVENT_SUSPEND) {

• console_lock();
  

• gx_powerdown(info);
  

• fb_set_suspend(info, 1);
  

• console_unlock();
  

• }

• console_lock();

• gx_powerdown(info);

• fb_set_suspend(info, 1);

• console_unlock();

  /* there's no point in setting PCI states; we emulate PCI, so

  • we don't end up getting power savings anyways */

@@ -340,9 +337,9 @@ static int gxfb_suspend(struct pci_dev *pdev, pm_message_t state) return 0; }

-static int gxfb_resume(struct pci_dev *pdev) +static int __maybe_unused gxfb_resume(struct device *dev) {

• struct fb_info *info = pci_get_drvdata(pdev);

• struct fb_info *info = dev_get_drvdata(dev); int ret;

  console_lock();

@@ -356,7 +353,6 @@ static int gxfb_resume(struct pci_dev *pdev) console_unlock(); return 0; } -#endif

static int gxfb_probe(struct pci_dev *pdev, const struct pci_device_id *id) { @@ -467,15 +463,23 @@ static const struct pci_device_id gxfb_id_table[] = {

MODULE_DEVICE_TABLE(pci, gxfb_id_table);

+static const struct dev_pm_ops gxfb_pm_ops = { +#ifdef CONFIG_PM_SLEEP

• .suspend = gxfb_suspend,
• .resume = gxfb_resume,
• .freeze = NULL,
• .thaw = gxfb_resume,
• .poweroff = NULL,
• .restore = gxfb_resume,

+#endif +};

static struct pci_driver gxfb_driver = { .name = "gxfb", .id_table = gxfb_id_table, .probe = gxfb_probe, .remove = gxfb_remove, -#ifdef CONFIG_PM

• .suspend = gxfb_suspend,
• .resume = gxfb_resume,

-#endif

• .driver.pm = &gxfb_pm_ops,

};

#ifndef MODULE diff --git a/drivers/video/fbdev/geode/suspend_gx.c b/drivers/video/fbdev/geode/suspend_gx.c index 1110a527c35c..8c49d4e98772 100644 --- a/drivers/video/fbdev/geode/suspend_gx.c +++ b/drivers/video/fbdev/geode/suspend_gx.c @@ -11,8 +11,6 @@

#include "gxfb.h"

-#ifdef CONFIG_PM

static void gx_save_regs(struct gxfb_par *par) { int i; @@ -259,5 +257,3 @@ int gx_powerup(struct fb_info *info) par->powered_down = 0; return 0; }

-#endif

2.27.0

On Sat, Aug 08, 2020 at 01:17:46PM +0200, Sam Ravnborg wrote:

Hi Vaibhav

On Wed, Aug 05, 2020 at 11:37:11PM +0530, Vaibhav Gupta wrote:

...

Drivers using legacy power management .suspen()/.resume() callbacks have to manage PCI states and device's PM states themselves. They also need to take care of standard configuration registers.

Switch to generic power management framework using a single "struct dev_pm_ops" variable

"to take the unnecessary load from the driver."

• I do not parse the above - I cannot see what load is removed.

But the code is simpler which is fine. The drawback is that we now always link in the suspend_gx functions but hopefultl the linker drops them later.

...

This also avoids the need for the driver to directly call most of the PCI helper functions and device power state control functions, as through the generic framework PCI Core takes care of the necessary operations, and drivers are required to do only device-specific jobs.

Again, I do not see what calles are removed. A single check for the state is dropped - anything else?

Yeah, the commit messages are bit misleading, I have modified them.

...

Signed-off-by: Vaibhav Gupta vaibhavgupta40@gmail.com

drivers/video/fbdev/geode/gxfb.h | 5 ---- drivers/video/fbdev/geode/gxfb_core.c | 36 ++++++++++++++------------ drivers/video/fbdev/geode/suspend_gx.c | 4 --- 3 files changed, 20 insertions(+), 25 deletions(-)

diff --git a/drivers/video/fbdev/geode/gxfb.h b/drivers/video/fbdev/geode/gxfb.h index d2e9c5c8e294..792c111c21e4 100644 --- a/drivers/video/fbdev/geode/gxfb.h +++ b/drivers/video/fbdev/geode/gxfb.h @@ -21,7 +21,6 @@ struct gxfb_par { void __iomem *dc_regs; void __iomem *vid_regs; void __iomem *gp_regs; -#ifdef CONFIG_PM int powered_down;

/* register state, for power management functionality */ @@ -36,7 +35,6 @@ struct gxfb_par { uint64_t fp[FP_REG_COUNT];

uint32_t pal[DC_PAL_COUNT]; -#endif };

unsigned int gx_frame_buffer_size(void); @@ -49,11 +47,8 @@ void gx_set_dclk_frequency(struct fb_info *info); void gx_configure_display(struct fb_info *info); int gx_blank_display(struct fb_info *info, int blank_mode);

-#ifdef CONFIG_PM int gx_powerdown(struct fb_info *info); int gx_powerup(struct fb_info *info); -#endif

/* Graphics Processor registers (table 6-23 from the data book) */ enum gp_registers { diff --git a/drivers/video/fbdev/geode/gxfb_core.c b/drivers/video/fbdev/geode/gxfb_core.c index d38a148d4746..44089b331f91 100644 --- a/drivers/video/fbdev/geode/gxfb_core.c +++ b/drivers/video/fbdev/geode/gxfb_core.c @@ -322,17 +322,14 @@ static struct fb_info *gxfb_init_fbinfo(struct device *dev) return info; }

-#ifdef CONFIG_PM -static int gxfb_suspend(struct pci_dev *pdev, pm_message_t state) +static int __maybe_unused gxfb_suspend(struct device *dev) {

• struct fb_info *info = pci_get_drvdata(pdev);

• struct fb_info *info = dev_get_drvdata(dev);

I do not see any dev_set_drvdata() so I guess we get a NULL pointer here which is not intended. Adding a dev_set_data() to gxfb_probe() would do the trick.

gxfb_probe() invokes pci_set_drvdata(pdev, info) which in turn calls dev_set_drvdata(&pdev->dev, data). Adding dev_get_drvdata() will be redundant.

...

• if (state.event == PM_EVENT_SUSPEND) {

• console_lock();
  

• gx_powerdown(info);
  

• fb_set_suspend(info, 1);
  

• console_unlock();
  

• }

• console_lock();

• gx_powerdown(info);

• fb_set_suspend(info, 1);

• console_unlock();

  /* there's no point in setting PCI states; we emulate PCI, so

  • we don't end up getting power savings anyways */

@@ -340,9 +337,9 @@ static int gxfb_suspend(struct pci_dev *pdev, pm_message_t state) return 0; }

-static int gxfb_resume(struct pci_dev *pdev) +static int __maybe_unused gxfb_resume(struct device *dev) {

• struct fb_info *info = pci_get_drvdata(pdev);

• struct fb_info *info = dev_get_drvdata(dev); int ret;

  console_lock();

@@ -356,7 +353,6 @@ static int gxfb_resume(struct pci_dev *pdev) console_unlock(); return 0; } -#endif

static int gxfb_probe(struct pci_dev *pdev, const struct pci_device_id *id) { @@ -467,15 +463,23 @@ static const struct pci_device_id gxfb_id_table[] = {

MODULE_DEVICE_TABLE(pci, gxfb_id_table);

+static const struct dev_pm_ops gxfb_pm_ops = { +#ifdef CONFIG_PM_SLEEP

• .suspend = gxfb_suspend,
• .resume = gxfb_resume,
• .freeze = NULL,
• .thaw = gxfb_resume,
• .poweroff = NULL,
• .restore = gxfb_resume,

+#endif +};

Can we use SET_SYSTEM_SLEEP_PM_OPS here? .freeze will be assigned gxfb_suspend, but gxfb_suspend will anyway be called as far as I read the code. Likewise for poweroff.

Earlier, gxfb_suspend() performed each operation just for suspend event. And as it was legacy code, it was invoked by pci_legacy_suspend() for pci_pm_suspend(), pci_pm_freeze() and pci_pm_poweroff(). Thus, the code was wrapped inside "if" container: if (state.event == PM_EVENT_SUSPEND) { }

After binding it with dev_pm_ops variable, pm->suspend() is invoked by just pci_pm_suspend() which is required.

So I removed the "if" container and bind the callback with pm->suspend pointer only.

Using SET_SYSTEM_PM_OPS will bring back the extra step of invoking gxfb_suspend() for freeze and poweroff, even though the function will do nothing in that case.

Vaibhav Gupta

Sam

...

static struct pci_driver gxfb_driver = { .name = "gxfb", .id_table = gxfb_id_table, .probe = gxfb_probe, .remove = gxfb_remove, -#ifdef CONFIG_PM

• .suspend = gxfb_suspend,
• .resume = gxfb_resume,

-#endif

• .driver.pm = &gxfb_pm_ops,

};

#ifndef MODULE diff --git a/drivers/video/fbdev/geode/suspend_gx.c b/drivers/video/fbdev/geode/suspend_gx.c index 1110a527c35c..8c49d4e98772 100644 --- a/drivers/video/fbdev/geode/suspend_gx.c +++ b/drivers/video/fbdev/geode/suspend_gx.c @@ -11,8 +11,6 @@

#include "gxfb.h"

-#ifdef CONFIG_PM

static void gx_save_regs(struct gxfb_par *par) { int i; @@ -259,5 +257,3 @@ int gx_powerup(struct fb_info *info) par->powered_down = 0; return 0; }

-#endif

2.27.0

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

Hi Vaibhav On Mon, Aug 10, 2020 at 03:09:48PM +0530, Vaibhav Gupta wrote:

On Sat, Aug 08, 2020 at 01:17:46PM +0200, Sam Ravnborg wrote:

...

Hi Vaibhav

On Wed, Aug 05, 2020 at 11:37:11PM +0530, Vaibhav Gupta wrote:

...

Drivers using legacy power management .suspen()/.resume() callbacks have to manage PCI states and device's PM states themselves. They also need to take care of standard configuration registers.

Switch to generic power management framework using a single "struct dev_pm_ops" variable

"to take the unnecessary load from the driver."

• I do not parse the above - I cannot see what load is removed.

But the code is simpler which is fine. The drawback is that we now always link in the suspend_gx functions but hopefultl the linker drops them later.

...

This also avoids the need for the driver to directly call most of the PCI helper functions and device power state control functions, as through the generic framework PCI Core takes care of the necessary operations, and drivers are required to do only device-specific jobs.

Again, I do not see what calles are removed. A single check for the state is dropped - anything else?

Yeah, the commit messages are bit misleading, I have modified them.

...

...

Signed-off-by: Vaibhav Gupta vaibhavgupta40@gmail.com

drivers/video/fbdev/geode/gxfb.h | 5 ---- drivers/video/fbdev/geode/gxfb_core.c | 36 ++++++++++++++------------ drivers/video/fbdev/geode/suspend_gx.c | 4 --- 3 files changed, 20 insertions(+), 25 deletions(-)

diff --git a/drivers/video/fbdev/geode/gxfb.h b/drivers/video/fbdev/geode/gxfb.h index d2e9c5c8e294..792c111c21e4 100644 --- a/drivers/video/fbdev/geode/gxfb.h +++ b/drivers/video/fbdev/geode/gxfb.h @@ -21,7 +21,6 @@ struct gxfb_par { void __iomem *dc_regs; void __iomem *vid_regs; void __iomem *gp_regs; -#ifdef CONFIG_PM int powered_down;

/* register state, for power management functionality */ @@ -36,7 +35,6 @@ struct gxfb_par { uint64_t fp[FP_REG_COUNT];

uint32_t pal[DC_PAL_COUNT]; -#endif };

unsigned int gx_frame_buffer_size(void); @@ -49,11 +47,8 @@ void gx_set_dclk_frequency(struct fb_info *info); void gx_configure_display(struct fb_info *info); int gx_blank_display(struct fb_info *info, int blank_mode);

-#ifdef CONFIG_PM int gx_powerdown(struct fb_info *info); int gx_powerup(struct fb_info *info); -#endif

/* Graphics Processor registers (table 6-23 from the data book) */ enum gp_registers { diff --git a/drivers/video/fbdev/geode/gxfb_core.c b/drivers/video/fbdev/geode/gxfb_core.c index d38a148d4746..44089b331f91 100644 --- a/drivers/video/fbdev/geode/gxfb_core.c +++ b/drivers/video/fbdev/geode/gxfb_core.c @@ -322,17 +322,14 @@ static struct fb_info *gxfb_init_fbinfo(struct device *dev) return info; }

-#ifdef CONFIG_PM -static int gxfb_suspend(struct pci_dev *pdev, pm_message_t state) +static int __maybe_unused gxfb_suspend(struct device *dev) {

• struct fb_info *info = pci_get_drvdata(pdev);

• struct fb_info *info = dev_get_drvdata(dev);

I do not see any dev_set_drvdata() so I guess we get a NULL pointer here which is not intended. Adding a dev_set_data() to gxfb_probe() would do the trick.

gxfb_probe() invokes pci_set_drvdata(pdev, info) which in turn calls dev_set_drvdata(&pdev->dev, data). Adding dev_get_drvdata() will be redundant.

OK, not obvious but you are right that calling dev_get_drvdata() would be redundant and no need. There is a pci_get_drvdata() user left so we cannot just skip it and use the dev_set_drvdata() direct.

...

...

• if (state.event == PM_EVENT_SUSPEND) {

• console_lock();
  

• gx_powerdown(info);
  

• fb_set_suspend(info, 1);
  

• console_unlock();
  

• }

• console_lock();

• gx_powerdown(info);

• fb_set_suspend(info, 1);

• console_unlock();

  /* there's no point in setting PCI states; we emulate PCI, so

  • we don't end up getting power savings anyways */

@@ -340,9 +337,9 @@ static int gxfb_suspend(struct pci_dev *pdev, pm_message_t state) return 0; }

-static int gxfb_resume(struct pci_dev *pdev) +static int __maybe_unused gxfb_resume(struct device *dev) {

• struct fb_info *info = pci_get_drvdata(pdev);

• struct fb_info *info = dev_get_drvdata(dev); int ret;

  console_lock();

@@ -356,7 +353,6 @@ static int gxfb_resume(struct pci_dev *pdev) console_unlock(); return 0; } -#endif

static int gxfb_probe(struct pci_dev *pdev, const struct pci_device_id *id) { @@ -467,15 +463,23 @@ static const struct pci_device_id gxfb_id_table[] = {

MODULE_DEVICE_TABLE(pci, gxfb_id_table);

+static const struct dev_pm_ops gxfb_pm_ops = { +#ifdef CONFIG_PM_SLEEP

• .suspend = gxfb_suspend,
• .resume = gxfb_resume,
• .freeze = NULL,
• .thaw = gxfb_resume,
• .poweroff = NULL,
• .restore = gxfb_resume,

+#endif +};

Can we use SET_SYSTEM_SLEEP_PM_OPS here? .freeze will be assigned gxfb_suspend, but gxfb_suspend will anyway be called as far as I read the code. Likewise for poweroff.

Earlier, gxfb_suspend() performed each operation just for suspend event. And as it was legacy code, it was invoked by pci_legacy_suspend() for pci_pm_suspend(), pci_pm_freeze() and pci_pm_poweroff(). Thus, the code was wrapped inside "if" container: if (state.event == PM_EVENT_SUSPEND) { }

After binding it with dev_pm_ops variable, pm->suspend() is invoked by just pci_pm_suspend() which is required.

So I removed the "if" container and bind the callback with pm->suspend pointer only.

Looking at platform.c I got the impression that freeze() would call pci_legacy_suspend() anyway - but I may have missind something. So I guess this is OK then.

I look forward for next revision with updated changelogs.

Sam

Using SET_SYSTEM_PM_OPS will bring back the extra step of invoking gxfb_suspend() for freeze and poweroff, even though the function will do nothing in that case.

Vaibhav Gupta

...

Sam

...

static struct pci_driver gxfb_driver = { .name = "gxfb", .id_table = gxfb_id_table, .probe = gxfb_probe, .remove = gxfb_remove, -#ifdef CONFIG_PM

• .suspend = gxfb_suspend,
• .resume = gxfb_resume,

-#endif

• .driver.pm = &gxfb_pm_ops,

};

#ifndef MODULE diff --git a/drivers/video/fbdev/geode/suspend_gx.c b/drivers/video/fbdev/geode/suspend_gx.c index 1110a527c35c..8c49d4e98772 100644 --- a/drivers/video/fbdev/geode/suspend_gx.c +++ b/drivers/video/fbdev/geode/suspend_gx.c @@ -11,8 +11,6 @@

#include "gxfb.h"

-#ifdef CONFIG_PM

static void gx_save_regs(struct gxfb_par *par) { int i; @@ -259,5 +257,3 @@ int gx_powerup(struct fb_info *info) par->powered_down = 0; return 0; }

-#endif

2.27.0

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

Hi Vaibhav

On Tue, Aug 11, 2020 at 12:27:12AM +0530, Vaibhav Gupta wrote:

Drivers should do only device-specific jobs. But in general, drivers using legacy PCI PM framework for .suspend()/.resume() have to manage many PCI PM-related tasks themselves which can be done by PCI Core itself. This brings extra load on the driver and it directly calls PCI helper functions to handle them.

Although the gxfb driver does not have that extra load,

Sorry, but I am lost here. If this drivers does not have the extra load that you describe here then I really cannot see why it is relevant for this driver to describe it.

This is a seldomly touched driver - so it helps if the changelog when we finally touch the code is easy to parse.

we should switch to the new generic framework by updating function signatures and define a "struct dev_pm_ops" variable to bind PM callbacks so that we can remove the legacy .suspend & .resume bindings.

This part matches the patch - good.

Additionally, this helps us to remove the unnecessary call to gxfb_suspend() in the event of Freeze and Hibernate, as the function does nothing in their case.

What I think you are explaining above is that the pci pm support will only call the suspend operation in case of suspend, so the state.event == PM_EVENT_SUSPEND can be dropped in gxfb_suspend().

For reference later I would prefer that this is explained a bit more explicit - not that the changelog needs update anyway.

Signed-off-by: Vaibhav Gupta vaibhavgupta40@gmail.com

Patch looks good, but please give the changelog one more go. I have not checked other patches - but I assume they would benefit from a similar clarification.

Sam

---

drivers/video/fbdev/geode/gxfb.h | 5 ---- drivers/video/fbdev/geode/gxfb_core.c | 36 ++++++++++++++------------ drivers/video/fbdev/geode/suspend_gx.c | 4 --- 3 files changed, 20 insertions(+), 25 deletions(-)

diff --git a/drivers/video/fbdev/geode/gxfb.h b/drivers/video/fbdev/geode/gxfb.h index d2e9c5c8e294..792c111c21e4 100644 --- a/drivers/video/fbdev/geode/gxfb.h +++ b/drivers/video/fbdev/geode/gxfb.h @@ -21,7 +21,6 @@ struct gxfb_par { void __iomem *dc_regs; void __iomem *vid_regs; void __iomem *gp_regs; -#ifdef CONFIG_PM int powered_down;

/* register state, for power management functionality */ @@ -36,7 +35,6 @@ struct gxfb_par { uint64_t fp[FP_REG_COUNT];

uint32_t pal[DC_PAL_COUNT]; -#endif };

unsigned int gx_frame_buffer_size(void); @@ -49,11 +47,8 @@ void gx_set_dclk_frequency(struct fb_info *info); void gx_configure_display(struct fb_info *info); int gx_blank_display(struct fb_info *info, int blank_mode);

-#ifdef CONFIG_PM int gx_powerdown(struct fb_info *info); int gx_powerup(struct fb_info *info); -#endif

/* Graphics Processor registers (table 6-23 from the data book) */ enum gp_registers { diff --git a/drivers/video/fbdev/geode/gxfb_core.c b/drivers/video/fbdev/geode/gxfb_core.c index d38a148d4746..44089b331f91 100644 --- a/drivers/video/fbdev/geode/gxfb_core.c +++ b/drivers/video/fbdev/geode/gxfb_core.c @@ -322,17 +322,14 @@ static struct fb_info *gxfb_init_fbinfo(struct device *dev) return info; }

-#ifdef CONFIG_PM -static int gxfb_suspend(struct pci_dev *pdev, pm_message_t state) +static int __maybe_unused gxfb_suspend(struct device *dev) {

• struct fb_info *info = pci_get_drvdata(pdev);

• struct fb_info *info = dev_get_drvdata(dev);

• if (state.event == PM_EVENT_SUSPEND) {

• console_lock();
  

• gx_powerdown(info);
  

• fb_set_suspend(info, 1);
  

• console_unlock();
  

• }

• console_lock();

• gx_powerdown(info);

• fb_set_suspend(info, 1);

• console_unlock();

  /* there's no point in setting PCI states; we emulate PCI, so

  • we don't end up getting power savings anyways */

@@ -340,9 +337,9 @@ static int gxfb_suspend(struct pci_dev *pdev, pm_message_t state) return 0; }

-static int gxfb_resume(struct pci_dev *pdev) +static int __maybe_unused gxfb_resume(struct device *dev) {

• struct fb_info *info = pci_get_drvdata(pdev);

• struct fb_info *info = dev_get_drvdata(dev); int ret;

  console_lock();

@@ -356,7 +353,6 @@ static int gxfb_resume(struct pci_dev *pdev) console_unlock(); return 0; } -#endif

static int gxfb_probe(struct pci_dev *pdev, const struct pci_device_id *id) { @@ -467,15 +463,23 @@ static const struct pci_device_id gxfb_id_table[] = {

MODULE_DEVICE_TABLE(pci, gxfb_id_table);

+static const struct dev_pm_ops gxfb_pm_ops = { +#ifdef CONFIG_PM_SLEEP

• .suspend = gxfb_suspend,
• .resume = gxfb_resume,
• .freeze = NULL,
• .thaw = gxfb_resume,
• .poweroff = NULL,
• .restore = gxfb_resume,

+#endif +};

static struct pci_driver gxfb_driver = { .name = "gxfb", .id_table = gxfb_id_table, .probe = gxfb_probe, .remove = gxfb_remove, -#ifdef CONFIG_PM

• .suspend = gxfb_suspend,
• .resume = gxfb_resume,

-#endif

• .driver.pm = &gxfb_pm_ops,

};

#ifndef MODULE diff --git a/drivers/video/fbdev/geode/suspend_gx.c b/drivers/video/fbdev/geode/suspend_gx.c index 1110a527c35c..8c49d4e98772 100644 --- a/drivers/video/fbdev/geode/suspend_gx.c +++ b/drivers/video/fbdev/geode/suspend_gx.c @@ -11,8 +11,6 @@

#include "gxfb.h"

-#ifdef CONFIG_PM

static void gx_save_regs(struct gxfb_par *par) { int i; @@ -259,5 +257,3 @@ int gx_powerup(struct fb_info *info) par->powered_down = 0; return 0; }

-#endif

2.27.0

Drivers should do only device-specific jobs. But in general, drivers using legacy PCI PM framework for .suspend()/.resume() have to manage many PCI PM-related tasks themselves which can be done by PCI Core itself. This brings extra load on the driver and it directly calls PCI helper functions to handle them.

Although the lxfb driver does not have that extra load, we should switch to the new generic framework by updating function signatures and define a "struct dev_pm_ops" variable to bind PM callbacks so that we can remove the legacy .suspend & .resume bindings. Additionally, this helps us to remove the unnecessary call to lxfb_suspend() in the event of Freeze and Hibernate, as the function does nothing in their case.

Signed-off-by: Vaibhav Gupta vaibhavgupta40@gmail.com --- drivers/video/fbdev/geode/lxfb.h | 5 ---- drivers/video/fbdev/geode/lxfb_core.c | 37 +++++++++++++++------------ drivers/video/fbdev/geode/lxfb_ops.c | 4 --- 3 files changed, 20 insertions(+), 26 deletions(-)

diff --git a/drivers/video/fbdev/geode/lxfb.h b/drivers/video/fbdev/geode/lxfb.h index ef24bf6d49dc..d37b32dbcd68 100644 --- a/drivers/video/fbdev/geode/lxfb.h +++ b/drivers/video/fbdev/geode/lxfb.h @@ -29,7 +29,6 @@ struct lxfb_par { void __iomem *gp_regs; void __iomem *dc_regs; void __iomem *vp_regs; -#ifdef CONFIG_PM int powered_down;

/* register state, for power mgmt functionality */ @@ -50,7 +49,6 @@ struct lxfb_par { uint32_t hcoeff[DC_HFILT_COUNT * 2]; uint32_t vcoeff[DC_VFILT_COUNT]; uint32_t vp_coeff[VP_COEFF_SIZE / 4]; -#endif };

static inline unsigned int lx_get_pitch(unsigned int xres, int bpp) @@ -64,11 +62,8 @@ int lx_blank_display(struct fb_info *, int); void lx_set_palette_reg(struct fb_info *, unsigned int, unsigned int, unsigned int, unsigned int);

-#ifdef CONFIG_PM int lx_powerdown(struct fb_info *info); int lx_powerup(struct fb_info *info); -#endif -

/* Graphics Processor registers (table 6-29 from the data book) */ enum gp_registers { diff --git a/drivers/video/fbdev/geode/lxfb_core.c b/drivers/video/fbdev/geode/lxfb_core.c index adc2d9c2395e..66c81262d18f 100644 --- a/drivers/video/fbdev/geode/lxfb_core.c +++ b/drivers/video/fbdev/geode/lxfb_core.c @@ -443,17 +443,14 @@ static struct fb_info *lxfb_init_fbinfo(struct device *dev) return info; }

-#ifdef CONFIG_PM -static int lxfb_suspend(struct pci_dev *pdev, pm_message_t state) +static int __maybe_unused lxfb_suspend(struct device *dev) { - struct fb_info *info = pci_get_drvdata(pdev); + struct fb_info *info = dev_get_drvdata(dev);

- if (state.event == PM_EVENT_SUSPEND) { - console_lock(); - lx_powerdown(info); - fb_set_suspend(info, 1); - console_unlock(); - } + console_lock(); + lx_powerdown(info); + fb_set_suspend(info, 1); + console_unlock();

/* there's no point in setting PCI states; we emulate PCI, so * we don't end up getting power savings anyways */ @@ -461,9 +458,9 @@ static int lxfb_suspend(struct pci_dev *pdev, pm_message_t state) return 0; }

-static int lxfb_resume(struct pci_dev *pdev) +static int __maybe_unused lxfb_resume(struct device *dev) { - struct fb_info *info = pci_get_drvdata(pdev); + struct fb_info *info = dev_get_drvdata(dev); int ret;

console_lock(); @@ -477,10 +474,6 @@ static int lxfb_resume(struct pci_dev *pdev) console_unlock(); return 0; } -#else -#define lxfb_suspend NULL -#define lxfb_resume NULL -#endif

static int lxfb_probe(struct pci_dev *pdev, const struct pci_device_id *id) { @@ -600,13 +593,23 @@ static struct pci_device_id lxfb_id_table[] = {

MODULE_DEVICE_TABLE(pci, lxfb_id_table);

+static const struct dev_pm_ops lxfb_pm_ops = { +#ifdef CONFIG_PM_SLEEP + .suspend = lxfb_suspend, + .resume = lxfb_resume, + .freeze = NULL, + .thaw = lxfb_resume, + .poweroff = NULL, + .restore = lxfb_resume, +#endif +}; + static struct pci_driver lxfb_driver = { .name = "lxfb", .id_table = lxfb_id_table, .probe = lxfb_probe, .remove = lxfb_remove, - .suspend = lxfb_suspend, - .resume = lxfb_resume, + .driver.pm = &lxfb_pm_ops, };

#ifndef MODULE diff --git a/drivers/video/fbdev/geode/lxfb_ops.c b/drivers/video/fbdev/geode/lxfb_ops.c index 5be8bc62844c..b3a041fce570 100644 --- a/drivers/video/fbdev/geode/lxfb_ops.c +++ b/drivers/video/fbdev/geode/lxfb_ops.c @@ -580,8 +580,6 @@ int lx_blank_display(struct fb_info *info, int blank_mode) return 0; }

-#ifdef CONFIG_PM - static void lx_save_regs(struct lxfb_par *par) { uint32_t filt; @@ -837,5 +835,3 @@ int lx_powerup(struct fb_info *info) par->powered_down = 0; return 0; } - -#endif

Drivers should do only device-specific jobs. But in general, drivers using legacy PCI PM framework for .suspend()/.resume() have to manage many PCI PM-related tasks themselves which can be done by PCI Core itself. This brings extra load on the driver and it directly calls PCI helper functions to handle them.

Switch to the new generic framework by updating function signatures and define a "struct dev_pm_ops" variable to bind PM callbacks. Also, remove unnecessary calls to the PCI Helper functions along with the legacy .suspend & .resume bindings.

Signed-off-by: Vaibhav Gupta vaibhavgupta40@gmail.com --- drivers/video/fbdev/aty/atyfb_base.c | 50 ++++++++++++++++++++-------- 1 file changed, 36 insertions(+), 14 deletions(-)

diff --git a/drivers/video/fbdev/aty/atyfb_base.c b/drivers/video/fbdev/aty/atyfb_base.c index b0ac895e5ac9..a24d5bf6ade1 100644 --- a/drivers/video/fbdev/aty/atyfb_base.c +++ b/drivers/video/fbdev/aty/atyfb_base.c @@ -132,8 +132,8 @@ #define PRINTKI(fmt, args...) printk(KERN_INFO "atyfb: " fmt, ## args) #define PRINTKE(fmt, args...) printk(KERN_ERR "atyfb: " fmt, ## args)

-#if defined(CONFIG_PM) || defined(CONFIG_PMAC_BACKLIGHT) || \ -defined (CONFIG_FB_ATY_GENERIC_LCD) || defined(CONFIG_FB_ATY_BACKLIGHT) +#if defined(CONFIG_PMAC_BACKLIGHT) || defined(CONFIG_FB_ATY_GENERIC_LCD) || \ +defined(CONFIG_FB_ATY_BACKLIGHT) static const u32 lt_lcd_regs[] = { CNFG_PANEL_LG, LCD_GEN_CNTL_LG, @@ -175,7 +175,7 @@ u32 aty_ld_lcd(int index, const struct atyfb_par *par) return aty_ld_le32(LCD_DATA, par); } } -#endif /* defined(CONFIG_PM) || defined(CONFIG_PMAC_BACKLIGHT) || defined (CONFIG_FB_ATY_GENERIC_LCD) */ +#endif /* defined(CONFIG_PMAC_BACKLIGHT) || defined (CONFIG_FB_ATY_GENERIC_LCD) */

#ifdef CONFIG_FB_ATY_GENERIC_LCD /* @@ -1994,7 +1994,7 @@ static int atyfb_mmap(struct fb_info *info, struct vm_area_struct *vma)

-#if defined(CONFIG_PM) && defined(CONFIG_PCI) +#if defined(CONFIG_PCI)

#ifdef CONFIG_PPC_PMAC /* Power management routines. Those are used for PowerBook sleep. @@ -2055,8 +2055,9 @@ static int aty_power_mgmt(int sleep, struct atyfb_par *par) } #endif /* CONFIG_PPC_PMAC */

-static int atyfb_pci_suspend(struct pci_dev *pdev, pm_message_t state) +static int atyfb_pci_suspend_late(struct device *dev, pm_message_t state) { + struct pci_dev *pdev = to_pci_dev(dev); struct fb_info *info = pci_get_drvdata(pdev); struct atyfb_par *par = (struct atyfb_par *) info->par;

@@ -2082,7 +2083,6 @@ static int atyfb_pci_suspend(struct pci_dev *pdev, pm_message_t state) * first save the config space content so the core can * restore it properly on resume. */ - pci_save_state(pdev);

#ifdef CONFIG_PPC_PMAC /* Set chip to "suspend" mode */ @@ -2094,8 +2094,6 @@ static int atyfb_pci_suspend(struct pci_dev *pdev, pm_message_t state) console_unlock(); return -EIO; } -#else - pci_set_power_state(pdev, pci_choose_state(pdev, state)); #endif

console_unlock(); @@ -2105,6 +2103,21 @@ static int atyfb_pci_suspend(struct pci_dev *pdev, pm_message_t state) return 0; }

+static int __maybe_unused atyfb_pci_suspend(struct device *dev) +{ + return atyfb_pci_suspend_late(dev, PMSG_SUSPEND); +} + +static int __maybe_unused atyfb_pci_hibernate(struct device *dev) +{ + return atyfb_pci_suspend_late(dev, PMSG_HIBERNATE); +} + +static int __maybe_unused atyfb_pci_freeze(struct device *dev) +{ + return atyfb_pci_suspend_late(dev, PMSG_FREEZE); +} + static void aty_resume_chip(struct fb_info *info) { struct atyfb_par *par = info->par; @@ -2119,8 +2132,9 @@ static void aty_resume_chip(struct fb_info *info) aty_ld_le32(BUS_CNTL, par) | BUS_APER_REG_DIS, par); }

-static int atyfb_pci_resume(struct pci_dev *pdev) +static int __maybe_unused atyfb_pci_resume(struct device *dev) { + struct pci_dev *pdev = to_pci_dev(dev); struct fb_info *info = pci_get_drvdata(pdev); struct atyfb_par *par = (struct atyfb_par *) info->par;

@@ -2162,7 +2176,18 @@ static int atyfb_pci_resume(struct pci_dev *pdev) return 0; }

-#endif /* defined(CONFIG_PM) && defined(CONFIG_PCI) */ +static const struct dev_pm_ops atyfb_pci_pm_ops = { +#ifdef CONFIG_PM_SLEEP + .suspend = atyfb_pci_suspend, + .resume = atyfb_pci_resume, + .freeze = atyfb_pci_freeze, + .thaw = atyfb_pci_resume, + .poweroff = atyfb_pci_hibernate, + .restore = atyfb_pci_resume, +#endif /* CONFIG_PM_SLEEP */ +}; + +#endif /* defined(CONFIG_PCI) */

/* Backlight */ #ifdef CONFIG_FB_ATY_BACKLIGHT @@ -3801,10 +3826,7 @@ static struct pci_driver atyfb_driver = { .id_table = atyfb_pci_tbl, .probe = atyfb_pci_probe, .remove = atyfb_pci_remove, -#ifdef CONFIG_PM - .suspend = atyfb_pci_suspend, - .resume = atyfb_pci_resume, -#endif /* CONFIG_PM */ + .driver.pm = &atyfb_pci_pm_ops, };

#endif /* CONFIG_PCI */

Drivers should do only device-specific jobs. But in general, drivers using legacy PCI PM framework for .suspend()/.resume() have to manage many PCI PM-related tasks themselves which can be done by PCI Core itself. This brings extra load on the driver and it directly calls PCI helper functions to handle them.

Switch to the new generic framework by updating function signatures and define a "struct dev_pm_ops" variable to bind PM callbacks. Also, remove unnecessary calls to the PCI Helper functions along with the legacy .suspend & .resume bindings.

Signed-off-by: Vaibhav Gupta vaibhavgupta40@gmail.com --- drivers/video/fbdev/nvidia/nvidia.c | 64 ++++++++++++++++------------- 1 file changed, 35 insertions(+), 29 deletions(-)

diff --git a/drivers/video/fbdev/nvidia/nvidia.c b/drivers/video/fbdev/nvidia/nvidia.c index c24de9107958..3a1a4330e0d3 100644 --- a/drivers/video/fbdev/nvidia/nvidia.c +++ b/drivers/video/fbdev/nvidia/nvidia.c @@ -1041,10 +1041,9 @@ static struct fb_ops nvidia_fb_ops = { .fb_sync = nvidiafb_sync, };

-#ifdef CONFIG_PM -static int nvidiafb_suspend(struct pci_dev *dev, pm_message_t mesg) +static int nvidiafb_suspend_late(struct device *dev, pm_message_t mesg) { - struct fb_info *info = pci_get_drvdata(dev); + struct fb_info *info = dev_get_drvdata(dev); struct nvidia_par *par = info->par;

if (mesg.event == PM_EVENT_PRETHAW) @@ -1056,46 +1055,54 @@ static int nvidiafb_suspend(struct pci_dev *dev, pm_message_t mesg) fb_set_suspend(info, 1); nvidiafb_blank(FB_BLANK_POWERDOWN, info); nvidia_write_regs(par, &par->SavedReg); - pci_save_state(dev); - pci_disable_device(dev); - pci_set_power_state(dev, pci_choose_state(dev, mesg)); } - dev->dev.power.power_state = mesg; + dev->power.power_state = mesg;

console_unlock(); return 0; }

-static int nvidiafb_resume(struct pci_dev *dev) +static int __maybe_unused nvidiafb_suspend(struct device *dev) { - struct fb_info *info = pci_get_drvdata(dev); - struct nvidia_par *par = info->par; + return nvidiafb_suspend_late(dev, PMSG_SUSPEND); +}

- console_lock(); - pci_set_power_state(dev, PCI_D0); +static int __maybe_unused nvidiafb_hibernate(struct device *dev) +{ + return nvidiafb_suspend_late(dev, PMSG_HIBERNATE); +}

- if (par->pm_state != PM_EVENT_FREEZE) { - pci_restore_state(dev); +static int __maybe_unused nvidiafb_freeze(struct device *dev) +{ + return nvidiafb_suspend_late(dev, PMSG_FREEZE); +}

- if (pci_enable_device(dev)) - goto fail; +static int __maybe_unused nvidiafb_resume(struct device *dev) +{ + struct fb_info *info = dev_get_drvdata(dev); + struct nvidia_par *par = info->par;

- pci_set_master(dev); - } + console_lock();

par->pm_state = PM_EVENT_ON; nvidiafb_set_par(info); fb_set_suspend (info, 0); nvidiafb_blank(FB_BLANK_UNBLANK, info);

-fail: console_unlock(); return 0; } -#else -#define nvidiafb_suspend NULL -#define nvidiafb_resume NULL -#endif + +static const struct dev_pm_ops nvidiafb_pm_ops = { +#ifdef CONFIG_PM_SLEEP + .suspend = nvidiafb_suspend, + .resume = nvidiafb_resume, + .freeze = nvidiafb_freeze, + .thaw = nvidiafb_resume, + .poweroff = nvidiafb_hibernate, + .restore = nvidiafb_resume, +#endif /* CONFIG_PM_SLEEP */ +};

static int nvidia_set_fbinfo(struct fb_info *info) { @@ -1496,12 +1503,11 @@ static int nvidiafb_setup(char *options) #endif /* !MODULE */

static struct pci_driver nvidiafb_driver = { - .name = "nvidiafb", - .id_table = nvidiafb_pci_tbl, - .probe = nvidiafb_probe, - .suspend = nvidiafb_suspend, - .resume = nvidiafb_resume, - .remove = nvidiafb_remove, + .name = "nvidiafb", + .id_table = nvidiafb_pci_tbl, + .probe = nvidiafb_probe, + .driver.pm = &nvidiafb_pm_ops, + .remove = nvidiafb_remove, };

/* ------------------------------------------------------------------------- *

Drivers should do only device-specific jobs. But in general, drivers using legacy PCI PM framework for .suspend()/.resume() have to manage many PCI PM-related tasks themselves which can be done by PCI Core itself. This brings extra load on the driver and it directly calls PCI helper functions to handle them.

Although the cyber2000fb driver does not have that extra load, we should switch to the new generic framework by updating function signatures and define a "struct dev_pm_ops" variable to bind PM callbacks so that we can remove the legacy .suspend & .resume bindings.

Signed-off-by: Vaibhav Gupta vaibhavgupta40@gmail.com --- drivers/video/fbdev/cyber2000fb.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/video/fbdev/cyber2000fb.c b/drivers/video/fbdev/cyber2000fb.c index 42d37bed518a..d45355b9a58c 100644 --- a/drivers/video/fbdev/cyber2000fb.c +++ b/drivers/video/fbdev/cyber2000fb.c @@ -1810,7 +1810,7 @@ static void cyberpro_pci_remove(struct pci_dev *dev) } }

-static int cyberpro_pci_suspend(struct pci_dev *dev, pm_message_t state) +static int __maybe_unused cyberpro_pci_suspend(struct device *dev) { return 0; } @@ -1818,9 +1818,9 @@ static int cyberpro_pci_suspend(struct pci_dev *dev, pm_message_t state) /* * Re-initialise the CyberPro hardware */ -static int cyberpro_pci_resume(struct pci_dev *dev) +static int __maybe_unused cyberpro_pci_resume(struct device *dev) { - struct cfb_info *cfb = pci_get_drvdata(dev); + struct cfb_info *cfb = dev_get_drvdata(dev);

if (cfb) { cyberpro_pci_enable_mmio(cfb); @@ -1846,12 +1846,15 @@ static struct pci_device_id cyberpro_pci_table[] = {

MODULE_DEVICE_TABLE(pci, cyberpro_pci_table);

+static SIMPLE_DEV_PM_OPS(cyberpro_pci_pm_ops, + cyberpro_pci_suspend, + cyberpro_pci_resume); + static struct pci_driver cyberpro_driver = { .name = "CyberPro", .probe = cyberpro_pci_probe, .remove = cyberpro_pci_remove, - .suspend = cyberpro_pci_suspend, - .resume = cyberpro_pci_resume, + .driver.pm = &cyberpro_pci_pm_ops, .id_table = cyberpro_pci_table };

Hi Vaibhav

On Tue, Aug 11, 2020 at 12:27:20AM +0530, Vaibhav Gupta wrote:

Drivers should do only device-specific jobs. But in general, drivers using legacy PCI PM framework for .suspend()/.resume() have to manage many PCI PM-related tasks themselves which can be done by PCI Core itself. This brings extra load on the driver and it directly calls PCI helper functions to handle them.

Switch to the new generic framework by updating function signatures and define a "struct dev_pm_ops" variable to bind PM callbacks. Also, remove unnecessary calls to the PCI Helper functions along with the legacy .suspend & .resume bindings.

Signed-off-by: Vaibhav Gupta vaibhavgupta40@gmail.com

I several of the drivers I briefly looked at a new set of helpers were introduced for the different types of pm actions. They then called a more generic function that uses the passes enumeration to decide what to do.

But in this driver the test "state.event == PM_EVENT_FREEZE" is dropped and there is no freeze operation. Please explain this change so the reader is not left wondering.

Sam

---

drivers/video/fbdev/i740fb.c | 40 +++++++++++++++--------------------- 1 file changed, 16 insertions(+), 24 deletions(-)

diff --git a/drivers/video/fbdev/i740fb.c b/drivers/video/fbdev/i740fb.c index c65ec7386e87..8d7f06fc8a5a 100644 --- a/drivers/video/fbdev/i740fb.c +++ b/drivers/video/fbdev/i740fb.c @@ -1175,16 +1175,11 @@ static void i740fb_remove(struct pci_dev *dev) } }

-#ifdef CONFIG_PM -static int i740fb_suspend(struct pci_dev *dev, pm_message_t state) +static int __maybe_unused i740fb_suspend(struct device *dev) {

• struct fb_info *info = pci_get_drvdata(dev);

• struct fb_info *info = dev_get_drvdata(dev); struct i740fb_par *par = info->par;

• /* don't disable console during hibernation and wakeup from it */
• if (state.event == PM_EVENT_FREEZE || state.event == PM_EVENT_PRETHAW)

• return 0;
  

• console_lock(); mutex_lock(&(par->open_lock));

@@ -1197,19 +1192,15 @@ static int i740fb_suspend(struct pci_dev *dev, pm_message_t state)

fb_set_suspend(info, 1);

• pci_save_state(dev);

• pci_disable_device(dev);

• pci_set_power_state(dev, pci_choose_state(dev, state));

• mutex_unlock(&(par->open_lock)); console_unlock();

  return 0;

}

-static int i740fb_resume(struct pci_dev *dev) +static int __maybe_unused i740fb_resume(struct device *dev) {

• struct fb_info *info = pci_get_drvdata(dev);

• struct fb_info *info = dev_get_drvdata(dev); struct i740fb_par *par = info->par;

  console_lock();

@@ -1218,11 +1209,6 @@ static int i740fb_resume(struct pci_dev *dev) if (par->ref_count == 0) goto fail;

• pci_set_power_state(dev, PCI_D0);
• pci_restore_state(dev);
• if (pci_enable_device(dev))

• goto fail;
  

• i740fb_set_par(info); fb_set_suspend(info, 0);

@@ -1231,10 +1217,17 @@ static int i740fb_resume(struct pci_dev *dev) console_unlock(); return 0; } -#else -#define i740fb_suspend NULL -#define i740fb_resume NULL -#endif /* CONFIG_PM */

+static const struct dev_pm_ops i740fb_pm_ops = { +#ifdef CONFIG_PM_SLEEP

• .suspend = i740fb_suspend,
• .resume = i740fb_resume,
• .freeze = NULL,
• .thaw = i740fb_resume,
• .poweroff = i740fb_suspend,
• .restore = i740fb_resume,

+#endif /* CONFIG_PM_SLEEP */ +};

#define I740_ID_PCI 0x00d1 #define I740_ID_AGP 0x7800 @@ -1251,8 +1244,7 @@ static struct pci_driver i740fb_driver = { .id_table = i740fb_id_table, .probe = i740fb_probe, .remove = i740fb_remove,

• .suspend = i740fb_suspend,
• .resume = i740fb_resume,

• .driver.pm = &i740fb_pm_ops,

};

#ifndef MODULE

2.27.0

Drivers should do only device-specific jobs. But in general, drivers using legacy PCI PM framework for .suspend()/.resume() have to manage many PCI PM-related tasks themselves which can be done by PCI Core itself. This brings extra load on the driver and it directly calls PCI helper functions to handle them.

Switch to the new generic framework by updating function signatures and define a "struct dev_pm_ops" variable to bind PM callbacks. Also, remove unnecessary calls to the PCI Helper functions along with the legacy .suspend & .resume bindings.

Signed-off-by: Vaibhav Gupta vaibhavgupta40@gmail.com --- drivers/video/fbdev/vt8623fb.c | 41 ++++++++++++++-------------------- 1 file changed, 17 insertions(+), 24 deletions(-)

diff --git a/drivers/video/fbdev/vt8623fb.c b/drivers/video/fbdev/vt8623fb.c index 7b3eef1b893f..c488e0117758 100644 --- a/drivers/video/fbdev/vt8623fb.c +++ b/drivers/video/fbdev/vt8623fb.c @@ -815,12 +815,11 @@ static void vt8623_pci_remove(struct pci_dev *dev) }

-#ifdef CONFIG_PM /* PCI suspend */

-static int vt8623_pci_suspend(struct pci_dev* dev, pm_message_t state) +static int __maybe_unused vt8623_pci_suspend(struct device *dev) { - struct fb_info *info = pci_get_drvdata(dev); + struct fb_info *info = dev_get_drvdata(dev); struct vt8623fb_info *par = info->par;

dev_info(info->device, "suspend\n"); @@ -828,7 +827,7 @@ static int vt8623_pci_suspend(struct pci_dev* dev, pm_message_t state) console_lock(); mutex_lock(&(par->open_lock));

- if ((state.event == PM_EVENT_FREEZE) || (par->ref_count == 0)) { + if (par->ref_count == 0) { mutex_unlock(&(par->open_lock)); console_unlock(); return 0; @@ -836,10 +835,6 @@ static int vt8623_pci_suspend(struct pci_dev* dev, pm_message_t state)

fb_set_suspend(info, 1);

- pci_save_state(dev); - pci_disable_device(dev); - pci_set_power_state(dev, pci_choose_state(dev, state)); - mutex_unlock(&(par->open_lock)); console_unlock();

@@ -849,9 +844,9 @@ static int vt8623_pci_suspend(struct pci_dev* dev, pm_message_t state)

/* PCI resume */

-static int vt8623_pci_resume(struct pci_dev* dev) +static int __maybe_unused vt8623_pci_resume(struct device *dev) { - struct fb_info *info = pci_get_drvdata(dev); + struct fb_info *info = dev_get_drvdata(dev); struct vt8623fb_info *par = info->par;

dev_info(info->device, "resume\n"); @@ -862,14 +857,6 @@ static int vt8623_pci_resume(struct pci_dev* dev) if (par->ref_count == 0) goto fail;

- pci_set_power_state(dev, PCI_D0); - pci_restore_state(dev); - - if (pci_enable_device(dev)) - goto fail; - - pci_set_master(dev); - vt8623fb_set_par(info); fb_set_suspend(info, 0);

@@ -879,10 +866,17 @@ static int vt8623_pci_resume(struct pci_dev* dev)

return 0; } -#else -#define vt8623_pci_suspend NULL -#define vt8623_pci_resume NULL -#endif /* CONFIG_PM */ + +static const struct dev_pm_ops vt8623_pci_pm_ops = { +#ifdef CONFIG_PM_SLEEP + .suspend = vt8623_pci_suspend, + .resume = vt8623_pci_resume, + .freeze = NULL, + .thaw = vt8623_pci_resume, + .poweroff = vt8623_pci_suspend, + .restore = vt8623_pci_resume, +#endif /* CONFIG_PM_SLEEP */ +};

/* List of boards that we are trying to support */

@@ -898,8 +892,7 @@ static struct pci_driver vt8623fb_pci_driver = { .id_table = vt8623_devices, .probe = vt8623_pci_probe, .remove = vt8623_pci_remove, - .suspend = vt8623_pci_suspend, - .resume = vt8623_pci_resume, + .driver.pm = &vt8623_pci_pm_ops, };

/* Cleanup */

Drivers should do only device-specific jobs. But in general, drivers using legacy PCI PM framework for .suspend()/.resume() have to manage many PCI PM-related tasks themselves which can be done by PCI Core itself. This brings extra load on the driver and it directly calls PCI helper functions to handle them.

Switch to the new generic framework by updating function signatures and define a "struct dev_pm_ops" variable to bind PM callbacks. Also, remove unnecessary calls to the PCI Helper functions along with the legacy .suspend & .resume bindings.

Signed-off-by: Vaibhav Gupta vaibhavgupta40@gmail.com --- drivers/video/fbdev/arkfb.c | 41 +++++++++++++++---------------------- 1 file changed, 17 insertions(+), 24 deletions(-)

diff --git a/drivers/video/fbdev/arkfb.c b/drivers/video/fbdev/arkfb.c index 11ab9a153860..6a4114db0dfd 100644 --- a/drivers/video/fbdev/arkfb.c +++ b/drivers/video/fbdev/arkfb.c @@ -1085,12 +1085,11 @@ static void ark_pci_remove(struct pci_dev *dev) }

-#ifdef CONFIG_PM /* PCI suspend */

-static int ark_pci_suspend (struct pci_dev* dev, pm_message_t state) +static int __maybe_unused ark_pci_suspend(struct device *dev) { - struct fb_info *info = pci_get_drvdata(dev); + struct fb_info *info = dev_get_drvdata(dev); struct arkfb_info *par = info->par;

dev_info(info->device, "suspend\n"); @@ -1098,7 +1097,7 @@ static int ark_pci_suspend (struct pci_dev* dev, pm_message_t state) console_lock(); mutex_lock(&(par->open_lock));

- if ((state.event == PM_EVENT_FREEZE) || (par->ref_count == 0)) { + if (par->ref_count == 0) { mutex_unlock(&(par->open_lock)); console_unlock(); return 0; @@ -1106,10 +1105,6 @@ static int ark_pci_suspend (struct pci_dev* dev, pm_message_t state)

fb_set_suspend(info, 1);

- pci_save_state(dev); - pci_disable_device(dev); - pci_set_power_state(dev, pci_choose_state(dev, state)); - mutex_unlock(&(par->open_lock)); console_unlock();

@@ -1119,9 +1114,9 @@ static int ark_pci_suspend (struct pci_dev* dev, pm_message_t state)

/* PCI resume */

-static int ark_pci_resume (struct pci_dev* dev) +static int __maybe_unused ark_pci_resume(struct device *dev) { - struct fb_info *info = pci_get_drvdata(dev); + struct fb_info *info = dev_get_drvdata(dev); struct arkfb_info *par = info->par;

dev_info(info->device, "resume\n"); @@ -1132,14 +1127,6 @@ static int ark_pci_resume (struct pci_dev* dev) if (par->ref_count == 0) goto fail;

- pci_set_power_state(dev, PCI_D0); - pci_restore_state(dev); - - if (pci_enable_device(dev)) - goto fail; - - pci_set_master(dev); - arkfb_set_par(info); fb_set_suspend(info, 0);

@@ -1148,10 +1135,17 @@ static int ark_pci_resume (struct pci_dev* dev) console_unlock(); return 0; } -#else -#define ark_pci_suspend NULL -#define ark_pci_resume NULL -#endif /* CONFIG_PM */ + +static const struct dev_pm_ops ark_pci_pm_ops = { +#ifdef CONFIG_PM_SLEEP + .suspend = ark_pci_suspend, + .resume = ark_pci_resume, + .freeze = ark_pci_suspend, + .thaw = ark_pci_resume, + .poweroff = ark_pci_suspend, + .restore = ark_pci_resume, +#endif +};

/* List of boards that we are trying to support */

@@ -1168,8 +1162,7 @@ static struct pci_driver arkfb_pci_driver = { .id_table = ark_devices, .probe = ark_pci_probe, .remove = ark_pci_remove, - .suspend = ark_pci_suspend, - .resume = ark_pci_resume, + .driver.pm = &ark_pci_pm_ops, };

/* Cleanup */

Hi Vaibhav

On Wed, Aug 05, 2020 at 11:37:12PM +0530, Vaibhav Gupta wrote:

Drivers using legacy power management .suspen()/.resume() callbacks have to manage PCI states and device's PM states themselves. They also need to take care of standard configuration registers.

Switch to generic power management framework using a single "struct dev_pm_ops" variable to take the unnecessary load from the driver. This also avoids the need for the driver to directly call most of the PCI helper functions and device power state control functions, as through the generic framework PCI Core takes care of the necessary operations, and drivers are required to do only device-specific jobs.

Signed-off-by: Vaibhav Gupta vaibhavgupta40@gmail.com

Some of the same comments from fxfb applies to lxfb. Please address these and re-submit.

Sam

---

drivers/video/fbdev/geode/lxfb.h | 5 ---- drivers/video/fbdev/geode/lxfb_core.c | 37 +++++++++++++++------------ drivers/video/fbdev/geode/lxfb_ops.c | 4 --- 3 files changed, 20 insertions(+), 26 deletions(-)

diff --git a/drivers/video/fbdev/geode/lxfb.h b/drivers/video/fbdev/geode/lxfb.h index ef24bf6d49dc..d37b32dbcd68 100644 --- a/drivers/video/fbdev/geode/lxfb.h +++ b/drivers/video/fbdev/geode/lxfb.h @@ -29,7 +29,6 @@ struct lxfb_par { void __iomem *gp_regs; void __iomem *dc_regs; void __iomem *vp_regs; -#ifdef CONFIG_PM int powered_down;

/* register state, for power mgmt functionality */ @@ -50,7 +49,6 @@ struct lxfb_par { uint32_t hcoeff[DC_HFILT_COUNT * 2]; uint32_t vcoeff[DC_VFILT_COUNT]; uint32_t vp_coeff[VP_COEFF_SIZE / 4]; -#endif };

static inline unsigned int lx_get_pitch(unsigned int xres, int bpp) @@ -64,11 +62,8 @@ int lx_blank_display(struct fb_info *, int); void lx_set_palette_reg(struct fb_info *, unsigned int, unsigned int, unsigned int, unsigned int);

-#ifdef CONFIG_PM int lx_powerdown(struct fb_info *info); int lx_powerup(struct fb_info *info); -#endif

/* Graphics Processor registers (table 6-29 from the data book) */ enum gp_registers { diff --git a/drivers/video/fbdev/geode/lxfb_core.c b/drivers/video/fbdev/geode/lxfb_core.c index adc2d9c2395e..66c81262d18f 100644 --- a/drivers/video/fbdev/geode/lxfb_core.c +++ b/drivers/video/fbdev/geode/lxfb_core.c @@ -443,17 +443,14 @@ static struct fb_info *lxfb_init_fbinfo(struct device *dev) return info; }

-#ifdef CONFIG_PM -static int lxfb_suspend(struct pci_dev *pdev, pm_message_t state) +static int __maybe_unused lxfb_suspend(struct device *dev) {

• struct fb_info *info = pci_get_drvdata(pdev);

• struct fb_info *info = dev_get_drvdata(dev);

• if (state.event == PM_EVENT_SUSPEND) {

• console_lock();
  

• lx_powerdown(info);
  

• fb_set_suspend(info, 1);
  

• console_unlock();
  

• }

• console_lock();

• lx_powerdown(info);

• fb_set_suspend(info, 1);

• console_unlock();

  /* there's no point in setting PCI states; we emulate PCI, so

  • we don't end up getting power savings anyways */

@@ -461,9 +458,9 @@ static int lxfb_suspend(struct pci_dev *pdev, pm_message_t state) return 0; }

-static int lxfb_resume(struct pci_dev *pdev) +static int __maybe_unused lxfb_resume(struct device *dev) {

• struct fb_info *info = pci_get_drvdata(pdev);

• struct fb_info *info = dev_get_drvdata(dev); int ret;

  console_lock();

@@ -477,10 +474,6 @@ static int lxfb_resume(struct pci_dev *pdev) console_unlock(); return 0; } -#else -#define lxfb_suspend NULL -#define lxfb_resume NULL -#endif

static int lxfb_probe(struct pci_dev *pdev, const struct pci_device_id *id) { @@ -600,13 +593,23 @@ static struct pci_device_id lxfb_id_table[] = {

MODULE_DEVICE_TABLE(pci, lxfb_id_table);

+static const struct dev_pm_ops lxfb_pm_ops = { +#ifdef CONFIG_PM_SLEEP

• .suspend = lxfb_suspend,
• .resume = lxfb_resume,
• .freeze = NULL,
• .thaw = lxfb_resume,
• .poweroff = NULL,
• .restore = lxfb_resume,

+#endif +};

static struct pci_driver lxfb_driver = { .name = "lxfb", .id_table = lxfb_id_table, .probe = lxfb_probe, .remove = lxfb_remove,

• .suspend = lxfb_suspend,
• .resume = lxfb_resume,

• .driver.pm = &lxfb_pm_ops,

};

#ifndef MODULE diff --git a/drivers/video/fbdev/geode/lxfb_ops.c b/drivers/video/fbdev/geode/lxfb_ops.c index 5be8bc62844c..b3a041fce570 100644 --- a/drivers/video/fbdev/geode/lxfb_ops.c +++ b/drivers/video/fbdev/geode/lxfb_ops.c @@ -580,8 +580,6 @@ int lx_blank_display(struct fb_info *info, int blank_mode) return 0; }

-#ifdef CONFIG_PM

static void lx_save_regs(struct lxfb_par *par) { uint32_t filt; @@ -837,5 +835,3 @@ int lx_powerup(struct fb_info *info) par->powered_down = 0; return 0; }

-#endif

2.27.0

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel