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

[Lucas De Marchi]

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


Reviewed-by: Lucas De Marchi <lucas.demarchi at intel.com>

that is very very much preferred.

Lucas De Marchi