Received: by 2002:a05:6a10:206:0:0:0:0 with SMTP id 6csp3717981pxj; Tue, 11 May 2021 10:20:54 -0700 (PDT) X-Google-Smtp-Source: ABdhPJxdQ7wyVYdaS6mgd13hCydCGzxRj/dsDlPx82ru3ZtBHhvj3F8/btSbYYzVszSjX8Rihjq7 X-Received: by 2002:a19:e215:: with SMTP id z21mr21835663lfg.658.1620753653794; Tue, 11 May 2021 10:20:53 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1620753653; cv=none; d=google.com; s=arc-20160816; b=VmqZHNcRX/MWntvBD4GejEVphpNY0zLCi9agU0S6UPFUkj+lEMm1UBa8sToKIKgau8 vimBFFCqc9YGrAvJBRNeUmyovkPrIQ/NTroDOTJlfAXl5/DdpuJ+RAN1MKec1oEIN3cz 1/eRr8jw8Hj3m1dGvL6WdesOBLSORSUXd14X+ljVu+8nO58EhZJ5suA3/hNUJ6eUH36R 2M0OQeXjxDA1MxStj9XifZoKEq6VLcXUYWfV8xFWumCXaF0Mbn3AeD609BJDqszuVxi7 Tm2JrFMLUitH1ny8/Eoy6SyDyUFjpt+S/EROCeCCnpWPu+M9HxksWZOqKU1JiU6GjHjE 4DRg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:subject:message-id:date:from:in-reply-to :references:mime-version:dkim-signature; bh=AmLyBAMDRBCsOH75F4t0O+Ip6nFyT0ALhZj16/9/cbg=; b=0/2N7PlaMiz3HHVqp05cehlWTmHcgq03/limjgzfWbGp/u46TnT3PQRGsaV9YVDBBS 29363H0vXyDOkuN9Ub+ri9/IptLOMhWysFWoMyQi+8+a3N1igYeelw44LT4QXQU3M8Lo /zjxA9NjyJ+nuPr3L2KWtsMK9SUA0ZKWIXpXqEOabyUc9xyPrZkbGZrqm1hyh9YMc4Zj B74TL8u0j179ypnSw1FX2pP21kn45bw2PSyWcsK7iEmKgphDC1QZEHksPb0ibDb2pqHt xI7qXYUpw7/D2ugYx6sidPUmVt7GHz0i4+ISLkliX3qQxqrrn745WuNFFn0NAlzksWy8 bSDA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=lBMFHEpy; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id b15si22042548lfe.460.2021.05.11.10.20.23; Tue, 11 May 2021 10:20:53 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=lBMFHEpy; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231551AbhEKRRY (ORCPT + 99 others); Tue, 11 May 2021 13:17:24 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:58690 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231407AbhEKRRY (ORCPT ); Tue, 11 May 2021 13:17:24 -0400 Received: from mail-wr1-x431.google.com (mail-wr1-x431.google.com [IPv6:2a00:1450:4864:20::431]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 6921AC06175F for ; Tue, 11 May 2021 10:16:17 -0700 (PDT) Received: by mail-wr1-x431.google.com with SMTP id v12so20891844wrq.6 for ; Tue, 11 May 2021 10:16:17 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=AmLyBAMDRBCsOH75F4t0O+Ip6nFyT0ALhZj16/9/cbg=; b=lBMFHEpyDx5Es9IsAK8IqK+k6lqgUlFGUikZqCu77qdqcGSi9glrZrMHv+O7PxlQY2 iWWt3EZv9O7yXHCMFqINeBvh50iQuVeSnckBoP4B9g+VWlN8TcRz1SMYwVYpKLyb538U c4Itipnh5LLnjNnGTH6qra463NToWcKARKCBWIm9pDAjGa3hAjNxuzpqU4Zz7ij+Ss8U lpIuZb+MYzvCRv/CBUakohbfhg4z2nL1tU2u6F5yINU3imyNJxfTadaHb+SRbRzuaL8E A8ruXmYCvaGg8mpZR9XxXnjiJ4TXnW3Dm3ROlWYCZsqqNS38WBve9K3yypNPDBCd545D Z9FA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=AmLyBAMDRBCsOH75F4t0O+Ip6nFyT0ALhZj16/9/cbg=; b=ay6HobrA1LBaJrKOf+99VEEEvyUmcP66KF/PXvNtNV4BEyyl1p0LAe0epeg3/Mz3by B7vcEDjxwho+ANzLVpf3A774NreHDtwjD5TJWVTjzSP9Ric4ge+TrbdCZdw4eH1yaqiy 0DbRezsqvvEswvNa7J/ka2SiF/xgQZVuzNaHpv8VnGxdvhanYhDEE5jY94dbJtHlpyEI H0a5Fz5CnMIfciO3LFkdOhxb2H8qwIFl1MfV4bvaEJblfr2Rwd1v8sdqvS+qx+Xc0c9n 8bjeFcIbGBOzZ2xBQ6NsSfNRVdnhE2uXodmt6DDu8f94um1qWWifzoGmgnhHDTxFL0gL RApw== X-Gm-Message-State: AOAM531MqATibiGNiozc533XGnmcAy2KlyKLkv03PespFrsmN+nErn3V bl+Bt0Rd1xSOAcaLhcVTR0krWmL0VDgY9JokPFg= X-Received: by 2002:a5d:5351:: with SMTP id t17mr39641067wrv.83.1620753375601; Tue, 11 May 2021 10:16:15 -0700 (PDT) MIME-Version: 1.0 References: <20210508195641.397198-1-robdclark@gmail.com> <20210508195641.397198-2-robdclark@gmail.com> In-Reply-To: From: Rob Clark Date: Tue, 11 May 2021 10:19:57 -0700 Message-ID: Subject: Re: [PATCH 1/2] drm: Fix dirtyfb stalls To: Rob Clark , dri-devel , Rob Clark , Maarten Lankhorst , Maxime Ripard , Thomas Zimmermann , David Airlie , open list Cc: Daniel Vetter Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, May 11, 2021 at 9:44 AM Daniel Vetter wrote: > > On Mon, May 10, 2021 at 12:06:05PM -0700, Rob Clark wrote: > > On Mon, May 10, 2021 at 10:44 AM Daniel Vetter wrote: > > > > > > On Mon, May 10, 2021 at 6:51 PM Rob Clark wrote: > > > > > > > > On Mon, May 10, 2021 at 9:14 AM Daniel Vetter wrote: > > > > > > > > > > On Sat, May 08, 2021 at 12:56:38PM -0700, Rob Clark wrote: > > > > > > From: Rob Clark > > > > > > > > > > > > drm_atomic_helper_dirtyfb() will end up stalling for vblank on "video > > > > > > mode" type displays, which is pointless and unnecessary. Add an > > > > > > optional helper vfunc to determine if a plane is attached to a CRTC > > > > > > that actually needs dirtyfb, and skip over them. > > > > > > > > > > > > Signed-off-by: Rob Clark > > > > > > > > > > So this is a bit annoying because the idea of all these "remap legacy uapi > > > > > to atomic constructs" helpers is that they shouldn't need/use anything > > > > > beyond what userspace also has available. So adding hacks for them feels > > > > > really bad. > > > > > > > > I suppose the root problem is that userspace doesn't know if dirtyfb > > > > (or similar) is actually required or is a no-op. > > > > > > > > But it is perhaps less of a problem because this essentially boils > > > > down to "x11 vs wayland", and it seems like wayland compositors for > > > > non-vsync'd rendering just pageflips and throws away extra frames from > > > > the app? > > > > > > Yeah it's about not adequately batching up rendering and syncing with > > > hw. bare metal x11 is just especially stupid about it :-) > > > > > > > > Also I feel like it's not entirely the right thing to do here either. > > > > > We've had this problem already on the fbcon emulation side (which also > > > > > shouldn't be able to peek behind the atomic kms uapi curtain), and the fix > > > > > there was to have a worker which batches up all the updates and avoids any > > > > > stalls in bad places. > > > > > > > > I'm not too worried about fbcon not being able to render faster than > > > > vblank. OTOH it is a pretty big problem for x11 > > > > > > That's why we'd let the worker get ahead at most one dirtyfb. We do > > > the same with fbcon, which trivially can get ahead of vblank otherwise > > > (if sometimes flushes each character, so you have to pile them up into > > > a single update if that's still pending). > > > > > > > > Since this is for frontbuffer rendering userspace only we can probably get > > > > > away with assuming there's only a single fb, so the implementation becomes > > > > > pretty simple: > > > > > > > > > > - 1 worker, and we keep track of a single pending fb > > > > > - if there's already a dirty fb pending on a different fb, we stall for > > > > > the worker to start processing that one already (i.e. the fb we track is > > > > > reset to NULL) > > > > > - if it's pending on the same fb we just toss away all the updates and go > > > > > with a full update, since merging the clip rects is too much work :-) I > > > > > think there's helpers so you could be slightly more clever and just have > > > > > an overall bounding box > > > > > > > > This doesn't really fix the problem, you still end up delaying sending > > > > the next back-buffer to mesa > > > > > > With this the dirtyfb would never block. Also glorious frontbuffer > > > tracking corruption is possible, but that's not the kernel's problem. > > > So how would anything get held up in userspace. > > > > the part about stalling if a dirtyfb is pending was what I was worried > > about.. but I suppose you meant the worker stalling, rather than > > userspace stalling (where I had interpreted it the other way around). > > As soon as userspace needs to stall, you're losing again. > > Nah, I did mean userspace stalling, so we can't pile up unlimited amounts > of dirtyfb request in the kernel. > > But also I never expect userspace that uses dirtyfb to actually hit this > stall point (otherwise we'd need to look at this again). It would really > be only there as defense against abuse. I don't believe modesetting ddx throttles dirtyfb, it (indirectly) calls this from it's BlockHandler.. so if you do end up blocking after the N'th dirtyfb, you are still going to end up stalling for vblank, you are just deferring that for a frame or two.. The thing is, for a push style panel, you don't necessarily have to wait for "vblank" (because "vblank" isn't necessarily a real thing), so in that scenario dirtyfb could in theory be fast. What you want to do is fundamentally different for push vs pull style displays. > > > > But we could re-work drm_framebuffer_funcs::dirty to operate on a > > > > per-crtc basis and hoist the loop and check if dirtyfb is needed out > > > > of drm_atomic_helper_dirtyfb() > > > > > > That's still using information that userspace doesn't have, which is a > > > bit irky. We might as well go with your thing here then. > > > > arguably, this is something we should expose to userspace.. for DSI > > command-mode panels, you probably want to make a different decision > > with regard to how many buffers in your flip-chain.. > > > > Possibly we should add/remove the fb_damage_clips property depending > > on the display type (ie. video/pull vs cmd/push mode)? > > I'm not sure whether atomic actually needs this exposed: > - clients will do full flips for every frame anyway, I've not heard of > anyone seriously doing frontbuffer rendering. Frontbuffer rendering is actually a thing, for ex. to reduce latency for stylus (android and CrOS do this.. fortunately AFAICT CrOS never uses the dirtyfb ioctl.. but as soon as someone has the nice idea to add that we'd be running into the same problem) Possibly one idea is to treat dirty-clip updates similarly to cursor updates, and let the driver accumulate the updates and then wait until vblank to apply them BR, -R > - transporting the cliprects around and then tossing them if the driver > doesn't need them in their flip is probably not a measurable win > > But yeah if I'm wrong and we have a need here and it's useful, then > exposing this to userspace should be done. Meanwhile I think a "offload to > worker like fbcon" trick for this legacy interface is probabyl the best > option. Plus it will fix things not just for the case where you don't need > dirty uploading, it will also fix things for the case where you _do_ need > dirty uploading (since right now we stall in a few bad places for that I > think). > -Daniel > > > > > BR, > > -R > > > > > -Daniel > > > > > > > BR, > > > > -R > > > > > > > > > > > > > > Could probably steal most of the implementation. > > > > > > > > > > This approach here feels a tad too much in the hacky area ... > > > > > > > > > > Thoughts? > > > > > -Daniel > > > > > > > > > > > --- > > > > > > drivers/gpu/drm/drm_damage_helper.c | 8 ++++++++ > > > > > > include/drm/drm_modeset_helper_vtables.h | 14 ++++++++++++++ > > > > > > 2 files changed, 22 insertions(+) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/drm_damage_helper.c b/drivers/gpu/drm/drm_damage_helper.c > > > > > > index 3a4126dc2520..a0bed1a2c2dc 100644 > > > > > > --- a/drivers/gpu/drm/drm_damage_helper.c > > > > > > +++ b/drivers/gpu/drm/drm_damage_helper.c > > > > > > @@ -211,6 +211,7 @@ int drm_atomic_helper_dirtyfb(struct drm_framebuffer *fb, > > > > > > retry: > > > > > > drm_for_each_plane(plane, fb->dev) { > > > > > > struct drm_plane_state *plane_state; > > > > > > + struct drm_crtc *crtc; > > > > > > > > > > > > ret = drm_modeset_lock(&plane->mutex, state->acquire_ctx); > > > > > > if (ret) > > > > > > @@ -221,6 +222,13 @@ int drm_atomic_helper_dirtyfb(struct drm_framebuffer *fb, > > > > > > continue; > > > > > > } > > > > > > > > > > > > + crtc = plane->state->crtc; > > > > > > + if (crtc->helper_private->needs_dirtyfb && > > > > > > + !crtc->helper_private->needs_dirtyfb(crtc)) { > > > > > > + drm_modeset_unlock(&plane->mutex); > > > > > > + continue; > > > > > > + } > > > > > > + > > > > > > plane_state = drm_atomic_get_plane_state(state, plane); > > > > > > if (IS_ERR(plane_state)) { > > > > > > ret = PTR_ERR(plane_state); > > > > > > diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h > > > > > > index eb706342861d..afa8ec5754e7 100644 > > > > > > --- a/include/drm/drm_modeset_helper_vtables.h > > > > > > +++ b/include/drm/drm_modeset_helper_vtables.h > > > > > > @@ -487,6 +487,20 @@ struct drm_crtc_helper_funcs { > > > > > > bool in_vblank_irq, int *vpos, int *hpos, > > > > > > ktime_t *stime, ktime_t *etime, > > > > > > const struct drm_display_mode *mode); > > > > > > + > > > > > > + /** > > > > > > + * @needs_dirtyfb > > > > > > + * > > > > > > + * Optional callback used by damage helpers to determine if fb_damage_clips > > > > > > + * update is needed. > > > > > > + * > > > > > > + * Returns: > > > > > > + * > > > > > > + * True if fb_damage_clips update is needed to handle DIRTYFB, False > > > > > > + * otherwise. If this callback is not implemented, then True is > > > > > > + * assumed. > > > > > > + */ > > > > > > + bool (*needs_dirtyfb)(struct drm_crtc *crtc); > > > > > > }; > > > > > > > > > > > > /** > > > > > > -- > > > > > > 2.30.2 > > > > > > > > > > > > > > > > -- > > > > > Daniel Vetter > > > > > Software Engineer, Intel Corporation > > > > > http://blog.ffwll.ch > > > > > > > > > > > > -- > > > Daniel Vetter > > > Software Engineer, Intel Corporation > > > http://blog.ffwll.ch > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch