Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 7 Mar 2018 11:35:29 +0000
From:   Alexandru-Cosmin Gheorghe <Alexandru-Cosmin.Gheorghe@arm.com>
To:     Liviu Dudau <Liviu.Dudau@arm.com>
Cc:     Brian Starkey <brian.starkey@arm.com>,
        Mali DP Maintainers <malidp@foss.arm.com>,
        DRI devel <dri-devel@lists.freedesktop.org>,
        LKML <linux-kernel@vger.kernel.org>, nd@arm.com
Subject: Re: [PATCH v3] drm/mali-dp: Fix malidp_atomic_commit_hw_done() for
 event sending.
Message-ID: <20180307113529.GA21042@e114479-lin.cambridge.arm.com>
References: <20180301174913.10875-1-Liviu.Dudau@arm.com>
 <20180305180316.12654-1-Liviu.Dudau@arm.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20180305180316.12654-1-Liviu.Dudau@arm.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
NoDisclaimer: True
Received-SPF: None (protection.outlook.com: arm.com does not designate
 permitted sender hosts)
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Mar 2018 11:35:32.3547 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 1e061634-e5c4-456d-9c44-08d5841f8955
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: f34e5979-57d9-4aaa-ad4d-b122a662184d
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB6PR0801MB1253
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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@arm.com>
> Signed-off-by: Liviu Dudau <liviu.dudau@arm.com>
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@arm.com>  

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