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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Nov 11, 2026 at 11:31:06AM +0000, sashiko-bot@kernel.org wrote:
> 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().

Yes, the xe pin_map() stuff is broken by design. I've mentioned it
before, and I think the correct way to fix it is by making it a
custom object type in xe, and have the object destructor itself do
the needed cleanup.

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

-- 
Ville Syrjälä
Intel