Re: [PATCH v4 23/23] drm/msm: Don't implicit-sync if only a single ring

[Rob Clark]

On Thu, Oct 29, 2020 at 9:14 AM Daniel Vetter <daniel@xxxxxxxx> wrote:
>
> On Mon, Oct 26, 2020 at 10:34 AM Daniel Vetter <daniel@xxxxxxxx> 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@xxxxxxxxxxxxxx> wrote:
> > > >
> > > > On Fr, 2020-10-23 at 09:51 -0700, Rob Clark wrote:
> > > > > From: Rob Clark <robdclark@xxxxxxxxxxxx>
> > > > >
> > > > > 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

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

BR,
-R


> -Daniel
>
> > -Daniel
> >
> > >
> > > BR,
> > > -R
> > >
> > >
> > >
> > > >
> > > > Regards,
> > > > Lucas
> > > >
> > > > > Signed-off-by: Rob Clark <robdclark@xxxxxxxxxxxx>
> > > > > Reviewed-by: Kristian H. Kristensen <hoegsberg@xxxxxxxxxx>
> > > > > ---
> > > > >  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@xxxxxxxxxxxxxxxxxxxxx
> > > 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