[PATCH 0/8] drm/arm/malidp: Enhance support for system and runtime power management on malidp.

[PATCH 0/8] drm/arm/malidp: Enhance support for system and runtime power management on malidp.

[Ayan Kumar Halder]

This patch series enhances and fixes certain issues relevant to system and
runtime power management on malidp.

Ayan Kumar Halder (8):
  drm/arm/malidp: Modified the prototype of malidp_de_irq_fini
  drm/arm/malidp: Modified the prototype of malidp_se_irq_fini
  drm/arm/malidp: Split malidp_de_irq_init
  drm/arm/malidp: Split malidp_se_irq_init
  drm/arm/malidp: Enable/disable interrupts in runtime pm
  drm/arm/malidp: Enable/disable the scaling engine interrupts with
    memory writeback
  drm/arm/malidp: Set the output_depth register in modeset
  drm/arm/malidp: Added the late system pm functions

 drivers/gpu/drm/arm/malidp_drv.c | 33 ++++++++++++++++++++----
 drivers/gpu/drm/arm/malidp_hw.c  | 55 +++++++++++++++++++++++++++-------------
 drivers/gpu/drm/arm/malidp_hw.h  |  6 +++--
 3 files changed, 70 insertions(+), 24 deletions(-)

-- 
2.7.4

[PATCH 1/8] drm/arm/malidp: Modified the prototype of malidp_de_irq_fini

[Ayan Kumar Halder]

'struct drm_device' is being replaced with 'struct malidp_hw_device'
as the function argument. The reason being the dependency of
malidp_de_irq_fini on 'struct drm_device' needs to be removed so as to
enable it to call from functions which receives 'struct malidp_hw_device'
as argument. Furthermore, there is no way to retrieve 'struct drm_device'
from 'struct malidp_hw_device'.

Change-Id: I39c38cc4c0c9dd951777fbcb13e2ee3168ea0141
Signed-off-by: Ayan Kumar Halder <ayan.halder@arm.com>
---
 drivers/gpu/drm/arm/malidp_drv.c | 9 ++++++---
 drivers/gpu/drm/arm/malidp_hw.c  | 5 +----
 drivers/gpu/drm/arm/malidp_hw.h  | 2 +-
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
index 4b0c4b4..ed38ba9 100644
--- a/drivers/gpu/drm/arm/malidp_drv.c
+++ b/drivers/gpu/drm/arm/malidp_drv.c
@@ -295,6 +295,8 @@ static int malidp_irq_init(struct platform_device *pdev)
 {
 	int irq_de, irq_se, ret = 0;
 	struct drm_device *drm = dev_get_drvdata(&pdev->dev);
+	struct malidp_drm *malidp = drm->dev_private;
+	struct malidp_hw_device *hwdev = malidp->dev;
 
 	/* fetch the interrupts from DT */
 	irq_de = platform_get_irq_byname(pdev, "DE");
@@ -314,7 +316,7 @@ static int malidp_irq_init(struct platform_device *pdev)
 
 	ret = malidp_se_irq_init(drm, irq_se);
 	if (ret) {
-		malidp_de_irq_fini(drm);
+		malidp_de_irq_fini(hwdev);
 		return ret;
 	}
 
@@ -652,7 +654,7 @@ static int malidp_bind(struct device *dev)
 	pm_runtime_get_sync(dev);
 vblank_fail:
 	malidp_se_irq_fini(drm);
-	malidp_de_irq_fini(drm);
+	malidp_de_irq_fini(hwdev);
 	drm->irq_enabled = false;
 irq_init_fail:
 	component_unbind_all(dev, drm);
@@ -681,6 +683,7 @@ static void malidp_unbind(struct device *dev)
 {
 	struct drm_device *drm = dev_get_drvdata(dev);
 	struct malidp_drm *malidp = drm->dev_private;
+	struct malidp_hw_device *hwdev = malidp->dev;
 
 	drm_dev_unregister(drm);
 	drm_fb_cma_fbdev_fini(drm);
@@ -688,7 +691,7 @@ static void malidp_unbind(struct device *dev)
 	pm_runtime_get_sync(dev);
 	drm_crtc_vblank_off(&malidp->crtc);
 	malidp_se_irq_fini(drm);
-	malidp_de_irq_fini(drm);
+	malidp_de_irq_fini(hwdev);
 	drm->irq_enabled = false;
 	component_unbind_all(dev, drm);
 	of_node_put(malidp->crtc.port);
diff --git a/drivers/gpu/drm/arm/malidp_hw.c b/drivers/gpu/drm/arm/malidp_hw.c
index e4d9ebc..b13dfac 100644
--- a/drivers/gpu/drm/arm/malidp_hw.c
+++ b/drivers/gpu/drm/arm/malidp_hw.c
@@ -900,11 +900,8 @@ int malidp_de_irq_init(struct drm_device *drm, int irq)
 	return 0;
 }
 
-void malidp_de_irq_fini(struct drm_device *drm)
+void malidp_de_irq_fini(struct malidp_hw_device *hwdev)
 {
-	struct malidp_drm *malidp = drm->dev_private;
-	struct malidp_hw_device *hwdev = malidp->dev;
-
 	malidp_hw_disable_irq(hwdev, MALIDP_DE_BLOCK,
 			      hwdev->hw->map.de_irq_map.irq_mask);
 	malidp_hw_disable_irq(hwdev, MALIDP_DC_BLOCK,
diff --git a/drivers/gpu/drm/arm/malidp_hw.h b/drivers/gpu/drm/arm/malidp_hw.h
index a242e97..6e2a2f6 100644
--- a/drivers/gpu/drm/arm/malidp_hw.h
+++ b/drivers/gpu/drm/arm/malidp_hw.h
@@ -297,7 +297,7 @@ static inline void malidp_hw_enable_irq(struct malidp_hw_device *hwdev,
 }
 
 int malidp_de_irq_init(struct drm_device *drm, int irq);
-void malidp_de_irq_fini(struct drm_device *drm);
+void malidp_de_irq_fini(struct malidp_hw_device *hwdev);
 int malidp_se_irq_init(struct drm_device *drm, int irq);
 void malidp_se_irq_fini(struct drm_device *drm);
 
-- 
2.7.4

[PATCH 2/8] drm/arm/malidp: Modified the prototype of malidp_se_irq_fini

[Ayan Kumar Halder]

'struct drm_device' is being replaced with 'struct malidp_hw_device'
as the function argument.The reason being the dependency of
malidp_se_irq_fini on 'struct drm_device' needs to be removed so as to
enable it to call from functions which receives 'struct malidp_hw_device'
as argument. Furthermore, there is no way to retrieve 'struct drm_device'
from 'struct malidp_hw_device'

Change-Id: Iab7ac99917f0faf739aee97b00e1758ad1ae787b
Signed-off-by: Ayan Kumar Halder <ayan.halder@arm.com>
---
 drivers/gpu/drm/arm/malidp_drv.c | 4 ++--
 drivers/gpu/drm/arm/malidp_hw.c  | 5 +----
 drivers/gpu/drm/arm/malidp_hw.h  | 2 +-
 3 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
index ed38ba9..f7a8beb 100644
--- a/drivers/gpu/drm/arm/malidp_drv.c
+++ b/drivers/gpu/drm/arm/malidp_drv.c
@@ -653,7 +653,7 @@ static int malidp_bind(struct device *dev)
 fbdev_fail:
 	pm_runtime_get_sync(dev);
 vblank_fail:
-	malidp_se_irq_fini(drm);
+	malidp_se_irq_fini(hwdev);
 	malidp_de_irq_fini(hwdev);
 	drm->irq_enabled = false;
 irq_init_fail:
@@ -690,7 +690,7 @@ static void malidp_unbind(struct device *dev)
 	drm_kms_helper_poll_fini(drm);
 	pm_runtime_get_sync(dev);
 	drm_crtc_vblank_off(&malidp->crtc);
-	malidp_se_irq_fini(drm);
+	malidp_se_irq_fini(hwdev);
 	malidp_de_irq_fini(hwdev);
 	drm->irq_enabled = false;
 	component_unbind_all(dev, drm);
diff --git a/drivers/gpu/drm/arm/malidp_hw.c b/drivers/gpu/drm/arm/malidp_hw.c
index b13dfac..8fb02f3 100644
--- a/drivers/gpu/drm/arm/malidp_hw.c
+++ b/drivers/gpu/drm/arm/malidp_hw.c
@@ -970,11 +970,8 @@ int malidp_se_irq_init(struct drm_device *drm, int irq)
 	return 0;
 }
 
-void malidp_se_irq_fini(struct drm_device *drm)
+void malidp_se_irq_fini(struct malidp_hw_device *hwdev)
 {
-	struct malidp_drm *malidp = drm->dev_private;
-	struct malidp_hw_device *hwdev = malidp->dev;
-
 	malidp_hw_disable_irq(hwdev, MALIDP_SE_BLOCK,
 			      hwdev->hw->map.se_irq_map.irq_mask);
 }
diff --git a/drivers/gpu/drm/arm/malidp_hw.h b/drivers/gpu/drm/arm/malidp_hw.h
index 6e2a2f6..6607aba 100644
--- a/drivers/gpu/drm/arm/malidp_hw.h
+++ b/drivers/gpu/drm/arm/malidp_hw.h
@@ -299,7 +299,7 @@ static inline void malidp_hw_enable_irq(struct malidp_hw_device *hwdev,
 int malidp_de_irq_init(struct drm_device *drm, int irq);
 void malidp_de_irq_fini(struct malidp_hw_device *hwdev);
 int malidp_se_irq_init(struct drm_device *drm, int irq);
-void malidp_se_irq_fini(struct drm_device *drm);
+void malidp_se_irq_fini(struct malidp_hw_device *hwdev);
 
 u8 malidp_hw_get_format_id(const struct malidp_hw_regmap *map,
 			   u8 layer_id, u32 format);
-- 
2.7.4

[PATCH 3/8] drm/arm/malidp: Split malidp_de_irq_init

[Ayan Kumar Halder]

Extract the hardware initialisation part from malidp_de_irq_init() into the
malidp_de_irq_hw_init() which will be later invoked from runtime_pm_resume
function when it needs to re-enable the interrupts.

Change-Id: If8bdb0e246653cb7d7b7d6d63919c45b01350c10
Signed-off-by: Ayan Kumar Halder <ayan.halder@arm.com>
---
 drivers/gpu/drm/arm/malidp_hw.c | 25 ++++++++++++++++++-------
 drivers/gpu/drm/arm/malidp_hw.h |  1 +
 2 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/arm/malidp_hw.c b/drivers/gpu/drm/arm/malidp_hw.c
index 8fb02f3..3e73370 100644
--- a/drivers/gpu/drm/arm/malidp_hw.c
+++ b/drivers/gpu/drm/arm/malidp_hw.c
@@ -869,6 +869,23 @@ static irqreturn_t malidp_de_irq_thread_handler(int irq, void *arg)
 	return IRQ_HANDLED;
 }
 
+void malidp_de_irq_hw_init(struct malidp_hw_device *hwdev)
+{
+	/* ensure interrupts are disabled */
+	malidp_hw_disable_irq(hwdev, MALIDP_DE_BLOCK, 0xffffffff);
+	malidp_hw_clear_irq(hwdev, MALIDP_DE_BLOCK, 0xffffffff);
+	malidp_hw_disable_irq(hwdev, MALIDP_DC_BLOCK, 0xffffffff);
+	malidp_hw_clear_irq(hwdev, MALIDP_DC_BLOCK, 0xffffffff);
+
+	/* first enable the DC block IRQs */
+	malidp_hw_enable_irq(hwdev, MALIDP_DC_BLOCK,
+			     hwdev->hw->map.dc_irq_map.irq_mask);
+
+	/* now enable the DE block IRQs */
+	malidp_hw_enable_irq(hwdev, MALIDP_DE_BLOCK,
+			     hwdev->hw->map.de_irq_map.irq_mask);
+}
+
 int malidp_de_irq_init(struct drm_device *drm, int irq)
 {
 	struct malidp_drm *malidp = drm->dev_private;
@@ -889,13 +906,7 @@ int malidp_de_irq_init(struct drm_device *drm, int irq)
 		return ret;
 	}
 
-	/* first enable the DC block IRQs */
-	malidp_hw_enable_irq(hwdev, MALIDP_DC_BLOCK,
-			     hwdev->hw->map.dc_irq_map.irq_mask);
-
-	/* now enable the DE block IRQs */
-	malidp_hw_enable_irq(hwdev, MALIDP_DE_BLOCK,
-			     hwdev->hw->map.de_irq_map.irq_mask);
+	malidp_de_irq_hw_init(hwdev);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/arm/malidp_hw.h b/drivers/gpu/drm/arm/malidp_hw.h
index 6607aba..3b049d0 100644
--- a/drivers/gpu/drm/arm/malidp_hw.h
+++ b/drivers/gpu/drm/arm/malidp_hw.h
@@ -297,6 +297,7 @@ static inline void malidp_hw_enable_irq(struct malidp_hw_device *hwdev,
 }
 
 int malidp_de_irq_init(struct drm_device *drm, int irq);
+void malidp_de_irq_hw_init(struct malidp_hw_device *hwdev);
 void malidp_de_irq_fini(struct malidp_hw_device *hwdev);
 int malidp_se_irq_init(struct drm_device *drm, int irq);
 void malidp_se_irq_fini(struct malidp_hw_device *hwdev);
-- 
2.7.4

[PATCH 4/8] drm/arm/malidp: Split malidp_se_irq_init

[Ayan Kumar Halder]

Extract the hardware initialisation part from malidp_se_irq_init() into the
malidp_se_irq_hw_init() which will be later invoked from
malidpxxx_enable_memwrite() when it needs to re-enable the interrupts.

Signed-off-by: Ayan Kumar Halder <ayan.halder@arm.com>
Change-Id: Ibb26e86b38141993539307705695e3f6a9e32caa
---
 drivers/gpu/drm/arm/malidp_hw.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/arm/malidp_hw.c b/drivers/gpu/drm/arm/malidp_hw.c
index 3e73370..f5633bc 100644
--- a/drivers/gpu/drm/arm/malidp_hw.c
+++ b/drivers/gpu/drm/arm/malidp_hw.c
@@ -163,6 +163,7 @@ static const u16 dp500_se_scaling_coeffs[][SE_N_SCALING_COEFFS] = {
 };
 
 #define MALIDP_DE_DEFAULT_PREFETCH_START	5
+static void malidp_se_irq_hw_init(struct malidp_hw_device *hwdev);
 
 static int malidp500_query_hw(struct malidp_hw_device *hwdev)
 {
@@ -952,6 +953,16 @@ static irqreturn_t malidp_se_irq(int irq, void *arg)
 	return IRQ_HANDLED;
 }
 
+static void malidp_se_irq_hw_init(struct malidp_hw_device *hwdev)
+{
+	/* ensure interrupts are disabled */
+	malidp_hw_disable_irq(hwdev, MALIDP_SE_BLOCK, 0xffffffff);
+	malidp_hw_clear_irq(hwdev, MALIDP_SE_BLOCK, 0xffffffff);
+
+	malidp_hw_enable_irq(hwdev, MALIDP_SE_BLOCK,
+			     hwdev->hw->map.se_irq_map.irq_mask);
+}
+
 static irqreturn_t malidp_se_irq_thread_handler(int irq, void *arg)
 {
 	return IRQ_HANDLED;
@@ -975,8 +986,7 @@ int malidp_se_irq_init(struct drm_device *drm, int irq)
 		return ret;
 	}
 
-	malidp_hw_enable_irq(hwdev, MALIDP_SE_BLOCK,
-			     hwdev->hw->map.se_irq_map.irq_mask);
+	malidp_se_irq_hw_init(hwdev);
 
 	return 0;
 }
-- 
2.7.4

[PATCH 5/8] drm/arm/malidp: Enable/disable interrupts in runtime pm

[Ayan Kumar Halder]

Display engine and core interrupts need to be disabled when the
system invokes malidp_runtime_pm_suspend. Consequently, they
need to be enabled in malidp_runtime_pm_resume.

Signed-off-by: Ayan Kumar Halder <ayan.halder@arm.com>
Change-Id: Ib8e5e8319fdd768f8a97d9b5960fcfa8ba90eba3
---
 drivers/gpu/drm/arm/malidp_drv.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
index f7a8beb..e5a1fa0 100644
--- a/drivers/gpu/drm/arm/malidp_drv.c
+++ b/drivers/gpu/drm/arm/malidp_drv.c
@@ -470,6 +470,7 @@ static int malidp_runtime_pm_suspend(struct device *dev)
 	/* we can only suspend if the hardware is in config mode */
 	WARN_ON(!hwdev->hw->in_config_mode(hwdev));
 
+	malidp_de_irq_fini(hwdev);
 	hwdev->pm_suspended = true;
 	clk_disable_unprepare(hwdev->mclk);
 	clk_disable_unprepare(hwdev->aclk);
@@ -488,6 +489,7 @@ static int malidp_runtime_pm_resume(struct device *dev)
 	clk_prepare_enable(hwdev->aclk);
 	clk_prepare_enable(hwdev->mclk);
 	hwdev->pm_suspended = false;
+	malidp_de_irq_hw_init(hwdev);
 
 	return 0;
 }
-- 
2.7.4

[PATCH 6/8] drm/arm/malidp: Enable/disable the scaling engine interrupts with memory writeback

[Ayan Kumar Halder]

Scaling engine interrupts need to be enabled/disabled as and when memwrite
is enabled and disabled. The reason being scaling engine interrupts are
used only by the memory writeout layer.

This patch depends on:

"[Patch v5,1/3] drm: mali-dp: Add support for writeback on DP550/DP650"

Change-Id: Ic78aa5cd7b53998a1947067c4a15c19de239583b
Signed-off-by: Ayan Kumar Halder <ayan.halder@arm.com>
---
 drivers/gpu/drm/arm/malidp_hw.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/arm/malidp_hw.c b/drivers/gpu/drm/arm/malidp_hw.c
index f5633bc..90d76e4 100644
--- a/drivers/gpu/drm/arm/malidp_hw.c
+++ b/drivers/gpu/drm/arm/malidp_hw.c
@@ -621,12 +621,14 @@ static int malidp550_enable_memwrite(struct malidp_hw_device *hwdev,
 	malidp_hw_setbits(hwdev, MALIDP550_SE_MEMWRITE_ONESHOT | MALIDP_SE_MEMWRITE_EN,
 			  MALIDP550_SE_CONTROL);
 
+	malidp_se_irq_hw_init(hwdev);
 	return 0;
 }
 
 static void malidp550_disable_memwrite(struct malidp_hw_device *hwdev)
 {
 	u32 base = malidp_get_block_base(hwdev, MALIDP_DE_BLOCK);
+	malidp_se_irq_fini(hwdev);
 	malidp_hw_clearbits(hwdev, MALIDP550_SE_MEMWRITE_ONESHOT | MALIDP_SE_MEMWRITE_EN,
 			    MALIDP550_SE_CONTROL);
 	malidp_hw_clearbits(hwdev, MALIDP_SCALE_ENGINE_EN, base + MALIDP_DE_DISPLAY_FUNC);
-- 
2.7.4

[PATCH 7/8] drm/arm/malidp: Set the output_depth register in modeset

[Ayan Kumar Halder]

One needs to store the value of the OUTPUT_DEPTH that one has parsed from
device tree, so that it can be restored on system resume. This value is
set in the modeset function as this gets reset when the system suspends.

Signed-off-by: Ayan Kumar Halder <ayan.halder@arm.com>
Change-Id: I361b1214cd4e5005d21eef3ca6bf39ca90be2506
---
 drivers/gpu/drm/arm/malidp_drv.c | 1 +
 drivers/gpu/drm/arm/malidp_hw.c  | 4 ++++
 drivers/gpu/drm/arm/malidp_hw.h  | 1 +
 3 files changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
index e5a1fa0..bd44a6d 100644
--- a/drivers/gpu/drm/arm/malidp_drv.c
+++ b/drivers/gpu/drm/arm/malidp_drv.c
@@ -601,6 +601,7 @@ static int malidp_bind(struct device *dev)
 	for (i = 0; i < MAX_OUTPUT_CHANNELS; i++)
 		out_depth = (out_depth << 8) | (output_width[i] & 0xf);
 	malidp_hw_write(hwdev, out_depth, hwdev->hw->map.out_depth_base);
+	hwdev->output_color_depth = out_depth;
 
 	atomic_set(&malidp->config_valid, 0);
 	init_waitqueue_head(&malidp->wq);
diff --git a/drivers/gpu/drm/arm/malidp_hw.c b/drivers/gpu/drm/arm/malidp_hw.c
index 90d76e4..1bf10fb 100644
--- a/drivers/gpu/drm/arm/malidp_hw.c
+++ b/drivers/gpu/drm/arm/malidp_hw.c
@@ -234,6 +234,8 @@ static void malidp500_modeset(struct malidp_hw_device *hwdev, struct videomode *
 {
 	u32 val = 0;
 
+	malidp_hw_write(hwdev, hwdev->output_color_depth,
+		hwdev->hw->map.out_depth_base);
 	malidp_hw_clearbits(hwdev, MALIDP500_DC_CLEAR_MASK, MALIDP500_DC_CONTROL);
 	if (mode->flags & DISPLAY_FLAGS_HSYNC_HIGH)
 		val |= MALIDP500_HSYNCPOL;
@@ -458,6 +460,8 @@ static void malidp550_modeset(struct malidp_hw_device *hwdev, struct videomode *
 {
 	u32 val = MALIDP_DE_DEFAULT_PREFETCH_START;
 
+	malidp_hw_write(hwdev, hwdev->output_color_depth,
+		hwdev->hw->map.out_depth_base);
 	malidp_hw_write(hwdev, val, MALIDP550_DE_CONTROL);
 	/*
 	 * Mali-DP550 and Mali-DP650 encode the background color like this:
diff --git a/drivers/gpu/drm/arm/malidp_hw.h b/drivers/gpu/drm/arm/malidp_hw.h
index 3b049d0..844732d 100644
--- a/drivers/gpu/drm/arm/malidp_hw.h
+++ b/drivers/gpu/drm/arm/malidp_hw.h
@@ -228,6 +228,7 @@ struct malidp_hw_device {
 
 	u8 min_line_size;
 	u16 max_line_size;
+	u32 output_color_depth;
 
 	/* track the device PM state */
 	bool pm_suspended;
-- 
2.7.4

[PATCH 8/8] drm/arm/malidp: Added the late system pm functions

[Ayan Kumar Halder]

malidp_pm_suspend_late checks if the runtime status is not suspended
and if so, invokes malidp_runtime_pm_suspend which disables the
display engine/core interrupts and the clocks. It sets the runtime status
as suspended. Subsequently, malidp_pm_resume_early will invoke
malidp_runtime_pm_resume which enables the clocks and the interrupts
(previously disabled) and sets the runtime status as active.

Signed-off-by: Ayan Kumar Halder <ayan.halder@arm.com>
Change-Id: I5f8c3d28f076314a1c9da2a46760a9c37039ccda
---
 drivers/gpu/drm/arm/malidp_drv.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
index bd44a6d..f6124d8 100644
--- a/drivers/gpu/drm/arm/malidp_drv.c
+++ b/drivers/gpu/drm/arm/malidp_drv.c
@@ -766,8 +766,25 @@ static int __maybe_unused malidp_pm_resume(struct device *dev)
 	return 0;
 }
 
+static int __maybe_unused malidp_pm_suspend_late(struct device *dev)
+{
+	if (!pm_runtime_status_suspended(dev)) {
+		malidp_runtime_pm_suspend(dev);
+		pm_runtime_set_suspended(dev);
+	}
+	return 0;
+}
+
+static int __maybe_unused malidp_pm_resume_early(struct device *dev)
+{
+	malidp_runtime_pm_resume(dev);
+	pm_runtime_set_active(dev);
+	return 0;
+}
+
 static const struct dev_pm_ops malidp_pm_ops = {
 	SET_SYSTEM_SLEEP_PM_OPS(malidp_pm_suspend, malidp_pm_resume) \
+	SET_LATE_SYSTEM_SLEEP_PM_OPS(malidp_pm_suspend_late, malidp_pm_resume_early) \
 	SET_RUNTIME_PM_OPS(malidp_runtime_pm_suspend, malidp_runtime_pm_resume, NULL)
 };
 
-- 
2.7.4

Re: [PATCH 8/8] drm/arm/malidp: Added the late system pm functions

[Daniel Vetter]

On Mon, Mar 26, 2018 at 06:03:20PM +0100, Ayan Kumar Halder wrote:
> malidp_pm_suspend_late checks if the runtime status is not suspended
> and if so, invokes malidp_runtime_pm_suspend which disables the
> display engine/core interrupts and the clocks. It sets the runtime status
> as suspended. Subsequently, malidp_pm_resume_early will invoke
> malidp_runtime_pm_resume which enables the clocks and the interrupts
> (previously disabled) and sets the runtime status as active.
> 
> Signed-off-by: Ayan Kumar Halder <ayan.halder@arm.com>
> Change-Id: I5f8c3d28f076314a1c9da2a46760a9c37039ccda

Why exactly do you need late/early hooks? If you have dependencies with
other devices, pls consider adding device_links instead. This here
shouldn't be necessary.
-Daniel
> ---
>  drivers/gpu/drm/arm/malidp_drv.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
> index bd44a6d..f6124d8 100644
> --- a/drivers/gpu/drm/arm/malidp_drv.c
> +++ b/drivers/gpu/drm/arm/malidp_drv.c
> @@ -766,8 +766,25 @@ static int __maybe_unused malidp_pm_resume(struct device *dev)
>  	return 0;
>  }
>  
> +static int __maybe_unused malidp_pm_suspend_late(struct device *dev)
> +{
> +	if (!pm_runtime_status_suspended(dev)) {
> +		malidp_runtime_pm_suspend(dev);
> +		pm_runtime_set_suspended(dev);
> +	}
> +	return 0;
> +}
> +
> +static int __maybe_unused malidp_pm_resume_early(struct device *dev)
> +{
> +	malidp_runtime_pm_resume(dev);
> +	pm_runtime_set_active(dev);
> +	return 0;
> +}
> +
>  static const struct dev_pm_ops malidp_pm_ops = {
>  	SET_SYSTEM_SLEEP_PM_OPS(malidp_pm_suspend, malidp_pm_resume) \
> +	SET_LATE_SYSTEM_SLEEP_PM_OPS(malidp_pm_suspend_late, malidp_pm_resume_early) \
>  	SET_RUNTIME_PM_OPS(malidp_runtime_pm_suspend, malidp_runtime_pm_resume, NULL)
>  };
>  
> -- 
> 2.7.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 8/8] drm/arm/malidp: Added the late system pm functions

[Ayan Halder]

On Tue, Mar 27, 2018 at 10:29:03AM +0200, Daniel Vetter wrote:
> On Mon, Mar 26, 2018 at 06:03:20PM +0100, Ayan Kumar Halder wrote:
> > malidp_pm_suspend_late checks if the runtime status is not suspended
> > and if so, invokes malidp_runtime_pm_suspend which disables the
> > display engine/core interrupts and the clocks. It sets the runtime status
> > as suspended. Subsequently, malidp_pm_resume_early will invoke
> > malidp_runtime_pm_resume which enables the clocks and the interrupts
> > (previously disabled) and sets the runtime status as active.
> >
> > Signed-off-by: Ayan Kumar Halder <ayan.halder@arm.com>
> > Change-Id: I5f8c3d28f076314a1c9da2a46760a9c37039ccda
>
> Why exactly do you need late/early hooks? If you have dependencies with
> other devices, pls consider adding device_links instead. This here
> shouldn't be necessary.
> -Daniel
We need to late/early hooks to disable malidp interrupts and the
clocks.
> > ---
> >  drivers/gpu/drm/arm/malidp_drv.c | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
> > index bd44a6d..f6124d8 100644
> > --- a/drivers/gpu/drm/arm/malidp_drv.c
> > +++ b/drivers/gpu/drm/arm/malidp_drv.c
> > @@ -766,8 +766,25 @@ static int __maybe_unused malidp_pm_resume(struct device *dev)
> >     return 0;
> >  }
> >
> > +static int __maybe_unused malidp_pm_suspend_late(struct device *dev)
> > +{
> > +   if (!pm_runtime_status_suspended(dev)) {
> > +           malidp_runtime_pm_suspend(dev);
> > +           pm_runtime_set_suspended(dev);
> > +   }
> > +   return 0;
> > +}
> > +
> > +static int __maybe_unused malidp_pm_resume_early(struct device *dev)
> > +{
> > +   malidp_runtime_pm_resume(dev);
> > +   pm_runtime_set_active(dev);
> > +   return 0;
> > +}
> > +
> >  static const struct dev_pm_ops malidp_pm_ops = {
> >     SET_SYSTEM_SLEEP_PM_OPS(malidp_pm_suspend, malidp_pm_resume) \
> > +   SET_LATE_SYSTEM_SLEEP_PM_OPS(malidp_pm_suspend_late, malidp_pm_resume_early) \
> >     SET_RUNTIME_PM_OPS(malidp_runtime_pm_suspend, malidp_runtime_pm_resume, NULL)
> >  };
> >
> > --
> > 2.7.4
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Re: [PATCH 8/8] drm/arm/malidp: Added the late system pm functions

[Daniel Vetter]

On Tue, Mar 27, 2018 at 11:59 AM, Ayan Halder <ayan.halder@arm.com> wrote:
> On Tue, Mar 27, 2018 at 10:29:03AM +0200, Daniel Vetter wrote:
>> On Mon, Mar 26, 2018 at 06:03:20PM +0100, Ayan Kumar Halder wrote:
>> > malidp_pm_suspend_late checks if the runtime status is not suspended
>> > and if so, invokes malidp_runtime_pm_suspend which disables the
>> > display engine/core interrupts and the clocks. It sets the runtime status
>> > as suspended. Subsequently, malidp_pm_resume_early will invoke
>> > malidp_runtime_pm_resume which enables the clocks and the interrupts
>> > (previously disabled) and sets the runtime status as active.
>> >
>> > Signed-off-by: Ayan Kumar Halder <ayan.halder@arm.com>
>> > Change-Id: I5f8c3d28f076314a1c9da2a46760a9c37039ccda
>>
>> Why exactly do you need late/early hooks? If you have dependencies with
>> other devices, pls consider adding device_links instead. This here
>> shouldn't be necessary.
>> -Daniel
> We need to late/early hooks to disable malidp interrupts and the
> clocks.

Yes, but why this ordering constraint? Why can't you just disable the
interrupts/clocks in the normal suspend code. I see that the patch
does this, I want to understand why it does it.
-Daniel

>> > ---
>> >  drivers/gpu/drm/arm/malidp_drv.c | 17 +++++++++++++++++
>> >  1 file changed, 17 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
>> > index bd44a6d..f6124d8 100644
>> > --- a/drivers/gpu/drm/arm/malidp_drv.c
>> > +++ b/drivers/gpu/drm/arm/malidp_drv.c
>> > @@ -766,8 +766,25 @@ static int __maybe_unused malidp_pm_resume(struct device *dev)
>> >     return 0;
>> >  }
>> >
>> > +static int __maybe_unused malidp_pm_suspend_late(struct device *dev)
>> > +{
>> > +   if (!pm_runtime_status_suspended(dev)) {
>> > +           malidp_runtime_pm_suspend(dev);
>> > +           pm_runtime_set_suspended(dev);
>> > +   }
>> > +   return 0;
>> > +}
>> > +
>> > +static int __maybe_unused malidp_pm_resume_early(struct device *dev)
>> > +{
>> > +   malidp_runtime_pm_resume(dev);
>> > +   pm_runtime_set_active(dev);
>> > +   return 0;
>> > +}
>> > +
>> >  static const struct dev_pm_ops malidp_pm_ops = {
>> >     SET_SYSTEM_SLEEP_PM_OPS(malidp_pm_suspend, malidp_pm_resume) \
>> > +   SET_LATE_SYSTEM_SLEEP_PM_OPS(malidp_pm_suspend_late, malidp_pm_resume_early) \
>> >     SET_RUNTIME_PM_OPS(malidp_runtime_pm_suspend, malidp_runtime_pm_resume, NULL)
>> >  };
>> >
>> > --
>> > 2.7.4
>> >
>> > _______________________________________________
>> > dri-devel mailing list
>> > dri-devel@lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>
>> --
>> Daniel Vetter
>> Software Engineer, Intel Corporation
>> http://blog.ffwll.ch
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 8/8] drm/arm/malidp: Added the late system pm functions

[Ayan Halder]

On Tue, Mar 27, 2018 at 01:09:36PM +0200, Daniel Vetter wrote:
> On Tue, Mar 27, 2018 at 11:59 AM, Ayan Halder <ayan.halder@arm.com> wrote:
> > On Tue, Mar 27, 2018 at 10:29:03AM +0200, Daniel Vetter wrote:
> >> On Mon, Mar 26, 2018 at 06:03:20PM +0100, Ayan Kumar Halder wrote:
> >> > malidp_pm_suspend_late checks if the runtime status is not suspended
> >> > and if so, invokes malidp_runtime_pm_suspend which disables the
> >> > display engine/core interrupts and the clocks. It sets the runtime status
> >> > as suspended. Subsequently, malidp_pm_resume_early will invoke
> >> > malidp_runtime_pm_resume which enables the clocks and the interrupts
> >> > (previously disabled) and sets the runtime status as active.
> >> >
> >> > Signed-off-by: Ayan Kumar Halder <ayan.halder@arm.com>
> >> > Change-Id: I5f8c3d28f076314a1c9da2a46760a9c37039ccda
> >>
> >> Why exactly do you need late/early hooks? If you have dependencies with
> >> other devices, pls consider adding device_links instead. This here
> >> shouldn't be necessary.
> >> -Daniel
> > We need to late/early hooks to disable malidp interrupts and the
> > clocks.
> 
> Yes, but why this ordering constraint? Why can't you just disable the
> interrupts/clocks in the normal suspend code. I see that the patch
> does this, I want to understand why it does it.
> -Daniel
Apologies for my delayed response on this. 

With reference to https://lwn.net/Articles/505683/ :-
1. "suspend() should leave the device in a quiescent state." We invoke
drm_mode_config_helper_suspend() which deactivates the crtc. I
understand that this is the quiescent state.

2. "suspend_late() can often be the same as runtime_suspend()."  We
invoke runtime suspend/resume calls in late/early hooks.

> >> > ---
> >> >  drivers/gpu/drm/arm/malidp_drv.c | 17 +++++++++++++++++
> >> >  1 file changed, 17 insertions(+)
> >> >
> >> > diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
> >> > index bd44a6d..f6124d8 100644
> >> > --- a/drivers/gpu/drm/arm/malidp_drv.c
> >> > +++ b/drivers/gpu/drm/arm/malidp_drv.c
> >> > @@ -766,8 +766,25 @@ static int __maybe_unused malidp_pm_resume(struct device *dev)
> >> >     return 0;
> >> >  }
> >> >
> >> > +static int __maybe_unused malidp_pm_suspend_late(struct device *dev)
> >> > +{
> >> > +   if (!pm_runtime_status_suspended(dev)) {
> >> > +           malidp_runtime_pm_suspend(dev);
> >> > +           pm_runtime_set_suspended(dev);
> >> > +   }
> >> > +   return 0;
> >> > +}
> >> > +
> >> > +static int __maybe_unused malidp_pm_resume_early(struct device *dev)
> >> > +{
> >> > +   malidp_runtime_pm_resume(dev);
> >> > +   pm_runtime_set_active(dev);
> >> > +   return 0;
> >> > +}
> >> > +
> >> >  static const struct dev_pm_ops malidp_pm_ops = {
> >> >     SET_SYSTEM_SLEEP_PM_OPS(malidp_pm_suspend, malidp_pm_resume) \
> >> > +   SET_LATE_SYSTEM_SLEEP_PM_OPS(malidp_pm_suspend_late, malidp_pm_resume_early) \
> >> >     SET_RUNTIME_PM_OPS(malidp_runtime_pm_suspend, malidp_runtime_pm_resume, NULL)
> >> >  };
> >> >
> >> > --
> >> > 2.7.4
> >> >
> >> > _______________________________________________
> >> > dri-devel mailing list
> >> > dri-devel@lists.freedesktop.org
> >> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >>
> >> --
> >> Daniel Vetter
> >> Software Engineer, Intel Corporation
> >> http://blog.ffwll.ch
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 8/8] drm/arm/malidp: Added the late system pm functions

[Daniel Vetter]

On Fri, Apr 06, 2018 at 08:02:16PM +0100, Ayan Halder wrote:
> On Tue, Mar 27, 2018 at 01:09:36PM +0200, Daniel Vetter wrote:
> > On Tue, Mar 27, 2018 at 11:59 AM, Ayan Halder <ayan.halder@arm.com> wrote:
> > > On Tue, Mar 27, 2018 at 10:29:03AM +0200, Daniel Vetter wrote:
> > >> On Mon, Mar 26, 2018 at 06:03:20PM +0100, Ayan Kumar Halder wrote:
> > >> > malidp_pm_suspend_late checks if the runtime status is not suspended
> > >> > and if so, invokes malidp_runtime_pm_suspend which disables the
> > >> > display engine/core interrupts and the clocks. It sets the runtime status
> > >> > as suspended. Subsequently, malidp_pm_resume_early will invoke
> > >> > malidp_runtime_pm_resume which enables the clocks and the interrupts
> > >> > (previously disabled) and sets the runtime status as active.
> > >> >
> > >> > Signed-off-by: Ayan Kumar Halder <ayan.halder@arm.com>
> > >> > Change-Id: I5f8c3d28f076314a1c9da2a46760a9c37039ccda
> > >>
> > >> Why exactly do you need late/early hooks? If you have dependencies with
> > >> other devices, pls consider adding device_links instead. This here
> > >> shouldn't be necessary.
> > >> -Daniel
> > > We need to late/early hooks to disable malidp interrupts and the
> > > clocks.
> > 
> > Yes, but why this ordering constraint? Why can't you just disable the
> > interrupts/clocks in the normal suspend code. I see that the patch
> > does this, I want to understand why it does it.
> > -Daniel
> Apologies for my delayed response on this. 
> 
> With reference to https://lwn.net/Articles/505683/ :-
> 1. "suspend() should leave the device in a quiescent state." We invoke
> drm_mode_config_helper_suspend() which deactivates the crtc. I
> understand that this is the quiescent state.
> 
> 2. "suspend_late() can often be the same as runtime_suspend()."  We
> invoke runtime suspend/resume calls in late/early hooks.

This article is from 2012. That's not really recommended best practices
anymore. device_links have only been added a few years ago, so ofc an
article from 2012 can't tell you that you should use those instead :-)

That's why I brought this up, we have much better ways to handle device
dependencies now.

Also, you still haven't explained what exactly the dependency is.
-Daniel

> 
> > >> > ---
> > >> >  drivers/gpu/drm/arm/malidp_drv.c | 17 +++++++++++++++++
> > >> >  1 file changed, 17 insertions(+)
> > >> >
> > >> > diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
> > >> > index bd44a6d..f6124d8 100644
> > >> > --- a/drivers/gpu/drm/arm/malidp_drv.c
> > >> > +++ b/drivers/gpu/drm/arm/malidp_drv.c
> > >> > @@ -766,8 +766,25 @@ static int __maybe_unused malidp_pm_resume(struct device *dev)
> > >> >     return 0;
> > >> >  }
> > >> >
> > >> > +static int __maybe_unused malidp_pm_suspend_late(struct device *dev)
> > >> > +{
> > >> > +   if (!pm_runtime_status_suspended(dev)) {
> > >> > +           malidp_runtime_pm_suspend(dev);
> > >> > +           pm_runtime_set_suspended(dev);
> > >> > +   }
> > >> > +   return 0;
> > >> > +}
> > >> > +
> > >> > +static int __maybe_unused malidp_pm_resume_early(struct device *dev)
> > >> > +{
> > >> > +   malidp_runtime_pm_resume(dev);
> > >> > +   pm_runtime_set_active(dev);
> > >> > +   return 0;
> > >> > +}
> > >> > +
> > >> >  static const struct dev_pm_ops malidp_pm_ops = {
> > >> >     SET_SYSTEM_SLEEP_PM_OPS(malidp_pm_suspend, malidp_pm_resume) \
> > >> > +   SET_LATE_SYSTEM_SLEEP_PM_OPS(malidp_pm_suspend_late, malidp_pm_resume_early) \
> > >> >     SET_RUNTIME_PM_OPS(malidp_runtime_pm_suspend, malidp_runtime_pm_resume, NULL)
> > >> >  };
> > >> >
> > >> > --
> > >> > 2.7.4
> > >> >
> > >> > _______________________________________________
> > >> > dri-devel mailing list
> > >> > dri-devel@lists.freedesktop.org
> > >> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > >>
> > >> --
> > >> Daniel Vetter
> > >> Software Engineer, Intel Corporation
> > >> http://blog.ffwll.ch
> > >> _______________________________________________
> > >> dri-devel mailing list
> > >> dri-devel@lists.freedesktop.org
> > >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > 
> > 
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 8/8] drm/arm/malidp: Added the late system pm functions

[Brian Starkey]

Hi Daniel,

On Mon, Apr 09, 2018 at 10:23:37AM +0200, Daniel Vetter wrote:
>On Fri, Apr 06, 2018 at 08:02:16PM +0100, Ayan Halder wrote:
>> On Tue, Mar 27, 2018 at 01:09:36PM +0200, Daniel Vetter wrote:
>> > On Tue, Mar 27, 2018 at 11:59 AM, Ayan Halder <ayan.halder@arm.com> wrote:
>> > > On Tue, Mar 27, 2018 at 10:29:03AM +0200, Daniel Vetter wrote:
>> > >> On Mon, Mar 26, 2018 at 06:03:20PM +0100, Ayan Kumar Halder wrote:
>> > >> > malidp_pm_suspend_late checks if the runtime status is not suspended
>> > >> > and if so, invokes malidp_runtime_pm_suspend which disables the
>> > >> > display engine/core interrupts and the clocks. It sets the runtime status
>> > >> > as suspended. Subsequently, malidp_pm_resume_early will invoke
>> > >> > malidp_runtime_pm_resume which enables the clocks and the interrupts
>> > >> > (previously disabled) and sets the runtime status as active.
>> > >> >
>> > >> > Signed-off-by: Ayan Kumar Halder <ayan.halder@arm.com>
>> > >> > Change-Id: I5f8c3d28f076314a1c9da2a46760a9c37039ccda
>> > >>
>> > >> Why exactly do you need late/early hooks? If you have dependencies with
>> > >> other devices, pls consider adding device_links instead. This here
>> > >> shouldn't be necessary.
>> > >> -Daniel
>> > > We need to late/early hooks to disable malidp interrupts and the
>> > > clocks.
>> >
>> > Yes, but why this ordering constraint? Why can't you just disable the
>> > interrupts/clocks in the normal suspend code. I see that the patch
>> > does this, I want to understand why it does it.
>> > -Daniel
>> Apologies for my delayed response on this.
>>
>> With reference to https://lwn.net/Articles/505683/ :-
>> 1. "suspend() should leave the device in a quiescent state." We invoke
>> drm_mode_config_helper_suspend() which deactivates the crtc. I
>> understand that this is the quiescent state.
>>
>> 2. "suspend_late() can often be the same as runtime_suspend()."  We
>> invoke runtime suspend/resume calls in late/early hooks.
>
>This article is from 2012. That's not really recommended best practices
>anymore. device_links have only been added a few years ago, so ofc an
>article from 2012 can't tell you that you should use those instead :-)
>
>That's why I brought this up, we have much better ways to handle device
>dependencies now.
>

We aren't trying to manage any device dependencies here, I don't know
where that idea came from?

The kernel-doc on drm-next this afternoon says effectively the same
thing:

   * @suspend: Executed before putting the system into a sleep state in which the
   *      contents of main memory are preserved.  The exact action to perform
   *      depends on the device's subsystem (PM domain, device type, class or bus
   *      type), but generally the device must be quiescent after subsystem-level
   *      @suspend() has returned, so that it doesn't do any I/O or DMA.
   *      Subsystem-level @suspend() is executed for all devices after invoking
   *      subsystem-level @prepare() for all of them.

(i.e. suspend() makes the device quiescent).

   * @suspend_late: Continue operations started by @suspend().  For a number of
   *      devices @suspend_late() may point to the same callback routine as the
   *      runtime suspend callback.

(suggests suspend_late() be assigned to the same function as runtime
suspend).

As for why, my understanding is like so:

For ->suspend(), we use the DRM helper, which disables the CRTC.
Normally disabling the CRTC would be enough to also invoke our
pm_runtime callback to do the final clock disable etc., however when a
system suspend is in-progress, the core forcibly takes a runtime
reference on all devices - preventing any pm_runtime paths from
running.

This means that after the CRTC is disabled in ->suspend(), our normal
pm_runtime path will not be invoked, and so the things done in
malidp_runtime_pm_suspend() will never happen.

We were just following the advice in the kernel-doc to deal with this.

The alternative would be to call malidp_runtime_pm_{suspend,resume}
from the "not late" hooks, but I'd ask why?

>Also, you still haven't explained what exactly the dependency is.

Because there isn't one :-)

Thanks,
-Brian

>-Daniel
>
>>
>> > >> > ---
>> > >> >  drivers/gpu/drm/arm/malidp_drv.c | 17 +++++++++++++++++
>> > >> >  1 file changed, 17 insertions(+)
>> > >> >
>> > >> > diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
>> > >> > index bd44a6d..f6124d8 100644
>> > >> > --- a/drivers/gpu/drm/arm/malidp_drv.c
>> > >> > +++ b/drivers/gpu/drm/arm/malidp_drv.c
>> > >> > @@ -766,8 +766,25 @@ static int __maybe_unused malidp_pm_resume(struct device *dev)
>> > >> >     return 0;
>> > >> >  }
>> > >> >
>> > >> > +static int __maybe_unused malidp_pm_suspend_late(struct device *dev)
>> > >> > +{
>> > >> > +   if (!pm_runtime_status_suspended(dev)) {
>> > >> > +           malidp_runtime_pm_suspend(dev);
>> > >> > +           pm_runtime_set_suspended(dev);
>> > >> > +   }
>> > >> > +   return 0;
>> > >> > +}
>> > >> > +
>> > >> > +static int __maybe_unused malidp_pm_resume_early(struct device *dev)
>> > >> > +{
>> > >> > +   malidp_runtime_pm_resume(dev);
>> > >> > +   pm_runtime_set_active(dev);
>> > >> > +   return 0;
>> > >> > +}
>> > >> > +
>> > >> >  static const struct dev_pm_ops malidp_pm_ops = {
>> > >> >     SET_SYSTEM_SLEEP_PM_OPS(malidp_pm_suspend, malidp_pm_resume) \
>> > >> > +   SET_LATE_SYSTEM_SLEEP_PM_OPS(malidp_pm_suspend_late, malidp_pm_resume_early) \
>> > >> >     SET_RUNTIME_PM_OPS(malidp_runtime_pm_suspend, malidp_runtime_pm_resume, NULL)
>> > >> >  };
>> > >> >
>> > >> > --
>> > >> > 2.7.4
>> > >> >
>> > >> > _______________________________________________
>> > >> > dri-devel mailing list
>> > >> > dri-devel@lists.freedesktop.org
>> > >> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>> > >>
>> > >> --
>> > >> Daniel Vetter
>> > >> Software Engineer, Intel Corporation
>> > >> http://blog.ffwll.ch
>> > >> _______________________________________________
>> > >> dri-devel mailing list
>> > >> dri-devel@lists.freedesktop.org
>> > >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>> > > IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
>> > > _______________________________________________
>> > > dri-devel mailing list
>> > > dri-devel@lists.freedesktop.org
>> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>> >
>> >
>> >
>> > --
>> > Daniel Vetter
>> > Software Engineer, Intel Corporation
>> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
>
>-- 
>Daniel Vetter
>Software Engineer, Intel Corporation
>http://blog.ffwll.ch

Re: [PATCH 8/8] drm/arm/malidp: Added the late system pm functions

[Daniel Vetter]

On Mon, Apr 09, 2018 at 05:15:08PM +0100, Brian Starkey wrote:
> Hi Daniel,
> 
> On Mon, Apr 09, 2018 at 10:23:37AM +0200, Daniel Vetter wrote:
> > On Fri, Apr 06, 2018 at 08:02:16PM +0100, Ayan Halder wrote:
> > > On Tue, Mar 27, 2018 at 01:09:36PM +0200, Daniel Vetter wrote:
> > > > On Tue, Mar 27, 2018 at 11:59 AM, Ayan Halder <ayan.halder@arm.com> wrote:
> > > > > On Tue, Mar 27, 2018 at 10:29:03AM +0200, Daniel Vetter wrote:
> > > > >> On Mon, Mar 26, 2018 at 06:03:20PM +0100, Ayan Kumar Halder wrote:
> > > > >> > malidp_pm_suspend_late checks if the runtime status is not suspended
> > > > >> > and if so, invokes malidp_runtime_pm_suspend which disables the
> > > > >> > display engine/core interrupts and the clocks. It sets the runtime status
> > > > >> > as suspended. Subsequently, malidp_pm_resume_early will invoke
> > > > >> > malidp_runtime_pm_resume which enables the clocks and the interrupts
> > > > >> > (previously disabled) and sets the runtime status as active.
> > > > >> >
> > > > >> > Signed-off-by: Ayan Kumar Halder <ayan.halder@arm.com>
> > > > >> > Change-Id: I5f8c3d28f076314a1c9da2a46760a9c37039ccda
> > > > >>
> > > > >> Why exactly do you need late/early hooks? If you have dependencies with
> > > > >> other devices, pls consider adding device_links instead. This here
> > > > >> shouldn't be necessary.
> > > > >> -Daniel
> > > > > We need to late/early hooks to disable malidp interrupts and the
> > > > > clocks.
> > > >
> > > > Yes, but why this ordering constraint? Why can't you just disable the
> > > > interrupts/clocks in the normal suspend code. I see that the patch
> > > > does this, I want to understand why it does it.
> > > > -Daniel
> > > Apologies for my delayed response on this.
> > > 
> > > With reference to https://lwn.net/Articles/505683/ :-
> > > 1. "suspend() should leave the device in a quiescent state." We invoke
> > > drm_mode_config_helper_suspend() which deactivates the crtc. I
> > > understand that this is the quiescent state.
> > > 
> > > 2. "suspend_late() can often be the same as runtime_suspend()."  We
> > > invoke runtime suspend/resume calls in late/early hooks.
> > 
> > This article is from 2012. That's not really recommended best practices
> > anymore. device_links have only been added a few years ago, so ofc an
> > article from 2012 can't tell you that you should use those instead :-)
> > 
> > That's why I brought this up, we have much better ways to handle device
> > dependencies now.
> > 
> 
> We aren't trying to manage any device dependencies here, I don't know
> where that idea came from?
> 
> The kernel-doc on drm-next this afternoon says effectively the same
> thing:
> 
>   * @suspend: Executed before putting the system into a sleep state in which the
>   *      contents of main memory are preserved.  The exact action to perform
>   *      depends on the device's subsystem (PM domain, device type, class or bus
>   *      type), but generally the device must be quiescent after subsystem-level
>   *      @suspend() has returned, so that it doesn't do any I/O or DMA.
>   *      Subsystem-level @suspend() is executed for all devices after invoking
>   *      subsystem-level @prepare() for all of them.
> 
> (i.e. suspend() makes the device quiescent).
> 
>   * @suspend_late: Continue operations started by @suspend().  For a number of
>   *      devices @suspend_late() may point to the same callback routine as the
>   *      runtime suspend callback.
> 
> (suggests suspend_late() be assigned to the same function as runtime
> suspend).
> 
> As for why, my understanding is like so:
> 
> For ->suspend(), we use the DRM helper, which disables the CRTC.
> Normally disabling the CRTC would be enough to also invoke our
> pm_runtime callback to do the final clock disable etc., however when a
> system suspend is in-progress, the core forcibly takes a runtime
> reference on all devices - preventing any pm_runtime paths from
> running.

I thought this was fixed. At least I remember we had to add some special
calls to i915 to opt out of the "do runtime pm as part of suspend/resume"
behaviour, since it doesn't match what we needed.

See

commit aae4518b3124b29f8dc81c829c704fd2df72e98b
Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Date:   Fri May 16 02:46:50 2014 +0200

    PM / sleep: Mechanism to avoid resuming runtime-suspended devices unnecessarily

So I thought this stuff was supposed to work now. Adding Rafael Wyzocki,
he's done plenty presentations recently about exactly this.

> This means that after the CRTC is disabled in ->suspend(), our normal
> pm_runtime path will not be invoked, and so the things done in
> malidp_runtime_pm_suspend() will never happen.
> 
> We were just following the advice in the kernel-doc to deal with this.
> 
> The alternative would be to call malidp_runtime_pm_{suspend,resume}
> from the "not late" hooks, but I'd ask why?
> 
> > Also, you still haven't explained what exactly the dependency is.
> 
> Because there isn't one :-)

Hm, if there really isn't one, then I guess it's ok. But it's way too easy
to screw this up, have an accidental depency on a different device. And
then you try to fix this up. Having a suspend_late hook still smells fishy
to me, but I might be out of the loop.
-Daniel



> 
> Thanks,
> -Brian
> 
> > -Daniel
> > 
> > > 
> > > > >> > ---
> > > > >> >  drivers/gpu/drm/arm/malidp_drv.c | 17 +++++++++++++++++
> > > > >> >  1 file changed, 17 insertions(+)
> > > > >> >
> > > > >> > diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
> > > > >> > index bd44a6d..f6124d8 100644
> > > > >> > --- a/drivers/gpu/drm/arm/malidp_drv.c
> > > > >> > +++ b/drivers/gpu/drm/arm/malidp_drv.c
> > > > >> > @@ -766,8 +766,25 @@ static int __maybe_unused malidp_pm_resume(struct device *dev)
> > > > >> >     return 0;
> > > > >> >  }
> > > > >> >
> > > > >> > +static int __maybe_unused malidp_pm_suspend_late(struct device *dev)
> > > > >> > +{
> > > > >> > +   if (!pm_runtime_status_suspended(dev)) {
> > > > >> > +           malidp_runtime_pm_suspend(dev);
> > > > >> > +           pm_runtime_set_suspended(dev);
> > > > >> > +   }
> > > > >> > +   return 0;
> > > > >> > +}
> > > > >> > +
> > > > >> > +static int __maybe_unused malidp_pm_resume_early(struct device *dev)
> > > > >> > +{
> > > > >> > +   malidp_runtime_pm_resume(dev);
> > > > >> > +   pm_runtime_set_active(dev);
> > > > >> > +   return 0;
> > > > >> > +}
> > > > >> > +
> > > > >> >  static const struct dev_pm_ops malidp_pm_ops = {
> > > > >> >     SET_SYSTEM_SLEEP_PM_OPS(malidp_pm_suspend, malidp_pm_resume) \
> > > > >> > +   SET_LATE_SYSTEM_SLEEP_PM_OPS(malidp_pm_suspend_late, malidp_pm_resume_early) \
> > > > >> >     SET_RUNTIME_PM_OPS(malidp_runtime_pm_suspend, malidp_runtime_pm_resume, NULL)
> > > > >> >  };
> > > > >> >
> > > > >> > --
> > > > >> > 2.7.4
> > > > >> >
> > > > >> > _______________________________________________
> > > > >> > dri-devel mailing list
> > > > >> > dri-devel@lists.freedesktop.org
> > > > >> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > > >>
> > > > >> --
> > > > >> Daniel Vetter
> > > > >> Software Engineer, Intel Corporation
> > > > >> http://blog.ffwll.ch
> > > > >> _______________________________________________
> > > > >> dri-devel mailing list
> > > > >> dri-devel@lists.freedesktop.org
> > > > >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > > > IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
> > > > > _______________________________________________
> > > > > dri-devel mailing list
> > > > > dri-devel@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > >
> > > >
> > > >
> > > > --
> > > > Daniel Vetter
> > > > Software Engineer, Intel Corporation
> > > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> > 
> > -- 
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 8/8] drm/arm/malidp: Added the late system pm functions

[Brian Starkey]

Hi Daniel,

On Fri, Apr 13, 2018 at 05:44:09PM +0200, Daniel Vetter wrote:
>On Mon, Apr 09, 2018 at 05:15:08PM +0100, Brian Starkey wrote:
>> Hi Daniel,
>>
>> On Mon, Apr 09, 2018 at 10:23:37AM +0200, Daniel Vetter wrote:
>> > On Fri, Apr 06, 2018 at 08:02:16PM +0100, Ayan Halder wrote:
>> > > On Tue, Mar 27, 2018 at 01:09:36PM +0200, Daniel Vetter wrote:
>> > > > On Tue, Mar 27, 2018 at 11:59 AM, Ayan Halder <ayan.halder@arm.com> wrote:

[snip]

>>
>> As for why, my understanding is like so:
>>
>> For ->suspend(), we use the DRM helper, which disables the CRTC.
>> Normally disabling the CRTC would be enough to also invoke our
>> pm_runtime callback to do the final clock disable etc., however when a
>> system suspend is in-progress, the core forcibly takes a runtime
>> reference on all devices - preventing any pm_runtime paths from
>> running.
>
>I thought this was fixed. At least I remember we had to add some special
>calls to i915 to opt out of the "do runtime pm as part of suspend/resume"
>behaviour, since it doesn't match what we needed.
>
>See
>
>commit aae4518b3124b29f8dc81c829c704fd2df72e98b
>Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>Date:   Fri May 16 02:46:50 2014 +0200
>
>    PM / sleep: Mechanism to avoid resuming runtime-suspended devices unnecessarily
>
>So I thought this stuff was supposed to work now. Adding Rafael Wyzocki,
>he's done plenty presentations recently about exactly this.

AFAIK direct-complete is a different thing - related, but not involved
here.

Ayan's patch is to deal with the case where the device is active
(runtime state = active), when a system suspend is triggered.

The core takes its reference (leaving direct-complete devices
'suspended' if possible, but our device is 'active' and so
direct-complete doesn't apply) and then we start the system suspend
and disable the CRTC.

Normally, after the CRTC is disabled, we rely on PM-core to notice we
can be runtime suspended, but it won't do this during system suspend
because of the reference PM-core took, and so we need to force the
suspend ourselves.

>
>> This means that after the CRTC is disabled in ->suspend(), our normal
>> pm_runtime path will not be invoked, and so the things done in
>> malidp_runtime_pm_suspend() will never happen.
>>
>> We were just following the advice in the kernel-doc to deal with this.
>>
>> The alternative would be to call malidp_runtime_pm_{suspend,resume}
>> from the "not late" hooks, but I'd ask why?
>>
>> > Also, you still haven't explained what exactly the dependency is.
>>
>> Because there isn't one :-)
>
>Hm, if there really isn't one, then I guess it's ok. But it's way too easy
>to screw this up, have an accidental depency on a different device. And
>then you try to fix this up. Having a suspend_late hook still smells fishy
>to me, but I might be out of the loop.
>-Daniel
>

Hopefully Rafael can set us straight. The kerneldoc doesn't make
suspend_late sound exceptional, but kernel-doc isn't perfect :-)

Thanks for the review,
-Brian

>
>
>>
>> Thanks,
>> -Brian
>>
>> > -Daniel
>> >
>> > >
>> > > > >> > ---
>> > > > >> >  drivers/gpu/drm/arm/malidp_drv.c | 17 +++++++++++++++++
>> > > > >> >  1 file changed, 17 insertions(+)
>> > > > >> >
>> > > > >> > diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
>> > > > >> > index bd44a6d..f6124d8 100644
>> > > > >> > --- a/drivers/gpu/drm/arm/malidp_drv.c
>> > > > >> > +++ b/drivers/gpu/drm/arm/malidp_drv.c
>> > > > >> > @@ -766,8 +766,25 @@ static int __maybe_unused malidp_pm_resume(struct device *dev)
>> > > > >> >     return 0;
>> > > > >> >  }
>> > > > >> >
>> > > > >> > +static int __maybe_unused malidp_pm_suspend_late(struct device *dev)
>> > > > >> > +{
>> > > > >> > +   if (!pm_runtime_status_suspended(dev)) {
>> > > > >> > +           malidp_runtime_pm_suspend(dev);
>> > > > >> > +           pm_runtime_set_suspended(dev);
>> > > > >> > +   }
>> > > > >> > +   return 0;
>> > > > >> > +}
>> > > > >> > +
>> > > > >> > +static int __maybe_unused malidp_pm_resume_early(struct device *dev)
>> > > > >> > +{
>> > > > >> > +   malidp_runtime_pm_resume(dev);
>> > > > >> > +   pm_runtime_set_active(dev);
>> > > > >> > +   return 0;
>> > > > >> > +}
>> > > > >> > +
>> > > > >> >  static const struct dev_pm_ops malidp_pm_ops = {
>> > > > >> >     SET_SYSTEM_SLEEP_PM_OPS(malidp_pm_suspend, malidp_pm_resume) \
>> > > > >> > +   SET_LATE_SYSTEM_SLEEP_PM_OPS(malidp_pm_suspend_late, malidp_pm_resume_early) \
>> > > > >> >     SET_RUNTIME_PM_OPS(malidp_runtime_pm_suspend, malidp_runtime_pm_resume, NULL)
>> > > > >> >  };
>> > > > >> >
>> > > > >> > --
>> > > > >> > 2.7.4
>> > > > >> >
>> > > > >> > _______________________________________________
>> > > > >> > dri-devel mailing list
>> > > > >> > dri-devel@lists.freedesktop.org
>> > > > >> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>> > > > >>
>> > > > >> --
>> > > > >> Daniel Vetter
>> > > > >> Software Engineer, Intel Corporation
>> > > > >> http://blog.ffwll.ch
>> > > > >> _______________________________________________
>> > > > >> dri-devel mailing list
>> > > > >> dri-devel@lists.freedesktop.org
>> > > > >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>> > > > > IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
>> > > > > _______________________________________________
>> > > > > dri-devel mailing list
>> > > > > dri-devel@lists.freedesktop.org
>> > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>> > > >
>> > > >
>> > > >
>> > > > --
>> > > > Daniel Vetter
>> > > > Software Engineer, Intel Corporation
>> > > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
>> >
>> > --
>> > Daniel Vetter
>> > Software Engineer, Intel Corporation
>> > http://blog.ffwll.ch
>
>-- 
>Daniel Vetter
>Software Engineer, Intel Corporation
>http://blog.ffwll.ch