[PATCH v3.1] drm/xe/display: Fix fbdev GGTT mapping handling.

[Lucas De Marchi]

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 at 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.

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 at intel.com>


Lucas De Marchi

>
>Cheers,
>~Maarten