2018-04-23 14:52:31

by Ayan Halder

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

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

---
Changes in v2:
- Removed the change ids and modified some commit messages

---
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



2018-04-23 14:52:32

by Ayan Halder

[permalink] [raw]
Subject: [PATCH v2 2/8] drm/arm/malidp: Modified the prototype of malidp_se_irq_fini

'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'

Signed-off-by: Ayan Kumar Halder <[email protected]>

---
Changes in v2:-
- Removed the change id
---
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


2018-04-23 14:53:11

by Ayan Halder

[permalink] [raw]
Subject: [PATCH v2 8/8] drm/arm/malidp: Added the late system pm functions

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.

The difference between suspend() and suspend_late() is as follows:-
1. suspend() makes the device quiescent. In our case, we invoke the DRM
helper which disables the CRTC. This would have invoked runtime pm
suspend but the system suspend process disables runtime pm.
2. suspend_late() It continues the suspend operations of the drm device
which was started by suspend(). In our case, it performs the same functionality
as runtime_suspend().

The complimentary functions are resume() and resume_early(). In the case of
resume_early(), we invoke malidp_runtime_pm_resume() which enables the clocks
and the interrupts. It sets the runtime status as active. If the device was
in runtime suspend mode before system suspend was called, pm_runtime_work()
will put the device back in runtime suspended mode( after the complete system
has been resumed).

Signed-off-by: Ayan Kumar Halder <[email protected]>

---
Changes in v2:-
- Removed the change id and modified the commit message
---
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


2018-04-23 14:53:50

by Ayan Halder

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

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 <[email protected]>

---
Changes in v2:-
- Removed the change id
---
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


2018-04-23 14:53:59

by Ayan Halder

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

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:
https://lkml.org/lkml/2017/5/15/695

Signed-off-by: Ayan Kumar Halder <[email protected]>

---
Changes in v2:-
- Removed the change id and modified the commit message
---
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


2018-04-23 14:54:13

by Ayan Halder

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

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 <[email protected]>

---
Changes in v2:-
- Removed the change id
---
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


2018-04-23 14:54:59

by Ayan Halder

[permalink] [raw]
Subject: [PATCH v2 3/8] drm/arm/malidp: Split malidp_de_irq_init

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.

Signed-off-by: Ayan Kumar Halder <[email protected]>

---
Changes in v2:-
- Removed the change id
---
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


2018-04-23 14:55:04

by Ayan Halder

[permalink] [raw]
Subject: [PATCH v2 4/8] drm/arm/malidp: Split malidp_se_irq_init

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 <[email protected]>

---
Changes in v2:-
- Removed the change id
---
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


2018-04-23 14:55:11

by Ayan Halder

[permalink] [raw]
Subject: [PATCH v2 1/8] drm/arm/malidp: Modified the prototype of malidp_de_irq_fini

'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'.

Signed-off-by: Ayan Kumar Halder <[email protected]>

---
Changes in v2:-
- Removed the change id
---
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


2018-04-23 15:38:41

by Liviu Dudau

[permalink] [raw]
Subject: Re: [PATCH v2 6/8] drm/arm/malidp: Enable/disable the scaling engine interrupts with memory writeback

On Mon, Apr 23, 2018 at 03:50:49PM +0100, Ayan Kumar Halder wrote:
> 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:
> https://lkml.org/lkml/2017/5/15/695
>
> Signed-off-by: Ayan Kumar Halder <[email protected]>
>
> ---
> Changes in v2:-
> - Removed the change id and modified the commit message
> ---
> 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);

We now know that this patch is broken, because it disables interrupts
for the writeback after a previous commit has enabled them but before
the writeback engine has finished writing, so the better place for
enabling and disabling the SE IRQs is in the malidp_pm_{suspend,resume}
functions, not here.

Only for this patch: NACK.

Best regards,
Liviu

> --
> 2.7.4
>

--
====================
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
¯\_(ツ)_/¯

2018-04-24 10:15:18

by Ayan Halder

[permalink] [raw]
Subject: Re: [PATCH v2 6/8] drm/arm/malidp: Enable/disable the scaling engine interrupts with memory writeback

Hi Liviu,

On Mon, Apr 23, 2018 at 04:36:47PM +0100, Liviu Dudau wrote:
> On Mon, Apr 23, 2018 at 03:50:49PM +0100, Ayan Kumar Halder wrote:
> > 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:
> > https://lkml.org/lkml/2017/5/15/695
> >
> > Signed-off-by: Ayan Kumar Halder <[email protected]>
> >
> > ---
> > Changes in v2:-
> > - Removed the change id and modified the commit message
> > ---
> > 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);
>
> We now know that this patch is broken, because it disables interrupts
> for the writeback after a previous commit has enabled them but before
> the writeback engine has finished writing, so the better place for
> enabling and disabling the SE IRQs is in the malidp_pm_{suspend,resume}
> functions, not here.
>
Sorry, I missed this issue. I will send an updated v3 patch for this
only.

Thanks,
Ayan Kumar Halder
> Only for this patch: NACK.
>
> Best regards,
> Liviu
>
> > --
> > 2.7.4
> >
>
> --
> ====================
> | I would like to |
> | fix the world, |
> | but they're not |
> | giving me the |
> \ source code! /
> ---------------
> ??\_(???)_/??