Re: [PATCH v3] drm/mali-dp: Fix malidp_atomic_commit_hw_done() for event sending.

[Alexandru-Cosmin Gheorghe]

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