| Message ID | 20201023165136.561680-24-robdclark@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | drm/msm: de-struct_mutex-ification | expand |
On Fr, 2020-10-23 at 09:51 -0700, Rob Clark wrote: > From: Rob Clark <robdclark@chromium.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)? Regards, Lucas > Signed-off-by: Rob Clark <robdclark@chromium.org> > Reviewed-by: Kristian H. Kristensen <hoegsberg@google.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; >
On Fri, Oct 23, 2020 at 11:20 AM Lucas Stach <l.stach@pengutronix.de> wrote: > > On Fr, 2020-10-23 at 09:51 -0700, Rob Clark wrote: > > From: Rob Clark <robdclark@chromium.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 BR, -R > > Regards, > Lucas > > > Signed-off-by: Rob Clark <robdclark@chromium.org> > > Reviewed-by: Kristian H. Kristensen <hoegsberg@google.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; > > >
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;