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 <d...@lankhorst.se>
---
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.
Cheers,
~Maarten
