Re: â Fi.CI.BAT: failure for drm/i915: (stolen) memory region related fixes

[Andrzej Hajda]

On 13.12.2023 17:13, Andrzej Hajda wrote:
> On 13.12.2023 02:37, Patchwork wrote:
>> *Patch Details*
>> *Series:*    drm/i915: (stolen) memory region related fixes
>> *URL:*    https://patchwork.freedesktop.org/series/127721/ 
>> <https://patchwork.freedesktop.org/series/127721/>
>> *State:*    failure
>> *Details:* 
>> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/index.html 
>> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/index.html>
>>
>>
>>   CI Bug Log - changes from CI_DRM_14010 -> Patchwork_127721v1
>>
>>
>>     Summary
>>
>> *FAILURE*
>>
>> Serious unknown changes coming with Patchwork_127721v1 absolutely need 
>> to be
>> verified manually.
>>
>> If you think the reported changes have nothing to do with the changes
>> introduced in Patchwork_127721v1, please notify your bug team 
>> (I915-ci-infra at lists.freedesktop.org) to allow them
>> to document this new failure mode, which will reduce false positives 
>> in CI.
>>
>> External URL: 
>> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/index.html
>>
>>
>>     Participating hosts (31 -> 33)
>>
>> Additional (4): bat-dg2-8 bat-dg2-9 bat-mtlp-8 fi-pnv-d510
>> Missing (2): bat-adlp-11 fi-snb-2520m
>>
>>
>>     Possible new issues
>>
>> Here are the unknown changes that may have been introduced in 
>> Patchwork_127721v1:
>>
>>
>>       IGT changes
>>
>>
>>         Possible regressions
>>
>>   *
>>
>>     igt at i915_module_load@load:
>>
>>       o bat-mtlp-8: NOTRUN -> INCOMPLETE
>>         
>> <https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_127721v1/bat-mtlp-8/igt@i915_module_load@load.html>
> 
> 
> This is due to overlapping initial fb's vma with GuC reserved area on 
> MTL, see ggtt_reserve_guc_top.
> 
> My dirty quick_fix proposed:
> @@ -143,6 +143,9 @@ initial_plane_vma(struct drm_i915_private *i915,
>          if (IS_ERR(vma))
>                  goto err_obj;
> 
> +       if (base + size > GUC_GGTT_TOP)
> +               base = min(base, GUC_GGTT_TOP) - size;
> +
>          pinctl = PIN_GLOBAL | PIN_OFFSET_FIXED | base;
>          if (HAS_GMCH(i915))
>                  pinctl |= PIN_MAPPABLE;


Copy/Paste Ville response for this proposition from another thread:

On 13.12.2023 17:03, Ville Syrjälä wrote:
 >
 > This is not a solution. The correct fix is either:
 > 1. fix the guc code to not insist on a fixed chunk of the ggtt
 >    and instead allocate it normally like a good boy. It could
 >    still try to allocate as high as possible to ideally
 >    land in the GUC_GGTT_TOP range

This would be the best solution from initial plane PoV for sure, I am 
not sure about GuC PoV.

 >
 > 2. relocate the display vma to a different part of the ggtt

I think this point is what I have proposed.

 >
 >
 > 1. should be far simpler by the looks of it, as 2. would involve:
 > a) pinning the same object into two places in the ggtt simultanously
 >    I don't think we have support for that except for special ggtt views,
 >    and xe doesn't have even that (should we need this trick there as 
well)

AFAIU the fb is not yet pinned in terms of kernel structures, so it 
doesn't seems to be an issue.
Of course there is problem with PLANE_SURF still pointing to old VA, it 
should be replaced with new VA before ggtt will be populated with new stuff.

 >
 > b) flip the plane(s) over to the relocated vma
 > c) wait for vblank(s)
 > d) discard the original vma
 > e) all of that would need to happen pretty early so we may not have
 >    a lot of the required normal machinery fully up and running yet

Async update to PLANE_SURF shouldn't be enough?

Regards
Andrzej