* [PATCH] drm/i915: Use a dummy timeline name for a signaled fence
@ 2017-03-30 11:16 Chris Wilson
2017-03-30 11:19 ` Chris Wilson
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Chris Wilson @ 2017-03-30 11:16 UTC (permalink / raw)
To: intel-gfx
Cc: Chris Wilson, Michał Winiarski, Joonas Lahtinen, # v4 . 10+
Michał Winiarski pointed out that the debugging infrastructure (such as
trace_dma_fence_release) likes to pretty print the timeline name, long
after we have freed the timeline. Our timelines currently live as part of
the GTT (due to the strict ordering we current use through each) which
belong to the context. We aim to free the context and release its
hardware resources as soon as we able to (i.e. when the last
fence/request using it has been signaled and retired). As the
.get_timeline_name is purely a debug feature, rather than extending the
lifetime of the context, or splitting it into many different release
phases just to keep the name along, replace the timeline name with a
constant after the fence has been signaled. This avoids the potential
use-after-free.
Reported-by: Michał Winiarski <michal.winiarski@intel.com>
Fixes: 80b204bce8f2 ("drm/i915: Enable multiple timelines")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: <stable@vger.kernel.org> # v4.10+
---
drivers/gpu/drm/i915/i915_gem_request.c | 11 +++++++++++
1 file changed, 11 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
index cb9209589d2e..2cb2206b2f6b 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.c
+++ b/drivers/gpu/drm/i915/i915_gem_request.c
@@ -37,6 +37,17 @@ static const char *i915_fence_get_driver_name(struct dma_fence *fence)
static const char *i915_fence_get_timeline_name(struct dma_fence *fence)
{
+ /* The timeline struct (as part of the ppgtt underneath a context)
+ * may be freed when the request is no longer in use by the GPU.
+ * We could extend the life of a context to beyond that of all
+ * fences, possibly keeping the hw resource around indefinitely,
+ * or we just give them a false name. Since
+ * dma_fence_ops.get_timeline_name is a debug feature, the occasional
+ * lie seems justifiable.
+ */
+ if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
+ return "signaled";
+
return to_request(fence)->timeline->common->name;
}
--
2.11.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915: Use a dummy timeline name for a signaled fence
2017-03-30 11:16 [PATCH] drm/i915: Use a dummy timeline name for a signaled fence Chris Wilson
@ 2017-03-30 11:19 ` Chris Wilson
2017-03-30 11:30 ` Joonas Lahtinen
2017-03-30 11:50 ` Michał Winiarski
2017-03-30 13:10 ` ✓ Fi.CI.BAT: success for " Patchwork
2 siblings, 1 reply; 7+ messages in thread
From: Chris Wilson @ 2017-03-30 11:19 UTC (permalink / raw)
To: intel-gfx; +Cc: Michał Winiarski, Joonas Lahtinen, # v4 . 10+
On Thu, Mar 30, 2017 at 12:16:14PM +0100, Chris Wilson wrote:
> Michał Winiarski pointed out that the debugging infrastructure (such as
> trace_dma_fence_release) likes to pretty print the timeline name, long
> after we have freed the timeline. Our timelines currently live as part of
> the GTT (due to the strict ordering we current use through each) which
s/current/currently/
> belong to the context. We aim to free the context and release its
> hardware resources as soon as we able to (i.e. when the last
> fence/request using it has been signaled and retired). As the
> .get_timeline_name is purely a debug feature, rather than extending the
> lifetime of the context, or splitting it into many different release
> phases just to keep the name along, replace the timeline name with a
s/along/around/
> constant after the fence has been signaled. This avoids the potential
> use-after-free.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915: Use a dummy timeline name for a signaled fence
2017-03-30 11:19 ` Chris Wilson
@ 2017-03-30 11:30 ` Joonas Lahtinen
0 siblings, 0 replies; 7+ messages in thread
From: Joonas Lahtinen @ 2017-03-30 11:30 UTC (permalink / raw)
To: Chris Wilson, intel-gfx; +Cc: Michał Winiarski, # v4 . 10+
On to, 2017-03-30 at 12:19 +0100, Chris Wilson wrote:
> On Thu, Mar 30, 2017 at 12:16:14PM +0100, Chris Wilson wrote:
> >
> > Michał Winiarski pointed out that the debugging infrastructure (such as
> > trace_dma_fence_release) likes to pretty print the timeline name, long
> > after we have freed the timeline. Our timelines currently live as part of
> > the GTT (due to the strict ordering we current use through each) which
> s/current/currently/
>
> >
> > belong to the context. We aim to free the context and release its
> > hardware resources as soon as we able to (i.e. when the last
> > fence/request using it has been signaled and retired). As the
> > .get_timeline_name is purely a debug feature, rather than extending the
> > lifetime of the context, or splitting it into many different release
> > phases just to keep the name along, replace the timeline name with a
> s/along/around/
>
> >
> > constant after the fence has been signaled. This avoids the potential
> > use-after-free.
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915: Use a dummy timeline name for a signaled fence
2017-03-30 11:16 [PATCH] drm/i915: Use a dummy timeline name for a signaled fence Chris Wilson
@ 2017-03-30 11:50 ` Michał Winiarski
2017-03-30 11:50 ` Michał Winiarski
2017-03-30 13:10 ` ✓ Fi.CI.BAT: success for " Patchwork
2 siblings, 0 replies; 7+ messages in thread
From: Michał Winiarski @ 2017-03-30 11:50 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx, Joonas Lahtinen, # v4 . 10+
On Thu, Mar 30, 2017 at 12:16:14PM +0100, Chris Wilson wrote:
> Michał Winiarski pointed out that the debugging infrastructure (such as
> trace_dma_fence_release) likes to pretty print the timeline name, long
> after we have freed the timeline. Our timelines currently live as part of
> the GTT (due to the strict ordering we current use through each) which
> belong to the context. We aim to free the context and release its
> hardware resources as soon as we able to (i.e. when the last
> fence/request using it has been signaled and retired). As the
> .get_timeline_name is purely a debug feature, rather than extending the
> lifetime of the context, or splitting it into many different release
> phases just to keep the name along, replace the timeline name with a
> constant after the fence has been signaled. This avoids the potential
> use-after-free.
>
> Reported-by: Michał Winiarski <michal.winiarski@intel.com>
Actually, I'm just a messenger ;)
Reported-by: Krzysztof Olinski <krzysztof.e.olinski@intel.com>
Reviewed-by: Michał Winiarski <michal.winiarski@intel.com>
-Michał
> Fixes: 80b204bce8f2 ("drm/i915: Enable multiple timelines")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: <stable@vger.kernel.org> # v4.10+
> ---
> drivers/gpu/drm/i915/i915_gem_request.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
> index cb9209589d2e..2cb2206b2f6b 100644
> --- a/drivers/gpu/drm/i915/i915_gem_request.c
> +++ b/drivers/gpu/drm/i915/i915_gem_request.c
> @@ -37,6 +37,17 @@ static const char *i915_fence_get_driver_name(struct dma_fence *fence)
>
> static const char *i915_fence_get_timeline_name(struct dma_fence *fence)
> {
> + /* The timeline struct (as part of the ppgtt underneath a context)
> + * may be freed when the request is no longer in use by the GPU.
> + * We could extend the life of a context to beyond that of all
> + * fences, possibly keeping the hw resource around indefinitely,
> + * or we just give them a false name. Since
> + * dma_fence_ops.get_timeline_name is a debug feature, the occasional
> + * lie seems justifiable.
> + */
> + if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
> + return "signaled";
> +
> return to_request(fence)->timeline->common->name;
> }
>
> --
> 2.11.0
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915: Use a dummy timeline name for a signaled fence
@ 2017-03-30 11:50 ` Michał Winiarski
0 siblings, 0 replies; 7+ messages in thread
From: Michał Winiarski @ 2017-03-30 11:50 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx, Joonas Lahtinen, # v4 . 10+
On Thu, Mar 30, 2017 at 12:16:14PM +0100, Chris Wilson wrote:
> Michał Winiarski pointed out that the debugging infrastructure (such as
> trace_dma_fence_release) likes to pretty print the timeline name, long
> after we have freed the timeline. Our timelines currently live as part of
> the GTT (due to the strict ordering we current use through each) which
> belong to the context. We aim to free the context and release its
> hardware resources as soon as we able to (i.e. when the last
> fence/request using it has been signaled and retired). As the
> .get_timeline_name is purely a debug feature, rather than extending the
> lifetime of the context, or splitting it into many different release
> phases just to keep the name along, replace the timeline name with a
> constant after the fence has been signaled. This avoids the potential
> use-after-free.
>
> Reported-by: Michał Winiarski <michal.winiarski@intel.com>
Actually, I'm just a messenger ;)
Reported-by: Krzysztof Olinski <krzysztof.e.olinski@intel.com>
Reviewed-by: Michał Winiarski <michal.winiarski@intel.com>
-Michał
> Fixes: 80b204bce8f2 ("drm/i915: Enable multiple timelines")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: <stable@vger.kernel.org> # v4.10+
> ---
> drivers/gpu/drm/i915/i915_gem_request.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
> index cb9209589d2e..2cb2206b2f6b 100644
> --- a/drivers/gpu/drm/i915/i915_gem_request.c
> +++ b/drivers/gpu/drm/i915/i915_gem_request.c
> @@ -37,6 +37,17 @@ static const char *i915_fence_get_driver_name(struct dma_fence *fence)
>
> static const char *i915_fence_get_timeline_name(struct dma_fence *fence)
> {
> + /* The timeline struct (as part of the ppgtt underneath a context)
> + * may be freed when the request is no longer in use by the GPU.
> + * We could extend the life of a context to beyond that of all
> + * fences, possibly keeping the hw resource around indefinitely,
> + * or we just give them a false name. Since
> + * dma_fence_ops.get_timeline_name is a debug feature, the occasional
> + * lie seems justifiable.
> + */
> + if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags))
> + return "signaled";
> +
> return to_request(fence)->timeline->common->name;
> }
>
> --
> 2.11.0
^ permalink raw reply [flat|nested] 7+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915: Use a dummy timeline name for a signaled fence
2017-03-30 11:16 [PATCH] drm/i915: Use a dummy timeline name for a signaled fence Chris Wilson
2017-03-30 11:19 ` Chris Wilson
2017-03-30 11:50 ` Michał Winiarski
@ 2017-03-30 13:10 ` Patchwork
2017-03-30 13:19 ` Chris Wilson
2 siblings, 1 reply; 7+ messages in thread
From: Patchwork @ 2017-03-30 13:10 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
== Series Details ==
Series: drm/i915: Use a dummy timeline name for a signaled fence
URL : https://patchwork.freedesktop.org/series/22193/
State : success
== Summary ==
Series 22193v1 drm/i915: Use a dummy timeline name for a signaled fence
https://patchwork.freedesktop.org/api/1.0/series/22193/revisions/1/mbox/
Test gem_exec_flush:
Subgroup basic-batch-kernel-default-uc:
fail -> PASS (fi-snb-2600) fdo#100007
fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007
fi-bdw-5557u total:278 pass:267 dwarn:0 dfail:0 fail:0 skip:11 time: 429s
fi-bdw-gvtdvm total:278 pass:256 dwarn:8 dfail:0 fail:0 skip:14 time: 426s
fi-bsw-n3050 total:278 pass:239 dwarn:0 dfail:0 fail:0 skip:39 time: 564s
fi-bxt-j4205 total:278 pass:259 dwarn:0 dfail:0 fail:0 skip:19 time: 509s
fi-bxt-t5700 total:278 pass:258 dwarn:0 dfail:0 fail:0 skip:20 time: 561s
fi-byt-j1900 total:278 pass:251 dwarn:0 dfail:0 fail:0 skip:27 time: 489s
fi-byt-n2820 total:278 pass:247 dwarn:0 dfail:0 fail:0 skip:31 time: 481s
fi-hsw-4770 total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time: 413s
fi-hsw-4770r total:278 pass:262 dwarn:0 dfail:0 fail:0 skip:16 time: 408s
fi-ilk-650 total:278 pass:228 dwarn:0 dfail:0 fail:0 skip:50 time: 423s
fi-ivb-3520m total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time: 496s
fi-ivb-3770 total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time: 463s
fi-kbl-7500u total:278 pass:260 dwarn:0 dfail:0 fail:0 skip:18 time: 452s
fi-kbl-7560u total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time: 568s
fi-skl-6260u total:278 pass:268 dwarn:0 dfail:0 fail:0 skip:10 time: 458s
fi-skl-6700hq total:278 pass:261 dwarn:0 dfail:0 fail:0 skip:17 time: 578s
fi-skl-6700k total:278 pass:256 dwarn:4 dfail:0 fail:0 skip:18 time: 462s
fi-skl-gvtdvm total:278 pass:265 dwarn:0 dfail:0 fail:0 skip:13 time: 433s
fi-snb-2520m total:278 pass:250 dwarn:0 dfail:0 fail:0 skip:28 time: 524s
fi-snb-2600 total:278 pass:249 dwarn:0 dfail:0 fail:0 skip:29 time: 399s
fi-skl-6770hq failed to collect. IGT log at Patchwork_4359/fi-skl-6770hq/igt.log
71dab8f683b2d609926524f9a6a60b00e4bfa543 drm-tip: 2017y-03m-30d-11h-44m-46s UTC integration manifest
e627708 drm/i915: Use a dummy timeline name for a signaled fence
== Logs ==
For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4359/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: ✓ Fi.CI.BAT: success for drm/i915: Use a dummy timeline name for a signaled fence
2017-03-30 13:10 ` ✓ Fi.CI.BAT: success for " Patchwork
@ 2017-03-30 13:19 ` Chris Wilson
0 siblings, 0 replies; 7+ messages in thread
From: Chris Wilson @ 2017-03-30 13:19 UTC (permalink / raw)
To: intel-gfx
On Thu, Mar 30, 2017 at 01:10:15PM -0000, Patchwork wrote:
> == Series Details ==
>
> Series: drm/i915: Use a dummy timeline name for a signaled fence
> URL : https://patchwork.freedesktop.org/series/22193/
> State : success
>
> == Summary ==
>
> Series 22193v1 drm/i915: Use a dummy timeline name for a signaled fence
> https://patchwork.freedesktop.org/api/1.0/series/22193/revisions/1/mbox/
>
> Test gem_exec_flush:
> Subgroup basic-batch-kernel-default-uc:
> fail -> PASS (fi-snb-2600) fdo#100007
>
> fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007
Pushed, and I put in my earplugs so I can't hear the cries of confusion.
Lalalalala,
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-03-30 13:19 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-30 11:16 [PATCH] drm/i915: Use a dummy timeline name for a signaled fence Chris Wilson
2017-03-30 11:19 ` Chris Wilson
2017-03-30 11:30 ` Joonas Lahtinen
2017-03-30 11:50 ` Michał Winiarski
2017-03-30 11:50 ` Michał Winiarski
2017-03-30 13:10 ` ✓ Fi.CI.BAT: success for " Patchwork
2017-03-30 13:19 ` Chris Wilson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.