| Message ID | 20250219153441.625352-1-dev@lankhorst.se (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3.1] drm/xe/display: Fix fbdev GGTT mapping handling. | expand |
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? +Matthew Brost / +Matthew Auld I think this was about commit dd886a63d6e2 ("drm/xe: Restore system memory GGTT mappings") Lucas De Marchi >+ > intel_hpd_poll_disable(xe); >+ } > > intel_opregion_resume(display); > >diff --git a/drivers/gpu/drm/xe/display/xe_fb_pin.c b/drivers/gpu/drm/xe/display/xe_fb_pin.c >index 11a6b996d739b..dee1f6531c849 100644 >--- a/drivers/gpu/drm/xe/display/xe_fb_pin.c >+++ b/drivers/gpu/drm/xe/display/xe_fb_pin.c >@@ -12,6 +12,7 @@ > #include "intel_fbdev.h" > #include "xe_bo.h" > #include "xe_device.h" >+#include "xe_fb_pin.h" > #include "xe_ggtt.h" > #include "xe_pm.h" > >@@ -398,7 +399,8 @@ static bool reuse_vma(struct intel_plane_state *new_plane_state, > goto found; > } > >- if (fb == intel_fbdev_framebuffer(xe->display.fbdev.fbdev)) { >+ if (fb == intel_fbdev_framebuffer(xe->display.fbdev.fbdev) && >+ new_plane_state->view.gtt.type == I915_GTT_VIEW_NORMAL) { > vma = intel_fbdev_vma_pointer(xe->display.fbdev.fbdev); > if (vma) > goto found; >@@ -444,6 +446,26 @@ void intel_plane_unpin_fb(struct intel_plane_state *old_plane_state) > old_plane_state->ggtt_vma = NULL; > } > >+void xe_fb_pin_resume(struct xe_device *xe) >+{ >+ struct xe_ggtt *ggtt = xe_device_get_root_tile(xe)->mem.ggtt; >+ struct i915_vma *vma = intel_fbdev_vma_pointer(xe->display.fbdev.fbdev); >+ struct xe_bo *bo; >+ >+ if (!vma) >+ return; >+ >+ bo = vma->bo; >+ >+ mutex_lock(&ggtt->lock); >+ for (u32 x = 0; x < bo->ttm.base.size; x += XE_PAGE_SIZE) { >+ u64 pte = ggtt->pt_ops->pte_encode_bo(bo, x, xe->pat.idx[XE_CACHE_NONE]); >+ >+ ggtt->pt_ops->ggtt_set_pte(ggtt, vma->node->base.start + x, pte); >+ } >+ mutex_unlock(&ggtt->lock); >+} >+ > /* > * For Xe introduce dummy intel_dpt_create which just return NULL, > * intel_dpt_destroy which does nothing, and fake intel_dpt_ofsset returning 0; >diff --git a/drivers/gpu/drm/xe/display/xe_fb_pin.h b/drivers/gpu/drm/xe/display/xe_fb_pin.h >new file mode 100644 >index 0000000000000..39d48ff637002 >--- /dev/null >+++ b/drivers/gpu/drm/xe/display/xe_fb_pin.h >@@ -0,0 +1,13 @@ >+/* SPDX-License-Identifier: MIT */ >+/* >+ * Copyright © 2025 Intel Corporation >+ */ >+ >+#ifndef _XE_FB_PIN_H_ >+#define _XE_FB_PIN_H_ >+ >+struct xe_device; >+ >+void xe_fb_pin_resume(struct xe_device *xe); >+ >+#endif /* _XE_FB_PIN_H_ */ >-- >2.47.1 >
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? > > +Matthew Brost / +Matthew Auld > I think this was about commit dd886a63d6e2 ("drm/xe: Restore system > memory GGTT mappings") > > Lucas De Marchi > >> + >> intel_hpd_poll_disable(xe); >> + } >> >> intel_opregion_resume(display); >> >> diff --git a/drivers/gpu/drm/xe/display/xe_fb_pin.c b/drivers/gpu/drm/ >> xe/display/xe_fb_pin.c >> index 11a6b996d739b..dee1f6531c849 100644 >> --- a/drivers/gpu/drm/xe/display/xe_fb_pin.c >> +++ b/drivers/gpu/drm/xe/display/xe_fb_pin.c >> @@ -12,6 +12,7 @@ >> #include "intel_fbdev.h" >> #include "xe_bo.h" >> #include "xe_device.h" >> +#include "xe_fb_pin.h" >> #include "xe_ggtt.h" >> #include "xe_pm.h" >> >> @@ -398,7 +399,8 @@ static bool reuse_vma(struct intel_plane_state >> *new_plane_state, >> goto found; >> } >> >> - if (fb == intel_fbdev_framebuffer(xe->display.fbdev.fbdev)) { >> + if (fb == intel_fbdev_framebuffer(xe->display.fbdev.fbdev) && >> + new_plane_state->view.gtt.type == I915_GTT_VIEW_NORMAL) { >> vma = intel_fbdev_vma_pointer(xe->display.fbdev.fbdev); >> if (vma) >> goto found; >> @@ -444,6 +446,26 @@ void intel_plane_unpin_fb(struct >> intel_plane_state *old_plane_state) >> old_plane_state->ggtt_vma = NULL; >> } >> >> +void xe_fb_pin_resume(struct xe_device *xe) >> +{ >> + struct xe_ggtt *ggtt = xe_device_get_root_tile(xe)->mem.ggtt; >> + struct i915_vma *vma = intel_fbdev_vma_pointer(xe- >> >display.fbdev.fbdev); >> + struct xe_bo *bo; >> + >> + if (!vma) >> + return; >> + >> + bo = vma->bo; >> + >> + mutex_lock(&ggtt->lock); >> + for (u32 x = 0; x < bo->ttm.base.size; x += XE_PAGE_SIZE) { >> + u64 pte = ggtt->pt_ops->pte_encode_bo(bo, x, xe- >> >pat.idx[XE_CACHE_NONE]); >> + >> + ggtt->pt_ops->ggtt_set_pte(ggtt, vma->node->base.start + x, >> pte); >> + } >> + mutex_unlock(&ggtt->lock); >> +} >> + >> /* >> * For Xe introduce dummy intel_dpt_create which just return NULL, >> * intel_dpt_destroy which does nothing, and fake intel_dpt_ofsset >> returning 0; >> diff --git a/drivers/gpu/drm/xe/display/xe_fb_pin.h b/drivers/gpu/drm/ >> xe/display/xe_fb_pin.h >> new file mode 100644 >> index 0000000000000..39d48ff637002 >> --- /dev/null >> +++ b/drivers/gpu/drm/xe/display/xe_fb_pin.h >> @@ -0,0 +1,13 @@ >> +/* SPDX-License-Identifier: MIT */ >> +/* >> + * Copyright © 2025 Intel Corporation >> + */ >> + >> +#ifndef _XE_FB_PIN_H_ >> +#define _XE_FB_PIN_H_ >> + >> +struct xe_device; >> + >> +void xe_fb_pin_resume(struct xe_device *xe); >> + >> +#endif /* _XE_FB_PIN_H_ */ >> -- >> 2.47.1 >>
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. Cheers, ~Maarten
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); + intel_hpd_poll_disable(xe); + } intel_opregion_resume(display); diff --git a/drivers/gpu/drm/xe/display/xe_fb_pin.c b/drivers/gpu/drm/xe/display/xe_fb_pin.c index 11a6b996d739b..dee1f6531c849 100644 --- a/drivers/gpu/drm/xe/display/xe_fb_pin.c +++ b/drivers/gpu/drm/xe/display/xe_fb_pin.c @@ -12,6 +12,7 @@ #include "intel_fbdev.h" #include "xe_bo.h" #include "xe_device.h" +#include "xe_fb_pin.h" #include "xe_ggtt.h" #include "xe_pm.h" @@ -398,7 +399,8 @@ static bool reuse_vma(struct intel_plane_state *new_plane_state, goto found; } - if (fb == intel_fbdev_framebuffer(xe->display.fbdev.fbdev)) { + if (fb == intel_fbdev_framebuffer(xe->display.fbdev.fbdev) && + new_plane_state->view.gtt.type == I915_GTT_VIEW_NORMAL) { vma = intel_fbdev_vma_pointer(xe->display.fbdev.fbdev); if (vma) goto found; @@ -444,6 +446,26 @@ void intel_plane_unpin_fb(struct intel_plane_state *old_plane_state) old_plane_state->ggtt_vma = NULL; } +void xe_fb_pin_resume(struct xe_device *xe) +{ + struct xe_ggtt *ggtt = xe_device_get_root_tile(xe)->mem.ggtt; + struct i915_vma *vma = intel_fbdev_vma_pointer(xe->display.fbdev.fbdev); + struct xe_bo *bo; + + if (!vma) + return; + + bo = vma->bo; + + mutex_lock(&ggtt->lock); + for (u32 x = 0; x < bo->ttm.base.size; x += XE_PAGE_SIZE) { + u64 pte = ggtt->pt_ops->pte_encode_bo(bo, x, xe->pat.idx[XE_CACHE_NONE]); + + ggtt->pt_ops->ggtt_set_pte(ggtt, vma->node->base.start + x, pte); + } + mutex_unlock(&ggtt->lock); +} + /* * For Xe introduce dummy intel_dpt_create which just return NULL, * intel_dpt_destroy which does nothing, and fake intel_dpt_ofsset returning 0; diff --git a/drivers/gpu/drm/xe/display/xe_fb_pin.h b/drivers/gpu/drm/xe/display/xe_fb_pin.h new file mode 100644 index 0000000000000..39d48ff637002 --- /dev/null +++ b/drivers/gpu/drm/xe/display/xe_fb_pin.h @@ -0,0 +1,13 @@ +/* SPDX-License-Identifier: MIT */ +/* + * Copyright © 2025 Intel Corporation + */ + +#ifndef _XE_FB_PIN_H_ +#define _XE_FB_PIN_H_ + +struct xe_device; + +void xe_fb_pin_resume(struct xe_device *xe); + +#endif /* _XE_FB_PIN_H_ */
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