On Wed, Mar 05, 2025 at 11:39:50AM +0100, Maarten Lankhorst wrote:
On 2025-03-05 00:09, Lucas De Marchi wrote:
On Thu, Feb 20, 2025 at 06:17:01PM +0100, Maarten Lankhorst wrote:
Hey,
On 2025-02-20 16:43, Matthew Auld wrote:
On 20/02/2025 14:22, Lucas De Marchi wrote:
On Wed, Feb 19, 2025 at 04:34:40PM +0100, Maarten Lankhorst wrote:
The fbdev vma is a normal unrotated VMA, so add ane explicit check
before re-using.
When re-using, we have to restore the GGTT mapping on resume, so add
some code to do that too.
Fixes: 67a98f7e27ba ("drm/xe/display: Re-use display vmas when possible")
Signed-off-by: Maarten Lankhorst <dev@xxxxxxxxxxxx>
---
drivers/gpu/drm/xe/display/xe_display.c | 6 +++++-
drivers/gpu/drm/xe/display/xe_fb_pin.c | 24 +++++++++++++++++++++++-
drivers/gpu/drm/xe/display/xe_fb_pin.h | 13 +++++++++++++
3 files changed, 41 insertions(+), 2 deletions(-)
create mode 100644 drivers/gpu/drm/xe/display/xe_fb_pin.h
diff --git a/drivers/gpu/drm/xe/display/xe_display.c b/drivers/gpu/ drm/xe/display/xe_display.c
index 02a413a073824..999f25a562c19 100644
--- a/drivers/gpu/drm/xe/display/xe_display.c
+++ b/drivers/gpu/drm/xe/display/xe_display.c
@@ -30,6 +30,7 @@
#include "intel_hotplug.h"
#include "intel_opregion.h"
#include "skl_watermark.h"
+#include "xe_fb_pin.h"
#include "xe_module.h"
/* Xe device functions */
@@ -492,8 +493,11 @@ void xe_display_pm_resume(struct xe_device *xe)
intel_display_driver_enable_user_access(display);
}
- if (has_display(xe))
+ if (has_display(xe)) {
+ xe_fb_pin_resume(xe);
this looks odd. I remember when we had issues with LNL about pages
coming with garbage after a resume we talked about marking them as
"needed" on suspend so they'd be saved by mm. The ggtt afair was one of
them. Or did we go with a different approach and I'm misremembering?
Hmm, I thought for display fbs we don't use the same pin tracking so it is rather skipped by the GGTT save/restore logic. But I thought previously the display stuff ensured all fb are unpinned by the time we do the suspend, so on resume we would just re-program the GGTT for fb when re-pinning it (handled by display code). But I guess issue now comes with re-using the vma and its GGTT mapping, since it will now also skip re-programming the GGTT on re-pin? But wouldn't we have this same issue for all fb, and not just this fbdev stuff or does reuse_vma() somehow handle this?
Correct. Display has its own GGTT tracking.
In general, all FB's are unpinned during suspend, and suspend will destroy the VMA. A new VMA and re-pinning will be done after resume, so this is not a problem in general.
The special case is unfortunately FBDEV vma pin, which we started re-using in the patch series. That one should be preserved across suspend/resume, otherwise we get pipe fault errors after a cycle because the GGTT is wiped.
The bug was there before, but never hit because we never used the initial GGTT mapping, it was only there to keep fbdev pinned.
I'm honestly wondering how much it's needed, but not doing so likely breaks i915. Perhaps we could make a dummy noop instead.
I stared at the current code in xe_fb_pin.c and related xe_display.c for
some time and for me it's hard to understand to suggest a better fix.
I'd rather use the traking we have instead of adding yet another hook to
call on resume.
But if it fixes the pipe fault, maybe better to land it and improve the
abstraction on top.
Reviewed-by: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
Hmmm....
This should work instead?
---->8------
FBDEV ggtt is not restored correctly, add missing GGTT flag to intel_fbdev_fb_alloc to make it work.
This ensures that the global GGTT mapping is always restored on resume. The GGTT mapping would
otherwise be created in intel_fb_pin_to_ggtt() by intel_fbdev anyway.
This fixes the fbdev device not working after resume.
Fixes: 67a98f7e27ba ("drm/xe/display: Re-use display vmas when possible")
Signed-off-by: Maarten Lankhorst <dev@xxxxxxxxxxxx>
Reviewed-by: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
that is very very much preferred.
Lucas De Marchi
