From patchwork Thu Jun 6 22:27:42 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Daniel Vetter X-Patchwork-Id: 10980517 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 1DFDE14B6 for ; Thu, 6 Jun 2019 22:28:09 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 0D3CC1FFE4 for ; Thu, 6 Jun 2019 22:28:09 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 00DE027EE2; Thu, 6 Jun 2019 22:28:08 +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 785531FFE4 for ; Thu, 6 Jun 2019 22:28:08 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 878FB899DC; Thu, 6 Jun 2019 22:28:05 +0000 (UTC) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mail-ed1-x543.google.com (mail-ed1-x543.google.com [IPv6:2a00:1450:4864:20::543]) by gabe.freedesktop.org (Postfix) with ESMTPS id 3C48B899DC for ; Thu, 6 Jun 2019 22:28:03 +0000 (UTC) Received: by mail-ed1-x543.google.com with SMTP id c26so5581347edt.1 for ; Thu, 06 Jun 2019 15:28:03 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=vQ7V/P5FFnDJXfv1tJb5nKMK43wXWwa030NP+k5b/b8=; b=mYXtL0J4zyKsAUqRkqpSMrF80er550Xt0w1mlDEiCAoH8Gs32B+ZOyF1FdH5u5U/ut ilGB7I2gckLztJgm4SVAhoYJXEhZeV5x9tfudv9t1PWEaq9smn9ipI4pUUPoJg46J3zf 1nQsf8eYXJeQXttac3lmDocfAj537/ErWsbY+spwQid7nP/eHrPJpTdG31WO+i5CwLe/ uJxCk82sZCm9rymxNU23JTFLj7aVItNM8iZk+zXM3JMALBOq9DvI1aWCGu71WwMvcDp4 N3qqU6AH1LjeBsy/Sscr6jw2UfMZ/sEjF03NTlHbZHpBlgBymr3u0u/LhwaX4k7A09MJ cbdQ== X-Gm-Message-State: APjAAAVhm2Egmfl4QfpDvGfIJLUEWTj5vuz8ZbKBY7dhVFDqeJL7YwME yHgg27WVLyEt63DywECbkZ/DtZTIRns= X-Google-Smtp-Source: APXvYqyrD16GZsSdZ/NZMC/tvezZJs7kY1WmL2Z9cu87PdvgMXKLb0wBZZXxLIAoDOQ/It5zaRZpFg== X-Received: by 2002:a50:90fa:: with SMTP id d55mr5690118eda.210.1559860081664; Thu, 06 Jun 2019 15:28:01 -0700 (PDT) Received: from phenom.ffwll.local ([2a02:168:569e:0:3106:d637:d723:e855]) by smtp.gmail.com with ESMTPSA id z10sm54228edl.35.2019.06.06.15.28.00 (version=TLS1_3 cipher=AEAD-AES256-GCM-SHA384 bits=256/256); Thu, 06 Jun 2019 15:28:00 -0700 (PDT) From: Daniel Vetter To: DRI Development Subject: [PATCH 01/10] drm/vkms: Fix crc worker races Date: Fri, 7 Jun 2019 00:27:42 +0200 Message-Id: <20190606222751.32567-2-daniel.vetter@ffwll.ch> X-Mailer: git-send-email 2.20.1 In-Reply-To: <20190606222751.32567-1-daniel.vetter@ffwll.ch> References: <20190606222751.32567-1-daniel.vetter@ffwll.ch> MIME-Version: 1.0 X-Mailman-Original-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ffwll.ch; s=google; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=vQ7V/P5FFnDJXfv1tJb5nKMK43wXWwa030NP+k5b/b8=; b=eUqULvArI92+oduO5cgeszXWdEb1c30IwlIwJButpacKENrO21kWq2YSJscOLcEJoY ILMX1wDq0YcX3P833JYdpPjWrv8ZZUC9UKvDhH/w8+By9U5Ivt3PElmDJLSGecRdCyI4 yZ/TSqrhGJ7N+kGVfYzwWhelWAVMuU23rXJQQ= 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: Haneen Mohammed , Rodrigo Siqueira , Daniel Vetter , Shayenne Moura , Daniel Vetter Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" X-Virus-Scanned: ClamAV using ClamSMTP 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 Signed-off-by: Daniel Vetter Cc: Haneen Mohammed Cc: Daniel Vetter --- 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 d7b409a3c0f8..66603da634fe 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 1bbe099b7db8..c727d8486e97 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 81f1cfbeb936..3c7e06b19efd 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; };