Re: [PATCH v4 5/5] drm/i915: Introduce intel_bo_fbdev_bios_fb_takeover()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] drm/xe/display: unchecked drm_exec_lock_obj() in bios fb takeover
- [High] drm/xe: invalid XE_PPGTT_PTE_DM bit for stolen memory in xe_migrate_copy()
- [High] drm/xe: missing fence put and wait in xe_display_bo_fbdev_bios_fb_takeover()
- [Medium] drm/xe/display: dma_fence leak in xe_display_bo_fbdev_bios_fb_takeover()
- [Medium] drm/xe: unconditional return false in xe_display_bo_fbdev_prefer_stolen()
- [Medium] drm/i915/fbdev: missing runtime PM ref around bios_fb_pick()
- [Low] drm/i915/fbdev: unused parameters in __intel_fbdev_fb_alloc() and realloc
--

commit 1492b8e8d567ba63a9e11c639a3b16b1f1bc88dc
Author: Maarten Lankhorst <dev@lankhorst.se>

drm/i915: Introduce intel_bo_fbdev_bios_fb_takeover()

This commit introduces the intel_bo_fbdev_bios_fb_takeover() helper to
defer the decision of whether to keep a stolen-memory BIOS framebuffer
until fbdev probe time. In Xe, when stolen memory is not preferred, it
allocates a system memory buffer object and uses the migration engine to
copy the BIOS framebuffer contents.

> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c
> index 219ee17de3f68..318b10dcea52c 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
> @@ -225,11 +225,11 @@ static void intel_fbdev_fill_mode_cmd(struct intel_display *display,
>  
>  static struct intel_framebuffer *
>  __intel_fbdev_fb_alloc(struct intel_display *display,
> -		       struct drm_fb_helper_surface_size *sizes)
> +		       struct drm_fb_helper_surface_size *sizes,
> +		       struct drm_gem_object *obj)
>  {
>  	struct drm_mode_fb_cmd2 mode_cmd = {};
>  	struct drm_framebuffer *fb;
> -	struct drm_gem_object *obj;
>  	int size;
>  
>  	intel_fbdev_fill_mode_cmd(display, sizes, &mode_cmd);
> @@ -262,32 +262,68 @@ __intel_fbdev_fb_alloc(struct intel_display *display,
>  
>  }
>  
> -static bool bios_fb_ok(const struct intel_framebuffer *fb,
> -		       const struct drm_fb_helper_surface_size *sizes)
> +static struct intel_framebuffer *
> +__intel_fbdev_fb_realloc(struct intel_display *display,
> +			 const struct drm_framebuffer *orig_fb,
> +			 struct drm_gem_object *obj)
> +{

[Severity: Low]
This isn't a bug, but in __intel_fbdev_fb_alloc() the local variable obj
was converted into a function parameter that is passed as NULL by its only
caller in intel_fbdev_driver_fbdev_probe() and immediately overwritten by
obj = intel_bo_fbdev_create(display, size) without being read. Similarly,
the display parameter in __intel_fbdev_fb_realloc() is never used. Could
these unused parameters be dropped?

[ ... ]

> @@ -305,9 +341,19 @@ int intel_fbdev_driver_fbdev_probe(struct drm_fb_helper *helper,
>  
>  	ifbdev->fb = NULL;
>  
> -	if (fb && !bios_fb_ok(fb, sizes)) {
> -		drm_framebuffer_put(&fb->base);
> -		fb = NULL;
> +	if (fb) {
> +		struct intel_framebuffer *bios_fb =
> +			bios_fb_pick(fb, sizes);
> +
> +		if (IS_ERR(bios_fb)) {
> +			drm_dbg_kms(display->drm, "Re-using BIOS fb failed (%pe).", bios_fb);
> +			bios_fb = NULL;
> +		}
> +
> +		if (fb != bios_fb) {
> +			drm_framebuffer_put(&fb->base);
> +			fb = bios_fb;
> +		}
>  	}
>  
>  	wakeref = intel_display_rpm_get(display);

[Severity: Medium]
Should wakeref = intel_display_rpm_get(display) be acquired before calling
bios_fb_pick(fb, sizes) in intel_fbdev_driver_fbdev_probe()?

Previously, only bios_fb_ok() ran before intel_display_rpm_get(), which was
a software-only check of the framebuffer dimensions and depth/bpp. Now,
bios_fb_pick() calls intel_bo_fbdev_bios_fb_takeover() ->
xe_display_bo_fbdev_bios_fb_takeover(), which calls
xe_bo_create_pin_map_novm() and xe_migrate_copy(). Both paths call
xe_pm_runtime_get_noresume() (via __xe_ggtt_insert_bo_at() and
xe_sched_job_create()) and access hardware (GGTT PTE writes and GPU blit
submission).

If initial fbdev setup is deferred when no outputs are connected (leaving
ifbdev->fb populated while CRTCs are disabled) and
intel_fbdev_driver_fbdev_probe() is later invoked from a hotplug event
after the initial runtime PM reference has been dropped, won't running
bios_fb_pick() before intel_display_rpm_get() trigger a missing outer
runtime PM warning in xe_pm_runtime_get_noresume() and access the device
while runtime-suspended?

[ ... ]

> diff --git a/drivers/gpu/drm/xe/display/xe_display_bo.c b/drivers/gpu/drm/xe/display/xe_display_bo.c
> index d78d893210c4f..188796ae85933 100644
> --- a/drivers/gpu/drm/xe/display/xe_display_bo.c
> +++ b/drivers/gpu/drm/xe/display/xe_display_bo.c
> @@ -120,7 +121,7 @@ static u32 xe_display_bo_fbdev_pitch_align(u32 stride)
>  	return ALIGN(stride, XE_PAGE_SIZE);
>  }
>  
> -bool xe_display_bo_fbdev_prefer_stolen(struct xe_device *xe, unsigned int size)
> +static bool xe_display_bo_fbdev_prefer_stolen(struct xe_device *xe, unsigned int size)
>  {
>  	struct ttm_resource_manager *stolen;
>  
> @@ -152,6 +153,58 @@ bool xe_display_bo_fbdev_prefer_stolen(struct xe_device *xe, unsigned int size)
>  	return stolen->size >= (size * 2) >> PAGE_SHIFT;
>  }

[Severity: Medium]
In xe_display_bo_fbdev_prefer_stolen(), there is an unconditional
"return false;" right before the stolen size check:

drivers/gpu/drm/xe/display/xe_display_bo.c:xe_display_bo_fbdev_prefer_stolen() {
    ...
	/* machine hang when doing lots of LMEMBAR accesses */
	if (XE_DEVICE_WA(xe, 22019338487_display))
		return false;

	return false;

	/*
	 * If the FB is too big, just don't use it since fbdev is not very
	 * important and we should probably use that space with FBC or other
	 * features.
	 */
	return stolen->size >= (size * 2) >> PAGE_SHIFT;
}

Does that unconditional return false make the stolen->size check dead code
and force xe_display_bo_fbdev_bios_fb_takeover() to always reject stolen
memory and reallocate the BIOS framebuffer into system memory on all
integrated platforms?

> +
> +static struct drm_gem_object *xe_display_bo_fbdev_bios_fb_takeover(struct drm_gem_object *obj)
> +{
> +	struct xe_bo *orig_bo = gem_to_xe_bo(obj), *copy_bo;
> +	struct xe_device *xe = xe_bo_device(orig_bo);
> +	struct xe_validation_ctx ctx;
> +	struct drm_exec exec;
> +	int err;
> +
> +	if (IS_DGFX(xe) ||
> +	    xe_display_bo_fbdev_prefer_stolen(xe, xe_bo_size(orig_bo)))
> +		return obj;
> +
> +	copy_bo = xe_bo_create_pin_map_novm(xe, xe_device_get_root_tile(xe), xe_bo_size(orig_bo),
> +					    ttm_bo_type_kernel,
> +					    XE_BO_FLAG_FORCE_WC | XE_BO_FLAG_SYSTEM | XE_BO_FLAG_GGTT,
> +					    false);
> +	if (IS_ERR(copy_bo))
> +		return ERR_CAST(copy_bo);
> +
> +	xe_validation_guard(&ctx, &xe->val, &exec, (struct xe_val_flags) {}, err) {
> +		struct dma_fence *fence;
> +
> +		drm_exec_lock_obj(&exec, &orig_bo->ttm.base);
> +		drm_exec_retry_on_contention(&exec);
> +
> +		drm_exec_lock_obj(&exec, &copy_bo->ttm.base);
> +		drm_exec_retry_on_contention(&exec);

[Severity: High]
As reported by Sashiko on v2 [1], in xe_display_bo_fbdev_bios_fb_takeover(),
the return values of both drm_exec_lock_obj() calls are ignored.

If drm_exec_lock_obj() fails with a non-contention error such as -ENOMEM
(for example when kvrealloc() fails in drm_exec_obj_locked()), it unlocks
obj->resv and returns the error without setting exec->contended:

drivers/gpu/drm/drm_exec.c:drm_exec_lock_obj() {
    ...
	ret = drm_exec_obj_locked(exec, obj);
	if (ret)
		dma_resv_unlock(obj->resv);

	return ret;
}

Because exec->contended is NULL, drm_exec_retry_on_contention(&exec) will
not retry, and execution will continue into dma_resv_reserve_fences(),
xe_migrate_copy(), and dma_resv_add_fence() without holding the reservation
lock on orig_bo or copy_bo. Should err = drm_exec_lock_obj(...) be checked
after drm_exec_retry_on_contention(&exec) on both calls?

> +
> +		err = dma_resv_reserve_fences(orig_bo->ttm.base.resv, 1);
> +		if (err)
> +			break;
> +
> +		err = dma_resv_reserve_fences(copy_bo->ttm.base.resv, 1);
> +		if (err)
> +			break;
> +
> +		fence = xe_migrate_copy(xe_device_get_root_tile(xe)->migrate, orig_bo, copy_bo,
> +					orig_bo->ttm.resource, copy_bo->ttm.resource, false);

[Severity: High]
When xe_display_bo_fbdev_bios_fb_takeover() calls xe_migrate_copy() for an
initial plane BO in stolen memory (XE_PL_STOLEN) on integrated platforms
with GRAPHICS_VERx100(xe) < 1270 (such as Tiger Lake, Rocket Lake, or Alder
Lake), does xe_migrate_copy() program an invalid XE_PPGTT_PTE_DM bit in the
page tables?

In drivers/gpu/drm/xe/xe_migrate.c, __xe_migrate_copy() determines
src_is_vram via is_devmem(), which returns true for all XE_PL_STOLEN
resources:

drivers/gpu/drm/xe/xe_migrate.c:is_devmem() {
	if (mem_type_is_vram(res->mem_type) || res->mem_type == XE_PL_STOLEN)
		return true;

	return false;
}

emit_pte() then unconditionally sets devmem = true when is_vram is true and
passes it to vm->pt_ops->pte_encode_addr(), which sets XE_PPGTT_PTE_DM in
the PTEs. However, stolen memory only uses the DM bit on platforms where
GRAPHICS_VERx100(xe) >= 1270, as seen in xe_bo_is_stolen_devmem():

drivers/gpu/drm/xe/xe_bo.c:xe_bo_is_stolen_devmem() {
	return xe_bo_is_stolen(bo) &&
		GRAPHICS_VERx100(xe_bo_device(bo)) >= 1270;
}

Won't setting XE_PPGTT_PTE_DM when migrating from stolen memory on pre-12.70
platforms cause GPU page faults or corrupted copies during BIOS framebuffer
takeover?

> +		if (IS_ERR(fence)) {
> +			err = PTR_ERR(fence);
> +			break;
> +		}
> +
> +		dma_resv_add_fence(copy_bo->ttm.base.resv, fence, DMA_RESV_USAGE_KERNEL);
> +		dma_resv_add_fence(orig_bo->ttm.base.resv, fence, DMA_RESV_USAGE_BOOKKEEP);
> +		return &copy_bo->ttm.base;
> +	}

[Severity: Medium]
As reported by Sashiko on v2 [1], does xe_display_bo_fbdev_bios_fb_takeover()
leak the struct dma_fence reference returned by xe_migrate_copy()?

xe_migrate_copy() returns fence with a reference acquired for the caller,
and each call to dma_resv_add_fence() takes its own reference via
dma_fence_get(fence), but dma_fence_put(fence) is never called before
returning &copy_bo->ttm.base.

[Severity: High]
As reported by Sashiko on v2 [1], in addition to leaking the fence
reference, could returning copy_bo from xe_display_bo_fbdev_bios_fb_takeover()
without waiting for fence to complete allow CPU accesses to info->screen_base
to race with the in-flight GPU migration blit?

In intel_fbdev_driver_fbdev_probe(), intel_parent_fb_pin_ggtt_pin() ->
__xe_pin_fb_vma() calls xe_bo_validate(), which returns immediately without
waiting on fences because copy_bo was already pinned by
xe_bo_create_pin_map_novm(). Then intel_bo_fbdev_fill_info() exposes
copy_bo->vmap.vaddr_iomem as info->screen_base.

If fb_set_par() is not run immediately during register_framebuffer() (for
example when CONFIG_FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER=y is active,
CONFIG_FRAMEBUFFER_CONSOLE=n, or when /dev/fb0 is accessed before fbcon_init()
runs), nothing waits on copy_bo's reservation fences before CPU reads or
writes hit info->screen_base. Should xe_display_bo_fbdev_bios_fb_takeover()
wait on the migration fence (or should the caller synchronize) before
exposing screen_base, and drop the caller's fence reference with
dma_fence_put(fence)?

[1] https://lore.kernel.org/r/20260721142610.93804-12-dev@lankhorst.se

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105161444.593259-7-dev@lankhorst.se?part=5