From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-9.8 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,USER_AGENT_GIT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 69C8DECE58B for ; Sun, 6 Oct 2019 17:33:53 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 41A1721841 for ; Sun, 6 Oct 2019 17:33:53 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1570383233; bh=rIKf7LtdZcL+QQJa9NkH9hjNvOfePb4mc/FQ3u++flo=; h=From:To:Cc:Subject:Date:In-Reply-To:References:List-ID:From; b=AFKIwtgJjt6O5HDUnCmXF/lPpruzTEzke4uH0gQHoryZZsSnB+7vHbgB+0d8CxQAT NP/g0+prSL+4F4Xgy1heyKG07iCxsFMwH5YZyd3uxCgSJ7kG+n/7U7sGnrCkx6K18D SrxRKm2LCFcO/ML0ZzXgOvcd4SkH0cNNYA3zJ34A= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729839AbfJFRdw (ORCPT ); Sun, 6 Oct 2019 13:33:52 -0400 Received: from mail.kernel.org ([198.145.29.99]:60436 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729816AbfJFRds (ORCPT ); Sun, 6 Oct 2019 13:33:48 -0400 Received: from localhost (83-86-89-107.cable.dynamic.v4.ziggo.nl [83.86.89.107]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id D12DB2133F; Sun, 6 Oct 2019 17:33:46 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1570383227; bh=rIKf7LtdZcL+QQJa9NkH9hjNvOfePb4mc/FQ3u++flo=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=Hd1YsyMSq+8HBwLw9mm8YYgvHw/jWVmWzBzYw/ts7dn5G2XGugtIux4BHnl0k129n gor9H/EXGt9qtSNckKhvKMeGyNCEZdxRjOisx3ww1/53/V/fCCKnkT22wTdYr3CwPD NzXA3VOrMmgdcPQg9JLDfz7XQROONmsDh91NBQ18= From: Greg Kroah-Hartman To: linux-kernel@vger.kernel.org Cc: Greg Kroah-Hartman , stable@vger.kernel.org, Shayenne Moura , Rodrigo Siqueira , Haneen Mohammed , Daniel Vetter , Daniel Vetter , Sasha Levin Subject: [PATCH 5.2 026/137] drm/vkms: Fix crc worker races Date: Sun, 6 Oct 2019 19:20:10 +0200 Message-Id: <20191006171211.366402397@linuxfoundation.org> X-Mailer: git-send-email 2.23.0 In-Reply-To: <20191006171209.403038733@linuxfoundation.org> References: <20191006171209.403038733@linuxfoundation.org> User-Agent: quilt/0.66 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org From: Daniel Vetter [ Upstream commit 18d0952a838ba559655b0cd9cf85097ad63d9bca ] The issue we have is that the crc worker might fall behind. We've tried to handle this by tracking both the earliest frame for which it still needs to compute a crc, and the last one. Plus when the crtc_state changes, we have a new work item, which are all run in order due to the ordered workqueue we allocate for each vkms crtc. Trouble is there's been a few small issues in the current code: - we need to capture frame_end in the vblank hrtimer, not in the worker. The worker might run much later, and then we generate a lot of crc for which there's already a different worker queued up. - frame number might be 0, so create a new crc_pending boolean to track this without confusion. - we need to atomically grab frame_start/end and clear it, so do that all in one go. This is not going to create a new race, because if we race with the hrtimer then our work will be re-run. - only race that can happen is the following: 1. worker starts 2. hrtimer runs and updates frame_end 3. worker grabs frame_start/end, already reading the new frame_end, and clears crc_pending 4. hrtimer calls queue_work() 5. worker completes 6. worker gets re-run, crc_pending is false Explain this case a bit better by rewording the comment. v2: Demote warning level output to debug when we fail to requeue, this is expected under high load when the crc worker can't quite keep up. Cc: Shayenne Moura Cc: Rodrigo Siqueira Cc: Haneen Mohammed Cc: Daniel Vetter Signed-off-by: Daniel Vetter Reviewed-by: Rodrigo Siqueira Tested-by: Rodrigo Siqueira Signed-off-by: Rodrigo Siqueira Link: https://patchwork.freedesktop.org/patch/msgid/20190606222751.32567-2-daniel.vetter@ffwll.ch Signed-off-by: Sasha Levin --- drivers/gpu/drm/vkms/vkms_crc.c | 27 +++++++++++++-------------- drivers/gpu/drm/vkms/vkms_crtc.c | 9 +++++++-- drivers/gpu/drm/vkms/vkms_drv.h | 2 ++ 3 files changed, 22 insertions(+), 16 deletions(-) diff --git a/drivers/gpu/drm/vkms/vkms_crc.c b/drivers/gpu/drm/vkms/vkms_crc.c index d7b409a3c0f8c..66603da634fe3 100644 --- a/drivers/gpu/drm/vkms/vkms_crc.c +++ b/drivers/gpu/drm/vkms/vkms_crc.c @@ -166,16 +166,24 @@ void vkms_crc_work_handle(struct work_struct *work) struct drm_plane *plane; u32 crc32 = 0; u64 frame_start, frame_end; + bool crc_pending; unsigned long flags; spin_lock_irqsave(&out->state_lock, flags); frame_start = crtc_state->frame_start; frame_end = crtc_state->frame_end; + crc_pending = crtc_state->crc_pending; + crtc_state->frame_start = 0; + crtc_state->frame_end = 0; + crtc_state->crc_pending = false; spin_unlock_irqrestore(&out->state_lock, flags); - /* _vblank_handle() hasn't updated frame_start yet */ - if (!frame_start || frame_start == frame_end) - goto out; + /* + * We raced with the vblank hrtimer and previous work already computed + * the crc, nothing to do. + */ + if (!crc_pending) + return; drm_for_each_plane(plane, &vdev->drm) { struct vkms_plane_state *vplane_state; @@ -196,20 +204,11 @@ void vkms_crc_work_handle(struct work_struct *work) if (primary_crc) crc32 = _vkms_get_crc(primary_crc, cursor_crc); - frame_end = drm_crtc_accurate_vblank_count(crtc); - - /* queue_work can fail to schedule crc_work; add crc for - * missing frames + /* + * The worker can fall behind the vblank hrtimer, make sure we catch up. */ while (frame_start <= frame_end) drm_crtc_add_crc_entry(crtc, true, frame_start++, &crc32); - -out: - /* to avoid using the same value for frame number again */ - spin_lock_irqsave(&out->state_lock, flags); - crtc_state->frame_end = frame_end; - crtc_state->frame_start = 0; - spin_unlock_irqrestore(&out->state_lock, flags); } static int vkms_crc_parse_source(const char *src_name, bool *enabled) diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c index e447b7588d06e..77a1f5fa5d5c8 100644 --- a/drivers/gpu/drm/vkms/vkms_crtc.c +++ b/drivers/gpu/drm/vkms/vkms_crtc.c @@ -30,13 +30,18 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer) * has read the data */ spin_lock(&output->state_lock); - if (!state->frame_start) + if (!state->crc_pending) state->frame_start = frame; + else + DRM_DEBUG_DRIVER("crc worker falling behind, frame_start: %llu, frame_end: %llu\n", + state->frame_start, frame); + state->frame_end = frame; + state->crc_pending = true; spin_unlock(&output->state_lock); ret = queue_work(output->crc_workq, &state->crc_work); if (!ret) - DRM_WARN("failed to queue vkms_crc_work_handle"); + DRM_DEBUG_DRIVER("vkms_crc_work_handle already queued\n"); } spin_unlock(&output->lock); diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h index 81f1cfbeb9362..3c7e06b19efd5 100644 --- a/drivers/gpu/drm/vkms/vkms_drv.h +++ b/drivers/gpu/drm/vkms/vkms_drv.h @@ -56,6 +56,8 @@ struct vkms_plane_state { struct vkms_crtc_state { struct drm_crtc_state base; struct work_struct crc_work; + + bool crc_pending; u64 frame_start; u64 frame_end; }; -- 2.20.1