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
