| Message ID | 20221109174058.912720-1-andi.shyti@linux.intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | add guard padding around i915_vma | expand |
Hi, Andi, This has been on the list before (three times I think) and at that point it (the guard pages) was NAK'd by Daniel as yet another complication, and a VT-d scanout workaround was implemented and pushed using a different approach, initially outlined by Daniel. Patch is 2ef6efa79fecd. Those suspend/resumes should now be fast. I then also discussed patch 1 separately with Dave Airlie and Daniel and since both me and Dave liked it, Daniel OK'd it, but it never made it upstream. Just a short heads up on the history. /Thomas On Wed, 2022-11-09 at 18:40 +0100, Andi Shyti wrote: > Hi, > > This series adds guards around vma's but setting a pages at the > beginning and at the end that work as padding. > > The first user of the vma guard are scanout objects which don't > need anymore to add scratch to all the unused ggtt's and speeding > up up considerably the boot and resume by several hundreds of > milliseconds up to over a full second in slower machines. > > Andi > > Chris Wilson (3): > drm/i915: Wrap all access to i915_vma.node.start|size > drm/i915: Introduce guard pages to i915_vma > drm/i915: Refine VT-d scanout workaround > > drivers/gpu/drm/i915/display/intel_fbdev.c | 2 +- > drivers/gpu/drm/i915/gem/i915_gem_domain.c | 13 ++++ > .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 33 ++++++----- > drivers/gpu/drm/i915/gem/i915_gem_mman.c | 2 +- > drivers/gpu/drm/i915/gem/i915_gem_shrinker.c | 2 +- > drivers/gpu/drm/i915/gem/i915_gem_tiling.c | 4 +- > .../gpu/drm/i915/gem/selftests/huge_pages.c | 2 +- > .../i915/gem/selftests/i915_gem_client_blt.c | 23 ++++---- > .../drm/i915/gem/selftests/i915_gem_context.c | 15 +++-- > .../drm/i915/gem/selftests/i915_gem_mman.c | 2 +- > .../drm/i915/gem/selftests/igt_gem_utils.c | 7 ++- > drivers/gpu/drm/i915/gt/gen7_renderclear.c | 2 +- > drivers/gpu/drm/i915/gt/intel_ggtt.c | 39 ++++-------- > drivers/gpu/drm/i915/gt/intel_ggtt_fencing.c | 3 +- > drivers/gpu/drm/i915/gt/intel_renderstate.c | 2 +- > .../gpu/drm/i915/gt/intel_ring_submission.c | 2 +- > drivers/gpu/drm/i915/gt/selftest_engine_cs.c | 8 +-- > drivers/gpu/drm/i915/gt/selftest_execlists.c | 18 +++--- > drivers/gpu/drm/i915/gt/selftest_hangcheck.c | 15 ++--- > drivers/gpu/drm/i915/gt/selftest_lrc.c | 16 ++--- > .../drm/i915/gt/selftest_ring_submission.c | 2 +- > drivers/gpu/drm/i915/gt/selftest_rps.c | 12 ++-- > .../gpu/drm/i915/gt/selftest_workarounds.c | 8 +-- > drivers/gpu/drm/i915/i915_cmd_parser.c | 4 +- > drivers/gpu/drm/i915/i915_debugfs.c | 2 +- > drivers/gpu/drm/i915/i915_gem_gtt.h | 3 +- > drivers/gpu/drm/i915/i915_perf.c | 2 +- > drivers/gpu/drm/i915/i915_vma.c | 59 +++++++++++++---- > -- > drivers/gpu/drm/i915/i915_vma.h | 52 +++++++++++++++- > drivers/gpu/drm/i915/i915_vma_resource.c | 4 +- > drivers/gpu/drm/i915/i915_vma_resource.h | 17 ++++-- > drivers/gpu/drm/i915/i915_vma_types.h | 3 +- > drivers/gpu/drm/i915/selftests/i915_request.c | 20 +++---- > drivers/gpu/drm/i915/selftests/igt_spinner.c | 8 +-- > 34 files changed, 246 insertions(+), 160 deletions(-) >
Hi Thomas, On Wed, Nov 09, 2022 at 07:03:03PM +0100, Thomas Hellström wrote: > Hi, Andi, > > This has been on the list before (three times I think) and at that > point it (the guard pages) was NAK'd by Daniel as yet another > complication, and a VT-d > scanout workaround was implemented and pushed using a different > approach, initially outlined by Daniel. > > Patch is 2ef6efa79fecd. Those suspend/resumes should now be fast. > > I then also discussed patch 1 separately with Dave Airlie and Daniel > and since both me and Dave liked it, Daniel OK'd it, but it never made > it upstream. > > Just a short heads up on the history. Thanks for letting me know, I missed this part of the history. Will check what happened in the previous mails and your improvement on the vt-d suspend/resume. Thanks, Andi
Hi, On 09/11/2022 18:03, Thomas Hellström wrote: > Hi, Andi, > > This has been on the list before (three times I think) and at that > point it (the guard pages) was NAK'd by Daniel as yet another > complication, and a VT-d > scanout workaround was implemented and pushed using a different > approach, initially outlined by Daniel. I can't find this discussion and NAKs on the list - do you have a link? > Patch is 2ef6efa79fecd. Those suspend/resumes should now be fast. So the initiator to re-start this series was actually the boot time is failing KPIs by quite a margin. Which means we may need a way forward after all. Especially if the most churny patch 1 was deemed okay, then I don't see why the concept of guard pages should be a problem. But again, I couldn't find the discussion you mention to read what were the objections.. For 2ef6efa79fecd specifically. I only looked at it today - do you think that the heuristic of checking one PTE and deciding all content was preserved is safe? What if someone scribbled at random locations? On a first thought it is making me a bit uncomfortable. Regards, Tvrtko > I then also discussed patch 1 separately with Dave Airlie and Daniel > and since both me and Dave liked it, Daniel OK'd it, but it never made > it upstream. > > Just a short heads up on the history. > > /Thomas > > > On Wed, 2022-11-09 at 18:40 +0100, Andi Shyti wrote: >> Hi, >> >> This series adds guards around vma's but setting a pages at the >> beginning and at the end that work as padding. >> >> The first user of the vma guard are scanout objects which don't >> need anymore to add scratch to all the unused ggtt's and speeding >> up up considerably the boot and resume by several hundreds of >> milliseconds up to over a full second in slower machines. >> >> Andi >> >> Chris Wilson (3): >> drm/i915: Wrap all access to i915_vma.node.start|size >> drm/i915: Introduce guard pages to i915_vma >> drm/i915: Refine VT-d scanout workaround >> >> drivers/gpu/drm/i915/display/intel_fbdev.c | 2 +- >> drivers/gpu/drm/i915/gem/i915_gem_domain.c | 13 ++++ >> .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 33 ++++++----- >> drivers/gpu/drm/i915/gem/i915_gem_mman.c | 2 +- >> drivers/gpu/drm/i915/gem/i915_gem_shrinker.c | 2 +- >> drivers/gpu/drm/i915/gem/i915_gem_tiling.c | 4 +- >> .../gpu/drm/i915/gem/selftests/huge_pages.c | 2 +- >> .../i915/gem/selftests/i915_gem_client_blt.c | 23 ++++---- >> .../drm/i915/gem/selftests/i915_gem_context.c | 15 +++-- >> .../drm/i915/gem/selftests/i915_gem_mman.c | 2 +- >> .../drm/i915/gem/selftests/igt_gem_utils.c | 7 ++- >> drivers/gpu/drm/i915/gt/gen7_renderclear.c | 2 +- >> drivers/gpu/drm/i915/gt/intel_ggtt.c | 39 ++++-------- >> drivers/gpu/drm/i915/gt/intel_ggtt_fencing.c | 3 +- >> drivers/gpu/drm/i915/gt/intel_renderstate.c | 2 +- >> .../gpu/drm/i915/gt/intel_ring_submission.c | 2 +- >> drivers/gpu/drm/i915/gt/selftest_engine_cs.c | 8 +-- >> drivers/gpu/drm/i915/gt/selftest_execlists.c | 18 +++--- >> drivers/gpu/drm/i915/gt/selftest_hangcheck.c | 15 ++--- >> drivers/gpu/drm/i915/gt/selftest_lrc.c | 16 ++--- >> .../drm/i915/gt/selftest_ring_submission.c | 2 +- >> drivers/gpu/drm/i915/gt/selftest_rps.c | 12 ++-- >> .../gpu/drm/i915/gt/selftest_workarounds.c | 8 +-- >> drivers/gpu/drm/i915/i915_cmd_parser.c | 4 +- >> drivers/gpu/drm/i915/i915_debugfs.c | 2 +- >> drivers/gpu/drm/i915/i915_gem_gtt.h | 3 +- >> drivers/gpu/drm/i915/i915_perf.c | 2 +- >> drivers/gpu/drm/i915/i915_vma.c | 59 +++++++++++++---- >> -- >> drivers/gpu/drm/i915/i915_vma.h | 52 +++++++++++++++- >> drivers/gpu/drm/i915/i915_vma_resource.c | 4 +- >> drivers/gpu/drm/i915/i915_vma_resource.h | 17 ++++-- >> drivers/gpu/drm/i915/i915_vma_types.h | 3 +- >> drivers/gpu/drm/i915/selftests/i915_request.c | 20 +++---- >> drivers/gpu/drm/i915/selftests/igt_spinner.c | 8 +-- >> 34 files changed, 246 insertions(+), 160 deletions(-) >> >
Hi Thomas, > This has been on the list before (three times I think) and at that > point it (the guard pages) was NAK'd by Daniel as yet another > complication, and a VT-d > scanout workaround was implemented and pushed using a different > approach, initially outlined by Daniel. I reckon that this is an old patch, but I've seen only reviews and acks. I haven't seen anything against this patch. So that as far as it concerns me, this patch should be good to go, unless I missed something or there is any technical concern. > Patch is 2ef6efa79fecd. Those suspend/resumes should now be fast. This patch is not actually resolving the boot time delays, that is the main concern coming from the users, and, just as a post-mortem review, as Tvrtko has pointed out, this: +/* Intel Rapid Start Technology ACPI device name */ +static const char irst_name[] = "INT3392"; is a bit scary because we are depending very much on the firmware and whatever happens there is not under our control. It's an hardcoded string that requires maintenance. In any case, the two patches are somehow doing different things: I don't see them conflicting and to me looks like a reasonable optimization (otherwise I wouldn't have put my signature on it :)) Thanks again a lot for your thoughts and inputs, Andi > I then also discussed patch 1 separately with Dave Airlie and Daniel > and since both me and Dave liked it, Daniel OK'd it, but it never made > it upstream. > > Just a short heads up on the history. > > /Thomas > > > On Wed, 2022-11-09 at 18:40 +0100, Andi Shyti wrote: > > Hi, > > > > This series adds guards around vma's but setting a pages at the > > beginning and at the end that work as padding. > > > > The first user of the vma guard are scanout objects which don't > > need anymore to add scratch to all the unused ggtt's and speeding > > up up considerably the boot and resume by several hundreds of > > milliseconds up to over a full second in slower machines. > > > > Andi > > > > Chris Wilson (3): > > drm/i915: Wrap all access to i915_vma.node.start|size > > drm/i915: Introduce guard pages to i915_vma > > drm/i915: Refine VT-d scanout workaround > > > > drivers/gpu/drm/i915/display/intel_fbdev.c | 2 +- > > drivers/gpu/drm/i915/gem/i915_gem_domain.c | 13 ++++ > > .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 33 ++++++----- > > drivers/gpu/drm/i915/gem/i915_gem_mman.c | 2 +- > > drivers/gpu/drm/i915/gem/i915_gem_shrinker.c | 2 +- > > drivers/gpu/drm/i915/gem/i915_gem_tiling.c | 4 +- > > .../gpu/drm/i915/gem/selftests/huge_pages.c | 2 +- > > .../i915/gem/selftests/i915_gem_client_blt.c | 23 ++++---- > > .../drm/i915/gem/selftests/i915_gem_context.c | 15 +++-- > > .../drm/i915/gem/selftests/i915_gem_mman.c | 2 +- > > .../drm/i915/gem/selftests/igt_gem_utils.c | 7 ++- > > drivers/gpu/drm/i915/gt/gen7_renderclear.c | 2 +- > > drivers/gpu/drm/i915/gt/intel_ggtt.c | 39 ++++-------- > > drivers/gpu/drm/i915/gt/intel_ggtt_fencing.c | 3 +- > > drivers/gpu/drm/i915/gt/intel_renderstate.c | 2 +- > > .../gpu/drm/i915/gt/intel_ring_submission.c | 2 +- > > drivers/gpu/drm/i915/gt/selftest_engine_cs.c | 8 +-- > > drivers/gpu/drm/i915/gt/selftest_execlists.c | 18 +++--- > > drivers/gpu/drm/i915/gt/selftest_hangcheck.c | 15 ++--- > > drivers/gpu/drm/i915/gt/selftest_lrc.c | 16 ++--- > > .../drm/i915/gt/selftest_ring_submission.c | 2 +- > > drivers/gpu/drm/i915/gt/selftest_rps.c | 12 ++-- > > .../gpu/drm/i915/gt/selftest_workarounds.c | 8 +-- > > drivers/gpu/drm/i915/i915_cmd_parser.c | 4 +- > > drivers/gpu/drm/i915/i915_debugfs.c | 2 +- > > drivers/gpu/drm/i915/i915_gem_gtt.h | 3 +- > > drivers/gpu/drm/i915/i915_perf.c | 2 +- > > drivers/gpu/drm/i915/i915_vma.c | 59 +++++++++++++---- > > -- > > drivers/gpu/drm/i915/i915_vma.h | 52 +++++++++++++++- > > drivers/gpu/drm/i915/i915_vma_resource.c | 4 +- > > drivers/gpu/drm/i915/i915_vma_resource.h | 17 ++++-- > > drivers/gpu/drm/i915/i915_vma_types.h | 3 +- > > drivers/gpu/drm/i915/selftests/i915_request.c | 20 +++---- > > drivers/gpu/drm/i915/selftests/igt_spinner.c | 8 +-- > > 34 files changed, 246 insertions(+), 160 deletions(-) > >