Re: [PATCH 11/11] drm/xe: Allow probe without a primary GT

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential
issues to consider:
- [High] drm/xe: graphics_desc features left enabled in xe_info_init()
  on media-only GT
- [High] drm/xe: xe_probe_ips() without graphics_ip leaves
  GRAPHICS_VERx100(xe) at 0
- [High] drm/xe: has_page_reclaim_hw_assist left set causes NULL deref
  in xe_pt.c
- [High] drm/xe: pick_engine_gt() returns NULL primary_gt in
  gt_irq_handler()
- [Medium] drm/xe: xe_ggtt_init() skipped in gt_init_with_gt_forcewake()
  without primary GT
- [Medium] drm/xe: unchecked tile->primary_gt dereference in xe_svm.c
  TLB inval stats
- [Low] drm/xe: xe_vram_memtest() calls xe_migrate_clear() when tile
  cannot blit
- [Low] drm/xe: commit message inaccurately claims VRAM is a graphics IP
  property
--

commit d8aec1e526d9f27e3b37444973501f4b05a0116d
Author: Stuart Summers <stuart.summers@intel.com>

drm/xe: Allow probe without a primary GT

This commit allows probing a device as long as either a graphics or media IP
is detected and each tile ends up with at least one GT.

> The dgpu concern the old comment raised is covered by VRAM being a
> graphics IP property, so a device without one has no VRAM to manage.

[Severity: Low]
Is this explanation in the commit message accurate?  xe->info.is_dgfx comes
from struct xe_device_desc rather than graphics_ip, so discrete devices
without a graphics IP or primary GT still allocate and probe VRAM, and
earlier commits in this series ("drm/xe: Fall back to CPU copies when the
tile cannot blit" and "drm/xe: Require a fully visible VRAM BAR without a
primary GT") handle VRAM management without a primary GT.

> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
> index e4220cb0cc298..26b20fa114d2b 100644
> --- a/drivers/gpu/drm/xe/xe_pci.c
> +++ b/drivers/gpu/drm/xe/xe_pci.c
> @@ -1028,11 +1028,11 @@ static int xe_probe_ips(struct xe_device *xe,
>  	}
>  
>  	/*
> -	 * If we couldn't detect the graphics IP, that's considered a fatal
> -	 * error and we should abort driver load.  Failing to detect media
> -	 * IP is non-fatal; we'll just proceed without enabling media support.
> +	 * Either IP on its own is enough to go on with.  Only a device where
> +	 * neither could be detected is a fatal error, since there would be
> +	 * nothing left to drive.
>  	 */
> -	if (!probed_info->graphics_ip)
> +	if (!probed_info->graphics_ip && !probed_info->media_ip)
>  		return -ENODEV;
>  
>  	return 0;

[Severity: High]
When probed_info->graphics_ip is NULL, xe->info.graphics_verx100 remains 0.
On any post-Meteor Lake GMD_ID platform, won't xe_pat_init_early() match
GRAPHICS_VERx100(xe) <= 1210 and select xelp_pat_ops, causing a NULL
function pointer dereference in xe_pat_init() when calling
xe->pat.ops->program_media on the media GT?

drivers/gpu/drm/xe/xe_pat.c:xe_pat_init_early() {
	...
	} else if (GRAPHICS_VERx100(xe) <= 1210) {
		WARN_ON_ONCE(!IS_DGFX(xe) && !xe->info.has_llc);
		xe->pat.ops = &xelp_pat_ops;
}

Having GRAPHICS_VERx100(xe) == 0 also causes xe_ggtt_init_early(),
read_media_fuses(), and the IRQ handlers in xe_irq.c to take legacy pre-1250
and pre-1210 branches.

> @@ -1147,21 +1147,14 @@ static int xe_info_init(struct xe_device *xe,
>  		if (IS_ERR(tile->primary_gt))
>  			return PTR_ERR(tile->primary_gt);
>  
> -		/*
> -		 * It's not currently possible to probe a device with the
> -		 * primary GT disabled.  With some work, this may be future in
> -		 * the possible for igpu platforms (although probably not for
> -		 * dgpu's since access to the primary GT's BCS engines is
> -		 * required for VRAM management).
> -		 */
> -		if (!tile->primary_gt) {
> -			drm_err(&xe->drm, "Cannot probe device with without a primary GT\n");
> -			return -ENODEV;
> -		}
> -
>  		tile->media_gt = alloc_media_gt(tile, media_desc);
>  		if (IS_ERR(tile->media_gt))
>  			return PTR_ERR(tile->media_gt);
> +
> +		if (!tile->primary_gt && !tile->media_gt) {
> +			xe_tile_err(tile, "Cannot probe tile without any GT\n");
> +			return -ENODEV;
> +		}
>  	}

[Severity: High]
When gt_types_allowed=media is set in configfs on a platform with a physical
graphics IP, handle_gmdid() still populates probed_info->graphics_ip, so
xe_info_init() sets xe->info.has_usm from graphics_desc and leaves
xe->info.has_flat_ccs enabled while alloc_primary_gt() returns NULL.  Does
this cause NULL pointer dereferences during probe in get_flat_ccs_offset()
(called with tile->primary_gt from tile_vram_size()) and in
xe_migrate_prepare_vm() (which dereferences
xe_tile_pt_gt(tile)->usm.bb_pool->bo, whereas gt->usm.bb_pool is only
allocated when xe_gt_is_main_type(gt))?

[Severity: High]
Can a VMA unbind dereference a NULL tile->primary_gt when
xe->info.has_page_reclaim_hw_assist is enabled (such as on Nova Lake-P)?
xe_info_init() does not clear has_page_reclaim_hw_assist when probing
without a primary GT, so unbind_op_prepare() allocates a page reclaim list
and generate_reclaim_entry() passes gt = tile->primary_gt to
xe_gt_stats_incr() and xe_page_reclaim_list_abort().

[Severity: High]
When a tile probes without a primary GT, can an interrupt such as PXP's
OTHER_KCR_INSTANCE dereference NULL in gt_irq_handler()?  pick_engine_gt()
falls through to return tile->primary_gt for any class/instance not
explicitly listed for media_gt, and gt_irq_handler() passes that NULL
engine_gt to xe_gt_hw_engine() and gt_other_irq_handler() before checking
for OTHER_KCR_INSTANCE:

drivers/gpu/drm/xe/xe_irq.c:gt_irq_handler() {
			...
			engine_gt = pick_engine_gt(tile, class, instance);

			hwe = xe_gt_hw_engine(engine_gt, class, instance, false);
			if (hwe) {
				xe_hw_engine_handle_irq(hwe, intr_vec);
				continue;
			}

			if (class == XE_ENGINE_CLASS_OTHER) {
				if (xe->info.has_heci_gscfi && instance == OTHER_GSC_INSTANCE)
					xe_heci_gsc_irq_handler(xe, intr_vec);
				else if (instance == OTHER_KCR_INSTANCE)
					xe_pxp_irq_handler(xe, intr_vec);
				else
					gt_other_irq_handler(engine_gt, instance, intr_vec);
			}
}

[Severity: Medium]
Does probing a tile without a primary GT skip xe_ggtt_init()?  Commit
"drm/xe: Run page table updates on the media GT when there is no primary"
updated the xe_migrate_init() check in gt_init_with_all_forcewake() to test
gt == xe_tile_pt_gt(gt_to_tile(gt)), but gt_init_with_gt_forcewake() still
guards xe_ggtt_init(gt_to_tile(gt)->mem.ggtt) with if (xe_gt_is_main_type(gt)),
leaving ggtt->scratch unallocated and skipping initial GGTT PTE clearing.

[Severity: Medium]
In xe_svm_range_notifier_event_begin() and xe_svm_invalidate(), tile->media_gt
is checked for NULL before updating TLB invalidation stats, but
tile->primary_gt is passed unconditionally:

drivers/gpu/drm/xe/xe_svm.c:xe_svm_invalidate() {
	...
	for_each_tile(tile, xe, id) {
		if (tile_mask & BIT(id)) {
			xe_svm_tlb_inval_us_stats_incr(tile->primary_gt, start);
			if (tile->media_gt)
				xe_svm_tlb_inval_us_stats_incr(tile->media_gt, start);
}

Won't this dereference a NULL gt in xe_gt_stats_incr() during SVM
invalidation when tile->primary_gt is NULL?

[Severity: Low]
When CONFIG_DRM_XE_DEBUG_MEM is enabled on a discrete GPU without a primary
GT, does xe_vram_memtest() fail probe by calling xe_migrate_clear() without
checking xe_migrate_can_blit(tile->migrate), emitting BCS blitter clear
commands to a media engine?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110231442.218721-13-stuart.summers@intel.com?part=11