From patchwork Thu Dec 6 06:46:53 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jayant Shekhar X-Patchwork-Id: 10715325 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id C607F13AF for ; Thu, 6 Dec 2018 06:47:31 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id B77A72D067 for ; Thu, 6 Dec 2018 06:47:31 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id ABEFD2E51D; Thu, 6 Dec 2018 06:47:31 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.2 required=2.0 tests=BAYES_00,MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED autolearn=ham version=3.3.1 Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 5B0372D067 for ; Thu, 6 Dec 2018 06:47:27 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id ED7826E513; Thu, 6 Dec 2018 06:47:25 +0000 (UTC) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from smtp.codeaurora.org (smtp.codeaurora.org [198.145.29.96]) by gabe.freedesktop.org (Postfix) with ESMTPS id 2F1BB6E106; Thu, 6 Dec 2018 06:47:24 +0000 (UTC) Received: by smtp.codeaurora.org (Postfix, from userid 1000) id A6F736140E; Thu, 6 Dec 2018 06:47:17 +0000 (UTC) Received: from jshekhar-linux.qualcomm.com (blr-c-bdr-fw-01_globalnat_allzones-outside.qualcomm.com [103.229.19.19]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-SHA256 (128/128 bits)) (No client certificate requested) (Authenticated sender: jshekhar@smtp.codeaurora.org) by smtp.codeaurora.org (Postfix) with ESMTPSA id 5898561374; Thu, 6 Dec 2018 06:47:03 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 smtp.codeaurora.org 5898561374 From: Jayant Shekhar To: dri-devel@lists.freedesktop.org, linux-arm-msm@vger.kernel.org, freedreno@lists.freedesktop.org, devicetree@vger.kernel.org Subject: [DPU PATCH] drm/msm/dpu: Fix vblank refcount mismatch Date: Thu, 6 Dec 2018 12:16:53 +0530 Message-Id: <1544078813-10945-1-git-send-email-jshekhar@codeaurora.org> X-Mailer: git-send-email 1.9.1 X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: linux-kernel@vger.kernel.org, abhinavk@codeaurora.org, Jayant Shekhar , seanpaul@chromium.org, hoegsberg@chromium.org, chandanu@codeaurora.org MIME-Version: 1.0 Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" X-Virus-Scanned: ClamAV using ClamSMTP _dpu_crtc_vblank_enable_no_lock releases crtc_lock as its needed for power handle operations. This opens up a window where in a thread running dpu_crtc_disable and a thread running dpu_crtc_vblank can race in using dpu_crtc->enabled. dpu_crtc_disable will change the state, where as dpu_crtc_vblank use the variable. The fix is to cache the crtc enabled state while holding the lock and use it as a gate in calling _dpu_crtc_vblank_enable_no_lock. This issue was introduced with the commit cf871c48 (drm/msm/dpu: Remove suspend state tracking from crtc). Below are stack traces of thread 1 and thread 2 in good case and bad case: Bad case: ------------- Thread 1 dpu_encoder_phys_vid_control_vblank_irq+0xd0/0x170 dpu_encoder_register_vblank_callback+0xb8/0x100 _dpu_crtc_vblank_enable_no_lock+0x240/0x288 dpu_crtc_disable+0xc4/0x288 drm_atomic_helper_commit_modeset_disables+0x19c/0x350 msm_atomic_commit_tail+0x48/0x144 commit_tail+0x44/0x70 drm_atomic_helper_commit+0xf0/0xf8 drm_atomic_commit+0x40/0x4c drm_mode_atomic_ioctl+0x374/0x90c drm_ioctl_kernel+0xac/0xec drm_ioctl+0x218/0x384 drm_compat_ioctl+0xd8/0xe8 Thread 2: dpu_encoder_phys_vid_control_vblank_irq+0x74/0x170 dpu_encoder_register_vblank_callback+0xb8/0x100 _dpu_crtc_vblank_enable_no_lock+0x240/0x288 dpu_crtc_vblank+0xa8/0x118 dpu_kms_disable_vblank+0x20/0x2c vblank_ctrl_worker+0xa0/0xe0 kthread_worker_fn+0xe4/0x1a4 kthread+0x11c/0x12c ret_from_fork+0x10/0x18 Good case: -------------- Thread 1: dpu_encoder_phys_vid_control_vblank_irq+0xd0/0x170 dpu_encoder_phys_vid_irq_control+0xc8/0x110 _dpu_encoder_irq_control+0x48/0xa0 _dpu_encoder_resource_control_helper+0xb4/0x10c dpu_encoder_resource_control+0x4e0/0x664 dpu_encoder_virt_enable+0xb8/0x120 dpu_kms_encoder_enable+0x34/0xcc drm_atomic_helper_commit_modeset_enables+0x120/0x1b8 msm_atomic_commit_tail+0x5c/0x144 commit_tail+0x44/0x70 drm_atomic_helper_commit+0xf0/0xf8 drm_atomic_commit+0x40/0x4c drm_mode_atomic_ioctl+0x374/0x90c Thread 2: dpu_crtc_vblank+0xc8/0x118 dpu_kms_disable_vblank+0x20/0x2c vblank_ctrl_worker+0xa0/0xe0 kthread_worker_fn+0xe4/0x1a4 kthread+0x11c/0x12c Signed-off-by: Jayant Shekhar --- drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c index 630cbaa..e81ad8c 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c @@ -877,6 +877,7 @@ static void dpu_crtc_disable(struct drm_crtc *crtc) struct drm_encoder *encoder; struct msm_drm_private *priv; unsigned long flags; + bool crtc_en; if (!crtc || !crtc->dev || !crtc->dev->dev_private || !crtc->state) { DPU_ERROR("invalid crtc\n"); @@ -901,11 +902,21 @@ static void dpu_crtc_disable(struct drm_crtc *crtc) atomic_read(&dpu_crtc->frame_pending)); trace_dpu_crtc_disable(DRMID(crtc), false, dpu_crtc); - if (dpu_crtc->enabled && dpu_crtc->vblank_requested) { - _dpu_crtc_vblank_enable_no_lock(dpu_crtc, false); - } + + /* + * Cache vblank enabled before calling _dpu_crtc_vblank_enable_no_lock, + * because we release crtc_lock inside and acquire it back. While lock + * is released, there are cases where dpu_crtc_vblank comes in between + * while disable is going on. dpu_crtc_vblank further calls + * _dpu_crtc_vblank_enable_no_lock which tries vblank disable again + * resulting in refcount mismatch. + */ + crtc_en = dpu_crtc->enabled; dpu_crtc->enabled = false; + if (crtc_en && dpu_crtc->vblank_requested) + _dpu_crtc_vblank_enable_no_lock(dpu_crtc, false); + if (atomic_read(&dpu_crtc->frame_pending)) { trace_dpu_crtc_disable_frame_pending(DRMID(crtc), atomic_read(&dpu_crtc->frame_pending));