video: s3c-fb: Use runtime suspend while blanked

-----Original Message-----
From: Mark Brown [mailto:[email protected]]
Sent: Tuesday, December 27, 2011 11:16 PM
To: Jingoo Han; Florian Tobias Schandinat
Cc: [email protected]; [email protected]; Mark Brown
Subject: [PATCH 3/6] video: s3c-fb: Disable runtime PM in error paths from probe

Signed-off-by Mark Brown <[email protected]>

---
 drivers/video/s3c-fb.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

drivers/video/s3c-fb.c +5 -3

index be4c218..2e0eef0 100644
@@ -1464,7 +1464,7 @@ static int __devinit s3c_fb_probe(struct platform_device *pdev)
 			dev_err(dev, "failed to create window %d\n", win);
 			for (; win >= 0; win--)
 				s3c_fb_release_win(sfb, sfb->windows[win]);
-			goto err_irq;
+			goto err_pm_runtime;
 		}
 	}
 

@@ -1473,7 +1473,8 @@ static int __devinit s3c_fb_probe(struct platform_device *pdev)
 
 	return 0;
 
-err_irq:
+err_pm_runtime:
+	pm_runtime_put_sync(sfb->dev);
 	free_irq(sfb->irq_no, sfb);
 
 err_ioremap:

@@ -1483,6 +1484,8 @@ err_req_region:
 	release_mem_region(sfb->regs_res->start, resource_size(sfb->regs_res));
 
 err_lcd_clk:
+	pm_runtime_disable(sfb->dev);
+
 	if (!sfb->variant.has_clksel) {
 		clk_disable(sfb->lcd_clk);
 		clk_put(sfb->lcd_clk);
--

1.7.7.3

-----Original Message-----
From: Mark Brown [mailto:[email protected]]
Sent: Tuesday, December 27, 2011 11:16 PM
To: Jingoo Han; Florian Tobias Schandinat
Cc: [email protected]; [email protected]; Mark Brown
Subject: [PATCH 4/6] video: s3c-fb: Take a runtime PM reference when unblanked

When the framebuffer is unblanked hold a runtime PM reference. This prevents us powering down when userspace has left an image on the framebuffer and prepares the way for being able to power down the hardware when an application still has the device open.

Since we now hold a runtime PM reference whenever the display is unblanked there is no need for the runtime power management to disable and enable the display, and doing so would lead to runtime PM trying to recurse into itself when called from the blanking code, so split the runtime PM into separate functions which only deal with the clocks. The PM core will runtime resume the device prior to system suspend.

Signed-off-by Mark Brown <[email protected]>

---
 drivers/video/s3c-fb.c |   66 +++++++++++++++++++++++++++++++++++++++--------
 1 files changed, 54 insertions(+), 12 deletions(-)

drivers/video/s3c-fb.c +54 -13

index 2e0eef0..688b9d8 100644
@@ -192,6 +192,7 @@ struct s3c_fb_vsync {
  * @regs: The mapped hardware registers.
  * @variant: Variant information for this hardware.
  * @enabled: A bitmask of enabled hardware windows.
+ * @output_on: Flag if the physical output is enabled.
  * @pdata: The platform configuration data passed with the device.
  * @windows: The hardware windows that have been claimed.
  * @irq_no: IRQ line number

@@ -208,6 +209,7 @@ struct s3c_fb {
 	struct s3c_fb_variant	 variant;
 
 	unsigned char		 enabled;
+	bool			 output_on;
 
 	struct s3c_fb_platdata	*pdata;
 	struct s3c_fb_win	*windows[S3C_FB_MAX_WIN];

@@ -449,21 +451,28 @@ static void s3c_fb_enable(struct s3c_fb *sfb, int enable)
 {
 	u32 vidcon0 = readl(sfb->regs + VIDCON0);
 
-	if (enable)
+	if (enable && !sfb->output_on)
+		pm_runtime_get_sync(sfb->dev);
+
+	if (enable) {
 		vidcon0 |= VIDCON0_ENVID | VIDCON0_ENVID_F;
-	else {
+	} else {
 		/* see the note in the framebuffer datasheet about
 		 * why you cannot take both of these bits down at the
 		 * same time. */
 
-		if (!(vidcon0 & VIDCON0_ENVID))
-			return;
-
-		vidcon0 |= VIDCON0_ENVID;
-		vidcon0 &= ~VIDCON0_ENVID_F;
+		if (vidcon0 & VIDCON0_ENVID) {
+			vidcon0 |= VIDCON0_ENVID;
+			vidcon0 &= ~VIDCON0_ENVID_F;
+		}
 	}
 
 	writel(vidcon0, sfb->regs + VIDCON0);
+
+	if (!enable && sfb->output_on)
+		pm_runtime_put_sync(sfb->dev);
+
+	sfb->output_on = enable;
 }
 
 /**

@@ -1539,7 +1548,7 @@ static int __devexit s3c_fb_remove(struct platform_device *pdev)
 	return 0;
 }
 
-#ifdef CONFIG_PM
+#ifdef CONFIG_PM_SLEEP
 static int s3c_fb_suspend(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);

@@ -1609,11 +1618,40 @@ static int s3c_fb_resume(struct device *dev)
 
 	return 0;
 }
-#else
-#define s3c_fb_suspend NULL
-#define s3c_fb_resume  NULL
 #endif
 
+#ifdef CONFIG_PM_RUNTIME
+static int s3c_fb_runtime_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct s3c_fb *sfb = platform_get_drvdata(pdev);
+
+	if (!sfb->variant.has_clksel)
+		clk_disable(sfb->lcd_clk);
+
+	clk_disable(sfb->bus_clk);
+
+	return 0;
+}
+
+static int s3c_fb_runtime_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct s3c_fb *sfb = platform_get_drvdata(pdev);
+	struct s3c_fb_platdata *pd = sfb->pdata;
+
+	clk_enable(sfb->bus_clk);
+
+	if (!sfb->variant.has_clksel)
+		clk_enable(sfb->lcd_clk);
+
+	/* setup gpio and output polarity controls */
+	pd->setup_gpio();
+	writel(pd->vidcon1, sfb->regs + VIDCON1);
+
+	return 0;
+}
+#endif
 
 #define VALID_BPP124 (VALID_BPP(1) | VALID_BPP(2) | VALID_BPP(4))
 #define VALID_BPP1248 (VALID_BPP124 | VALID_BPP(8))

@@ -1936,7 +1974,11 @@ static struct platform_device_id s3c_fb_driver_ids[] = {
 };
 MODULE_DEVICE_TABLE(platform, s3c_fb_driver_ids);
 
-static UNIVERSAL_DEV_PM_OPS(s3cfb_pm_ops, s3c_fb_suspend, s3c_fb_resume, NULL);
+static const struct dev_pm_ops s3cfb_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(s3c_fb_suspend, s3c_fb_resume)
+	SET_RUNTIME_PM_OPS(s3c_fb_runtime_suspend, s3c_fb_runtime_resume,
+			   NULL)
+};
 
 static struct platform_driver s3c_fb_driver = {
 	.probe		= s3c_fb_probe,
--

1.7.7.3

-----Original Message-----
From: Mark Brown [mailto:[email protected]]
Sent: Tuesday, December 27, 2011 11:16 PM
To: Jingoo Han; Florian Tobias Schandinat
Cc: [email protected]; [email protected]; Mark Brown
Subject: [PATCH 5/6] video: s3c-fb: Hold runtime PM references when touching registers

Take a runtime PM reference whenever updating registers in preparation for suspending the device when the framebuffer is blanked.

Signed-off-by Mark Brown <[email protected]>

---
 drivers/video/s3c-fb.c |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)

drivers/video/s3c-fb.c +22 -1

index 688b9d8..84cf631 100644
@@ -496,6 +496,8 @@ static int s3c_fb_set_par(struct fb_info *info)
 
 	dev_dbg(sfb->dev, "setting framebuffer parameters\n");
 
+	pm_runtime_get_sync(sfb->dev);
+
 	shadow_protect_win(win, 1);
 
 	switch (var->bits_per_pixel) {

@@ -692,6 +694,8 @@ static int s3c_fb_set_par(struct fb_info *info)
 
 	shadow_protect_win(win, 0);
 
+	pm_runtime_put_sync(sfb->dev);
+
 	return 0;
 }
 

@@ -763,6 +767,8 @@ static int s3c_fb_setcolreg(unsigned regno,
 	dev_dbg(sfb->dev, "%s: win %d: %d => rgb=%d/%d/%d\n",
 		__func__, win->index, regno, red, green, blue);
 
+	pm_runtime_get_sync(sfb->dev);
+
 	switch (info->fix.visual) {
 	case FB_VISUAL_TRUECOLOR:
 		/* true-colour, use pseudo-palette */

@@ -790,9 +796,11 @@ static int s3c_fb_setcolreg(unsigned regno,
 		break;
 
 	default:
+		pm_runtime_put_sync(sfb->dev);
 		return 1;	/* unknown type */
 	}
 
+	pm_runtime_put_sync(sfb->dev);
 	return 0;
 }
 

@@ -812,6 +820,8 @@ static int s3c_fb_blank(int blank_mode, struct fb_info *info)
 
 	dev_dbg(sfb->dev, "blank mode %d\n", blank_mode);
 
+	pm_runtime_get_sync(sfb->dev);
+
 	wincon = readl(sfb->regs + sfb->variant.wincon + (index * 4));
 
 	switch (blank_mode) {

@@ -839,6 +849,7 @@ static int s3c_fb_blank(int blank_mode, struct fb_info *info)
 	case FB_BLANK_VSYNC_SUSPEND:
 	case FB_BLANK_HSYNC_SUSPEND:
 	default:
+		pm_runtime_put_sync(sfb->dev);
 		return 1;
 	}
 

@@ -869,6 +880,8 @@ static int s3c_fb_blank(int blank_mode, struct fb_info *info)
 		shadow_protect_win(win, 0);
 	}
 
+	pm_runtime_put_sync(sfb->dev);
+
 	return 0;
 }
 

@@ -891,6 +904,8 @@ static int s3c_fb_pan_display(struct fb_var_screeninfo *var,
 	void __iomem *buf	= sfb->regs + win->index * 8;
 	unsigned int start_boff, end_boff;
 
+	pm_runtime_get_sync(sfb->dev);
+
 	/* Offset in bytes to the start of the displayed area */
 	start_boff = var->yoffset * info->fix.line_length;
 	/* X offset depends on the current bpp */

@@ -909,6 +924,7 @@ static int s3c_fb_pan_display(struct fb_var_screeninfo *var,
 			break;
 		default:
 			dev_err(sfb->dev, "invalid bpp\n");
+			pm_runtime_put_sync(sfb->dev);
 			return -EINVAL;
 		}
 	}

@@ -924,6 +940,7 @@ static int s3c_fb_pan_display(struct fb_var_screeninfo *var,
 
 	shadow_protect_win(win, 0);
 
+	pm_runtime_put_sync(sfb->dev);
 	return 0;
 }
 

@@ -1013,11 +1030,16 @@ static int s3c_fb_wait_for_vsync(struct s3c_fb *sfb, u32 crtc)
 	if (crtc != 0)
 		return -ENODEV;
 
+	pm_runtime_get_sync(sfb->dev);
+
 	count = sfb->vsync_info.count;
 	s3c_fb_enable_irq(sfb);
 	ret = wait_event_interruptible_timeout(sfb->vsync_info.wait,
 				       count != sfb->vsync_info.count,
 				       msecs_to_jiffies(VSYNC_TIMEOUT_MSEC));
+
+	pm_runtime_put_sync(sfb->dev);
+
 	if (ret = 0)
 		return -ETIMEDOUT;
 
--

1.7.7.3

-----Original Message-----
From: Mark Brown [mailto:[email protected]]
Sent: Tuesday, December 27, 2011 11:16 PM
To: Jingoo Han; Florian Tobias Schandinat
Cc: [email protected]; [email protected]; Mark Brown
Subject: [PATCH 6/6] video: s3c-fb: Don't keep device runtime active when open

Allow the controller to be runtime suspended when the screen is blanked by not taking a runtime reference while the device is open. This allows greater system wide power savings when used with a standard application layer and ensures that the screen does not blank unless requested.

Signed-off-by Mark Brown <[email protected]>

---
 drivers/video/s3c-fb.c |   22 ----------------------
 1 files changed, 0 insertions(+), 22 deletions(-)

drivers/video/s3c-fb.c +0 -23

index 84cf631..0c63b69 100644
@@ -1070,30 +1070,8 @@ static int s3c_fb_ioctl(struct fb_info *info, unsigned int cmd,
 	return ret;
 }
 
-static int s3c_fb_open(struct fb_info *info, int user)
-{
-	struct s3c_fb_win *win = info->par;
-	struct s3c_fb *sfb = win->parent;
-
-	pm_runtime_get_sync(sfb->dev);
-
-	return 0;
-}
-
-static int s3c_fb_release(struct fb_info *info, int user)
-{
-	struct s3c_fb_win *win = info->par;
-	struct s3c_fb *sfb = win->parent;
-
-	pm_runtime_put_sync(sfb->dev);
-
-	return 0;
-}
-
 static struct fb_ops s3c_fb_ops = {
 	.owner		= THIS_MODULE,
-	.fb_open	= s3c_fb_open,
-	.fb_release	= s3c_fb_release,
 	.fb_check_var	= s3c_fb_check_var,
 	.fb_set_par	= s3c_fb_set_par,
 	.fb_blank	= s3c_fb_blank,
--

1.7.7.3

-----Original Message-----
From: Mark Brown [mailto:[email protected]]
Sent: Tuesday, December 27, 2011 11:16 PM
To: Jingoo Han; Florian Tobias Schandinat
Cc: [email protected]; [email protected]; Mark Brown
Subject: [PATCH 1/6] video: s3c-fb: Make runtime PM functional again

The change in "video: s3c-fb: modify runtime pm functions" (commit 35784b) renders the runtime power management for the device completely ineffectual as while it leaves runtime power management notionally enabled a runtime power reference is held for the entire time the device is registered meaning it will never actually do anything.

A further issue is introduced as runtime power management is added during the system suspend path which is not something which drivers are supposed to do and would interact poorly if there were any operations done in the runtime power management callbacks.

While this does make things simpler (the main motivation for the original change) it will not only cause us to use more power in the framebuffer controller but will also prevent us entering lower power domain and SoC wide states as we can never power down the domain containing the device. Since neither of these things is desirable revert the change.

Signed-off-by Mark Brown <[email protected]>

---
 drivers/video/s3c-fb.c |   51 +++++++++++++++++++++++++++++------------------
 1 files changed, 31 insertions(+), 20 deletions(-)

drivers/video/s3c-fb.c +31 -21

index a0b3fd6..b1a75a0 100644
@@ -1038,8 +1038,30 @@ static int s3c_fb_ioctl(struct fb_info *info, unsigned int cmd,
 	return ret;
 }
 
+static int s3c_fb_open(struct fb_info *info, int user)
+{
+	struct s3c_fb_win *win = info->par;
+	struct s3c_fb *sfb = win->parent;
+
+	pm_runtime_get_sync(sfb->dev);
+
+	return 0;
+}
+
+static int s3c_fb_release(struct fb_info *info, int user)
+{
+	struct s3c_fb_win *win = info->par;
+	struct s3c_fb *sfb = win->parent;
+
+	pm_runtime_put_sync(sfb->dev);
+
+	return 0;
+}
+
 static struct fb_ops s3c_fb_ops = {
 	.owner		= THIS_MODULE,
+	.fb_open	= s3c_fb_open,
+	.fb_release	= s3c_fb_release,
 	.fb_check_var	= s3c_fb_check_var,
 	.fb_set_par	= s3c_fb_set_par,
 	.fb_blank	= s3c_fb_blank,

@@ -1446,6 +1468,7 @@ static int __devinit s3c_fb_probe(struct platform_device *pdev)
 	}
 
 	platform_set_drvdata(pdev, sfb);
+	pm_runtime_put_sync(sfb->dev);
 
 	return 0;
 

@@ -1485,6 +1508,8 @@ static int __devexit s3c_fb_remove(struct platform_device *pdev)
 	struct s3c_fb *sfb = platform_get_drvdata(pdev);
 	int win;
 
+	pm_runtime_get_sync(sfb->dev);
+
 	for (win = 0; win < S3C_FB_MAX_WIN; win++)
 		if (sfb->windows[win])
 			s3c_fb_release_win(sfb, sfb->windows[win]);

@@ -1510,7 +1535,7 @@ static int __devexit s3c_fb_remove(struct platform_device *pdev)
 	return 0;
 }
 
-#ifdef CONFIG_PM_SLEEP
+#ifdef CONFIG_PM
 static int s3c_fb_suspend(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);

@@ -1531,8 +1556,6 @@ static int s3c_fb_suspend(struct device *dev)
 		clk_disable(sfb->lcd_clk);
 
 	clk_disable(sfb->bus_clk);
-	pm_runtime_put_sync(sfb->dev);
-
 	return 0;
 }
 

@@ -1544,7 +1567,6 @@ static int s3c_fb_resume(struct device *dev)
 	struct s3c_fb_win *win;
 	int win_no;
 
-	pm_runtime_get_sync(sfb->dev);
 	clk_enable(sfb->bus_clk);
 
 	if (!sfb->variant.has_clksel)

@@ -1583,19 +1605,11 @@ static int s3c_fb_resume(struct device *dev)
 
 	return 0;
 }
+#else
+#define s3c_fb_suspend NULL
+#define s3c_fb_resume  NULL
 #endif
 
-#ifdef CONFIG_PM_RUNTIME
-static int s3c_fb_runtime_suspend(struct device *dev)
-{
-	return 0;
-}
-
-static int s3c_fb_runtime_resume(struct device *dev)
-{
-	return 0;
-}
-#endif
 
 #define VALID_BPP124 (VALID_BPP(1) | VALID_BPP(2) | VALID_BPP(4))
 #define VALID_BPP1248 (VALID_BPP124 | VALID_BPP(8))

@@ -1918,10 +1932,7 @@ static struct platform_device_id s3c_fb_driver_ids[] = {
 };
 MODULE_DEVICE_TABLE(platform, s3c_fb_driver_ids);
 
-static const struct dev_pm_ops s3c_fb_pm_ops = {
-	SET_SYSTEM_SLEEP_PM_OPS(s3c_fb_suspend, s3c_fb_resume)
-	SET_RUNTIME_PM_OPS(s3c_fb_runtime_suspend, s3c_fb_runtime_resume, NULL)
-};
+static UNIVERSAL_DEV_PM_OPS(s3cfb_pm_ops, s3c_fb_suspend, s3c_fb_resume, NULL);
 
 static struct platform_driver s3c_fb_driver = {
 	.probe		= s3c_fb_probe,

@@ -1930,7 +1941,7 @@ static struct platform_driver s3c_fb_driver = {
 	.driver		= {
 		.name	= "s3c-fb",
 		.owner	= THIS_MODULE,
-		.pm	= &s3c_fb_pm_ops,
+		.pm	= &s3cfb_pm_ops,
 	},
 };
 
--

1.7.7.3

This patch series (the first two of which I posted the other day) improves the runtime power management in the s3c-fb driver by moving it to keeping the device runtime suspended when the screen is powered down by userspace. This saves a trivial amount of power in the framebuffer controller and allows the SoC core code to enter system wide idle states which offer much more substantial gains.

Currently it's only lightly tested but it certainly seems to the right thing for me.

Mark Brown (6):
video: s3c-fb: Make runtime PM functional again
video: s3c-fb: Use s3c_fb_enable() to enable the framebuffer
video: s3c-fb: Disable runtime PM in error paths from probe
video: s3c-fb: Take a runtime PM reference when unblanked
video: s3c-fb: Hold runtime PM references when touching registers
video: s3c-fb: Don't keep device runtime active when open

 drivers/video/s3c-fb.c |  127 ++++++++++++++++++++++++++++++++++-------------
 1 files changed, 92 insertions(+), 35 deletions(-)