| Message ID | 20170225154330.28247-1-robdclark@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Andy Gross |
| Headers | show |
On Sat, Feb 25, 2017 at 10:43 AM, Rob Clark <robdclark@gmail.com> wrote: > Probably a symptom of needing finer grained locking, but if we wait on > the incoming fence-fd (which could come from a different context) while > holding struct_mutex, that blocks retire_worker so gpu fences cannot get > scheduled. s/scheduled/signalled/ > This causes a problem if userspace manages to get more than a frame > ahead, leaving the atomic-commit worker blocked waiting on fences that > cannot be signaled because submit is blocked waiting for a fence > signalled from vblank (after the atomic commit which is blocked). > > If we start having multiple fence ctxs for the gpu, submit_fence_sync() > would probably need to move outside of struct_mutex as well. > > Signed-off-by: Rob Clark <robdclark@gmail.com> > --- > drivers/gpu/drm/msm/msm_gem_submit.c | 39 +++++++++++++++++------------------- > 1 file changed, 18 insertions(+), 21 deletions(-) > > diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c > index 1172fe7..1c545eb 100644 > --- a/drivers/gpu/drm/msm/msm_gem_submit.c > +++ b/drivers/gpu/drm/msm/msm_gem_submit.c > @@ -404,6 +404,24 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data, > if (MSM_PIPE_FLAGS(args->flags) & ~MSM_SUBMIT_FLAGS) > return -EINVAL; > > + if (args->flags & MSM_SUBMIT_FENCE_FD_IN) { > + in_fence = sync_file_get_fence(args->fence_fd); > + > + if (!in_fence) > + return -EINVAL; > + > + /* TODO if we get an array-fence due to userspace merging multiple > + * fences, we need a way to determine if all the backing fences > + * are from our own context.. > + */ > + > + if (in_fence->context != gpu->fctx->context) { > + ret = dma_fence_wait(in_fence, true); > + if (ret) > + return ret; > + } > + } > + > ret = mutex_lock_interruptible(&dev->struct_mutex); > if (ret) > return ret; > @@ -431,27 +449,6 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data, > if (ret) > goto out; > > - if (args->flags & MSM_SUBMIT_FENCE_FD_IN) { > - in_fence = sync_file_get_fence(args->fence_fd); > - > - if (!in_fence) { > - ret = -EINVAL; > - goto out; > - } > - > - /* TODO if we get an array-fence due to userspace merging multiple > - * fences, we need a way to determine if all the backing fences > - * are from our own context.. > - */ > - > - if (in_fence->context != gpu->fctx->context) { > - ret = dma_fence_wait(in_fence, true); > - if (ret) > - goto out; > - } > - > - } > - > if (!(args->fence & MSM_SUBMIT_NO_IMPLICIT)) { > ret = submit_fence_sync(submit); > if (ret) > -- > 2.9.3 > -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Feb 25, 2017 at 4:43 PM, Rob Clark <robdclark@gmail.com> wrote: > Probably a symptom of needing finer grained locking, but if we wait on > the incoming fence-fd (which could come from a different context) while > holding struct_mutex, that blocks retire_worker so gpu fences cannot get > scheduled. > > This causes a problem if userspace manages to get more than a frame > ahead, leaving the atomic-commit worker blocked waiting on fences that > cannot be signaled because submit is blocked waiting for a fence > signalled from vblank (after the atomic commit which is blocked). > > If we start having multiple fence ctxs for the gpu, submit_fence_sync() > would probably need to move outside of struct_mutex as well. > > Signed-off-by: Rob Clark <robdclark@gmail.com> It's unfortunately not yet landed, but as soon as it's there I think we should annotate fences using the cross-release stuff that's cooking: https://lwn.net/Articles/708628/ That should be able to catch deps between waiter/signaller vs. locking. -Daniel
On Sat, Feb 25, 2017 at 12:36 PM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Sat, Feb 25, 2017 at 4:43 PM, Rob Clark <robdclark@gmail.com> wrote: >> Probably a symptom of needing finer grained locking, but if we wait on >> the incoming fence-fd (which could come from a different context) while >> holding struct_mutex, that blocks retire_worker so gpu fences cannot get >> scheduled. >> >> This causes a problem if userspace manages to get more than a frame >> ahead, leaving the atomic-commit worker blocked waiting on fences that >> cannot be signaled because submit is blocked waiting for a fence >> signalled from vblank (after the atomic commit which is blocked). >> >> If we start having multiple fence ctxs for the gpu, submit_fence_sync() >> would probably need to move outside of struct_mutex as well. >> >> Signed-off-by: Rob Clark <robdclark@gmail.com> > > It's unfortunately not yet landed, but as soon as it's there I think > we should annotate fences using the cross-release stuff that's > cooking: > > https://lwn.net/Articles/708628/ > > That should be able to catch deps between waiter/signaller vs. locking. ohh, that would be spiffy BR, -R -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c index 1172fe7..1c545eb 100644 --- a/drivers/gpu/drm/msm/msm_gem_submit.c +++ b/drivers/gpu/drm/msm/msm_gem_submit.c @@ -404,6 +404,24 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data, if (MSM_PIPE_FLAGS(args->flags) & ~MSM_SUBMIT_FLAGS) return -EINVAL; + if (args->flags & MSM_SUBMIT_FENCE_FD_IN) { + in_fence = sync_file_get_fence(args->fence_fd); + + if (!in_fence) + return -EINVAL; + + /* TODO if we get an array-fence due to userspace merging multiple + * fences, we need a way to determine if all the backing fences + * are from our own context.. + */ + + if (in_fence->context != gpu->fctx->context) { + ret = dma_fence_wait(in_fence, true); + if (ret) + return ret; + } + } + ret = mutex_lock_interruptible(&dev->struct_mutex); if (ret) return ret; @@ -431,27 +449,6 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data, if (ret) goto out; - if (args->flags & MSM_SUBMIT_FENCE_FD_IN) { - in_fence = sync_file_get_fence(args->fence_fd); - - if (!in_fence) { - ret = -EINVAL; - goto out; - } - - /* TODO if we get an array-fence due to userspace merging multiple - * fences, we need a way to determine if all the backing fences - * are from our own context.. - */ - - if (in_fence->context != gpu->fctx->context) { - ret = dma_fence_wait(in_fence, true); - if (ret) - goto out; - } - - } - if (!(args->fence & MSM_SUBMIT_NO_IMPLICIT)) { ret = submit_fence_sync(submit); if (ret)
Probably a symptom of needing finer grained locking, but if we wait on the incoming fence-fd (which could come from a different context) while holding struct_mutex, that blocks retire_worker so gpu fences cannot get scheduled. This causes a problem if userspace manages to get more than a frame ahead, leaving the atomic-commit worker blocked waiting on fences that cannot be signaled because submit is blocked waiting for a fence signalled from vblank (after the atomic commit which is blocked). If we start having multiple fence ctxs for the gpu, submit_fence_sync() would probably need to move outside of struct_mutex as well. Signed-off-by: Rob Clark <robdclark@gmail.com> --- drivers/gpu/drm/msm/msm_gem_submit.c | 39 +++++++++++++++++------------------- 1 file changed, 18 insertions(+), 21 deletions(-)