Received: by 2002:a05:6a10:206:0:0:0:0 with SMTP id 6csp3731825pxj; Tue, 11 May 2021 10:40:12 -0700 (PDT) X-Google-Smtp-Source: ABdhPJx+u+VmPfbbt/RU7EK8idot0yzLLG/8GoR7OT8GIAWsAwJsFop6jii7ydWBg+rxOwdpMYbo X-Received: by 2002:a4a:a5c2:: with SMTP id k2mr24399376oom.5.1620754812405; Tue, 11 May 2021 10:40:12 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1620754812; cv=none; d=google.com; s=arc-20160816; b=sJAIShYzdg0wErUI3/rWOupEAnRwdAivlNzUJ/x7exObNP3GykZHCVkh2xTlmJQnkX zat+92kmZ/Gc4qmbU0OztKx7meHxYkdo2Ir/i5Y4pXf7+JFUiyUZEzANVOMVM+HVwVK4 ay8zgifT6bMIzFixQiA2Uuat36jp0aDNfC78A1oK5ITOuTecWRtwNJaW8MaZW06VvBC8 AxTOiKArOjXoluZC+wKmcacJlok3PylLGyWI0Z8SkvTrT53fn+sksET9ECf/lDaF/aZN LjFhQDmNncNEImTpeT8YWYhkAvzNvFv+43lw/rP0/hBdipO8g6Jj/XxCf5fiEz/OV29U LPqQ== 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=4Iip13muq1NYXw8ciJ4gZx7sLT2i59LHm3b1xhb3qro=; b=u71E2FFVRMVt0M1mRsx1TPWUdGbptQh4N7C/Up36rNOaxPVrvb+TWWYvyGUypQwYDe G8iIIe56CA86ZwP4QFouxuTRVViq2604N4e2ickvV8pKWm2Ou+r4qQ+Of9fPXPrzSCwj MUjjauPxQEdzzEw9F+M0LHJtRYEX0TQxpEY6j3iMOSYcrkrVtRDAlqUHHvx6+mWr0g6l XF9t5IU5CfsZp4tAjLDs1WStLLBpVA2PFCr8rwaOn4tqFr3gxGRClLahcbe2pQHzyU5l xaDb+zI3la5XFCq840RFWt06oYvxvEJ/M3Fl8HEa2tciZ2ul7hUTtzRrkR/EhvBtpZfV Wo7g== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b="l0I/l9ir"; 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 f25si19158561otp.132.2021.05.11.10.39.59; Tue, 11 May 2021 10:40:12 -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="l0I/l9ir"; 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 S231824AbhEKRk3 (ORCPT + 99 others); Tue, 11 May 2021 13:40:29 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:35704 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231329AbhEKRk2 (ORCPT ); Tue, 11 May 2021 13:40:28 -0400 Received: from mail-wr1-x42c.google.com (mail-wr1-x42c.google.com [IPv6:2a00:1450:4864:20::42c]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id A8F47C06174A for ; Tue, 11 May 2021 10:39:18 -0700 (PDT) Received: by mail-wr1-x42c.google.com with SMTP id t18so20987229wry.1 for ; Tue, 11 May 2021 10:39:18 -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=4Iip13muq1NYXw8ciJ4gZx7sLT2i59LHm3b1xhb3qro=; b=l0I/l9irc0L33+3w+vShxZyAO9/CKiQR0T+VECzaTKU4TJFiZJMNe0Csx99AWQKMQT GW7fxShroooRhKQ1FFVZuV63BkI25ij7VJ0n78MYAWEbPu5dounOLMpogEsKo7yUJaor jNFx+ClrGCFK3ycq0eJevgyLSsunK9jbkjFY7hr4FwAhlB/S1kSp6X2QWmscwA3Bnqed qpnisUHdd1yLawQxLD7EAukbXIdDimWGCKMpqOJYgECzqnpVLH6TmUzIaDlZiihYQQgp z1DU/uJV9GO16Qmls0Q7TNyRGE5rVkjgFa6+6FtJYkyCBq1/8xdIRcNZEEVS3mcjQpt3 36Ww== 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=4Iip13muq1NYXw8ciJ4gZx7sLT2i59LHm3b1xhb3qro=; b=AWh5UCOVL8kLyYnOqC1NkS7dERKpIahIh2jQ7PGIRf9vHHNCTS65ZdBak7AoGtSv/h saYATrUJzDoYKDw/ieEk3HXz/W82yfHS1EjijlV1NvvAsZavXRsyu2nqNi6avDJXknDH JlYOyhO4mWc54ACJ1dpaMM9BqkS0bhwBzWSA/aNPv1qHnWk8A734aNg4e0tOiBe9wffI EZtb695mM/dsZZEpQAk+VjnPrF16uYJY1Ph9rTglJ9L8Q6qAV1ghh94pJ0MzlTPvdBrw 95V2KgyLByHSyuKEiTpGjNAYIHPiZElSS+Ofk71+g9gJK1khqTQxY+Nq1hafAvrd9s71 kn6g== X-Gm-Message-State: AOAM533JE1uynsKL+RV+jxK26xtko3kRnHCb5mijyp5OKY9FJU1uw6Co 9gJMGcNHtfbJvLrKpXTsOPFMbSdxaaDQMJwARNQ= X-Received: by 2002:a5d:64cf:: with SMTP id f15mr38403209wri.327.1620754757270; Tue, 11 May 2021 10:39:17 -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:42:58 -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 10:21 AM Daniel Vetter wrote: > > On Tue, May 11, 2021 at 10:19:57AM -0700, Rob Clark wrote: > > 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.. > > Nope, that's not what I mean. > > By default we pile up the updates, so you _never_ stall. The worker then > takes the entire update every time it runs and batches them up. > > We _only_ stall when we get a dirtyfb with a different fb. Because that's > much harder to pile up, plus frontbuffer rendering userspace uses a single > fb across all screens anyway. > > So really I don't expect X to ever stall in it's BlockHandler with this. ok, sorry, I missed the "different fb" part.. but I could see a userspace that uses multiple fb's wanting to do front buffer rendering.. although they are probably only going to do it on a single display at a time, so maybe that is a bit of an edge case > > 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. > > Yeah, but we'd only stall if userspace does a modeset (which means > different fb) and at that point you'll stall anyway a bit. So shouldn't > hurt. > > Well you can do frontbuffer rendering even with atomic ioctl. Just don't > use dirtyfb. > > But also you really shouldn't use frontbuffer rendering right now, since > we don't have the interfaces right now to tell userspace whether it's > cmd-mode or something else and what kind of corruption (if any) to expect > when they do that. Compressed formats and front-buffer rendering don't really work out in a pleasant way.. minigbm has a usage flag to indicate that the surface will be used for front-buffer rendering (and it is a thing we should probably port to real gbm). I think this aspect of it is better solved in userspace. > > > > > > 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 > > Yeah that's what I mean. Except implemented cheaper. fbcon code already > does it. I think we're seriously talking past each another. Hmm, well 'state->async_update = true' is a pretty cheap implementation.. BR, -R > -Daniel > > > > > 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 > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch