From: Daniel Vetter <daniel@ffwll.ch>
To: Matthew Auld <matthew.auld@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>,
Andi Shyti <andi.shyti@intel.com>,
intel-gfx@lists.freedesktop.org, stable@vger.kernel.org,
Chris Wilson <chris@chris-wilson.co.uk>,
Chintan M Patel <chintan.m.patel@intel.com>,
dri-devel@lists.freedesktop.org
Subject: Re: [PATCH] drm/i915: Use DRIVER_NAME for tracing unattached requests
Date: Thu, 20 May 2021 16:28:13 +0200 [thread overview]
Message-ID: <YKZx/U05aRaxKw44@phenom.ffwll.local> (raw)
In-Reply-To: <20210520073514.314893-1-matthew.auld@intel.com>
On Thu, May 20, 2021 at 08:35:14AM +0100, Matthew Auld wrote:
> From: Chris Wilson <chris@chris-wilson.co.uk>
>
> The first tracepoint for a request is trace_dma_fence_init called before
> we have associated the request with a device. The tracepoint uses
> fence->ops->get_driver_name() as a pretty name, and as we try to report
> the device name this oopses as it is then NULL. Support the early
> tracepoint by reporting the DRIVER_NAME instead of the actual device
> name.
>
> Note that rq->engine remains during the course of request recycling
> (SLAB_TYPESAFE_BY_RCU). For the physical engines, the pointer remains
> valid, however a virtual engine may be destroyed after the request is
> retired. If we process a preempt-to-busy completed request along the
> virtual engine, we should make sure we mark the request as no longer
> belonging to the virtual engine to remove the dangling pointers from the
> tracepoint.
Why can't we assign the request beforehand? The idea behind these
tracepoints is that they actually match up, if trace_dma_fence_init is
different, then we're breaking that.
-Daniel
>
> Fixes: 855e39e65cfc ("drm/i915: Initialise basic fence before acquiring seqno")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Chintan M Patel <chintan.m.patel@intel.com>
> Cc: Andi Shyti <andi.shyti@intel.com>
> Cc: <stable@vger.kernel.org> # v5.7+
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> ---
> .../drm/i915/gt/intel_execlists_submission.c | 20 ++++++++++++++-----
> drivers/gpu/drm/i915/i915_request.c | 7 ++++++-
> 2 files changed, 21 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> index de124870af44..75604e927d34 100644
> --- a/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> +++ b/drivers/gpu/drm/i915/gt/intel_execlists_submission.c
> @@ -3249,6 +3249,18 @@ static struct list_head *virtual_queue(struct virtual_engine *ve)
> return &ve->base.execlists.default_priolist.requests;
> }
>
> +static void
> +virtual_submit_completed(struct virtual_engine *ve, struct i915_request *rq)
> +{
> + GEM_BUG_ON(!__i915_request_is_complete(rq));
> + GEM_BUG_ON(rq->engine != &ve->base);
> +
> + __i915_request_submit(rq);
> +
> + /* Remove the dangling pointer to the stale virtual engine */
> + WRITE_ONCE(rq->engine, ve->siblings[0]);
> +}
> +
> static void rcu_virtual_context_destroy(struct work_struct *wrk)
> {
> struct virtual_engine *ve =
> @@ -3265,8 +3277,7 @@ static void rcu_virtual_context_destroy(struct work_struct *wrk)
>
> old = fetch_and_zero(&ve->request);
> if (old) {
> - GEM_BUG_ON(!__i915_request_is_complete(old));
> - __i915_request_submit(old);
> + virtual_submit_completed(ve, old);
> i915_request_put(old);
> }
>
> @@ -3538,13 +3549,12 @@ static void virtual_submit_request(struct i915_request *rq)
>
> /* By the time we resubmit a request, it may be completed */
> if (__i915_request_is_complete(rq)) {
> - __i915_request_submit(rq);
> + virtual_submit_completed(ve, rq);
> goto unlock;
> }
>
> if (ve->request) { /* background completion from preempt-to-busy */
> - GEM_BUG_ON(!__i915_request_is_complete(ve->request));
> - __i915_request_submit(ve->request);
> + virtual_submit_completed(ve, ve->request);
> i915_request_put(ve->request);
> }
>
> diff --git a/drivers/gpu/drm/i915/i915_request.c b/drivers/gpu/drm/i915/i915_request.c
> index 970d8f4986bb..aa124adb1051 100644
> --- a/drivers/gpu/drm/i915/i915_request.c
> +++ b/drivers/gpu/drm/i915/i915_request.c
> @@ -61,7 +61,12 @@ static struct i915_global_request {
>
> static const char *i915_fence_get_driver_name(struct dma_fence *fence)
> {
> - return dev_name(to_request(fence)->engine->i915->drm.dev);
> + struct i915_request *rq = to_request(fence);
> +
> + if (unlikely(!rq->engine)) /* not yet attached to any device */
> + return DRIVER_NAME;
> +
> + return dev_name(rq->engine->i915->drm.dev);
> }
>
> static const char *i915_fence_get_timeline_name(struct dma_fence *fence)
> --
> 2.26.3
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
next prev parent reply other threads:[~2021-05-20 14:28 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-20 7:35 [PATCH] drm/i915: Use DRIVER_NAME for tracing unattached requests Matthew Auld
2021-05-20 14:28 ` Daniel Vetter [this message]
2021-05-31 7:53 ` Daniel Vetter
2021-06-01 11:13 ` Matthew Auld
2021-06-01 12:20 ` Daniel Vetter
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=YKZx/U05aRaxKw44@phenom.ffwll.local \
--to=daniel@ffwll.ch \
--cc=andi.shyti@intel.com \
--cc=chintan.m.patel@intel.com \
--cc=chris@chris-wilson.co.uk \
--cc=dri-devel@lists.freedesktop.org \
--cc=intel-gfx@lists.freedesktop.org \
--cc=matthew.auld@intel.com \
--cc=stable@vger.kernel.org \
--cc=tvrtko.ursulin@linux.intel.com \
/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).