Mali DP hardware has a 'go' bit (config_valid) for making the new scene
parameters active at the next page flip. The problem with the current
code is that the driver first sets this bit and then proceeds to wait
for confirmation from the hardware that the configuration has been
updated before arming the vblank event. As config_valid is actually
asserted by the hardware after the vblank event, during the prefetch
phase, when we get to arming the vblank event we are going to send it
at the next vblank, in effect halving the vblank rate from the userspace
perspective.
Fix it by sending the userspace event from the IRQ handler, when we
handle the config_valid interrupt, which syncs with the time when the
hardware is active with the new parameters.
Reported-by: Alexandru-Cosmin Gheorghe <[email protected]>
Signed-off-by: Liviu Dudau <[email protected]>
---
drivers/gpu/drm/arm/malidp_drv.c | 27 ++++++++++++---------------
drivers/gpu/drm/arm/malidp_drv.h | 1 +
drivers/gpu/drm/arm/malidp_hw.c | 12 +++++++++---
3 files changed, 22 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
index d88a3b9d59cc..7be5d48eefd0 100644
--- a/drivers/gpu/drm/arm/malidp_drv.c
+++ b/drivers/gpu/drm/arm/malidp_drv.c
@@ -185,27 +185,26 @@ static int malidp_set_and_wait_config_valid(struct drm_device *drm)
static void malidp_atomic_commit_hw_done(struct drm_atomic_state *state)
{
- struct drm_pending_vblank_event *event;
struct drm_device *drm = state->dev;
struct malidp_drm *malidp = drm->dev_private;
+ malidp->event = malidp->crtc.state->event;
+ malidp->crtc.state->event = NULL;
+
+ /*
+ * if we have an event to deliver to userspace, make sure
+ * the vblank is enabled as we are sending it from the IRQ
+ * handler.
+ */
+ if (malidp->event)
+ drm_crtc_vblank_get(&malidp->crtc);
+
if (malidp->crtc.enabled) {
/* only set config_valid if the CRTC is enabled */
- if (malidp_set_and_wait_config_valid(drm))
+ if (malidp_set_and_wait_config_valid(drm) < 0)
DRM_DEBUG_DRIVER("timed out waiting for updated configuration\n");
}
- event = malidp->crtc.state->event;
- if (event) {
- malidp->crtc.state->event = NULL;
-
- spin_lock_irq(&drm->event_lock);
- if (drm_crtc_vblank_get(&malidp->crtc) == 0)
- drm_crtc_arm_vblank_event(&malidp->crtc, event);
- else
- drm_crtc_send_vblank_event(&malidp->crtc, event);
- spin_unlock_irq(&drm->event_lock);
- }
drm_atomic_helper_commit_hw_done(state);
}
@@ -232,8 +231,6 @@ static void malidp_atomic_commit_tail(struct drm_atomic_state *state)
malidp_atomic_commit_hw_done(state);
- drm_atomic_helper_wait_for_vblanks(drm, state);
-
pm_runtime_put(drm->dev);
drm_atomic_helper_cleanup_planes(drm, state);
diff --git a/drivers/gpu/drm/arm/malidp_drv.h b/drivers/gpu/drm/arm/malidp_drv.h
index e0d12c9fc6b8..c2375bb49619 100644
--- a/drivers/gpu/drm/arm/malidp_drv.h
+++ b/drivers/gpu/drm/arm/malidp_drv.h
@@ -22,6 +22,7 @@ struct malidp_drm {
struct malidp_hw_device *dev;
struct drm_crtc crtc;
wait_queue_head_t wq;
+ struct drm_pending_vblank_event *event;
atomic_t config_valid;
u32 core_id;
};
diff --git a/drivers/gpu/drm/arm/malidp_hw.c b/drivers/gpu/drm/arm/malidp_hw.c
index 2bfb542135ac..8abd335ec313 100644
--- a/drivers/gpu/drm/arm/malidp_hw.c
+++ b/drivers/gpu/drm/arm/malidp_hw.c
@@ -782,9 +782,15 @@ static irqreturn_t malidp_de_irq(int irq, void *arg)
/* first handle the config valid IRQ */
dc_status = malidp_hw_read(hwdev, hw->map.dc_base + MALIDP_REG_STATUS);
if (dc_status & hw->map.dc_irq_map.vsync_irq) {
- /* we have a page flip event */
- atomic_set(&malidp->config_valid, 1);
malidp_hw_clear_irq(hwdev, MALIDP_DC_BLOCK, dc_status);
+ /* do we have a page flip event? */
+ if (malidp->event != NULL) {
+ spin_lock(&drm->event_lock);
+ drm_crtc_send_vblank_event(&malidp->crtc, malidp->event);
+ malidp->event = NULL;
+ spin_unlock(&drm->event_lock);
+ }
+ atomic_set(&malidp->config_valid, 1);
ret = IRQ_WAKE_THREAD;
}
@@ -794,7 +800,7 @@ static irqreturn_t malidp_de_irq(int irq, void *arg)
mask = malidp_hw_read(hwdev, MALIDP_REG_MASKIRQ);
status &= mask;
- if (status & de->vsync_irq)
+ if ((status & de->vsync_irq) && malidp->crtc.enabled)
drm_crtc_handle_vblank(&malidp->crtc);
malidp_hw_clear_irq(hwdev, MALIDP_DE_BLOCK, status);
--
2.16.2
Mali DP hardware has a 'go' bit (config_valid) for making the new scene
parameters active at the next page flip. The problem with the current
code is that the driver first sets this bit and then proceeds to wait
for confirmation from the hardware that the configuration has been
updated before arming the vblank event. As config_valid is actually
asserted by the hardware after the vblank event, during the prefetch
phase, when we get to arming the vblank event we are going to send it
at the next vblank, in effect halving the vblank rate from the userspace
perspective.
Fix it by sending the userspace event from the IRQ handler, when we
handle the config_valid interrupt, which syncs with the time when the
hardware is active with the new parameters.
v2: Brian reminded me that interrupts won't fire when CRTC is off,
so we need to do the sending ourselves.
Cc: Brian Starkey <[email protected]>
Reported-by: Alexandru-Cosmin Gheorghe <[email protected]>
Signed-off-by: Liviu Dudau <[email protected]>
---
drivers/gpu/drm/arm/malidp_drv.c | 30 ++++++++++++++++--------------
drivers/gpu/drm/arm/malidp_drv.h | 1 +
drivers/gpu/drm/arm/malidp_hw.c | 12 +++++++++---
3 files changed, 26 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
index d88a3b9d59cc..a12bef526f92 100644
--- a/drivers/gpu/drm/arm/malidp_drv.c
+++ b/drivers/gpu/drm/arm/malidp_drv.c
@@ -185,25 +185,29 @@ static int malidp_set_and_wait_config_valid(struct drm_device *drm)
static void malidp_atomic_commit_hw_done(struct drm_atomic_state *state)
{
- struct drm_pending_vblank_event *event;
struct drm_device *drm = state->dev;
struct malidp_drm *malidp = drm->dev_private;
+ malidp->event = malidp->crtc.state->event;
+ malidp->crtc.state->event = NULL;
+
if (malidp->crtc.enabled) {
+ /*
+ * if we have an event to deliver to userspace, make sure
+ * the vblank is enabled as we are sending it from the IRQ
+ * handler.
+ */
+ if (malidp->event)
+ drm_crtc_vblank_get(&malidp->crtc);
+
/* only set config_valid if the CRTC is enabled */
- if (malidp_set_and_wait_config_valid(drm))
+ if (malidp_set_and_wait_config_valid(drm) < 0)
DRM_DEBUG_DRIVER("timed out waiting for updated configuration\n");
- }
-
- event = malidp->crtc.state->event;
- if (event) {
- malidp->crtc.state->event = NULL;
-
+ } else if (malidp->event) {
+ /* CRTC disabled means vblank IRQ is disabled, send event directly */
spin_lock_irq(&drm->event_lock);
- if (drm_crtc_vblank_get(&malidp->crtc) == 0)
- drm_crtc_arm_vblank_event(&malidp->crtc, event);
- else
- drm_crtc_send_vblank_event(&malidp->crtc, event);
+ drm_crtc_send_vblank_event(&malidp->crtc, malidp->event);
+ malidp->event = NULL;
spin_unlock_irq(&drm->event_lock);
}
drm_atomic_helper_commit_hw_done(state);
@@ -232,8 +236,6 @@ static void malidp_atomic_commit_tail(struct drm_atomic_state *state)
malidp_atomic_commit_hw_done(state);
- drm_atomic_helper_wait_for_vblanks(drm, state);
-
pm_runtime_put(drm->dev);
drm_atomic_helper_cleanup_planes(drm, state);
diff --git a/drivers/gpu/drm/arm/malidp_drv.h b/drivers/gpu/drm/arm/malidp_drv.h
index e0d12c9fc6b8..c2375bb49619 100644
--- a/drivers/gpu/drm/arm/malidp_drv.h
+++ b/drivers/gpu/drm/arm/malidp_drv.h
@@ -22,6 +22,7 @@ struct malidp_drm {
struct malidp_hw_device *dev;
struct drm_crtc crtc;
wait_queue_head_t wq;
+ struct drm_pending_vblank_event *event;
atomic_t config_valid;
u32 core_id;
};
diff --git a/drivers/gpu/drm/arm/malidp_hw.c b/drivers/gpu/drm/arm/malidp_hw.c
index 2bfb542135ac..8abd335ec313 100644
--- a/drivers/gpu/drm/arm/malidp_hw.c
+++ b/drivers/gpu/drm/arm/malidp_hw.c
@@ -782,9 +782,15 @@ static irqreturn_t malidp_de_irq(int irq, void *arg)
/* first handle the config valid IRQ */
dc_status = malidp_hw_read(hwdev, hw->map.dc_base + MALIDP_REG_STATUS);
if (dc_status & hw->map.dc_irq_map.vsync_irq) {
- /* we have a page flip event */
- atomic_set(&malidp->config_valid, 1);
malidp_hw_clear_irq(hwdev, MALIDP_DC_BLOCK, dc_status);
+ /* do we have a page flip event? */
+ if (malidp->event != NULL) {
+ spin_lock(&drm->event_lock);
+ drm_crtc_send_vblank_event(&malidp->crtc, malidp->event);
+ malidp->event = NULL;
+ spin_unlock(&drm->event_lock);
+ }
+ atomic_set(&malidp->config_valid, 1);
ret = IRQ_WAKE_THREAD;
}
@@ -794,7 +800,7 @@ static irqreturn_t malidp_de_irq(int irq, void *arg)
mask = malidp_hw_read(hwdev, MALIDP_REG_MASKIRQ);
status &= mask;
- if (status & de->vsync_irq)
+ if ((status & de->vsync_irq) && malidp->crtc.enabled)
drm_crtc_handle_vblank(&malidp->crtc);
malidp_hw_clear_irq(hwdev, MALIDP_DE_BLOCK, status);
--
2.16.2
Mali DP hardware has a 'go' bit (config_valid) for making the new scene
parameters active at the next page flip. The problem with the current
code is that the driver first sets this bit and then proceeds to wait
for confirmation from the hardware that the configuration has been
updated before arming the vblank event. As config_valid is actually
asserted by the hardware after the vblank event, during the prefetch
phase, when we get to arming the vblank event we are going to send it
at the next vblank, in effect halving the vblank rate from the userspace
perspective.
Fix it by sending the userspace event from the IRQ handler, when we
handle the config_valid interrupt, which syncs with the time when the
hardware is active with the new parameters.
v2: Brian reminded me that interrupts won't fire when CRTC is off,
so we need to do the sending ourselves.
v3: crtc->enabled is the wrong flag to use here, as when we get an
atomic commit that turns off the CRTC it will still be enabled until
after the commit is done. Use the crtc->state->active for test.
Reported-by: Alexandru-Cosmin Gheorghe <[email protected]>
Signed-off-by: Liviu Dudau <[email protected]>
---
drivers/gpu/drm/arm/malidp_drv.c | 32 +++++++++++++++++---------------
drivers/gpu/drm/arm/malidp_drv.h | 1 +
drivers/gpu/drm/arm/malidp_hw.c | 12 +++++++++---
3 files changed, 27 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
index d88a3b9d59cc..3c628e43bf25 100644
--- a/drivers/gpu/drm/arm/malidp_drv.c
+++ b/drivers/gpu/drm/arm/malidp_drv.c
@@ -185,25 +185,29 @@ static int malidp_set_and_wait_config_valid(struct drm_device *drm)
static void malidp_atomic_commit_hw_done(struct drm_atomic_state *state)
{
- struct drm_pending_vblank_event *event;
struct drm_device *drm = state->dev;
struct malidp_drm *malidp = drm->dev_private;
- if (malidp->crtc.enabled) {
- /* only set config_valid if the CRTC is enabled */
- if (malidp_set_and_wait_config_valid(drm))
- DRM_DEBUG_DRIVER("timed out waiting for updated configuration\n");
- }
+ malidp->event = malidp->crtc.state->event;
+ malidp->crtc.state->event = NULL;
- event = malidp->crtc.state->event;
- if (event) {
- malidp->crtc.state->event = NULL;
+ if (malidp->crtc.state->active) {
+ /*
+ * if we have an event to deliver to userspace, make sure
+ * the vblank is enabled as we are sending it from the IRQ
+ * handler.
+ */
+ if (malidp->event)
+ drm_crtc_vblank_get(&malidp->crtc);
+ /* only set config_valid if the CRTC is enabled */
+ if (malidp_set_and_wait_config_valid(drm) < 0)
+ DRM_DEBUG_DRIVER("timed out waiting for updated configuration\n");
+ } else if (malidp->event) {
+ /* CRTC inactive means vblank IRQ is disabled, send event directly */
spin_lock_irq(&drm->event_lock);
- if (drm_crtc_vblank_get(&malidp->crtc) == 0)
- drm_crtc_arm_vblank_event(&malidp->crtc, event);
- else
- drm_crtc_send_vblank_event(&malidp->crtc, event);
+ drm_crtc_send_vblank_event(&malidp->crtc, malidp->event);
+ malidp->event = NULL;
spin_unlock_irq(&drm->event_lock);
}
drm_atomic_helper_commit_hw_done(state);
@@ -232,8 +236,6 @@ static void malidp_atomic_commit_tail(struct drm_atomic_state *state)
malidp_atomic_commit_hw_done(state);
- drm_atomic_helper_wait_for_vblanks(drm, state);
-
pm_runtime_put(drm->dev);
drm_atomic_helper_cleanup_planes(drm, state);
diff --git a/drivers/gpu/drm/arm/malidp_drv.h b/drivers/gpu/drm/arm/malidp_drv.h
index e0d12c9fc6b8..c2375bb49619 100644
--- a/drivers/gpu/drm/arm/malidp_drv.h
+++ b/drivers/gpu/drm/arm/malidp_drv.h
@@ -22,6 +22,7 @@ struct malidp_drm {
struct malidp_hw_device *dev;
struct drm_crtc crtc;
wait_queue_head_t wq;
+ struct drm_pending_vblank_event *event;
atomic_t config_valid;
u32 core_id;
};
diff --git a/drivers/gpu/drm/arm/malidp_hw.c b/drivers/gpu/drm/arm/malidp_hw.c
index 2bfb542135ac..8abd335ec313 100644
--- a/drivers/gpu/drm/arm/malidp_hw.c
+++ b/drivers/gpu/drm/arm/malidp_hw.c
@@ -782,9 +782,15 @@ static irqreturn_t malidp_de_irq(int irq, void *arg)
/* first handle the config valid IRQ */
dc_status = malidp_hw_read(hwdev, hw->map.dc_base + MALIDP_REG_STATUS);
if (dc_status & hw->map.dc_irq_map.vsync_irq) {
- /* we have a page flip event */
- atomic_set(&malidp->config_valid, 1);
malidp_hw_clear_irq(hwdev, MALIDP_DC_BLOCK, dc_status);
+ /* do we have a page flip event? */
+ if (malidp->event != NULL) {
+ spin_lock(&drm->event_lock);
+ drm_crtc_send_vblank_event(&malidp->crtc, malidp->event);
+ malidp->event = NULL;
+ spin_unlock(&drm->event_lock);
+ }
+ atomic_set(&malidp->config_valid, 1);
ret = IRQ_WAKE_THREAD;
}
@@ -794,7 +800,7 @@ static irqreturn_t malidp_de_irq(int irq, void *arg)
mask = malidp_hw_read(hwdev, MALIDP_REG_MASKIRQ);
status &= mask;
- if (status & de->vsync_irq)
+ if ((status & de->vsync_irq) && malidp->crtc.enabled)
drm_crtc_handle_vblank(&malidp->crtc);
malidp_hw_clear_irq(hwdev, MALIDP_DE_BLOCK, status);
--
2.16.2
On Mon, Mar 05, 2018 at 06:03:16PM +0000, Liviu Dudau wrote:
> Mali DP hardware has a 'go' bit (config_valid) for making the new scene
> parameters active at the next page flip. The problem with the current
> code is that the driver first sets this bit and then proceeds to wait
> for confirmation from the hardware that the configuration has been
> updated before arming the vblank event. As config_valid is actually
> asserted by the hardware after the vblank event, during the prefetch
> phase, when we get to arming the vblank event we are going to send it
> at the next vblank, in effect halving the vblank rate from the userspace
> perspective.
>
> Fix it by sending the userspace event from the IRQ handler, when we
> handle the config_valid interrupt, which syncs with the time when the
> hardware is active with the new parameters.
>
> v2: Brian reminded me that interrupts won't fire when CRTC is off,
> so we need to do the sending ourselves.
>
> v3: crtc->enabled is the wrong flag to use here, as when we get an
> atomic commit that turns off the CRTC it will still be enabled until
> after the commit is done. Use the crtc->state->active for test.
>
> Reported-by: Alexandru-Cosmin Gheorghe <[email protected]>
> Signed-off-by: Liviu Dudau <[email protected]>
Tested this with Mali DP-650, modetest is able to do page flipping at the
expected frequency. Also, I didn't observe any regressions in the
driver unit tests.
Tested-by: Alexandru Gheorghe <[email protected]>
> ---
> drivers/gpu/drm/arm/malidp_drv.c | 32 +++++++++++++++++---------------
> drivers/gpu/drm/arm/malidp_drv.h | 1 +
> drivers/gpu/drm/arm/malidp_hw.c | 12 +++++++++---
> 3 files changed, 27 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
> index d88a3b9d59cc..3c628e43bf25 100644
> --- a/drivers/gpu/drm/arm/malidp_drv.c
> +++ b/drivers/gpu/drm/arm/malidp_drv.c
> @@ -185,25 +185,29 @@ static int malidp_set_and_wait_config_valid(struct drm_device *drm)
>
> static void malidp_atomic_commit_hw_done(struct drm_atomic_state *state)
> {
> - struct drm_pending_vblank_event *event;
> struct drm_device *drm = state->dev;
> struct malidp_drm *malidp = drm->dev_private;
>
> - if (malidp->crtc.enabled) {
> - /* only set config_valid if the CRTC is enabled */
> - if (malidp_set_and_wait_config_valid(drm))
> - DRM_DEBUG_DRIVER("timed out waiting for updated configuration\n");
> - }
> + malidp->event = malidp->crtc.state->event;
> + malidp->crtc.state->event = NULL;
>
> - event = malidp->crtc.state->event;
> - if (event) {
> - malidp->crtc.state->event = NULL;
> + if (malidp->crtc.state->active) {
> + /*
> + * if we have an event to deliver to userspace, make sure
> + * the vblank is enabled as we are sending it from the IRQ
> + * handler.
> + */
> + if (malidp->event)
> + drm_crtc_vblank_get(&malidp->crtc);
>
> + /* only set config_valid if the CRTC is enabled */
> + if (malidp_set_and_wait_config_valid(drm) < 0)
> + DRM_DEBUG_DRIVER("timed out waiting for updated configuration\n");
> + } else if (malidp->event) {
> + /* CRTC inactive means vblank IRQ is disabled, send event directly */
> spin_lock_irq(&drm->event_lock);
> - if (drm_crtc_vblank_get(&malidp->crtc) == 0)
> - drm_crtc_arm_vblank_event(&malidp->crtc, event);
> - else
> - drm_crtc_send_vblank_event(&malidp->crtc, event);
> + drm_crtc_send_vblank_event(&malidp->crtc, malidp->event);
> + malidp->event = NULL;
> spin_unlock_irq(&drm->event_lock);
> }
> drm_atomic_helper_commit_hw_done(state);
> @@ -232,8 +236,6 @@ static void malidp_atomic_commit_tail(struct drm_atomic_state *state)
>
> malidp_atomic_commit_hw_done(state);
>
> - drm_atomic_helper_wait_for_vblanks(drm, state);
> -
> pm_runtime_put(drm->dev);
>
> drm_atomic_helper_cleanup_planes(drm, state);
> diff --git a/drivers/gpu/drm/arm/malidp_drv.h b/drivers/gpu/drm/arm/malidp_drv.h
> index e0d12c9fc6b8..c2375bb49619 100644
> --- a/drivers/gpu/drm/arm/malidp_drv.h
> +++ b/drivers/gpu/drm/arm/malidp_drv.h
> @@ -22,6 +22,7 @@ struct malidp_drm {
> struct malidp_hw_device *dev;
> struct drm_crtc crtc;
> wait_queue_head_t wq;
> + struct drm_pending_vblank_event *event;
> atomic_t config_valid;
> u32 core_id;
> };
> diff --git a/drivers/gpu/drm/arm/malidp_hw.c b/drivers/gpu/drm/arm/malidp_hw.c
> index 2bfb542135ac..8abd335ec313 100644
> --- a/drivers/gpu/drm/arm/malidp_hw.c
> +++ b/drivers/gpu/drm/arm/malidp_hw.c
> @@ -782,9 +782,15 @@ static irqreturn_t malidp_de_irq(int irq, void *arg)
> /* first handle the config valid IRQ */
> dc_status = malidp_hw_read(hwdev, hw->map.dc_base + MALIDP_REG_STATUS);
> if (dc_status & hw->map.dc_irq_map.vsync_irq) {
> - /* we have a page flip event */
> - atomic_set(&malidp->config_valid, 1);
> malidp_hw_clear_irq(hwdev, MALIDP_DC_BLOCK, dc_status);
> + /* do we have a page flip event? */
> + if (malidp->event != NULL) {
> + spin_lock(&drm->event_lock);
> + drm_crtc_send_vblank_event(&malidp->crtc, malidp->event);
> + malidp->event = NULL;
> + spin_unlock(&drm->event_lock);
> + }
> + atomic_set(&malidp->config_valid, 1);
> ret = IRQ_WAKE_THREAD;
> }
>
> @@ -794,7 +800,7 @@ static irqreturn_t malidp_de_irq(int irq, void *arg)
>
> mask = malidp_hw_read(hwdev, MALIDP_REG_MASKIRQ);
> status &= mask;
> - if (status & de->vsync_irq)
> + if ((status & de->vsync_irq) && malidp->crtc.enabled)
> drm_crtc_handle_vblank(&malidp->crtc);
>
> malidp_hw_clear_irq(hwdev, MALIDP_DE_BLOCK, status);
> --
> 2.16.2