Re: [PATCH 2/2] drm/xe/display: Use xe_bo_addr_iter in remapped DPT pinning

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] drm/xe/display: missing row stride advance in write_dpt_remapped_tiled()
- [High] drm/xe/display: signed 32-bit overflow in write_dpt_remapped_tiled()
--

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

drm/xe/display: Use xe_bo_addr_iter in remapped DPT pinning

This commit replaces individual xe_bo_addr() lookups with
xe_bo_addr_iter_first() and xe_bo_addr_iter_addr() in
write_dpt_remapped_linear() and write_dpt_remapped_tiled() to reduce
remapped page table pinning complexity from O(N^2) to O(N).

> diff --git a/drivers/gpu/drm/xe/display/xe_fb_pin.c b/drivers/gpu/drm/xe/display/xe_fb_pin.c
> index e09116265ae95..e7e5c70c7b236 100644
> --- a/drivers/gpu/drm/xe/display/xe_fb_pin.c
> +++ b/drivers/gpu/drm/xe/display/xe_fb_pin.c

[ ... ]

> @@ -90,18 +91,17 @@ write_dpt_remapped_tiled(struct xe_bo *bo, struct iosys_map *map,
>  	struct xe_ggtt *ggtt = xe_device_get_root_tile(xe)->mem.ggtt;
>  	const u64 pte = xe_ggtt_encode_pte_flags(ggtt, bo,
>  						 xe_cache_pat_idx(xe, XE_CACHE_NONE));
> -	unsigned int offset, column, row;
> +	u32 offset = plane->offset * XE_PAGE_SIZE;
> +	struct xe_res_cursor cur;
>  
> -	for (row = 0; row < plane->height; row++) {
> -		offset = (plane->offset + plane->src_stride * row) *
> -			 XE_PAGE_SIZE;
> -
> -		for (column = 0; column < plane->width; column++) {
> -			u64 addr = xe_bo_addr(bo, offset, XE_PAGE_SIZE);
> +	xe_bo_addr_iter_first(bo, &cur, offset, plane->height * plane->src_stride * XE_PAGE_SIZE);

[Severity: High]
Can the size expression in write_dpt_remapped_tiled() in
drivers/gpu/drm/xe/display/xe_fb_pin.c overflow a 32-bit signed integer
before being converted to u64 for xe_bo_addr_iter_first()?

In include/drm/intel/gtt_view_types.h, struct intel_remapped_plane_info
defines height and src_stride as u16:

	struct {
		u16 width;
		u16 height;
		u16 src_stride;
		u16 dst_stride;
	};

And in drivers/gpu/drm/xe/xe_bo.h, the page size macro is defined with a
signed int literal:

	#define XE_PTE_SHIFT			12
	#define XE_PAGE_SIZE			(1 << XE_PTE_SHIFT)

Under C integer promotion rules, multiplying plane->height,
plane->src_stride, and the page size is evaluated in 32-bit signed int
arithmetic. When a large tiled framebuffer is created and pinned via
intel_fill_fb_info() -> calc_plane_remap_info() and
__xe_pin_fb_vma_dpt() -> write_dpt_remapped() -> write_dpt_remapped_tiled()
(for example, a 16384x16384 Tile4 framebuffer with a 131072-byte pitch where
plane->src_stride is 1024 and plane->height is 512, totaling 524,288 pages),
the 32-bit signed multiplication overflows to -2147483648 and sign-extends
to 0xffffffff80000000ULL in cur.remaining.

Could this use mul_u32_u32() or a u64 cast when computing the byte size?

> +	while (cur.remaining) {
> +		for (u32 column = 0; column < plane->width; column++) {
> +			u64 addr = xe_bo_addr_iter_addr(&cur);
>  
>  			iosys_map_wr(map, dest, u64, addr | pte);
>  			dest += sizeof(u64);
> -			offset += XE_PAGE_SIZE;
> +			xe_res_next(&cur, XE_PAGE_SIZE);
>  		}
>  
>  		dest = write_dpt_padding(map, dest,

[Severity: Critical]
When plane->src_stride > plane->width (for example, when the framebuffer
pitch in tiles exceeds the plane width in tiles via calc_plane_remap_info()
or intel_plane_remap_gtt()), doesn't write_dpt_remapped_tiled() in
drivers/gpu/drm/xe/display/xe_fb_pin.c miss advancing cur across the
remaining (plane->src_stride - plane->width) pages at the end of each row?

Previously, the outer loop iterated over row = 0 .. plane->height - 1 and
recomputed offset from plane->src_stride * row, skipping
(plane->src_stride - plane->width) pages in bo between rows. With the new
loop, cur is initialized with a total size covering plane->height *
plane->src_stride pages, but each iteration of while (cur.remaining) only
advances cur by plane->width pages in the inner column loop.

When plane->src_stride > plane->width, this appears to cause several
regressions:

1. Row 1 and subsequent rows map the wrong pages from bo into map, reading
from offset + row * plane->width pages instead of offset + row *
plane->src_stride pages.

2. Because cur.remaining starts at plane->height * plane->src_stride pages
and only decreases by plane->width pages per outer loop iteration, the
while (cur.remaining) loop executes more than plane->height times. However,
__xe_pin_fb_vma_dpt() in drivers/gpu/drm/xe/display/xe_fb_pin.c allocates
dpt for intel_remapped_info_size(&view->remapped) entries (plane->height *
plane->dst_stride per tiled plane):

	else if (intel_gtt_view_is_remapped(view))
		dpt_size = ALIGN(intel_remapped_info_size(&view->remapped) * 8,
				 XE_PAGE_SIZE);

The extra outer loop iterations write past the end of dpt->vmap via
iosys_map_wr(map, dest, u64, addr | pte).

3. If (plane->height * plane->src_stride) is not a multiple of
plane->width, cur.remaining reaches 0 in the middle of the inner column
loop and underflows on the next column iteration in xe_res_next() in
drivers/gpu/drm/xe/xe_res_cursor.h:

	XE_WARN_ON(size > cur->remaining);

	cur->remaining -= size;
	if (!cur->remaining)
		return;

This wraps cur->remaining to 0xfffffffffffff000ULL and walks off the end
of cur->sgl in __xe_res_sg_next() or the gpu_buddy_block list.

4. When plane->offset > 0 (such as a non-zero horizontal tile offset
src_x > 0 in intel_plane_remap_gtt()), passing plane->height *
plane->src_stride pages to xe_bo_addr_iter_first() includes
(plane->src_stride - plane->width) trailing pages after the last row, which
can exceed the allocated size of bo and trigger a warning in xe_res_first().

Should write_dpt_remapped_tiled() advance cur by
(plane->src_stride - plane->width) pages after each row when
plane->src_stride > plane->width (or size the iterator to
((plane->height - 1) * plane->src_stride + plane->width) pages and advance
by the stride gap between rows)?

>
> [ ... ]
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021150116.516131-3-dev@lankhorst.se?part=2