Skip to content
  1. Jul 23, 2021
  3. Jul 22, 2021
    • Faith Ekstrand's avatar
      drm/i915: Make the kmem slab for i915_buddy_block a global · 0f465135
      Faith Ekstrand authored 
      

There's no reason that I can tell why this should be per-i915_buddy_mm
and doing so causes KMEM_CACHE to throw dmesg warnings because it tries
to create a debugfs entry with the name i915_buddy_block multiple times.
We could handle this by carefully giving each slab its own name but that
brings its own pain because then we have to store that string somewhere
and manage the lifetimes of the different slabs.  The most likely
outcome would be a global atomic which we increment to get a new name or
something like that.

The much easier solution is to use the i915_globals system like we do
for every other slab in i915.  This ensures that we have exactly one of
them for each i915 driver load and it gets neatly created on module load
and destroyed on module unload.  Using the globals system also means
that its now tied into the shrink handler so we can properly respond to
low-memory situations.

Signed-off-by: default avatarJason Ekstrand <jason@jlekstrand.net>
Fixes: 88be9a0a

 ("drm/i915/ttm: add ttm_buddy_man")
Reviewed-by: default avatarMatthew Auld <matthew.auld@intel.com>
Cc: Matthew Auld <matthew.auld@intel.com>
Cc: Christian König <christian.koenig@amd.com>
[danvet: Rebase against removal of global shrink code]
Signed-off-by: default avatarDaniel Vetter <daniel.vetter@ffwll.ch>
Link: https://patchwork.freedesktop.org/patch/msgid/20210721152358.2893314-7-jason@jlekstrand.net
      0f465135
    • Faith Ekstrand's avatar
      drm/i915: Use a table for i915_init/exit (v2) · a04ea6ae
      Faith Ekstrand authored 
      

If the driver was not fully loaded, we may still have globals lying
around.  If we don't tear those down in i915_exit(), we'll leak a bunch
of memory slabs.  This can happen two ways: use_kms = false and if we've
run mock selftests.  In either case, we have an early exit from
i915_init which happens after i915_globals_init() and we need to clean
up those globals.

The mock selftests case is especially sticky.  The load isn't entirely
a no-op.  We actually do quite a bit inside those selftests including
allocating a bunch of mock objects and running tests on them.  Once all
those tests are complete, we exit early from i915_init().  Perviously,
i915_init() would return a non-zero error code on failure and a zero
error code on success.  In the success case, we would get to i915_exit()
and check i915_pci_driver.driver.owner to detect if i915_init exited early
and do nothing.  In the failure case, we would fail i915_init() but
there would be no opportunity to clean up globals.

The most annoying part is that you don't actually notice the failure as
part of the self-tests since leaking a bit of memory, while bad, doesn't
result in anything observable from userspace.  Instead, the next time we
load the driver (usually for next IGT test), i915_globals_init() gets
invoked again, we go to allocate a bunch of new memory slabs, those
implicitly create debugfs entries, and debugfs warns that we're trying
to create directories and files that already exist.  Since this all
happens as part of the next driver load, it shows up in the dmesg-warn
of whatever IGT test ran after the mock selftests.

While the obvious thing to do here might be to call i915_globals_exit()
after selftests, that's not actually safe.  The dma-buf selftests call
i915_gem_prime_export which creates a file.  We call dma_buf_put() on
the resulting dmabuf which calls fput() on the file.  However, fput()
isn't immediate and gets flushed right before syscall returns.  This
means that all the fput()s from the selftests don't happen until right
before the module load syscall used to fire off the selftests returns
which is after i915_init().  If we call i915_globals_exit() in
i915_init() after selftests, we end up freeing slabs out from under
objects which won't get released until fput() is flushed at the end of
the module load syscall.

The solution here is to let i915_init() return success early and detect
the early success in i915_exit() and only tear down globals and nothing
else.  This way the module loads successfully, regardless of the success
or failure of the tests.  Because we've not enumerated any PCI devices,
no device nodes are created and it's entirely useless from userspace.
The only thing the module does at that point is hold on to a bit of
memory until we unload it and i915_exit() is called.  Importantly, this
means that everything from our selftests has the ability to properly
flush out between i915_init() and i915_exit() because there is at least
one syscall boundary in between.

In order to handle all the delicate init/exit cases, we convert the
whole thing to a table of init/exit pairs and track the init status in
the new init_progress global.  This allows us to ensure that i915_exit()
always tears down exactly the things that i915_init() successfully
initialized.  We also allow early-exit of i915_init() without failure by
an init function returning > 0.  This is useful for nomodeset, and
selftests.  For the mock selftests, we convert them to always return 1
so we get the desired behavior of the driver always succeeding to load
the driver and then properly tearing down the partially loaded driver.

v2 (Tvrtko Ursulin):
 - Guard init_funcs[i].exit with GEM_BUG_ON(i >= ARRAY_SIZE(init_funcs))
v2 (Daniel Vetter):
 - Update the docstring for i915.mock_selftests

Signed-off-by: default avatarJason Ekstrand <jason@jlekstrand.net>
Reviewed-by: default avatarDaniel Vetter <daniel.vetter@ffwll.ch>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: default avatarDaniel Vetter <daniel.vetter@ffwll.ch>
Link: https://patchwork.freedesktop.org/patch/msgid/20210721152358.2893314-4-jason@jlekstrand.net
      a04ea6ae
    • Faith Ekstrand's avatar
      drm/i915: Call i915_globals_exit() if pci_register_device() fails · db484889
      Faith Ekstrand authored 
      

In the unlikely event that pci_register_device() fails, we were tearing
down our PMU setup but not globals.  This leaves a bunch of memory slabs
lying around.

Signed-off-by: default avatarJason Ekstrand <jason@jlekstrand.net>
Fixes: 32eb6bcf

 ("drm/i915: Make request allocation caches global")
[danvet: Fix conflicts against removal of the globals_flush
infrastructure.]
Reviewed-by: default avatarDaniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: default avatarDaniel Vetter <daniel.vetter@ffwll.ch>
Link: https://patchwork.freedesktop.org/patch/msgid/20210721152358.2893314-3-jason@jlekstrand.net
      db484889
    • Faith Ekstrand's avatar
      drm/i915: Call i915_globals_exit() after i915_pmu_exit() · 75d3bf84
      Faith Ekstrand authored 
      

We should tear down in the opposite order we set up.

Signed-off-by: default avatarJason Ekstrand <jason@jlekstrand.net>
Reviewed-by: default avatarDaniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: default avatarTvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: default avatarDaniel Vetter <daniel.vetter@ffwll.ch>
Link: https://patchwork.freedesktop.org/patch/msgid/20210721152358.2893314-2-jason@jlekstrand.net
      75d3bf84
    • Daniel Vetter's avatar
      drm/i915: Ditch i915 globals shrink infrastructure · 4f62a7e0
      Daniel Vetter authored 
      This essentially reverts

commit 84a10749


Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Wed Jan 24 11:36:08 2018 +0000

    drm/i915: Shrink the GEM kmem_caches upon idling

mm/vmscan.c:do_shrink_slab() is a thing, if there's an issue with it
then we need to fix that there, not hand-roll our own slab shrinking
code in i915.

Also when this was added there was only one other caller of
kmem_cache_shrink (added 2005 to the acpi code). Now there's a 2nd one
outside of i915 code in a kunit test, which seems legit since that
wants to very carefully control what's in the kmem_cache. This out of
a total of over 500 calls to kmem_cache_create. This alone should have
been warning sign enough that we're doing something silly.

Noticed while reviewing a patch set from Jason to fix up some issues
in our i915_init() and i915_exit() module load/cleanup code. Now that
i915_globals.c isn't any different than normal init/exit functions, we
should convert them over to one unified table and remove
i915_globals.[hc] entirely.

v2: Improve commit message (Jason)

Reviewed-by: default avatarJason Ekstrand <jason@jlekstrand.net>
Cc: David Airlie <airlied@linux.ie>
Cc: Jason Ekstrand <jason@jlekstrand.net>
Signed-off-by: default avatarDaniel Vetter <daniel.vetter@intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20210721183229.4136488-1-daniel.vetter@ffwll.ch
      4f62a7e0
    • Matt Roper's avatar
      drm/i915: Make GT workaround upper bounds exclusive · 6b73a7f3
      Matt Roper authored 
      

Workarounds are documented in the bspec with an exclusive upper bound
(i.e., a "fixed" stepping that no longer needs the workaround).  This
makes our driver's use of an inclusive upper bound for stepping ranges
confusing; the differing notation between code and bspec makes it very
easy for mistakes to creep in.

Let's switch the upper bound of our IS_{GT,DISP}_STEP macros over to use
an exclusive upper bound like the bspec does.  This also has the benefit
of helping make sure workarounds are properly handled for new minor
steppings that show up (e.g., an A1 between the A0 and B0 we already
knew about) --- if the new intermediate stepping pulls in hardware fixes
early, there will be an update to the workaround definition which lets
us know we need to change our code.  If the new stepping does not pull a
hardware fix earlier, then the new stepping will already be captured
properly by the "[begin, fix)" range in the code.

We'll probably need to be extra vigilant in code review of new
workarounds for the near future to make sure developers notice the new
semantics of workaround bounds.  But we just migrated a bunch of our
platforms from the IS_REVID bounds over to IS_{GT,DISP}_STEP, so people
are already adjusting to the new macros and now is a good time to make
this change too.

[mattrope: Split out GT changes to apply through gt-next tree]
Signed-off-by: default avatarMatt Roper <matthew.d.roper@intel.com>
Reviewed-by: default avatarJosé Roberto de Souza <jose.souza@intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20210717051426.4120328-8-matthew.d.roper@intel.com
      6b73a7f3
    • Matt Roper's avatar
      drm/i915: Program DFR enable/disable as a GT workaround · 1e1824de
      Matt Roper authored 
      

DFR programming (which we enable as an optimization on gen11, but must
ensure is disabled on gen12) should be handled as a GT workaround rather
than clock gating initialization.  This will ensure that the programming
of these registers is verified with our typical workaround checks.

Signed-off-by: default avatarMatt Roper <matthew.d.roper@intel.com>
Reviewed-by: default avatarJosé Roberto de Souza <jose.souza@intel.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20210717051426.4120328-4-matthew.d.roper@intel.com
      1e1824de