| Message ID | 18de13d0-124c-40dd-b987-7675fd851eeb@lankhorst.se (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3.2] drm/xe/display: Fix fbdev GGTT mapping handling. | expand |
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@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@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@lankhorst.se> Reviewed-by: Lucas De Marchi <lucas.demarchi@intel.com> that is very very much preferred. Lucas De Marchi
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)) {