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

[Matthew Auld <matthew.auld@intel.com>]

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