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