diff mbox

drm/vc4: Add runtime PM support to the HDMI encoder driver

Message ID 1491928765-13995-1-git-send-email-boris.brezillon@free-electrons.com (mailing list archive)
State New, archived
Headers show

Commit Message

Boris BREZILLON April 11, 2017, 4:39 p.m. UTC
The HDMI driver is currently enabling all clks and probe time and keep
the power-domain connected to the HDMI encoder enabled.

Move all activation code to vc4_hdmi_encoder_enable() and make sure
the clks and power domain are released when the HDMI encoder is not used
by adding deactivation steps in vc4_hdmi_encoder_disable().

Note that the sequencing imposed by the IP requires that we move
vc4_hdmi_encoder_mode_set() code into vc4_hdmi_encoder_enable().

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
Hi Eric,

As for the previous submission, I first send a preliminary version of
the patch for internal review.

Let me know if you want me to submit this patch.

Regards,

Boris

 drivers/gpu/drm/vc4/vc4_hdmi.c | 174 ++++++++++++++++++++++-------------------
 1 file changed, 92 insertions(+), 82 deletions(-)

Comments

Eric Anholt April 14, 2017, 6:20 p.m. UTC | #1
Boris Brezillon <boris.brezillon@free-electrons.com> writes:

> The HDMI driver is currently enabling all clks and probe time and keep
> the power-domain connected to the HDMI encoder enabled.

How about "The HDMI driver is currently enabling all clocks at probe
time and keeps the power domain..."?

> Move all activation code to vc4_hdmi_encoder_enable() and make sure
> the clks and power domain are released when the HDMI encoder is not used
> by adding deactivation steps in vc4_hdmi_encoder_disable().
>
> Note that the sequencing imposed by the IP requires that we move
> vc4_hdmi_encoder_mode_set() code into vc4_hdmi_encoder_enable().

I'm quite happy to see _mode_set() gone.

I'm hoping to go through a bunch of mode switching testing with this
Monday.
Boris BREZILLON April 14, 2017, 7:31 p.m. UTC | #2
On Fri, 14 Apr 2017 11:20:52 -0700
Eric Anholt <eric@anholt.net> wrote:

> Boris Brezillon <boris.brezillon@free-electrons.com> writes:
> 
> > The HDMI driver is currently enabling all clks and probe time and keep
> > the power-domain connected to the HDMI encoder enabled.  
> 
> How about "The HDMI driver is currently enabling all clocks at probe
> time and keeps the power domain..."?

Yep.

> 
> > Move all activation code to vc4_hdmi_encoder_enable() and make sure
> > the clks and power domain are released when the HDMI encoder is not used
> > by adding deactivation steps in vc4_hdmi_encoder_disable().
> >
> > Note that the sequencing imposed by the IP requires that we move
> > vc4_hdmi_encoder_mode_set() code into vc4_hdmi_encoder_enable().  
> 
> I'm quite happy to see _mode_set() gone.
> 
> I'm hoping to go through a bunch of mode switching testing with this
> Monday.

Ok, cool. Let me know if you have any problem.

Thanks,

Boris
Eric Anholt April 18, 2017, 9:34 p.m. UTC | #3
Boris Brezillon <boris.brezillon@free-electrons.com> writes:

> On Fri, 14 Apr 2017 11:20:52 -0700
> Eric Anholt <eric@anholt.net> wrote:
>
>> Boris Brezillon <boris.brezillon@free-electrons.com> writes:
>> 
>> > The HDMI driver is currently enabling all clks and probe time and keep
>> > the power-domain connected to the HDMI encoder enabled.  
>> 
>> How about "The HDMI driver is currently enabling all clocks at probe
>> time and keeps the power domain..."?
>
> Yep.
>
>> 
>> > Move all activation code to vc4_hdmi_encoder_enable() and make sure
>> > the clks and power domain are released when the HDMI encoder is not used
>> > by adding deactivation steps in vc4_hdmi_encoder_disable().
>> >
>> > Note that the sequencing imposed by the IP requires that we move
>> > vc4_hdmi_encoder_mode_set() code into vc4_hdmi_encoder_enable().  
>> 
>> I'm quite happy to see _mode_set() gone.
>> 
>> I'm hoping to go through a bunch of mode switching testing with this
>> Monday.
>
> Ok, cool. Let me know if you have any problem.

I did a whole bunch of modesets today and things seemed good.  VEC
didn't turn on when I switched to it, but switching back to HDMI worked,
which didn't before.

Reviewed and pushed.  Thanks!
diff mbox

Patch

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 93d5994f3a04..08085e137ae3 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -33,6 +33,7 @@ 
 #include "linux/i2c.h"
 #include "linux/of_gpio.h"
 #include "linux/of_platform.h"
+#include "linux/pm_runtime.h"
 #include "vc4_drv.h"
 #include "vc4_regs.h"
 
@@ -387,13 +388,38 @@  static void vc4_hdmi_set_infoframes(struct drm_encoder *encoder)
 	vc4_hdmi_set_spd_infoframe(encoder);
 }
 
-static void vc4_hdmi_encoder_mode_set(struct drm_encoder *encoder,
-				      struct drm_display_mode *unadjusted_mode,
-				      struct drm_display_mode *mode)
+static void vc4_hdmi_encoder_disable(struct drm_encoder *encoder)
+{
+	struct drm_device *dev = encoder->dev;
+	struct vc4_dev *vc4 = to_vc4_dev(dev);
+	struct vc4_hdmi *hdmi = vc4->hdmi;
+	int ret;
+
+	HDMI_WRITE(VC4_HDMI_RAM_PACKET_CONFIG, 0);
+
+	HDMI_WRITE(VC4_HDMI_TX_PHY_RESET_CTL, 0xf << 16);
+	HD_WRITE(VC4_HD_VID_CTL,
+		 HD_READ(VC4_HD_VID_CTL) & ~VC4_HD_VID_CTL_ENABLE);
+
+	HD_WRITE(VC4_HD_M_CTL, VC4_HD_M_SW_RST);
+	udelay(1);
+	HD_WRITE(VC4_HD_M_CTL, 0);
+
+	clk_disable_unprepare(hdmi->hsm_clock);
+	clk_disable_unprepare(hdmi->pixel_clock);
+
+	ret = pm_runtime_put(&hdmi->pdev->dev);
+	if (ret < 0)
+		DRM_ERROR("Failed to release power domain: %d\n", ret);
+}
+
+static void vc4_hdmi_encoder_enable(struct drm_encoder *encoder)
 {
+	struct drm_display_mode *mode = &encoder->crtc->state->adjusted_mode;
 	struct vc4_hdmi_encoder *vc4_encoder = to_vc4_hdmi_encoder(encoder);
 	struct drm_device *dev = encoder->dev;
 	struct vc4_dev *vc4 = to_vc4_dev(dev);
+	struct vc4_hdmi *hdmi = vc4->hdmi;
 	bool debug_dump_regs = false;
 	bool hsync_pos = mode->flags & DRM_MODE_FLAG_PHSYNC;
 	bool vsync_pos = mode->flags & DRM_MODE_FLAG_PVSYNC;
@@ -413,6 +439,64 @@  static void vc4_hdmi_encoder_mode_set(struct drm_encoder *encoder,
 					interlaced,
 					VC4_HDMI_VERTB_VBP));
 	u32 csc_ctl;
+	int ret;
+
+	ret = pm_runtime_get_sync(&hdmi->pdev->dev);
+	if (ret < 0) {
+		DRM_ERROR("Failed to retain power domain: %d\n", ret);
+		return;
+	}
+
+	/* This is the rate that is set by the firmware.  The number
+	 * needs to be a bit higher than the pixel clock rate
+	 * (generally 148.5Mhz).
+	 */
+	ret = clk_set_rate(hdmi->hsm_clock, 163682864);
+	if (ret) {
+		DRM_ERROR("Failed to set HSM clock rate: %d\n", ret);
+		return;
+	}
+
+	ret = clk_set_rate(hdmi->pixel_clock,
+			   mode->clock * 1000 *
+			   ((mode->flags & DRM_MODE_FLAG_DBLCLK) ? 2 : 1));
+	if (ret) {
+		DRM_ERROR("Failed to set pixel clock rate: %d\n", ret);
+		return;
+	}
+
+	ret = clk_prepare_enable(hdmi->pixel_clock);
+	if (ret) {
+		DRM_ERROR("Failed to turn on pixel clock: %d\n", ret);
+		return;
+	}
+
+	ret = clk_prepare_enable(hdmi->hsm_clock);
+	if (ret) {
+		DRM_ERROR("Failed to turn on HDMI state machine clock: %d\n",
+			  ret);
+		clk_disable_unprepare(hdmi->pixel_clock);
+		return;
+	}
+
+	HD_WRITE(VC4_HD_M_CTL, VC4_HD_M_SW_RST);
+	udelay(1);
+	HD_WRITE(VC4_HD_M_CTL, 0);
+
+	HD_WRITE(VC4_HD_M_CTL, VC4_HD_M_ENABLE);
+
+	HDMI_WRITE(VC4_HDMI_SW_RESET_CONTROL,
+		   VC4_HDMI_SW_RESET_HDMI |
+		   VC4_HDMI_SW_RESET_FORMAT_DETECT);
+
+	HDMI_WRITE(VC4_HDMI_SW_RESET_CONTROL, 0);
+
+	/* PHY should be in reset, like
+	 * vc4_hdmi_encoder_disable() does.
+	 */
+	HDMI_WRITE(VC4_HDMI_TX_PHY_RESET_CTL, 0xf << 16);
+
+	HDMI_WRITE(VC4_HDMI_TX_PHY_RESET_CTL, 0);
 
 	if (debug_dump_regs) {
 		DRM_INFO("HDMI regs before:\n");
@@ -421,9 +505,6 @@  static void vc4_hdmi_encoder_mode_set(struct drm_encoder *encoder,
 
 	HD_WRITE(VC4_HD_VID_CTL, 0);
 
-	clk_set_rate(vc4->hdmi->pixel_clock, mode->clock * 1000 *
-		     ((mode->flags & DRM_MODE_FLAG_DBLCLK) ? 2 : 1));
-
 	HDMI_WRITE(VC4_HDMI_SCHEDULER_CONTROL,
 		   HDMI_READ(VC4_HDMI_SCHEDULER_CONTROL) |
 		   VC4_HDMI_SCHEDULER_CONTROL_MANUAL_FORMAT |
@@ -497,28 +578,6 @@  static void vc4_hdmi_encoder_mode_set(struct drm_encoder *encoder,
 		DRM_INFO("HDMI regs after:\n");
 		vc4_hdmi_dump_regs(dev);
 	}
-}
-
-static void vc4_hdmi_encoder_disable(struct drm_encoder *encoder)
-{
-	struct drm_device *dev = encoder->dev;
-	struct vc4_dev *vc4 = to_vc4_dev(dev);
-
-	HDMI_WRITE(VC4_HDMI_RAM_PACKET_CONFIG, 0);
-
-	HDMI_WRITE(VC4_HDMI_TX_PHY_RESET_CTL, 0xf << 16);
-	HD_WRITE(VC4_HD_VID_CTL,
-		 HD_READ(VC4_HD_VID_CTL) & ~VC4_HD_VID_CTL_ENABLE);
-}
-
-static void vc4_hdmi_encoder_enable(struct drm_encoder *encoder)
-{
-	struct vc4_hdmi_encoder *vc4_encoder = to_vc4_hdmi_encoder(encoder);
-	struct drm_device *dev = encoder->dev;
-	struct vc4_dev *vc4 = to_vc4_dev(dev);
-	int ret;
-
-	HDMI_WRITE(VC4_HDMI_TX_PHY_RESET_CTL, 0);
 
 	HD_WRITE(VC4_HD_VID_CTL,
 		 HD_READ(VC4_HD_VID_CTL) |
@@ -584,7 +643,6 @@  static void vc4_hdmi_encoder_enable(struct drm_encoder *encoder)
 }
 
 static const struct drm_encoder_helper_funcs vc4_hdmi_encoder_helper_funcs = {
-	.mode_set = vc4_hdmi_encoder_mode_set,
 	.disable = vc4_hdmi_encoder_disable,
 	.enable = vc4_hdmi_encoder_enable,
 };
@@ -644,33 +702,6 @@  static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
 		return -EPROBE_DEFER;
 	}
 
-	/* Enable the clocks at startup.  We can't quite recover from
-	 * turning off the pixel clock during disable/enables yet, so
-	 * it's always running.
-	 */
-	ret = clk_prepare_enable(hdmi->pixel_clock);
-	if (ret) {
-		DRM_ERROR("Failed to turn on pixel clock: %d\n", ret);
-		goto err_put_i2c;
-	}
-
-	/* This is the rate that is set by the firmware.  The number
-	 * needs to be a bit higher than the pixel clock rate
-	 * (generally 148.5Mhz).
-	 */
-	ret = clk_set_rate(hdmi->hsm_clock, 163682864);
-	if (ret) {
-		DRM_ERROR("Failed to set HSM clock rate: %d\n", ret);
-		goto err_unprepare_pix;
-	}
-
-	ret = clk_prepare_enable(hdmi->hsm_clock);
-	if (ret) {
-		DRM_ERROR("Failed to turn on HDMI state machine clock: %d\n",
-			  ret);
-		goto err_unprepare_pix;
-	}
-
 	/* Only use the GPIO HPD pin if present in the DT, otherwise
 	 * we'll use the HDMI core's register.
 	 */
@@ -682,7 +713,7 @@  static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
 							 &hpd_gpio_flags);
 		if (hdmi->hpd_gpio < 0) {
 			ret = hdmi->hpd_gpio;
-			goto err_unprepare_hsm;
+			goto err_put_i2c;
 		}
 
 		hdmi->hpd_active_low = hpd_gpio_flags & OF_GPIO_ACTIVE_LOW;
@@ -690,25 +721,7 @@  static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
 
 	vc4->hdmi = hdmi;
 
-	/* HDMI core must be enabled. */
-	if (!(HD_READ(VC4_HD_M_CTL) & VC4_HD_M_ENABLE)) {
-		HD_WRITE(VC4_HD_M_CTL, VC4_HD_M_SW_RST);
-		udelay(1);
-		HD_WRITE(VC4_HD_M_CTL, 0);
-
-		HD_WRITE(VC4_HD_M_CTL, VC4_HD_M_ENABLE);
-
-		HDMI_WRITE(VC4_HDMI_SW_RESET_CONTROL,
-			   VC4_HDMI_SW_RESET_HDMI |
-			   VC4_HDMI_SW_RESET_FORMAT_DETECT);
-
-		HDMI_WRITE(VC4_HDMI_SW_RESET_CONTROL, 0);
-
-		/* PHY should be in reset, like
-		 * vc4_hdmi_encoder_disable() does.
-		 */
-		HDMI_WRITE(VC4_HDMI_TX_PHY_RESET_CTL, 0xf << 16);
-	}
+	pm_runtime_enable(dev);
 
 	drm_encoder_init(drm, hdmi->encoder, &vc4_hdmi_encoder_funcs,
 			 DRM_MODE_ENCODER_TMDS, NULL);
@@ -724,10 +737,7 @@  static int vc4_hdmi_bind(struct device *dev, struct device *master, void *data)
 
 err_destroy_encoder:
 	vc4_hdmi_encoder_destroy(hdmi->encoder);
-err_unprepare_hsm:
-	clk_disable_unprepare(hdmi->hsm_clock);
-err_unprepare_pix:
-	clk_disable_unprepare(hdmi->pixel_clock);
+	pm_runtime_disable(dev);
 err_put_i2c:
 	put_device(&hdmi->ddc->dev);
 
@@ -744,8 +754,8 @@  static void vc4_hdmi_unbind(struct device *dev, struct device *master,
 	vc4_hdmi_connector_destroy(hdmi->connector);
 	vc4_hdmi_encoder_destroy(hdmi->encoder);
 
-	clk_disable_unprepare(hdmi->pixel_clock);
-	clk_disable_unprepare(hdmi->hsm_clock);
+	pm_runtime_disable(dev);
+
 	put_device(&hdmi->ddc->dev);
 
 	vc4->hdmi = NULL;