Re: [PATCH v2] drm/xe: improve hibernation on igpu

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Nov 01, 2024 at 12:38:19PM -0500, Lucas De Marchi wrote:
> On Fri, Nov 01, 2024 at 05:01:57PM +0000, Matthew Auld wrote:
> > The GGTT looks to be stored inside stolen memory on igpu which is not
> > treated as normal RAM.  The core kernel skips this memory range when
> > creating the hibernation image, therefore when coming back from
> 
> can you add the log for e820 mapping to confirm?
> 
> > hibernation the GGTT programming is lost. This seems to cause issues
> > with broken resume where GuC FW fails to load:
> > 
> > [drm] *ERROR* GT0: load failed: status = 0x400000A0, time = 10ms, freq = 1250MHz (req 1300MHz), done = -1
> > [drm] *ERROR* GT0: load failed: status: Reset = 0, BootROM = 0x50, UKernel = 0x00, MIA = 0x00, Auth = 0x01
> > [drm] *ERROR* GT0: firmware signature verification failed
> > [drm] *ERROR* CRITICAL: Xe has declared device 0000:00:02.0 as wedged.
> 
> it seems the message above is cut short. Just above these lines don't
> you have a log with __xe_guc_upload? Which means: we actually upload the
> firmware again to stolen and it doesn't matter that we lost it when
> hibernating.
> 

The image is always uploaded. The upload logic uses a GGTT address to
find firmware image in SRAM...

See snippet from uc_fw_xfer:

821         /* Set the source address for the uCode */
822         src_offset = uc_fw_ggtt_offset(uc_fw) + uc_fw->css_offset;
823         xe_mmio_write32(mmio, DMA_ADDR_0_LOW, lower_32_bits(src_offset));
824         xe_mmio_write32(mmio, DMA_ADDR_0_HIGH,
825                         upper_32_bits(src_offset) | DMA_ADDRESS_SPACE_GGTT);

If the GGTT mappings are in stolen and not restored we will not be
uploading the correct data for the image.

See the gitlab issue, this has been confirmed to fix a real problem from
a customer.

Matt

> It'd be good to know the size of the rsa key in the failing scenarios.
> 
> Also it seems this is also reproduced in DG2 and I wonder if it's the
> same issue or something different:
> 
> 	[drm:__xe_guc_upload.isra.0 [xe]] GT0: load still in progress, timeouts = 0, freq = 1700MHz (req 2050MHz), status = 0x00000064 [0x32/00]
> 	[drm:__xe_guc_upload.isra.0 [xe]] GT0: load still in progress, timeouts = 0, freq = 1700MHz (req 2050MHz), status = 0x00000072 [0x39/00]
> 	[drm:__xe_guc_upload.isra.0 [xe]] GT0: load still in progress, timeouts = 0, freq = 1700MHz (req 2050MHz), status = 0x00000086 [0x43/00]
> 	[drm] *ERROR* GT0: load failed: status = 0x400000A0, time = 5ms, freq = 1700MHz (req 2050MHz), done = -1
> 	[drm] *ERROR* GT0: load failed: status: Reset = 0, BootROM = 0x50, UKernel = 0x00, MIA = 0x00, Auth = 0x01
> 	[drm] *ERROR* GT0: firmware signature verification failed
> 
> Cc Ulisses.
> 
> > 
> > Current GGTT users are kernel internal and tracked as pinned, so it
> > should be possible to hook into the existing save/restore logic that we
> > use for dgpu, where the actual evict is skipped but on restore we
> > importantly restore the GGTT programming.  This has been confirmed to
> > fix hibernation on at least ADL and MTL, though likely all igpu
> > platforms are affected.
> > 
> > This also means we have a hole in our testing, where the existing s4
> > tests only really test the driver hooks, and don't go as far as actually
> > rebooting and restoring from the hibernation image and in turn powering
> > down RAM (and therefore losing the contents of stolen).
> 
> yeah, the problem is that enabling it to go through the entire sequence
> we reproduce all kind of issues in other parts of the kernel and userspace
> env leading to flaky tests that are usually red in CI. The most annoying
> one is the network not coming back so we mark the test as failure
> (actually abort. since we stop running everything).
> 
> 
> > 
> > v2 (Brost)
> > - Remove extra newline and drop unnecessary parentheses.
> > 
> > Fixes: dd08ebf6c352 ("drm/xe: Introduce a new DRM driver for Intel GPUs")
> > Link: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/3275
> > Signed-off-by: Matthew Auld <matthew.auld@xxxxxxxxx>
> > Cc: Matthew Brost <matthew.brost@xxxxxxxxx>
> > Cc: <stable@xxxxxxxxxxxxxxx> # v6.8+
> > Reviewed-by: Matthew Brost <matthew.brost@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/xe/xe_bo.c       | 37 ++++++++++++++------------------
> > drivers/gpu/drm/xe/xe_bo_evict.c |  6 ------
> > 2 files changed, 16 insertions(+), 27 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
> > index 8286cbc23721..549866da5cd1 100644
> > --- a/drivers/gpu/drm/xe/xe_bo.c
> > +++ b/drivers/gpu/drm/xe/xe_bo.c
> > @@ -952,7 +952,10 @@ int xe_bo_restore_pinned(struct xe_bo *bo)
> > 	if (WARN_ON(!xe_bo_is_pinned(bo)))
> > 		return -EINVAL;
> > 
> > -	if (WARN_ON(xe_bo_is_vram(bo) || !bo->ttm.ttm))
> > +	if (WARN_ON(xe_bo_is_vram(bo)))
> > +		return -EINVAL;
> > +
> > +	if (WARN_ON(!bo->ttm.ttm && !xe_bo_is_stolen(bo)))
> > 		return -EINVAL;
> > 
> > 	if (!mem_type_is_vram(place->mem_type))
> > @@ -1774,6 +1777,7 @@ int xe_bo_pin_external(struct xe_bo *bo)
> > 
> > int xe_bo_pin(struct xe_bo *bo)
> > {
> > +	struct ttm_place *place = &bo->placements[0];
> > 	struct xe_device *xe = xe_bo_device(bo);
> > 	int err;
> > 
> > @@ -1804,8 +1808,6 @@ int xe_bo_pin(struct xe_bo *bo)
> > 	 */
> > 	if (IS_DGFX(xe) && !(IS_ENABLED(CONFIG_DRM_XE_DEBUG) &&
> > 	    bo->flags & XE_BO_FLAG_INTERNAL_TEST)) {
> > -		struct ttm_place *place = &(bo->placements[0]);
> > -
> > 		if (mem_type_is_vram(place->mem_type)) {
> > 			xe_assert(xe, place->flags & TTM_PL_FLAG_CONTIGUOUS);
> > 
> > @@ -1813,13 +1815,12 @@ int xe_bo_pin(struct xe_bo *bo)
> > 				       vram_region_gpu_offset(bo->ttm.resource)) >> PAGE_SHIFT;
> > 			place->lpfn = place->fpfn + (bo->size >> PAGE_SHIFT);
> > 		}
> > +	}
> > 
> > -		if (mem_type_is_vram(place->mem_type) ||
> > -		    bo->flags & XE_BO_FLAG_GGTT) {
> > -			spin_lock(&xe->pinned.lock);
> > -			list_add_tail(&bo->pinned_link, &xe->pinned.kernel_bo_present);
> > -			spin_unlock(&xe->pinned.lock);
> > -		}
> > +	if (mem_type_is_vram(place->mem_type) || bo->flags & XE_BO_FLAG_GGTT) {
> 
> should this test for devmem so we restore everything rather than just
> ggtt?
> 
> Lucas De Marchi
> 
> > +		spin_lock(&xe->pinned.lock);
> > +		list_add_tail(&bo->pinned_link, &xe->pinned.kernel_bo_present);
> > +		spin_unlock(&xe->pinned.lock);
> > 	}
> > 
> > 	ttm_bo_pin(&bo->ttm);
> > @@ -1867,24 +1868,18 @@ void xe_bo_unpin_external(struct xe_bo *bo)
> > 
> > void xe_bo_unpin(struct xe_bo *bo)
> > {
> > +	struct ttm_place *place = &bo->placements[0];
> > 	struct xe_device *xe = xe_bo_device(bo);
> > 
> > 	xe_assert(xe, !bo->ttm.base.import_attach);
> > 	xe_assert(xe, xe_bo_is_pinned(bo));
> > 
> > -	if (IS_DGFX(xe) && !(IS_ENABLED(CONFIG_DRM_XE_DEBUG) &&
> > -	    bo->flags & XE_BO_FLAG_INTERNAL_TEST)) {
> > -		struct ttm_place *place = &(bo->placements[0]);
> > -
> > -		if (mem_type_is_vram(place->mem_type) ||
> > -		    bo->flags & XE_BO_FLAG_GGTT) {
> > -			spin_lock(&xe->pinned.lock);
> > -			xe_assert(xe, !list_empty(&bo->pinned_link));
> > -			list_del_init(&bo->pinned_link);
> > -			spin_unlock(&xe->pinned.lock);
> > -		}
> > +	if (mem_type_is_vram(place->mem_type) || bo->flags & XE_BO_FLAG_GGTT) {
> > +		spin_lock(&xe->pinned.lock);
> > +		xe_assert(xe, !list_empty(&bo->pinned_link));
> > +		list_del_init(&bo->pinned_link);
> > +		spin_unlock(&xe->pinned.lock);
> > 	}
> > -
> > 	ttm_bo_unpin(&bo->ttm);
> > }
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_bo_evict.c b/drivers/gpu/drm/xe/xe_bo_evict.c
> > index 32043e1e5a86..b01bc20eb90b 100644
> > --- a/drivers/gpu/drm/xe/xe_bo_evict.c
> > +++ b/drivers/gpu/drm/xe/xe_bo_evict.c
> > @@ -34,9 +34,6 @@ int xe_bo_evict_all(struct xe_device *xe)
> > 	u8 id;
> > 	int ret;
> > 
> > -	if (!IS_DGFX(xe))
> > -		return 0;
> > -
> > 	/* User memory */
> > 	for (mem_type = XE_PL_VRAM0; mem_type <= XE_PL_VRAM1; ++mem_type) {
> > 		struct ttm_resource_manager *man =
> > @@ -125,9 +122,6 @@ int xe_bo_restore_kernel(struct xe_device *xe)
> > 	struct xe_bo *bo;
> > 	int ret;
> > 
> > -	if (!IS_DGFX(xe))
> > -		return 0;
> > -
> > 	spin_lock(&xe->pinned.lock);
> > 	for (;;) {
> > 		bo = list_first_entry_or_null(&xe->pinned.evicted,
> > -- 
> > 2.47.0
> > 




[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux