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 <[email protected]>
---
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 <[email protected]>
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 <[email protected]>
Reviewed-by: Lucas De Marchi <[email protected]>
that is very very much preferred.
Lucas De Marchi
