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;
MIME-Version: 1.0
References: <20210519183855.1523927-1-robdclark@gmail.com> <20210519183855.1523927-3-robdclark@gmail.com>
 <YKaOY3AWgHh5kplS@phenom.ffwll.local>
In-Reply-To: <YKaOY3AWgHh5kplS@phenom.ffwll.local>
From:   Rob Clark <robdclark@gmail.com>
Date:   Sun, 30 May 2021 07:33:57 -0700
Message-ID: <CAF6AEGv470U7fujLrJOE8fJh1o-BW3=mOpKJ45FFz=Xb8Q0D6A@mail.gmail.com>
Subject: Re: [RFC 2/3] drm/atomic: Call dma_fence_boost() when we've missed a vblank
To:     Rob Clark <robdclark@gmail.com>,
        dri-devel <dri-devel@lists.freedesktop.org>,
        freedreno <freedreno@lists.freedesktop.org>,
        linux-arm-msm <linux-arm-msm@vger.kernel.org>,
        Rob Clark <robdclark@chromium.org>,
        Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
        Maxime Ripard <mripard@kernel.org>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        David Airlie <airlied@linux.ie>,
        open list <linux-kernel@vger.kernel.org>,
        Matthew Brost <matthew.brost@intel.com>
Cc:     Daniel Vetter <daniel@ffwll.ch>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, May 20, 2021 at 9:29 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Wed, May 19, 2021 at 11:38:53AM -0700, Rob Clark wrote:
> > From: Rob Clark <robdclark@chromium.org>
> >
> > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > ---
> >  drivers/gpu/drm/drm_atomic_helper.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index 560aaecba31b..fe10fc2e7f86 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -1435,11 +1435,15 @@ int drm_atomic_helper_wait_for_fences(struct drm_device *dev,
> >       int i, ret;
> >
> >       for_each_new_plane_in_state(state, plane, new_plane_state, i) {
> > +             u64 vblank_count;
> > +
> >               if (!new_plane_state->fence)
> >                       continue;
> >
> >               WARN_ON(!new_plane_state->fb);
> >
> > +             vblank_count = drm_crtc_vblank_count(new_plane_state->crtc);
> > +
> >               /*
> >                * If waiting for fences pre-swap (ie: nonblock), userspace can
> >                * still interrupt the operation. Instead of blocking until the
> > @@ -1449,6 +1453,13 @@ int drm_atomic_helper_wait_for_fences(struct drm_device *dev,
> >               if (ret)
> >                       return ret;
> >
> > +             /*
> > +              * Check if we've missed a vblank while waiting, and if we have
> > +              * signal the fence that it's signaler should be boosted
> > +              */
> > +             if (vblank_count != drm_crtc_vblank_count(new_plane_state->crtc))
> > +                     dma_fence_boost(new_plane_state->fence);
>
> I think we should do a lot better here:
> - maybe only bother doing this for single-crtc updates, and only if
>   modeset isn't set. No one else cares about latency.
>
> - We should boost _right_ when we've missed the frame, so I think we
>   should have a _timeout wait here that guesstimates when the vblank is
>   over (might need to throw in a vblank wait if we missed) and then boost
>   immediately. Not wait a bunch of frames (worst case) until we finally
>   decide to boost.

I was thinking about this a bit more.. How about rather than calling
some fence->op->boost() type thing when we are about to miss a vblank
(IMO that is also already too late), we do something more like
fence->ops->set_deadline() before we even wait?

It's probably a bit impossible for a gpu driver to really predict how
long some rendering will take, but other cases like video decoder are
somewhat more predictable.. the fence provider could predict given the
remaining time until the deadline what clk rates are required to get
you there.

BR,
-R


>
> Otherwise I really like this, I think it's about the only real reason i915
> isn't using atomic helpers.
>
> Also adding Matt B for this topic.
> -Daniel
>
> > +
> >               dma_fence_put(new_plane_state->fence);
> >               new_plane_state->fence = NULL;
> >       }
> > --
> > 2.30.2
> >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch