Re: [PATCH v1 1/1] drm/i915/gem: Use simpler versions of min*()

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Data corruption when splitting large folios into multiple scatterlist segments due to small max_segment size.
--

--- Patch [1]: [PATCH v1 1/1] drm/i915/gem: Use simpler versions of min*() ---
commit a3571cfa95156ba739dd7334aec0101cdc3aa63e
Author: Andy Shevchenko <[email protected]>

drm/i915/gem: Use simpler versions of min*()

This commit replaces the harder-to-read min_array() and min_t() 
with min3() and min() in shmem_sg_alloc_table(). No functional 
changes are intended.

> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> @@ -153,11 +153,8 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table *st,
>  			}
>  		} while (1);
>  
> -		nr_pages = min_array(((unsigned long[]) {
> -					folio_nr_pages(folio),
> -					page_count - i,
> -					max_segment / PAGE_SIZE,
> -				      }), 3);
> +		nr_pages = min3(folio_nr_pages(folio),
> +				page_count - i, max_segment / PAGE_SIZE);

[Severity: High]
This is a pre-existing issue, but does this correctly calculate the available
pages for a large folio?

It looks like folio_nr_pages(folio) uses the total folio size rather than the
remaining size from the current offset (e.g. folio_nr_pages(folio) - (i -
folio->index)).

>  
>  		if (!i ||
>  		    sg->length >= max_segment ||
> @@ -168,8 +165,7 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table *st,
>  			st->nents++;
>  			sg_set_folio(sg, folio, nr_pages * PAGE_SIZE, 0);

[Severity: High]
This isn't a bug introduced by this patch, but can this hardcoded offset of 0
corrupt data when splitting large folios?

When a GEM object backed by shmem memory receives a large folio and the
IOMMU's max_segment is smaller than the large folio size, this restricts
nr_pages to the segment size. 

In the next iteration, i advances into the middle of the large folio, and
shmem_read_folio_gfp() returns the same head folio. The code evaluates
folio_pfn(folio) != next_pfn as true (since next_pfn advanced but
folio_pfn still points to the head) and calls sg_set_folio() again. 

Because the offset is hardcoded to 0, it appears the GPU will repeatedly map
the first max_segment bytes of the large folio rather than its subsequent
pages, causing the GPU and CPU memory views to become desynchronized.

>  		} else {
> -			nr_pages = min_t(unsigned long, nr_pages,
> -					 (max_segment - sg->length) / PAGE_SIZE);
> +			nr_pages = min(nr_pages, (max_segment - sg->length) / PAGE_SIZE);
>  
>  			sg->length += nr_pages * PAGE_SIZE;
>  		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1