Re: [PATCH 06/13] drm/i915/dpt: Compute DPT size in the DPT code

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [High] drm/xe/display: double BO unpin on __intel_fbdev_fb_alloc()
  error path [1]
--

commit ae1435739339ed58396679163558aae13db1d253
Author: Ville Syrjälä <ville.syrjala@linux.intel.com>

drm/i915/dpt: Compute DPT size in the DPT code

This commit passes the GTT view instead of a precomputed size to
intel_parent_dpt_create() so that the DPT code computes the DPT size for
both normal and remapped views.

> diff --git a/drivers/gpu/drm/i915/display/intel_fb.c b/drivers/gpu/drm/i915/display/intel_fb.c
> index b3344e3f187fb..35f1d5dd31d7d 100644
> --- a/drivers/gpu/drm/i915/display/intel_fb.c
> +++ b/drivers/gpu/drm/i915/display/intel_fb.c
> @@ -2275,13 +2275,15 @@ int intel_framebuffer_init(struct intel_framebuffer *intel_fb,
>  
>  	if (intel_fb_uses_dpt(fb)) {
>  		struct drm_gem_object *obj = intel_fb_bo(&intel_fb->base);
> +		const struct intel_gtt_view *view;
>  		struct intel_dpt *dpt;
> -		size_t size = 0;
>  
>  		if (intel_fb_needs_pot_stride_remap(intel_fb))
> -			size = intel_remapped_info_size(&intel_fb->remapped_view.gtt.remapped);
> +			view = &intel_fb->remapped_view.gtt;
> +		else
> +			view = &intel_fb->normal_view.gtt;
>  
> -		dpt = intel_parent_dpt_create(display, obj, size);
> +		dpt = intel_parent_dpt_create(display, obj, view);
>  		if (IS_ERR(dpt)) {
>  			drm_dbg_kms(display->drm, "failed to create DPT\n");
>  			ret = PTR_ERR(dpt);

[Severity: High]
This problem wasn't introduced by this patch, but can failing in
intel_framebuffer_init() after intel_bo_framebuffer_init() cause a double
BO unpin on Xe when called from __intel_fbdev_fb_alloc() [1]?

In __intel_fbdev_fb_alloc(), intel_bo_fbdev_create() allocates the BO via
xe_bo_create_pin_map_novm() with XE_BO_FLAG_PINNED set and a pin count of
1, and xe_display_bo_framebuffer_init() only takes a reference without
pinning it again.  If a later step in intel_framebuffer_init() fails and
jumps to err_bo_framebuffer_fini, xe_display_bo_framebuffer_fini() sees
XE_BO_FLAG_PINNED and unpins the BO:

drivers/gpu/drm/xe/display/xe_display_bo.c:xe_display_bo_framebuffer_fini() {
	struct xe_bo *bo = gem_to_xe_bo(obj);

	if (bo->flags & XE_BO_FLAG_PINNED) {
		/* Unpin our kernel fb first */
		xe_bo_lock(bo, false);
		xe_bo_unpin(bo);
		xe_bo_unlock(bo);
	}
	xe_bo_put(bo);
}

When control returns to __intel_fbdev_fb_alloc(), error handling calls
intel_bo_fbdev_destroy() -> xe_display_bo_fbdev_destroy() ->
xe_bo_unpin_map_no_vm(), which unpins the BO a second time, triggering
assertions in xe_bo_unpin() and underflowing bo->pin_count in
ttm_bo_unpin().

[1] https://sashiko.dev/#/bug/linux-227b0044-5f2a-4c7c-81e6-93b67cb4f477

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111112007.16790-1-ville.syrjala@linux.intel.com?part=6