Message-ID: <20201029182256.GD401619@phenom.ffwll.local>
Date:   Thu, 29 Oct 2020 19:22:56 +0100
From:   Daniel Vetter <daniel@...ll.ch>
To:     Rob Clark <robdclark@...il.com>
Cc:     Daniel Vetter <daniel@...ll.ch>,
        Lucas Stach <l.stach@...gutronix.de>,
        Rob Clark <robdclark@...omium.org>,
        freedreno <freedreno@...ts.freedesktop.org>,
        David Airlie <airlied@...ux.ie>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        "Kristian H . Kristensen" <hoegsberg@...gle.com>,
        Sean Paul <sean@...rly.run>
Subject: Re: [PATCH v4 23/23] drm/msm: Don't implicit-sync if only a single
 ring

On Thu, Oct 29, 2020 at 09:59:09AM -0700, Rob Clark wrote:
> On Thu, Oct 29, 2020 at 9:14 AM Daniel Vetter <daniel@...ll.ch> wrote:
> >
> > On Mon, Oct 26, 2020 at 10:34 AM Daniel Vetter <daniel@...ll.ch> wrote:
> > >
> > > On Fri, Oct 23, 2020 at 08:49:14PM -0700, Rob Clark wrote:
> > > > On Fri, Oct 23, 2020 at 11:20 AM Lucas Stach <l.stach@...gutronix.de> wrote:
> > > > >
> > > > > On Fr, 2020-10-23 at 09:51 -0700, Rob Clark wrote:
> > > > > > From: Rob Clark <robdclark@...omium.org>
> > > > > >
> > > > > > If there is only a single ring (no-preemption), everything is FIFO order
> > > > > > and there is no need to implicit-sync.
> > > > > >
> > > > > > Mesa should probably just always use MSM_SUBMIT_NO_IMPLICIT, as behavior
> > > > > > is undefined when fences are not used to synchronize buffer usage across
> > > > > > contexts (which is the only case where multiple different priority rings
> > > > > > could come into play).
> > > > >
> > > > > Really, doesn't this break cross-device implicit sync? Okay, you may
> > > > > not have many peripherals that rely on implicit sync on devices where
> > > > > Adreno is usually found, but it seems rather heavy-handed.
> > > > >
> > > > > Wouldn't it be better to only ignore fences from your own ring context
> > > > > in the implicit sync, like we do in the common DRM scheduler
> > > > > (drm_sched_dependency_optimized)?
> > > >
> > > > we already do this.. as was discussed on an earlier iteration of this patchset
> > > >
> > > > But I'm not aware of any other non-gpu related implicit sync use-case
> > > > (even on imx devices where display is decoupled from gpu).. I'll
> > > > revert the patch if someone comes up with one, but otherwise lets let
> > > > the implicit sync baggage die
> > >
> > > The thing is, dma_resv won't die, even if implicit sync is dead. We're
> > > using internally for activity tracking and memory management. If you don't
> > > set these, then we can't share generic code with msm, and I think everyone
> > > inventing their own memory management is a bit a mistake.
> > >
> > > Now you only kill the implicit write sync stuff here, but I'm not sure
> > > that's worth much since you still install all the read fences for
> > > consistency. And if userspace doesn't want to be synced, they can set the
> > > flag and do this on their own: I think you should be able to achieve
> > > exactly the same thing in mesa.
> > >
> > > Aside: If you're worried about overhead, you can do O(1) submit if you
> > > manage your ppgtt like amdgpu does.
> >
> > So just remember a use-case which is maybe a bit yucky, but it is
> > actually possible to implement race-free. If you have implicit sync.
> >
> > There's screen-capture tool in mplayer and obs which capture your
> > compositor by running getfb2 in a loop. It works, and after some
> > initial screaming I realized it does actually work race-free. If you
> > have implicit sync.
> >
> > I really don't think you can sunset this, as much as you want to. And
> > sunsetting it inconsistently is probably the worst.
> 
> For the case where you only have a single ring, as long as it is
> importing the fb in to egl to read it (which it would need to do to
> get a linear view), this would still all work

Hm right we still have the implicit sync of the ringbuffer. At least until
you add a submit scheduler to msm ...

> (but I may drop this patch because it is just a micro-optimization and
> seems to cause more confusion)

Yeah I'd say without numbers to justify it it feels a bit on thin ice :-)
-Daniel

> 
> BR,
> -R
> 
> 
> > -Daniel
> >
> > > -Daniel
> > >
> > > >
> > > > BR,
> > > > -R
> > > >
> > > >
> > > >
> > > > >
> > > > > Regards,
> > > > > Lucas
> > > > >
> > > > > > Signed-off-by: Rob Clark <robdclark@...omium.org>
> > > > > > Reviewed-by: Kristian H. Kristensen <hoegsberg@...gle.com>
> > > > > > ---
> > > > > >  drivers/gpu/drm/msm/msm_gem_submit.c | 7 ++++---
> > > > > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
> > > > > > index d04c349d8112..b6babc7f9bb8 100644
> > > > > > --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> > > > > > +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> > > > > > @@ -283,7 +283,7 @@ static int submit_lock_objects(struct msm_gem_submit *submit)
> > > > > >       return ret;
> > > > > >  }
> > > > > >
> > > > > > -static int submit_fence_sync(struct msm_gem_submit *submit, bool no_implicit)
> > > > > > +static int submit_fence_sync(struct msm_gem_submit *submit, bool implicit_sync)
> > > > > >  {
> > > > > >       int i, ret = 0;
> > > > > >
> > > > > > @@ -303,7 +303,7 @@ static int submit_fence_sync(struct msm_gem_submit *submit, bool no_implicit)
> > > > > >                               return ret;
> > > > > >               }
> > > > > >
> > > > > > -             if (no_implicit)
> > > > > > +             if (!implicit_sync)
> > > > > >                       continue;
> > > > > >
> > > > > >               ret = msm_gem_sync_object(&msm_obj->base, submit->ring->fctx,
> > > > > > @@ -774,7 +774,8 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
> > > > > >       if (ret)
> > > > > >               goto out;
> > > > > >
> > > > > > -     ret = submit_fence_sync(submit, !!(args->flags & MSM_SUBMIT_NO_IMPLICIT));
> > > > > > +     ret = submit_fence_sync(submit, (gpu->nr_rings > 1) &&
> > > > > > +                     !(args->flags & MSM_SUBMIT_NO_IMPLICIT));
> > > > > >       if (ret)
> > > > > >               goto out;
> > > > > >
> > > > >
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > dri-devel@...ts.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > >
> > > --
> > > 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

Powered by blists - more mailing lists