From: Rob Clark <robdclark@gmail.com>
To: Akhil P Oommen <akhilpo@codeaurora.org>
Cc: "Rob Clark" <robdclark@chromium.org>,
"David Airlie" <airlied@linux.ie>,
"open list:DRM DRIVER FOR MSM ADRENO GPU"
<linux-arm-msm@vger.kernel.org>,
"open list" <linux-kernel@vger.kernel.org>,
dri-devel <dri-devel@lists.freedesktop.org>,
"moderated list:DMA BUFFER SHARING FRAMEWORK"
<linaro-mm-sig@lists.linaro.org>, "Sean Paul" <sean@poorly.run>,
freedreno <freedreno@lists.freedesktop.org>,
"Christian König" <christian.koenig@amd.com>,
"open list:DMA BUFFER SHARING FRAMEWORK"
<linux-media@vger.kernel.org>
Subject: Re: [Freedreno] [PATCH v4 07/13] drm/msm: Track "seqno" fences by idr
Date: Thu, 11 Nov 2021 09:30:02 -0800 [thread overview]
Message-ID: <CAF6AEGsy1WZHOnuOgviFxW8y7yzmfGE4pWYzoAFXfxbS09m5pg@mail.gmail.com> (raw)
In-Reply-To: <7bc57359-0cf1-d657-f23e-e89404da6e91@codeaurora.org>
On Thu, Nov 11, 2021 at 7:54 AM Akhil P Oommen <akhilpo@codeaurora.org> wrote:
>
> On 11/10/2021 10:25 PM, Rob Clark wrote:
> > On Wed, Nov 10, 2021 at 7:28 AM Akhil P Oommen <akhilpo@codeaurora.org> wrote:
> >>
> >> On 7/28/2021 6:36 AM, Rob Clark wrote:
> >>> From: Rob Clark <robdclark@chromium.org>
> >>>
> >>> Previously the (non-fd) fence returned from submit ioctl was a raw
> >>> seqno, which is scoped to the ring. But from UABI standpoint, the
> >>> ioctls related to seqno fences all specify a submitqueue. We can
> >>> take advantage of that to replace the seqno fences with a cyclic idr
> >>> handle.
> >>>
> >>> This is in preperation for moving to drm scheduler, at which point
> >>> the submit ioctl will return after queuing the submit job to the
> >>> scheduler, but before the submit is written into the ring (and
> >>> therefore before a ring seqno has been assigned). Which means we
> >>> need to replace the dma_fence that userspace may need to wait on
> >>> with a scheduler fence.
> >>>
> >>> Signed-off-by: Rob Clark <robdclark@chromium.org>
> >>> Acked-by: Christian König <christian.koenig@amd.com>
> >>> ---
> >>> drivers/gpu/drm/msm/msm_drv.c | 30 +++++++++++++++++--
> >>> drivers/gpu/drm/msm/msm_fence.c | 42 ---------------------------
> >>> drivers/gpu/drm/msm/msm_fence.h | 3 --
> >>> drivers/gpu/drm/msm/msm_gem.h | 1 +
> >>> drivers/gpu/drm/msm/msm_gem_submit.c | 23 ++++++++++++++-
> >>> drivers/gpu/drm/msm/msm_gpu.h | 5 ++++
> >>> drivers/gpu/drm/msm/msm_submitqueue.c | 5 ++++
> >>> 7 files changed, 61 insertions(+), 48 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> >>> index 9b8fa2ad0d84..1594ae39d54f 100644
> >>> --- a/drivers/gpu/drm/msm/msm_drv.c
> >>> +++ b/drivers/gpu/drm/msm/msm_drv.c
> >>> @@ -911,6 +911,7 @@ static int msm_ioctl_wait_fence(struct drm_device *dev, void *data,
> >>> ktime_t timeout = to_ktime(args->timeout);
> >>> struct msm_gpu_submitqueue *queue;
> >>> struct msm_gpu *gpu = priv->gpu;
> >>> + struct dma_fence *fence;
> >>> int ret;
> >>>
> >>> if (args->pad) {
> >>> @@ -925,10 +926,35 @@ static int msm_ioctl_wait_fence(struct drm_device *dev, void *data,
> >>> if (!queue)
> >>> return -ENOENT;
> >>>
> >>> - ret = msm_wait_fence(gpu->rb[queue->prio]->fctx, args->fence, &timeout,
> >>> - true);
> >>> + /*
> >>> + * Map submitqueue scoped "seqno" (which is actually an idr key)
> >>> + * back to underlying dma-fence
> >>> + *
> >>> + * The fence is removed from the fence_idr when the submit is
> >>> + * retired, so if the fence is not found it means there is nothing
> >>> + * to wait for
> >>> + */
> >>> + ret = mutex_lock_interruptible(&queue->lock);
> >>> + if (ret)
> >>> + return ret;
> >>> + fence = idr_find(&queue->fence_idr, args->fence);
> >>> + if (fence)
> >>> + fence = dma_fence_get_rcu(fence);
> >>> + mutex_unlock(&queue->lock);
> >>> +
> >>> + if (!fence)
> >>> + return 0;
> >>>
> >>> + ret = dma_fence_wait_timeout(fence, true, timeout_to_jiffies(&timeout));
> >>> + if (ret == 0) {
> >>> + ret = -ETIMEDOUT;
> >>> + } else if (ret != -ERESTARTSYS) {
> >>> + ret = 0;
> >>> + }
> >>> +
> >>> + dma_fence_put(fence);
> >>> msm_submitqueue_put(queue);
> >>> +
> >>> return ret;
> >>> }
> >>>
> >>> diff --git a/drivers/gpu/drm/msm/msm_fence.c b/drivers/gpu/drm/msm/msm_fence.c
> >>> index b92a9091a1e2..f2cece542c3f 100644
> >>> --- a/drivers/gpu/drm/msm/msm_fence.c
> >>> +++ b/drivers/gpu/drm/msm/msm_fence.c
> >>> @@ -24,7 +24,6 @@ msm_fence_context_alloc(struct drm_device *dev, volatile uint32_t *fenceptr,
> >>> strncpy(fctx->name, name, sizeof(fctx->name));
> >>> fctx->context = dma_fence_context_alloc(1);
> >>> fctx->fenceptr = fenceptr;
> >>> - init_waitqueue_head(&fctx->event);
> >>> spin_lock_init(&fctx->spinlock);
> >>>
> >>> return fctx;
> >>> @@ -45,53 +44,12 @@ static inline bool fence_completed(struct msm_fence_context *fctx, uint32_t fenc
> >>> (int32_t)(*fctx->fenceptr - fence) >= 0;
> >>> }
> >>>
> >>> -/* legacy path for WAIT_FENCE ioctl: */
> >>> -int msm_wait_fence(struct msm_fence_context *fctx, uint32_t fence,
> >>> - ktime_t *timeout, bool interruptible)
> >>> -{
> >>> - int ret;
> >>> -
> >>> - if (fence > fctx->last_fence) {
> >>> - DRM_ERROR_RATELIMITED("%s: waiting on invalid fence: %u (of %u)\n",
> >>> - fctx->name, fence, fctx->last_fence);
> >>> - return -EINVAL;
> >>
> >> Rob, we changed this pre-existing behaviour in this patch. Now, when
> >> userspace tries to wait on a future fence, we don't return an error.
> >>
> >> I just want to check if this was accidental or not?
> >
> > Hmm, perhaps we should do this to restore the previous behavior:
> >
> > -------------
> > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> > index 73e827641024..3dd6da56eae6 100644
> > --- a/drivers/gpu/drm/msm/msm_drv.c
> > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > @@ -1000,8 +1000,12 @@ static int msm_ioctl_wait_fence(struct
> > drm_device *dev, void *data,
> > fence = dma_fence_get_rcu(fence);
> > mutex_unlock(&queue->lock);
> >
> > - if (!fence)
> > - return 0;
> > + if (!fence) {
> > + struct msm_fence_context *fctx = gpu->rb[queue->ring_nr]->fctx;
> > + DRM_ERROR_RATELIMITED("%s: waiting on invalid fence:
> > %u (of %u)\n",
> > + fctx->name, fence, fctx->last_fence);
> > + return -EINVAL;
> > + }
>
> With this, when userspace tries to wait on a fence which is already
> retired, it gets -EINVAL instead of success. Will this break userspace?
Oh, right, we definitely don't want that.. I guess that was the reason
for the original logic.
I have a different idea.. will send a patch in a bit.
BR,
-R
> -Akhil.
>
> >
> > ret = dma_fence_wait_timeout(fence, true, timeout_to_jiffies(&timeout));
> > if (ret == 0) {
> > -------------
> >
> > BR,
> > -R
> >
> >> -Akhil.
> >>
> >>> - }
> >>> -
> >>> - if (!timeout) {
> >>> - /* no-wait: */
> >>> - ret = fence_completed(fctx, fence) ? 0 : -EBUSY;
> >>> - } else {
> >>> - unsigned long remaining_jiffies = timeout_to_jiffies(timeout);
> >>> -
> >>> - if (interruptible)
> >>> - ret = wait_event_interruptible_timeout(fctx->event,
> >>> - fence_completed(fctx, fence),
> >>> - remaining_jiffies);
> >>> - else
> >>> - ret = wait_event_timeout(fctx->event,
> >>> - fence_completed(fctx, fence),
> >>> - remaining_jiffies);
> >>> -
> >>> - if (ret == 0) {
> >>> - DBG("timeout waiting for fence: %u (completed: %u)",
> >>> - fence, fctx->completed_fence);
> >>> - ret = -ETIMEDOUT;
> >>> - } else if (ret != -ERESTARTSYS) {
> >>> - ret = 0;
> >>> - }
> >>> - }
> >>> -
> >>> - return ret;
> >>> -}
> >>> -
> >>> /* called from workqueue */
> >>> void msm_update_fence(struct msm_fence_context *fctx, uint32_t fence)
> >>> {
> >>> spin_lock(&fctx->spinlock);
> >>> fctx->completed_fence = max(fence, fctx->completed_fence);
> >>> spin_unlock(&fctx->spinlock);
> >>> -
> >>> - wake_up_all(&fctx->event);
> >>> }
> >>>
> >>> struct msm_fence {
> >>> diff --git a/drivers/gpu/drm/msm/msm_fence.h b/drivers/gpu/drm/msm/msm_fence.h
> >>> index 6ab97062ff1a..4783db528bcc 100644
> >>> --- a/drivers/gpu/drm/msm/msm_fence.h
> >>> +++ b/drivers/gpu/drm/msm/msm_fence.h
> >>> @@ -49,7 +49,6 @@ struct msm_fence_context {
> >>> */
> >>> volatile uint32_t *fenceptr;
> >>>
> >>> - wait_queue_head_t event;
> >>> spinlock_t spinlock;
> >>> };
> >>>
> >>> @@ -57,8 +56,6 @@ struct msm_fence_context * msm_fence_context_alloc(struct drm_device *dev,
> >>> volatile uint32_t *fenceptr, const char *name);
> >>> void msm_fence_context_free(struct msm_fence_context *fctx);
> >>>
> >>> -int msm_wait_fence(struct msm_fence_context *fctx, uint32_t fence,
> >>> - ktime_t *timeout, bool interruptible);
> >>> void msm_update_fence(struct msm_fence_context *fctx, uint32_t fence);
> >>>
> >>> struct dma_fence * msm_fence_alloc(struct msm_fence_context *fctx);
> >>> diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
> >>> index da3af702a6c8..e0579abda5b9 100644
> >>> --- a/drivers/gpu/drm/msm/msm_gem.h
> >>> +++ b/drivers/gpu/drm/msm/msm_gem.h
> >>> @@ -320,6 +320,7 @@ struct msm_gem_submit {
> >>> struct ww_acquire_ctx ticket;
> >>> uint32_t seqno; /* Sequence number of the submit on the ring */
> >>> struct dma_fence *fence;
> >>> + int fence_id; /* key into queue->fence_idr */
> >>> struct msm_gpu_submitqueue *queue;
> >>> struct pid *pid; /* submitting process */
> >>> bool fault_dumped; /* Limit devcoredump dumping to one per submit */
> >>> diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
> >>> index 4f02fa3c78f9..f6f595aae2c5 100644
> >>> --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> >>> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> >>> @@ -68,7 +68,14 @@ void __msm_gem_submit_destroy(struct kref *kref)
> >>> container_of(kref, struct msm_gem_submit, ref);
> >>> unsigned i;
> >>>
> >>> + if (submit->fence_id) {
> >>> + mutex_lock(&submit->queue->lock);
> >>> + idr_remove(&submit->queue->fence_idr, submit->fence_id);
> >>> + mutex_unlock(&submit->queue->lock);
> >>> + }
> >>> +
> >>> dma_fence_put(submit->fence);
> >>> +
> >>> put_pid(submit->pid);
> >>> msm_submitqueue_put(submit->queue);
> >>>
> >>> @@ -872,6 +879,20 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
> >>> goto out;
> >>> }
> >>>
> >>> + /*
> >>> + * Allocate an id which can be used by WAIT_FENCE ioctl to map back
> >>> + * to the underlying fence.
> >>> + */
> >>> + mutex_lock(&queue->lock);
> >>> + submit->fence_id = idr_alloc_cyclic(&queue->fence_idr,
> >>> + submit->fence, 0, INT_MAX, GFP_KERNEL);
> >>> + mutex_unlock(&queue->lock);
> >>> + if (submit->fence_id < 0) {
> >>> + ret = submit->fence_id = 0;
> >>> + submit->fence_id = 0;
> >>> + goto out;
> >>> + }
> >>> +
> >>> if (args->flags & MSM_SUBMIT_FENCE_FD_OUT) {
> >>> struct sync_file *sync_file = sync_file_create(submit->fence);
> >>> if (!sync_file) {
> >>> @@ -886,7 +907,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
> >>>
> >>> msm_gpu_submit(gpu, submit);
> >>>
> >>> - args->fence = submit->fence->seqno;
> >>> + args->fence = submit->fence_id;
> >>>
> >>> msm_reset_syncobjs(syncobjs_to_reset, args->nr_in_syncobjs);
> >>> msm_process_post_deps(post_deps, args->nr_out_syncobjs,
> >>> diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
> >>> index 96efcb31e502..579627252540 100644
> >>> --- a/drivers/gpu/drm/msm/msm_gpu.h
> >>> +++ b/drivers/gpu/drm/msm/msm_gpu.h
> >>> @@ -263,6 +263,9 @@ struct msm_gpu_perfcntr {
> >>> * which set of pgtables do submits jobs associated with the
> >>> * submitqueue use)
> >>> * @node: node in the context's list of submitqueues
> >>> + * @fence_idr: maps fence-id to dma_fence for userspace visible fence
> >>> + * seqno, protected by submitqueue lock
> >>> + * @lock: submitqueue lock
> >>> * @ref: reference count
> >>> */
> >>> struct msm_gpu_submitqueue {
> >>> @@ -272,6 +275,8 @@ struct msm_gpu_submitqueue {
> >>> int faults;
> >>> struct msm_file_private *ctx;
> >>> struct list_head node;
> >>> + struct idr fence_idr;
> >>> + struct mutex lock;
> >>> struct kref ref;
> >>> };
> >>>
> >>> diff --git a/drivers/gpu/drm/msm/msm_submitqueue.c b/drivers/gpu/drm/msm/msm_submitqueue.c
> >>> index 9e9fec61d629..66f8d0fb38b0 100644
> >>> --- a/drivers/gpu/drm/msm/msm_submitqueue.c
> >>> +++ b/drivers/gpu/drm/msm/msm_submitqueue.c
> >>> @@ -12,6 +12,8 @@ void msm_submitqueue_destroy(struct kref *kref)
> >>> struct msm_gpu_submitqueue *queue = container_of(kref,
> >>> struct msm_gpu_submitqueue, ref);
> >>>
> >>> + idr_destroy(&queue->fence_idr);
> >>> +
> >>> msm_file_private_put(queue->ctx);
> >>>
> >>> kfree(queue);
> >>> @@ -89,6 +91,9 @@ int msm_submitqueue_create(struct drm_device *drm, struct msm_file_private *ctx,
> >>> if (id)
> >>> *id = queue->id;
> >>>
> >>> + idr_init(&queue->fence_idr);
> >>> + mutex_init(&queue->lock);
> >>> +
> >>> list_add_tail(&queue->node, &ctx->submitqueues);
> >>>
> >>> write_unlock(&ctx->queuelock);
> >>>
> >>
>
next prev parent reply other threads:[~2021-11-11 17:25 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-28 1:06 [PATCH v4 00/13] drm/msm: drm scheduler conversion and cleanups Rob Clark
2021-07-28 1:06 ` [PATCH v4 01/13] drm/msm: Docs and misc cleanup Rob Clark
2021-07-28 1:06 ` [PATCH v4 02/13] drm/msm: Small submitqueue creation cleanup Rob Clark
2021-07-28 1:06 ` [PATCH v4 03/13] drm/msm: drop drm_gem_object_put_locked() Rob Clark
2021-07-28 1:06 ` [PATCH v4 04/13] drm: Drop drm_gem_object_put_locked() Rob Clark
2021-07-28 1:06 ` [PATCH v4 05/13] drm/msm/submit: Simplify out-fence-fd handling Rob Clark
2021-07-28 1:06 ` [PATCH v4 06/13] drm/msm: Consolidate submit bo state Rob Clark
2021-07-28 1:06 ` [PATCH v4 07/13] drm/msm: Track "seqno" fences by idr Rob Clark
2021-11-10 15:28 ` Akhil P Oommen
2021-11-10 16:55 ` Rob Clark
2021-11-11 15:53 ` Akhil P Oommen
2021-11-11 17:30 ` Rob Clark [this message]
2021-07-28 1:06 ` [PATCH v4 08/13] drm/msm: Return ERR_PTR() from submit_create() Rob Clark
2021-07-28 1:06 ` [PATCH v4 09/13] drm/msm: Conversion to drm scheduler Rob Clark
2021-07-28 1:06 ` [PATCH v4 10/13] drm/msm: Drop submit bo_list Rob Clark
2021-07-28 1:06 ` [PATCH v4 11/13] drm/msm: Drop struct_mutex in submit path Rob Clark
2021-07-28 1:06 ` [PATCH v4 12/13] drm/msm: Utilize gpu scheduler priorities Rob Clark
2022-05-23 14:45 ` Tvrtko Ursulin
2022-05-23 22:53 ` Rob Clark
2022-05-24 13:45 ` Tvrtko Ursulin
2022-05-24 14:50 ` Rob Clark
2022-05-25 9:46 ` Tvrtko Ursulin
2022-05-25 13:41 ` Rob Clark
2022-05-25 16:22 ` Tvrtko Ursulin
2022-05-26 3:37 ` Rob Clark
2022-05-26 11:38 ` Tvrtko Ursulin
2022-05-27 4:25 ` [Freedreno] " Rob Clark
2022-06-07 12:43 ` Tvrtko Ursulin
2022-05-24 14:57 ` Rob Clark
2022-05-25 3:34 ` Rob Clark
2022-05-25 16:11 ` Tvrtko Ursulin
2022-05-26 3:15 ` Rob Clark
2022-05-26 13:29 ` Tvrtko Ursulin
2022-05-27 4:44 ` Rob Clark
2021-07-28 1:06 ` [PATCH v4 13/13] drm/msm/gem: Mark active before pinning Rob Clark
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAF6AEGsy1WZHOnuOgviFxW8y7yzmfGE4pWYzoAFXfxbS09m5pg@mail.gmail.com \
--to=robdclark@gmail.com \
--cc=airlied@linux.ie \
--cc=akhilpo@codeaurora.org \
--cc=christian.koenig@amd.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=freedreno@lists.freedesktop.org \
--cc=linaro-mm-sig@lists.linaro.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=robdclark@chromium.org \
--cc=sean@poorly.run \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).