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

[Maarten Lankhorst]

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 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>
> 
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 at lankhorst.se>
---
 drivers/gpu/drm/xe/display/intel_fbdev_fb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/xe/display/intel_fbdev_fb.c b/drivers/gpu/drm/xe/display/intel_fbdev_fb.c
index ca95fcd098ec7..3a1e505ff1820 100644
--- a/drivers/gpu/drm/xe/display/intel_fbdev_fb.c
+++ b/drivers/gpu/drm/xe/display/intel_fbdev_fb.c
@@ -45,7 +45,7 @@ struct intel_framebuffer *intel_fbdev_fb_alloc(struct drm_fb_helper *helper,
 					   NULL, size,
 					   ttm_bo_type_kernel, XE_BO_FLAG_SCANOUT |
 					   XE_BO_FLAG_STOLEN |
-					   XE_BO_FLAG_PINNED);
+					   XE_BO_FLAG_GGTT | XE_BO_FLAG_PINNED);
 		if (!IS_ERR(obj))
 			drm_info(&xe->drm, "Allocated fbdev into stolen\n");
 		else
@@ -56,7 +56,7 @@ struct intel_framebuffer *intel_fbdev_fb_alloc(struct drm_fb_helper *helper,
 		obj = xe_bo_create_pin_map(xe, xe_device_get_root_tile(xe), NULL, size,
 					   ttm_bo_type_kernel, XE_BO_FLAG_SCANOUT |
 					   XE_BO_FLAG_VRAM_IF_DGFX(xe_device_get_root_tile(xe)) |
-					   XE_BO_FLAG_PINNED);
+					   XE_BO_FLAG_GGTT | XE_BO_FLAG_PINNED);
 	}
 
 	if (IS_ERR(obj)) {
-- 
2.45.2