John Harrison [Tue, 27 Jul 2021 00:23:36 +0000 (17:23 -0700)]
drm/i915/guc: Connect reset modparam updates to GuC policy flags
Changing the reset module parameter has no effect on a running GuC.
The corresponding entry in the ADS must be updated and then the GuC
informed via a Host2GuC message.
The new debugfs interface to module parameters allows this to happen.
However, connecting the parameter data address back to anything useful
is messy. One option would be to pass a new private data structure
address through instead of just the parameter pointer. However, that
means having a new (and different) data structure for each parameter
and a new (and different) write function for each parameter. This
method keeps everything generic by instead using a string lookup on
the directory entry name.
John Harrison [Tue, 27 Jul 2021 00:23:34 +0000 (17:23 -0700)]
drm/i915/guc: Fix for error capture after full GPU reset with GuC
In the case of a full GPU reset (e.g. because GuC has died or because
GuC's hang detection has been disabled), the driver can't rely on GuC
reporting the guilty context. Instead, the driver needs to scan all
active contexts and find one that is currently executing, as per the
execlist mode behaviour. In GuC mode, this scan is different to
execlist mode as the active request list is handled very differently.
Similarly, the request state dump in debugfs needs to be handled
differently when in GuC submission mode.
Also refactured some of the request scanning code to avoid duplication
across the multiple code paths that are now replicating it.
Matthew Brost [Tue, 27 Jul 2021 00:23:33 +0000 (17:23 -0700)]
drm/i915/guc: Capture error state on context reset
We receive notification of an engine reset from GuC at its
completion. Meaning GuC has potentially cleared any HW state
we may have been interested in capturing. GuC resumes scheduling
on the engine post-reset, as the resets are meant to be transparent,
further muddling our error state.
There is ongoing work to define an API for a GuC debug state dump. The
suggestion for now is to manually disable FW initiated resets in cases
where debug state is needed.
John Harrison [Tue, 27 Jul 2021 00:23:31 +0000 (17:23 -0700)]
drm/i915/guc: Don't complain about reset races
It is impossible to seal all race conditions of resets occurring
concurrent to other operations. At least, not without introducing
excesive mutex locking. Instead, don't complain if it occurs. In
particular, don't complain if trying to send a H2G during a reset.
Whatever the H2G was about should get redone once the reset is over.
John Harrison [Tue, 27 Jul 2021 00:23:30 +0000 (17:23 -0700)]
drm/i915/guc: Provide mmio list to be saved/restored on engine reset
The driver must provide GuC with a list of mmio registers
that should be saved/restored during a GuC-based engine reset.
Unfortunately, the list must be dynamically allocated as its size is
variable. That means the driver must generate the list twice - once to
work out the size and a second time to actually save it.
v2:
(Alan / CI)
- GEN7_GT_MODE -> GEN6_GT_MODE to fix WA selftest failure
Signed-off-by: John Harrison <John.C.Harrison@Intel.com> Signed-off-by: Fernando Pacheco <fernando.pacheco@intel.com> Signed-off-by: Matthew Brost <matthew.brost@intel.com> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Reviewed-by: Matthew Brost <matthew.brost@intel.com> Signed-off-by: John Harrison <John.C.Harrison@Intel.com> Link: https://patchwork.freedesktop.org/patch/msgid/20210727002348.97202-16-matthew.brost@intel.com
Matthew Brost [Tue, 27 Jul 2021 00:23:27 +0000 (17:23 -0700)]
drm/i915/guc: Handle context reset notification
GuC will issue a reset on detecting an engine hang and will notify
the driver via a G2H message. The driver will service the notification
by resetting the guilty context to a simple state or banning it
completely.
Matthew Brost [Tue, 27 Jul 2021 00:23:26 +0000 (17:23 -0700)]
drm/i915/guc: Suspend/resume implementation for new interface
The new GuC interface introduces an MMIO H2G command,
INTEL_GUC_ACTION_RESET_CLIENT, which is used to implement suspend. This
MMIO tears down any active contexts generating a context reset G2H CTB
for each. Once that step completes the GuC tears down the CTB
channels. It is safe to suspend once this MMIO H2G command completes
and all G2H CTBs have been processed. In practice the i915 will likely
never receive a G2H as suspend should only be called after the GPU is
idle.
Resume is implemented in the same manner as before - simply reload the
GuC firmware and reinitialize everything (e.g. CTB channels, contexts,
etc..).
v2:
(Michel / John H)
- INTEL_GUC_ACTION_RESET_CLIENT 0x5B01 -> 0x5507
Cc: John Harrison <john.c.harrison@intel.com> Signed-off-by: Matthew Brost <matthew.brost@intel.com> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com> Reviewed-by: John Harrison <John.C.Harrison@Intel.com> Signed-off-by: John Harrison <John.C.Harrison@Intel.com> Link: https://patchwork.freedesktop.org/patch/msgid/20210727002348.97202-12-matthew.brost@intel.com
Matthew Brost [Tue, 27 Jul 2021 00:23:24 +0000 (17:23 -0700)]
drm/i915: Reset GPU immediately if submission is disabled
If submission is disabled by the backend for any reason, reset the GPU
immediately in the heartbeat code as the backend can't be reenabled
until the GPU is reset.
Matthew Brost [Tue, 27 Jul 2021 00:23:23 +0000 (17:23 -0700)]
drm/i915/guc: Reset implementation for new GuC interface
Reset implementation for new GuC interface. This is the legacy reset
implementation which is called when the i915 owns the engine hang check.
Future patches will offload the engine hang check to GuC but we will
continue to maintain this legacy path as a fallback and this code path
is also required if the GuC dies.
With the new GuC interface it is not possible to reset individual
engines - it is only possible to reset the GPU entirely. This patch
forces an entire chip reset if any engine hangs.
v2:
(Michal)
- Check for -EPIPE rather than -EIO (CT deadlock/corrupt check)
v3:
(John H)
- Split into a series of smaller patches
v4:
(John H)
- Fix typo
- Add braces around if statements in reset code
v5:
(Checkpatch)
- Fix warnings
Matthew Brost [Tue, 27 Jul 2021 00:23:22 +0000 (17:23 -0700)]
drm/i915: Move active request tracking to a vfunc
Move active request tracking to a backend vfunc rather than assuming all
backends want to do this in the manner. In the of case execlists /
ring submission the tracking is on the physical engine while with GuC
submission it is on the context.
Matthew Brost [Tue, 27 Jul 2021 00:23:20 +0000 (17:23 -0700)]
drm/i915/guc: Direct all breadcrumbs for a class to single breadcrumbs
With GuC virtual engines the physical engine which a request executes
and completes on isn't known to the i915. Therefore we can't attach a
request to a physical engines breadcrumbs. To work around this we create
a single breadcrumbs per engine class when using GuC submission and
direct all physical engine interrupts to this breadcrumbs.
v2:
(John H)
- Rework header file structure so intel_engine_mask_t can be in
intel_engine_types.h
Matthew Brost [Tue, 27 Jul 2021 00:23:19 +0000 (17:23 -0700)]
drm/i915/guc: Disable bonding extension with GuC submission
Update the bonding extension to return -ENODEV when using GuC submission
as this extension fundamentally will not work with the GuC submission
interface.
Matthew Brost [Tue, 27 Jul 2021 00:23:18 +0000 (17:23 -0700)]
drm/i915: Hold reference to intel_context over life of i915_request
Hold a reference to the intel_context over life of an i915_request.
Without this an i915_request can exist after the context has been
destroyed (e.g. request retired, context closed, but user space holds a
reference to the request from an out fence). In the case of GuC
submission + virtual engine, the engine that the request references is
also destroyed which can trigger bad pointer dref in fence ops (e.g.
i915_fence_get_driver_name). We could likely change
i915_fence_get_driver_name to avoid touching the engine but let's just
be safe and hold the intel_context reference.
v2:
(John Harrison)
- Update comment explaining how GuC mode and execlists mode deal with
virtual engines differently
John Harrison [Tue, 27 Jul 2021 00:23:17 +0000 (17:23 -0700)]
drm/i915/guc: Make hangcheck work with GuC virtual engines
The serial number tracking of engines happens at the backend of
request submission and was expecting to only be given physical
engines. However, in GuC submission mode, the decomposition of virtual
to physical engines does not happen in i915. Instead, requests are
submitted to their virtual engine mask all the way through to the
hardware (i.e. to GuC). This would mean that the heart beat code
thinks the physical engines are idle due to the serial number not
incrementing. Which in turns means hangcheck does not work for
GuC virtual engines.
This patch updates the tracking to decompose virtual engines into
their physical constituents and tracks the request against each. This
is not entirely accurate as the GuC will only be issuing the request
to one physical engine. However, it is the best that i915 can do given
that it has no knowledge of the GuC's scheduling decisions.
Downside of this is that all physical engines constituting a GuC
virtual engine will be periodically unparked (even during just a single
context executing) in order to be pinged with a heartbeat request.
However the power and performance cost of this is not expected to be
measurable (due low frequency of heartbeat pulses) and it is considered
an easier option than trying to make changes to GuC firmware.
v2:
(Tvrtko)
- Update commit message
- Have default behavior if no vfunc present
Matthew Brost [Tue, 27 Jul 2021 00:23:16 +0000 (17:23 -0700)]
drm/i915/guc: GuC virtual engines
Implement GuC virtual engines. Rather simple implementation, basically
just allocate an engine, setup context enter / exit function to virtual
engine specific functions, set all other variables / functions to guc
versions, and set the engine mask to that of all the siblings.
v2: Update to work with proto-ctx
v3:
(Daniele)
- Drop include, add comment to intel_virtual_engine_has_heartbeat
Matthew Auld [Fri, 23 Jul 2021 10:50:45 +0000 (11:50 +0100)]
drm/i915/ehl: unconditionally flush the pages on acquire
EHL and JSL add the 'Bypass LLC' MOCS entry, which should make it
possible for userspace to bypass the GTT caching bits set by the kernel,
as per the given object cache_level. This is troublesome since the heavy
flush we apply when first acquiring the pages is skipped if the kernel
thinks the object is coherent with the GPU. As a result it might be
possible to bypass the cache and read the contents of the page directly,
which could be stale data. If it's just a case of userspace shooting
themselves in the foot then so be it, but since i915 takes the stance of
always zeroing memory before handing it to userspace, we need to prevent
this.
v2: this time actually set cache_dirty in put_pages()
v3: move to get_pages() which looks simpler
BSpec: 34007
References: dbd3c2e04bef ("Revert "drm/i915/ehl: Update MOCS table for EHL"") Signed-off-by: Matthew Auld <matthew.auld@intel.com> Cc: Tejas Upadhyay <tejaskumarx.surendrakumar.upadhyay@intel.com> Cc: Francisco Jerez <francisco.jerez.plata@intel.com> Cc: Lucas De Marchi <lucas.demarchi@intel.com> Cc: Jon Bloomfield <jon.bloomfield@intel.com> Cc: Chris Wilson <chris.p.wilson@intel.com> Cc: Matt Roper <matthew.d.roper@intel.com> Cc: Daniel Vetter <daniel@ffwll.ch> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Link: https://patchwork.freedesktop.org/patch/msgid/20210723105045.400841-2-matthew.auld@intel.com
Matthew Auld [Fri, 23 Jul 2021 10:50:44 +0000 (11:50 +0100)]
drm/i915: document caching related bits
Try to document the object caching related bits, like cache_coherent and
cache_dirty.
v2(Ville):
- As pointed out by Ville, fix the completely incorrect assumptions
about the "partial" coherency on shared LLC platforms.
v3(Daniel):
- Fix nonsense about "dirtying" the cache with reads.
v4(Daniel):
- Various improvements, including adding some more details for WT.
Thomas Hellström [Fri, 23 Jul 2021 17:21:42 +0000 (12:21 -0500)]
drm/i915/gem: Migrate to system at dma-buf attach time (v7)
Until we support p2p dma or as a complement to that, migrate data
to system memory at dma-buf attach time if possible.
v2:
- Rebase on dynamic exporter. Update the igt_dmabuf_import_same_driver
selftest to migrate if we are LMEM capable.
v3:
- Migrate also in the pin() callback.
v4:
- Migrate in attach
v5: (jason)
- Lock around the migration
v6: (jason)
- Move the can_migrate check outside the lock
- Rework the selftests to test more migration conditions. In
particular, SMEM, LMEM, and LMEM+SMEM are all checked.
v7: (mauld)
- Misc style nits
Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com> Reported-by: kernel test robot <lkp@intel.com> Signed-off-by: Jason Ekstrand <jason@jlekstrand.net> Reviewed-by: Jason Ekstrand <jason@jlekstrand.net> Signed-off-by: Matthew Auld <matthew.auld@intel.com> Link: https://patchwork.freedesktop.org/patch/msgid/20210723172142.3273510-9-jason@jlekstrand.net
Thomas Hellström [Fri, 23 Jul 2021 17:21:41 +0000 (12:21 -0500)]
drm/i915/gem: Correct the locking and pin pattern for dma-buf (v8)
If our exported dma-bufs are imported by another instance of our driver,
that instance will typically have the imported dma-bufs locked during
dma_buf_map_attachment(). But the exporter also locks the same reservation
object in the map_dma_buf() callback, which leads to recursive locking.
So taking the lock inside _pin_pages_unlocked() is incorrect.
Additionally, the current pinning code path is contrary to the defined
way that pinning should occur.
Remove the explicit pin/unpin from the map/umap functions and move them
to the attach/detach allowing correct locking to occur, and to match
the static dma-buf drm_prime pattern.
Add a live selftest to exercise both dynamic and non-dynamic
exports.
v2:
- Extend the selftest with a fake dynamic importer.
- Provide real pin and unpin callbacks to not abuse the interface.
v3: (ruhl)
- Remove the dynamic export support and move the pinning into the
attach/detach path.
v4: (ruhl)
- Put pages does not need to assert on the dma-resv
v5: (jason)
- Lock around dma_buf_unmap_attachment() when emulating a dynamic
importer in the subtests.
- Use pin_pages_unlocked
v6: (jason)
- Use dma_buf_attach instead of dma_buf_attach_dynamic in the selftests
v7: (mauld)
- Use __i915_gem_object_get_pages (2 __underscores) instead of the
4 ____underscore version in the selftests
v8: (mauld)
- Drop the kernel doc from the static i915_gem_dmabuf_attach function
- Add missing "err = PTR_ERR()" to a bunch of selftest error cases
Reported-by: Michael J. Ruhl <michael.j.ruhl@intel.com> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com> Signed-off-by: Michael J. Ruhl <michael.j.ruhl@intel.com> Signed-off-by: Jason Ekstrand <jason@jlekstrand.net> Reviewed-by: Jason Ekstrand <jason@jlekstrand.net> Signed-off-by: Matthew Auld <matthew.auld@intel.com> Link: https://patchwork.freedesktop.org/patch/msgid/20210723172142.3273510-8-jason@jlekstrand.net
Without TTM, we have no such hook so we exit early but this is fine
because we use TTM on all LMEM platforms and, on integrated platforms,
there is no real migration. If we do have the hook, it's better to just
let TTM handle the migration because it knows where things are actually
placed.
This fixes a bug where i915_gem_object_migrate fails to migrate newly
created LMEM objects. In that scenario, the object has obj->mm.region
set to LMEM but TTM has it in SMEM because that's where all new objects
are placed there prior to getting actual pages. When we invoke
i915_gem_object_migrate, it exits early because, from the point of view
of the GEM object, it's already in LMEM and no migration is needed.
Then, when we try to pin the pages, __i915_ttm_get_pages is called
which, unaware of our failed attempt at a migration, places the object
in SMEM. This only happens on newly created objects because they have
this weird state where TTM thinks they're in SMEM, GEM thinks they're in
LMEM, and the reality is that they don't exist at all.
It's better if GEM just always calls into TTM and let's TTM handle
things. That way the lies stay better contained. Once the migration is
complete, the object will have pages, obj->mm.region will be correct,
and we're done lying.
Jason Ekstrand [Fri, 23 Jul 2021 17:21:39 +0000 (12:21 -0500)]
drm/i915/gem/ttm: Only call __i915_gem_object_set_pages if needed
__i915_ttm_get_pages does two things. First, it calls ttm_bo_validate()
to check the given placement and migrate the BO if needed. Then, it
updates the GEM object to match, in case the object was migrated. If
no migration occured, however, we might still have pages on the GEM
object in which case we don't need to fetch them from TTM and call
__i915_gem_object_set_pages. This hasn't been a problem before because
the primary user of __i915_ttm_get_pages is __i915_gem_object_get_pages
which only calls it if the GEM object doesn't have pages.
However, i915_ttm_migrate also uses __i915_ttm_get_pages to do the
migration so this meant it was unsafe to call on an already populated
object. This patch checks i915_gem_object_has_pages() before trying to
__i915_gem_object_set_pages so i915_ttm_migrate is safe to call, even on
populated objects.
Jason Ekstrand [Fri, 23 Jul 2021 17:21:38 +0000 (12:21 -0500)]
drm/i915/gem: Unify user object creation (v3)
Instead of hand-rolling the same three calls in each function, pull them
into an i915_gem_object_create_user helper. Apart from re-ordering of
the placements array ENOMEM check, there should be no functional change.
v2 (Matthew Auld):
- Add the call to i915_gem_flush_free_objects() from
i915_gem_dumb_create() in a separate patch
- Move i915_gem_object_alloc() below the simple error checks
v3 (Matthew Auld):
- Add __ to i915_gem_object_create_user and kerneldoc which warns the
caller that it's not validating anything.
Jason Ekstrand [Fri, 23 Jul 2021 17:21:37 +0000 (12:21 -0500)]
drm/i915/gem: Call i915_gem_flush_free_objects() in i915_gem_dumb_create()
This doesn't really fix anything serious since the chances of a client
creating and destroying a mass of dumb BOs is pretty low. However, it
is called by the other two create IOCTLs to garbage collect old objects.
Call it here too for consistency.
Jason Ekstrand [Fri, 23 Jul 2021 17:21:36 +0000 (12:21 -0500)]
drm/i915/gem: Refactor placement setup for i915_gem_object_create* (v2)
Since we don't allow changing the set of regions after creation, we can
make ext_set_placements() build up the region set directly in the
create_ext and assign it to the object later. This is similar to what
we did for contexts with the proto-context only simpler because there's
no funny object shuffling. This will be used in the next patch to allow
us to de-duplicate a bunch of code. Also, since we know the maximum
number of regions up-front, we can use a fixed-size temporary array for
the regions. This simplifies memory management a bit for this new
delayed approach.
v2 (Matthew Auld):
- Get rid of MAX_N_PLACEMENTS
- Drop kfree(placements) from set_placements()
v3 (Matthew Auld):
- Properly set ext_data->n_placements
Jason Ekstrand [Fri, 23 Jul 2021 17:21:35 +0000 (12:21 -0500)]
drm/i915/gem: Check object_can_migrate from object_migrate
We don't roll them together entirely because there are still a couple
cases where we want a separate can_migrate check. For instance, the
display code checks that you can migrate a buffer to LMEM before it
accepts it in fb_create. The dma-buf import code also uses it to do an
early check and return a different error code if someone tries to attach
a LMEM-only dma-buf to another driver.
However, no one actually wants to call object_migrate when can_migrate
has failed. The stated intention is for self-tests but none of those
actually take advantage of this unsafe migration.
Lucas De Marchi [Fri, 23 Jul 2021 00:25:51 +0000 (17:25 -0700)]
drm/i915/gt: nuke gen6_hw_id
This is only used by GRAPHICS_VER == 6 and GRAPHICS_VER == 7. All other
recent platforms do not depend on this field, so it doesn't make much
sense to keep it generic like that. Instead, just do a mapping from
engine class to HW ID in the single place that is needed.
v2: use macros with the direct register address instead of calculating
from the legacy HW_ID (Matt Roper)
John Harrison [Fri, 23 Jul 2021 19:10:24 +0000 (12:10 -0700)]
drm/i915/xehp: Extra media engines - Part 1 (engine definitions)
Xe_HP can have a lot of extra media engines. This patch adds the basic
definitions for them.
v2:
- Re-order intel_gt_info and intel_device_info slightly to avoid
unnecessary padding now that we've increased the size of
intel_engine_mask_t. (Tvrtko)
v3:
- Drop the .hw_id assignments. (Lucas)
v4:
- Fix graphics_ver typo for VCS4 (should be 12, not 11). (Lucas)
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Cc: Lucas De Marchi <lucas.demarchi@intel.com> Signed-off-by: John Harrison <John.C.Harrison@Intel.com> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com> Signed-off-by: Matt Roper <matthew.d.roper@intel.com> Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com> Link: https://patchwork.freedesktop.org/patch/msgid/20210723191024.1553405-1-matthew.d.roper@intel.com
Matt Roper [Wed, 14 Jul 2021 03:15:00 +0000 (20:15 -0700)]
drm/i915/xehp: Define multicast register ranges
Since we can't steer multicast register reads during ring-based
workaround verification, we need to define the multicast ranges where
failure to steer could potentially cause us to read back from a
fused-off register instance.
As with gen12, we can ignore the multicast ranges that the bspec
describes as 'SQIDI' since all instances of those registers will always
be present and we'll always be able to read back a workaround value that
was written with multicast.
Matthew Auld [Thu, 15 Jul 2021 10:15:36 +0000 (11:15 +0100)]
drm/i915/uapi: reject set_domain for discrete
The CPU domain should be static for discrete, and on DG1 we don't need
any flushing since everything is already coherent, so really all this
does is an object wait, for which we have an ioctl. Longer term the
desired caching should be an immutable creation time property for the
BO, which can be set with something like gem_create_ext.
One other user is iris + userptr, which uses the set_domain to probe all
the pages to check if the GUP succeeds, however we now have a PROBE
flag for this purpose.
v2: add some more kernel doc, also add the implicit rules with caching
Suggested-by: Daniel Vetter <daniel@ffwll.ch> Signed-off-by: Matthew Auld <matthew.auld@intel.com> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> Cc: Jordan Justen <jordan.l.justen@intel.com> Cc: Kenneth Graunke <kenneth@whitecape.org> Cc: Jason Ekstrand <jason@jlekstrand.net> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: Ramalingam C <ramalingam.c@intel.com> Reviewed-by: Ramalingam C <ramalingam.c@intel.com> Acked-by: Kenneth Graunke <kenneth@whitecape.org> Link: https://patchwork.freedesktop.org/patch/msgid/20210715101536.2606307-5-matthew.auld@intel.com
drm/i915/xehp: VDBOX/VEBOX fusing registers are enable-based
On Xe_HP the fusing register is renamed and changed to have the "enable"
semantics, but otherwise remains compatible (mmio address, bitmask
ranges) with older platforms.
To simplify things we do not add a new register definition but just stop
inverting the fusing masks before processing them.
Bspec: 52615 Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Signed-off-by: Matt Roper <matthew.d.roper@intel.com> Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com> Reviewed-by: Matt Atwood <matthew.s.atwood@intel.com> Link: https://patchwork.freedesktop.org/patch/msgid/20210721223043.834562-6-matthew.d.roper@intel.com
Lucas De Marchi [Tue, 20 Jul 2021 23:20:13 +0000 (16:20 -0700)]
drm/i915/gt: rename legacy engine->hw_id to engine->gen6_hw_id
We kept adding new engines and for that increasing hw_id unnecessarily:
it's not used since GRAPHICS_VER == 8. Prepend "gen6" to the field and
try to pack it in the structs to give a hint this field is actually not
used in recent platforms.
Lucas De Marchi [Tue, 20 Jul 2021 23:20:12 +0000 (16:20 -0700)]
drm/i915/gt: nuke unused legacy engine hw_id
The engine hw_id is only used by RING_FAULT_REG(), which is not used
by GRAPHICS_VER >= 8. We did use hw_id on recent platforms to set
the engine's guc_id, but that is not the case anymore since
commit 84c63b43eef2 ("drm/i915/guc: Update to use firmware v49.0.1"):
now we only use class and id information to generate guc_id.
We tend to keep adding new defines just to be consistent, but let's try
to remove them and let them defined to 0 for engines that only exist on
gen8+ platforms.
v2: Reword commit message and add information about when we stopped
using hw_id (Matt Roper)
Lucas De Marchi [Tue, 20 Jul 2021 23:20:11 +0000 (16:20 -0700)]
drm/i915/gt: fix platform prefix
gen8_clear_engine_error_register() is actually not used by
GRAPHICS_VER >= 8, since for those we are using another register that is
not engine-dependent. Fix the platform prefix, to make clear we are not
using any GEN6_RING_FAULT_REG_* one GRAPHICS_VER >= 8.
Matthew Brost [Wed, 21 Jul 2021 21:51:01 +0000 (14:51 -0700)]
drm/i915: Add intel_context tracing
Add intel_context tracing. These trace points are particular helpful
when debugging the GuC firmware and can be enabled via
CONFIG_DRM_I915_LOW_LEVEL_TRACEPOINTS kernel config option.
Matthew Brost [Wed, 21 Jul 2021 21:50:58 +0000 (14:50 -0700)]
drm/i915/guc: Update intel_gt_wait_for_idle to work with GuC
When running the GuC the GPU can't be considered idle if the GuC still
has contexts pinned. As such, a call has been added in
intel_gt_wait_for_idle to idle the UC and in turn the GuC by waiting for
the number of unpinned contexts to go to zero.
v2: rtimeout -> remaining_timeout
v3: Drop unnecessary includes, guc_submission_busy_loop ->
guc_submission_send_busy_loop, drop negatie timeout trick, move a
refactor of guc_context_unpin to earlier path (John H)
v4: Add stddef.h back into intel_gt_requests.h, sort circuit idle
function if not in GuC submission mode
Matthew Brost [Wed, 21 Jul 2021 21:50:56 +0000 (14:50 -0700)]
drm/i915/guc: Disable semaphores when using GuC scheduling
Semaphores are an optimization and not required for basic GuC submission
to work properly. Disable until we have time to do the implementation to
enable semaphores and tune them for performance. Also long direction is
just to delete semaphores from the i915 so another reason to not enable
these for GuC submission.
This patch fixes an existing bugs where I915_ENGINE_HAS_SEMAPHORES was
not honored correctly.
v2: Reword commit message
v3:
(John H)
- Add text to commit indicating this also fixing an existing bug
v4:
(John H)
- s/bug/bugs
Matthew Brost [Wed, 21 Jul 2021 21:50:55 +0000 (14:50 -0700)]
drm/i915/guc: Ensure request ordering via completion fences
If two requests are on the same ring, they are explicitly ordered by the
HW. So, a submission fence is sufficient to ensure ordering when using
the new GuC submission interface. Conversely, if two requests share a
timeline and are on the same physical engine but different context this
doesn't ensure ordering on the new GuC submission interface. So, a
completion fence needs to be used to ensure ordering.
Matthew Brost [Wed, 21 Jul 2021 21:50:52 +0000 (14:50 -0700)]
drm/i915/guc: Disable engine barriers with GuC during unpin
Disable engine barriers for unpinning with GuC. This feature isn't
needed with the GuC as it disables context scheduling before unpinning
which guarantees the HW will not reference the context. Hence it is
not necessary to defer unpinning until a kernel context request
completes on each engine in the context engine mask.
Cc: John Harrison <john.c.harrison@intel.com> Signed-off-by: Matthew Brost <matthew.brost@intel.com> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Reviewed-by: John Harrison <John.C.Harrison@Intel.com> Signed-off-by: John Harrison <John.C.Harrison@Intel.com> Link: https://patchwork.freedesktop.org/patch/msgid/20210721215101.139794-10-matthew.brost@intel.com
Matthew Brost [Wed, 21 Jul 2021 21:50:51 +0000 (14:50 -0700)]
drm/i915/guc: Defer context unpin until scheduling is disabled
With GuC scheduling, it isn't safe to unpin a context while scheduling
is enabled for that context as the GuC may touch some of the pinned
state (e.g. LRC). To ensure scheduling isn't enabled when an unpin is
done, a call back is added to intel_context_unpin when pin count == 1
to disable scheduling for that context. When the response CTB is
received it is safe to do the final unpin.
Future patches may add a heuristic / delay to schedule the disable
call back to avoid thrashing on schedule enable / disable.
v2:
(John H)
- s/drm_dbg/drm_err
(Daneiel)
- Clean up sched state function
Matthew Brost [Wed, 21 Jul 2021 21:50:50 +0000 (14:50 -0700)]
drm/i915/guc: Insert fence on context when deregistering
Sometimes during context pinning a context with the same guc_id is
registered with the GuC. In this a case deregister must be done before
the context can be registered. A fence is inserted on all requests while
the deregister is in flight. Once the G2H is received indicating the
deregistration is complete the context is registered and the fence is
released.
Matthew Brost [Wed, 21 Jul 2021 21:50:49 +0000 (14:50 -0700)]
drm/i915/guc: Implement GuC context operations for new inteface
Implement GuC context operations which includes GuC specific operations
alloc, pin, unpin, and destroy.
v2:
(Daniel Vetter)
- Use msleep_interruptible rather than cond_resched in busy loop
(Michal)
- Remove C++ style comment
v3:
(Matthew Brost)
- Drop GUC_ID_START
(John Harrison)
- Fix a bunch of typos
- Use drm_err rather than drm_dbg for G2H errors
(Daniele)
- Fix ;; typo
- Clean up sched state functions
- Add lockdep for guc_id functions
- Don't call __release_guc_id when guc_id is invalid
- Use MISSING_CASE
- Add comment in guc_context_pin
- Use shorter path to rpm
(Daniele / CI)
- Don't call release_guc_id on an invalid guc_id in destroy
v4:
(Daniel Vetter)
- Add FIXME comment
Matthew Brost [Wed, 21 Jul 2021 21:50:47 +0000 (14:50 -0700)]
drm/i915/guc: Implement GuC submission tasklet
Implement GuC submission tasklet for new interface. The new GuC
interface uses H2G to submit contexts to the GuC. Since H2G use a single
channel, a single tasklet is used for the submission path.
Also the per engine interrupt handler has been updated to disable the
rescheduling of the physical engine tasklet, when using GuC scheduling,
as the physical engine tasklet is no longer used.
In this patch the field, guc_id, has been added to intel_context and is
not assigned. Patches later in the series will assign this value.
v2:
(John Harrison)
- Clean up some comments
v3:
(John Harrison)
- More comment cleanups
Add LRC descriptor context lookup array which can resolve the
intel_context from the LRC descriptor index. In addition to lookup, it
can determine if the LRC descriptor context is currently registered with
the GuC by checking if an entry for a descriptor index is present.
Future patches in the series will make use of this array.
In Gen12 there are various fuse combinations and in each configuration
vdbox engine may be connected to SFC depending on which engines are
available, so we need to set the SFC capability based on fuse value from
the hardware. Even numbered physical instance always have SFC, odd
numbered physical instances have SFC only if previous even instance is
fused off.
v2:
- Minor style & typo fixes (Tvrtko)
- Drop an unwanted 'inline' (Tvrtko)
Bspec: 48028 Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Signed-off-by: Venkata Sandeep Dhanalakota <venkata.s.dhanalakota@intel.com> Signed-off-by: Matt Roper <matthew.d.roper@intel.com> Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com> Link: https://patchwork.freedesktop.org/patch/msgid/20210721223043.834562-7-matthew.d.roper@intel.com
Matt Roper [Thu, 22 Jul 2021 16:35:16 +0000 (09:35 -0700)]
Merge branch 'topic/xehp-dg2-definitions-2021-07-21' into drm-intel-gt-next
As we begin applying XeHP and DG2 patches, the basic platform
definitions and macros (like IS_DG2()) will be needed in both
drm-intel-next and drm-intel-gt-next. Those initial definition patches
are applied to a topic branch and merged to both trees.
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
Matt Roper [Wed, 21 Jul 2021 22:30:28 +0000 (15:30 -0700)]
drm/i915/dg2: add DG2 platform info
DG2 has Xe_LPD display (version 13) and Xe_HPG (version 12.55) graphics.
There are two variants (treated as subplatforms in the code): DG2-G10
and DG2-G11 that require independent programming in some areas (e.g.,
workarounds).
XeHP SDV is a Intel® dGPU without display. This is just the definition
of some basic platform macros, by large a copy of current state of
Tigerlake which does not reflect the end state of this platform.
v2:
- Switch to intel_step infrastructure for stepping matches. (Jani)
v3:
- Bring earlier in patch series and leave addition of new media engines
to the engine mask for a later patch.
Bspec: 44467, 48077 Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> Signed-off-by: José Roberto de Souza <jose.souza@intel.com> Signed-off-by: Stuart Summers <stuart.summers@intel.com> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com> Signed-off-by: Matt Roper <matthew.d.roper@intel.com> Reviewed-by: José Roberto de Souza <jose.souza@intel.com> Link: https://patchwork.freedesktop.org/patch/msgid/20210721223043.834562-3-matthew.d.roper@intel.com
Lucas De Marchi [Wed, 21 Jul 2021 22:30:26 +0000 (15:30 -0700)]
drm/i915: Add XE_HP initial definitions
Our _FEATURES macro went back to GEN7, extending each other, making it
difficult to grasp what was really enabled/disabled. Take the
opportunity of the GEN -> XE_HP name break and also break with the
feature inheritance.
For XE_HP this basically goes from GEN12 back to GEN7 coalescing the
features making sure the overrides remain, remove all the
display-specific features and sort it.
Then also remove the definitions that would be overridden by
DGFX_FEATURES and those that were 0 (since that is the default).
Exception here is has_master_unit_irq: although it is a feature that
started with DG1 and is true for all DGFX platforms, it's also true for
XE_HP in general.
Lucas De Marchi [Wed, 7 Jul 2021 23:59:21 +0000 (16:59 -0700)]
drm/i915: Add release id version
Besides the arch version returned by GRAPHICS_VER(), new platforms
contain a "release id" to make clear the difference from one platform to
another.
The release id number is not formally defined by hardware until future
platforms that will expose it via a new GMD_ID register. For the
platforms we support before that register becomes available we will set
the values in software and we can set them as we please. So the plan is
to set them so we can group different features under a single
GRAPHICS_VER_FULL() check.
After GMD_ID is used, the usefulness of a "full version check" will be
greatly reduced and will be mostly used for deciding workarounds and a
few code paths. So it makes sense to keep it as a separate field from
graphics_ver. Also, as a platform with `release == n` may be closer
feature-wise to `n - 2` than to `n - 1`, use the word "release" rather
than the more common "minor" for this
This is a mix of 2 independent changes: one by me and the other by Matt
Roper.
v2:
- Reword commit message to make it clearer why we don't call it
"minor" (Matt Roper and Tvrtko)
- Rename variables s/*_ver_release/*_rel/ and print them in a single
line formatted as {ver}.{rel:2} (Jani and Matt Roper)
Jason Ekstrand [Wed, 21 Jul 2021 15:23:58 +0000 (10:23 -0500)]
drm/i915: Make the kmem slab for i915_buddy_block a global
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: Jason Ekstrand <jason@jlekstrand.net> Fixes: 34ec12e31296 ("drm/i915/ttm: add ttm_buddy_man") Reviewed-by: Matthew 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: Daniel Vetter <daniel.vetter@ffwll.ch> Link: https://patchwork.freedesktop.org/patch/msgid/20210721152358.2893314-7-jason@jlekstrand.net
Jason Ekstrand [Wed, 21 Jul 2021 15:23:55 +0000 (10:23 -0500)]
drm/i915: Use a table for i915_init/exit (v2)
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
Jason Ekstrand [Wed, 21 Jul 2021 15:23:54 +0000 (10:23 -0500)]
drm/i915: Call i915_globals_exit() if pci_register_device() fails
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: Jason Ekstrand <jason@jlekstrand.net> Fixes: a42894693d68 ("drm/i915: Make request allocation caches global")
[danvet: Fix conflicts against removal of the globals_flush
infrastructure.] Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> Link: https://patchwork.freedesktop.org/patch/msgid/20210721152358.2893314-3-jason@jlekstrand.net
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.
Matt Roper [Sat, 17 Jul 2021 05:14:26 +0000 (22:14 -0700)]
drm/i915: Make GT workaround upper bounds exclusive
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.
Matt Roper [Sat, 17 Jul 2021 05:14:22 +0000 (22:14 -0700)]
drm/i915: Program DFR enable/disable as a GT workaround
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.
Matt Roper [Sat, 17 Jul 2021 05:14:21 +0000 (22:14 -0700)]
drm/i915/icl: Drop a couple unnecessary workarounds
While doing a quick sanity check of the ICL workarounds in the driver I
noticed a few things that should be updated:
* There's no mention in the bspec that WaPipelineFlushCoherentLines
is needed on gen11 (both the current WA database and the old,
deprecated page 20196 were checked); it appears this might have just
been copied from the gen9 list? Even if this were needed, it doesn't
seem like this was the correct implementation anyway since the gen9
workaround is supposed to be implemented in the indirect context bb
(as we do in gen8_emit_flush_coherentl3_wa() on gen8/gen9).
* WaForwardProgressSoftReset does not appear in the current workaround
database. The old deprecated workaround list has a note indicating
the workaround was dropped in 2017, so we should be safe to drop it
from the code too.
While we're at it, add the formal workaround ID number to
WaDisableBankHangMode (our hardware team made a transition from
text-based workaround names to ID numbers partway through the
development of ICL, which is why some workarounds only have names, some
only have numbers, and some have both).
Matt Roper [Sat, 17 Jul 2021 05:14:20 +0000 (22:14 -0700)]
drm/i915: Fix application of WaInPlaceDecompressionHang
On SKL we've been applying this workaround on H0+ steppings, which is
actually backwards; H0 is supposed to be the first stepping where the
workaround is no longer needed. Flip the bounds so that the workaround
applies to all steppings _before_ H0.
On BXT we've been applying this workaround to all steppings, but the
bspec tells us it's only needed until C0. Pre-C0 GT steppings only
appeared in pre-production hardware, which we no longer support in the
driver, so we can drop the workaround completely for this platform.
On ICL we've been applying this workaround to all steppings, but there
doesn't seem to be any indication that this workaround was ever needed
for this platform (even now-deprecated page 20196 of the bspec doesn't
mention it). We can go ahead and drop it.
I also don't see any mention of this workaround being needed for KBL,
although this may be an oversight since the workaround is needed for all
steppings of CFL. I'll leave the workaround in place for KBL to be
safe.
The FIXED mapping is only used for ttm, and tells userspace that the
mapping type is pre-defined. This disables the other type of mmap
offsets when discrete memory is used, so fix the selftests as well.
Document the struct as well, so it shows up in docbook.
Cc: Jason Ekstrand <jason@jlekstrand.net> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> Reviewed-by: Jason Ekstrand <jason@jlekstrand.net> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
[mauld: Included minor fixes from the review comments] Signed-off-by: Matthew Auld <matthew.auld@intel.com> Link: https://patchwork.freedesktop.org/patch/msgid/20210714122833.766586-1-maarten.lankhorst@linux.intel.com
Jason Ekstrand [Tue, 20 Jul 2021 18:21:08 +0000 (13:21 -0500)]
drm/i915: Correct the docs for intel_engine_cmd_parser
In e1cba7888ba7 ("drm/i915: Revert "drm/i915/gem: Asynchronous
cmdparser""), the parameters to intel_engine_cmd_parser() were altered
without updating the docs, causing Fi.CI.DOCS to start failing.
Fixes: e1cba7888ba7 ("drm/i915: Revert "drm/i915/gem: Asynchronous cmdparser"") Signed-off-by: Jason Ekstrand <jason@jlekstrand.net> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> Link: https://patchwork.freedesktop.org/patch/msgid/20210720182108.2761496-1-jason@jlekstrand.net Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
[Added 'Fixes:' tag and corrected the hash for the ancestor]
Matthew Auld [Thu, 15 Jul 2021 10:15:33 +0000 (11:15 +0100)]
drm/i915/uapi: reject caching ioctls for discrete
It's a noop on DG1, and in the future when need to support other devices
which let us control the coherency, then it should be an immutable
creation time property for the BO. This will likely be controlled
through a new gem_create_ext extension.
v2: add some kernel doc for the discrete changes, and document the
implicit rules
Suggested-by: Daniel Vetter <daniel@ffwll.ch> Signed-off-by: Matthew Auld <matthew.auld@intel.com> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> Cc: Jordan Justen <jordan.l.justen@intel.com> Cc: Kenneth Graunke <kenneth@whitecape.org> Cc: Jason Ekstrand <jason@jlekstrand.net> Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: Ramalingam C <ramalingam.c@intel.com> Reviewed-by: Ramalingam C <ramalingam.c@intel.com> Reviewed-by: Kenneth Graunke <kenneth@whitecape.org> Link: https://patchwork.freedesktop.org/patch/msgid/20210715101536.2606307-2-matthew.auld@intel.com
Jason Ekstrand [Wed, 14 Jul 2021 19:34:19 +0000 (14:34 -0500)]
Revert "drm/i915: Skip over MI_NOOP when parsing"
This reverts a67a096e9137 ("drm/i915: Skip over MI_NOOP when parsing").
It complicates the batch parsing code a bit and increases indentation
for no reason other than fast-skipping a command that userspace uses
only rarely. Sure, there may be IGT tests that fill batches with NOOPs
but that's not a case we should optimize for in the kernel. We should
optimize for code clarity instead.
Jason Ekstrand [Wed, 14 Jul 2021 19:34:18 +0000 (14:34 -0500)]
drm/i915: Drop error handling from dma_fence_work
Asynchronous command parsing was the only thing which ever returned a
non-zero error. With that gone, we can drop the error handling from
dma_fence_work.
Jason Ekstrand [Wed, 14 Jul 2021 19:34:17 +0000 (14:34 -0500)]
drm/i915: Remove allow_alloc from i915_gem_object_get_sg*
This reverts the rest of 9c65f67d8f10 ("drm/i915: Move cmd parser
pinning to execbuffer"). Now that the only user of i915_gem_object_get_sg
without allow_alloc has been removed, we can drop the parameter. This
portion of the revert was broken into its own patch to aid review.
Signed-off-by: Jason Ekstrand <jason@jlekstrand.net> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Reviewed-by: Jon Bloomfield <jon.bloomfield@intel.com> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> Link: https://patchwork.freedesktop.org/patch/msgid/20210714193419.1459723-4-jason@jlekstrand.net
Jason Ekstrand [Wed, 14 Jul 2021 19:34:16 +0000 (14:34 -0500)]
Revert "drm/i915: Propagate errors on awaiting already signaled fences"
This reverts commit 607def56e15054617662167c1b235ed77bfe3643. Ever
since that commit, we've been having issues where a hang in one client
can propagate to another. In particular, a hang in an app can propagate
to the X server which causes the whole desktop to lock up.
Error propagation along fences sound like a good idea, but as your bug
shows, surprising consequences, since propagating errors across security
boundaries is not a good thing.
What we do have is track the hangs on the ctx, and report information to
userspace using RESET_STATS. That's how arb_robustness works. Also, if my
understanding is still correct, the EIO from execbuf is when your context
is banned (because not recoverable or too many hangs). And in all these
cases it's up to userspace to figure out what is all impacted and should
be reported to the application, that's not on the kernel to guess and
automatically propagate.
What's more, we're also building more features on top of ctx error
reporting with RESET_STATS ioctl: Encrypted buffers use the same, and the
userspace fence wait also relies on that mechanism. So it is the path
going forward for reporting gpu hangs and resets to userspace.
So all together that's why I think we should just bury this idea again as
not quite the direction we want to go to, hence why I think the revert is
the right option here.
For backporters: Please note that you _must_ have a backport of
https://lore.kernel.org/dri-devel/20210602164149.391653-2-jason@jlekstrand.net/
for otherwise backporting just this patch opens up a security bug.
v2: Augment commit message. Also restore Jason's sob that I
accidentally lost.
v3: Add a note for backporters
Signed-off-by: Jason Ekstrand <jason@jlekstrand.net> Reported-by: Marcin Slusarz <marcin.slusarz@intel.com> Cc: <stable@vger.kernel.org> # v5.6+ Cc: Jason Ekstrand <jason.ekstrand@intel.com> Cc: Marcin Slusarz <marcin.slusarz@intel.com> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/3080 Fixes: 607def56e150 ("drm/i915: Propagate errors on awaiting already signaled fences") Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> Reviewed-by: Jon Bloomfield <jon.bloomfield@intel.com> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> Link: https://patchwork.freedesktop.org/patch/msgid/20210714193419.1459723-3-jason@jlekstrand.net
This reverts f1c2907b8e7e ("drm/i915/gem: Asynchronous cmdparser"). The
justification for this commit in the git history was a vague comment
about getting it out from under the struct_mutex. While this may
improve perf for some workloads on Gen7 platforms where we rely on the
command parser for features such as indirect rendering, no numbers were
provided to prove such an improvement. It claims to closed two
gitlab/bugzilla issues but with no explanation whatsoever as to why or
what bug it's fixing.
Meanwhile, by moving command parsing off to an async callback, it leaves
us with a problem of what to do on error. When things were synchronous,
EXECBUFFER2 would fail with an error code if parsing failed. When
moving it to async, we needed another way to handle that error and the
solution employed was to set an error on the dma_fence and then trust
that said error gets propagated to the client eventually. Moving back
to synchronous will help us untangle the fence error propagation mess.
This also reverts most of 9c65f67d8f10 ("drm/i915: Move cmd parser
pinning to execbuffer") which is a refactor of some of our allocation
paths for asynchronous parsing. Now that everything is synchronous, we
don't need it.
v2 (Daniel Vetter):
- Add stabel Cc and Fixes tag
Signed-off-by: Jason Ekstrand <jason@jlekstrand.net> Cc: <stable@vger.kernel.org> # v5.6+ Fixes: 607def56e150 ("drm/i915: Propagate errors on awaiting already signaled fences") Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Reviewed-by: Jon Bloomfield <jon.bloomfield@intel.com> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> Link: https://patchwork.freedesktop.org/patch/msgid/20210714193419.1459723-2-jason@jlekstrand.net
drm/i915: Fix missing docbook chapters for i915 uapi.
I noticed when grepping for DOC: that those were defined
in the header, but not actually used. Fix it by removing
all chapters and the internal annotation, so the docbook
generated chapters are used.
This reverts the changes to driver-uapi.rst by the referenced commit 2bbd25fd64b0 ("drm/i915: Document the Virtual Engine uAPI")
Matt Roper [Thu, 15 Jul 2021 00:54:25 +0000 (17:54 -0700)]
Merge branch 'topic/revid_steppings' into drm-intel-gt-next
The switch from old old IS_FOO_REVID() macros to the new table-based
IS_FOO_{GT,DISP}_STEP() macros is needed on both drm-intel-next (for
display-based DMC matching) and drm-intel-gt-next (for workaround
guards). To avoid conflicts, we'll apply the patches to a topic branch
and merge it to both intel branches to ensure the transition to the
new macros is clean.
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
Matt Roper [Tue, 13 Jul 2021 19:36:35 +0000 (12:36 -0700)]
drm/i915/icl: Drop workarounds that only apply to pre-production steppings
We're past the point at which we usually drop workarounds that were
never needed on production hardware. The driver will already print an
error and apply taint if loaded on pre-production hardware.
Matt Roper [Tue, 13 Jul 2021 19:36:34 +0000 (12:36 -0700)]
drm/i915/cnl: Drop all workarounds
All of the Cannon Lake hardware that came out had graphics fused off,
and our userspace drivers have already dropped their support for the
platform; CNL-specific code in i915 that isn't inherited by subsequent
platforms is effectively dead code. Let's remove all of the
CNL-specific workarounds as a quick and easy first step.
Matt Roper [Tue, 13 Jul 2021 19:36:33 +0000 (12:36 -0700)]
drm/i915/dg1: Use revid->stepping tables
Switch DG1 to use a revid->stepping table as we're trying to do on all
platforms going forward.
This removes the last use of IS_REVID() and REVID_FOREVER, so remove
those now-unused macros as well to prevent their accidental use on
future platforms.
Matt Roper [Tue, 13 Jul 2021 19:36:30 +0000 (12:36 -0700)]
drm/i915/icl: Use revid->stepping tables
Switch ICL to use a revid->stepping table as we're trying to do on all
platforms going forward. While we're at it, let's include some
additional steppings that have popped up, even if we don't yet have any
workarounds tied to those steppings (we probably need to audit our
workaround list soon to see if any of the bounds have moved or if new
workarounds have appeared).
Note that the current bspec table is missing information about how to
map PCI revision ID to GT/display steppings; it only provides an SoC
stepping. The mapping to GT/display steppings (which aren't always the
same as the SoC stepping) used to be in the bspec, but was apparently
dropped during an update in Nov 2019; I've made my changes here based on
an older bspec snapshot that still had the necessary information. We've
requested that the missing information be restored.
I'm only including the production revids in the table here since we're
past the point at which we usually stop trying to support pre-production
hardware. An appropriate check is added to
intel_detect_preproduction_hw() to print an error and taint the kernel
just in case someone still tries to load the driver on old
pre-production hardware.
v2:
- Drop pre-production steppings and add error/taint at startup when
loading on pre-production hardware.
Matt Roper [Tue, 13 Jul 2021 19:36:28 +0000 (12:36 -0700)]
drm/i915/bxt: Use revid->stepping tables
Switch BXT to use a revid->stepping table as we're trying to do on all
platforms going forward. Note that the REVID macros we had before
weren't being used anywhere in the code and weren't even correct; the
table values come from the bspec (and omits all the placeholder and
preproduction revisions).
Although nothing in the code is using the data from this table at the
moment, we expect some upcoming DMC patches to start utilizing it.
projects / kernel.git / log