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 <alexandru-cosmin.gheorghe@xxxxxxx>
> Signed-off-by: Liviu Dudau <liviu.dudau@xxxxxxx>
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 <alexandru-cosmin.gheorghe@xxxxxxx>
> ---
> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
