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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential
issues to consider:
- [High] drm/xe: NULL dereference in xe_migrate_copy() on pre-12.70
  stolen BO
- [Medium] drm/xe: unconditional return false in
  xe_display_bo_fbdev_prefer_stolen()
- [Medium] drm/i915: deferred BIOS FB takeover in bios_fb_pick() breaks
  FBC and drm_panic
- [Medium] drm/i915/fbdev: double unpin on error in
  __intel_fbdev_fb_realloc()
--

commit 4b91f1cbc821e07604b613317e6ff23539bcbdbd
Author: Maarten Lankhorst <dev@lankhorst.se>

drm/i915: Introduce intel_bo_fbdev_bios_fb_takeover()

This commit defers the decision of whether to keep a BIOS framebuffer in
stolen memory from initial plane readout to fbdev probing via a new
intel_bo_fbdev_bios_fb_takeover() helper, copying the framebuffer to system
memory on Xe when stolen memory is not preferred.

> diff --git a/drivers/gpu/drm/i915/display/intel_fbdev.c b/drivers/gpu/drm/i915/display/intel_fbdev.c
> index 219ee17de3f68..97d5e1279dbc1 100644
> --- a/drivers/gpu/drm/i915/display/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/display/intel_fbdev.c
> @@ -228,8 +228,8 @@ __intel_fbdev_fb_alloc(struct intel_display *display,
>  		       struct drm_fb_helper_surface_size *sizes)
>  {
>  	struct drm_mode_fb_cmd2 mode_cmd = {};
> -	struct drm_framebuffer *fb;
>  	struct drm_gem_object *obj;
> +	struct drm_framebuffer *fb;
>  	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)
> +{
> +	struct drm_framebuffer *new_fb;
> +	struct drm_mode_fb_cmd2 mode_cmd = {
> +		.flags = DRM_MODE_FB_MODIFIERS,
> +		.width = orig_fb->width,
> +		.height = orig_fb->height,
> +		.pitches[0] = orig_fb->pitches[0],
> +		.offsets[0] = orig_fb->offsets[0],
> +		.pixel_format = orig_fb->format->format,
> +		.modifier[0] = orig_fb->modifier,
> +	};
> +
> +	new_fb = intel_framebuffer_create(obj, orig_fb->format, &mode_cmd);
> +
> +	if (IS_ERR(new_fb)) {
> +		intel_bo_fbdev_destroy(obj);
> +		return ERR_CAST(new_fb);
> +	}

[Severity: Medium]
Can this error path unpin obj twice on Xe when intel_framebuffer_create()
fails after intel_bo_framebuffer_init()?

In xe_display_bo_fbdev_bios_fb_takeover(), copy_bo is allocated with
xe_bo_create_pin_map_novm(), which sets XE_BO_FLAG_PINNED and pins the BO
once.  Inside intel_framebuffer_init(), xe_display_bo_framebuffer_init()
only takes a GEM reference, but if a later step in intel_framebuffer_init()
fails, its error unwind calls xe_display_bo_framebuffer_fini(), which sees
XE_BO_FLAG_PINNED and calls xe_bo_unpin().  Calling intel_bo_fbdev_destroy()
-> xe_bo_unpin_map_no_vm() here then unpins copy_bo a second time when
pin_count is already zero, triggering an xe_assert() failure and a
WARN_ON_ONCE(!bo->pin_count) in ttm_bo_unpin().

> +
> +	drm_gem_object_put(obj);
> +	return to_intel_framebuffer(new_fb);
> +}
> +
> +static struct intel_framebuffer *bios_fb_pick(struct intel_framebuffer *fb,
> +					      const struct drm_fb_helper_surface_size *sizes)
>  {
>  	struct intel_display *display = to_intel_display(fb->base.dev);
>  	int width = fb->base.width;
>  	int height = fb->base.height;
>  	int depth = fb->base.format->depth;
>  	int bpp = fb->base.format->cpp[0] * 8;
> +	struct drm_gem_object *new;
>  
>  	if (sizes->fb_width > width || sizes->fb_height > height) {
>  		drm_dbg_kms(display->drm,
>  			    "BIOS fb too small (%dx%d), we require (%dx%d), releasing it\n",
>  			    width, height, sizes->fb_width, sizes->fb_height);
> -		return false;
> +		return NULL;
>  	}
>  
>  	if (sizes->surface_depth != depth || sizes->surface_bpp != bpp) {
>  		drm_dbg_kms(display->drm,
>  			    "BIOS fb using wrong depth/bpp (%d/%d), we require (%d/%d), releasing it\n",
>  			    depth, bpp, sizes->surface_depth, sizes->surface_bpp);
> -		return false;
> +		return NULL;
>  	}
>  
> -	return true;
> +	new = intel_bo_fbdev_bios_fb_takeover(fb->base.obj[0]);
> +	if (IS_ERR(new))
> +		return ERR_CAST(new);
> +	if (fb->base.obj[0] == new)
> +		return fb;
> +
> +	/* Different object, re-create an intel_framebuffer around it */
> +	return __intel_fbdev_fb_realloc(display, &fb->base, new);
>  }
>  
> +
>  int intel_fbdev_driver_fbdev_probe(struct drm_fb_helper *helper,
>  				   struct drm_fb_helper_surface_size *sizes)
>  {
> @@ -305,13 +341,23 @@ 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;
> -	}
> -
>  	wakeref = intel_display_rpm_get(display);
>  
> +	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;
> +		}
> +	}

[Severity: Medium]
Does deferring the stolen memory check from initial_plane_vma() and
initial_plane_bo() to bios_fb_pick() regress FBC CFB allocation and
drm_panic?

Because the initial BIOS framebuffer in stolen memory is now kept across
intel_initial_commit() (and only unpinned after the later fbcon modeset
commit completes), intel_fbc_alloc_cfb() runs while the BIOS FB still
occupies more than half of stolen memory.  When a 1:1 CFB no longer fits in
the remaining stolen space, find_compression_limit() either degrades FBC to
a reduced compression ratio that intel_fbc_is_cfb_ok() never upgrades after
the BIOS FB is freed, or fails with -ENOSPC.

In addition, drm_framebuffer_put(&fb->base) here only drops ifbdev's
reference while the original stolen FB remains active on
plane_state->hw.fb until fbcon performs a modeset.  When console takeover is
deferred (CONFIG_FRAMEBUFFER_CONSOLE_DEFERRED_TAKEOVER=y during a quiet
boot) or CONFIG_FRAMEBUFFER_CONSOLE=n, the primary plane continues scanning
out the stolen BIOS FB while ifbdev->fb points to the new system memory FB.
If a panic occurs before takeover, intel_get_scanout_buffer() sees
plane_state->hw.fb != intel_fbdev_framebuffer(display->fbdev.fbdev) and
calls intel_parent_panic_setup() on the stolen BO, which fails on both i915
and Xe.

[ ... ]

> diff --git a/drivers/gpu/drm/xe/display/xe_display_bo.c b/drivers/gpu/drm/xe/display/xe_display_bo.c
> index d78d893210c4f..43d05a58bcb47 100644
> --- a/drivers/gpu/drm/xe/display/xe_display_bo.c
> +++ b/drivers/gpu/drm/xe/display/xe_display_bo.c

[ ... ]

> @@ -152,6 +153,64 @@ bool xe_display_bo_fbdev_prefer_stolen(struct xe_device *xe, unsigned int size)
>  	return stolen->size >= (size * 2) >> PAGE_SHIFT;
>  }
>  
> +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;

[Severity: Medium]
Does xe_display_bo_fbdev_prefer_stolen() still contain an unconditional
"return false;" right before the stolen->size check from commit "HACK:
Always fallback"?

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

That makes the size check unreachable and forces
xe_display_bo_fbdev_bios_fb_takeover() to always copy the BIOS framebuffer
out of stolen memory on every integrated Xe platform.

> +
> +	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;
> +
> +		err = drm_exec_lock_obj(&exec, &orig_bo->ttm.base);
> +		drm_exec_retry_on_contention(&exec);
> +		if (err)
> +			break;
> +
> +		err = drm_exec_lock_obj(&exec, &copy_bo->ttm.base);
> +		drm_exec_retry_on_contention(&exec);
> +		if (err)
> +			break;
> +
> +		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]
Does xe_migrate_copy() dereference a NULL pointer when copying a stolen
orig_bo on integrated platforms with GRAPHICS_VERx100(xe) < 1270 (such as
Tiger Lake, Rocket Lake, Alder Lake, and Raptor Lake)?

On integrated platforms, initial_plane_bo() allocates orig_bo in
XE_PL_STOLEN, where the manager has use_tt = false and orig_bo->ttm.ttm is
NULL.  Inside __xe_migrate_copy(), src_is_vram is determined by
is_devmem(), which calls xe_bo_is_stolen_devmem() and returns false when
GRAPHICS_VERx100(xe) < 1270:

drivers/gpu/drm/xe/xe_migrate.c:__xe_migrate_copy() {
    ...
	bool src_is_vram = is_devmem(src_bo, src);
    ...
	if (!src_is_vram)
		xe_res_first_sg(xe_bo_sg(src_bo), 0, size, &src_it);
	else
		xe_res_first(src, 0, size, &src_it);
    ...
}

Taking the !src_is_vram branch calls xe_bo_sg(orig_bo), which dereferences
orig_bo->ttm.ttm.  In addition, emit_pte() gates adding
vram_region_gpu_offset(res) on is_vram alongside setting devmem = true,
whereas pre-12.70 stolen memory needs the stolen GPU offset added without
setting the devmem PTE bit.

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