Hi Krzysztof, On Mon, 2026-07-13 at 09:58 +0000, Krzysztof Karas wrote: > Change the main "for" loop into "while" to get rid of obscure > iterator "i" and use more descriptive name to indicate how many > pages were already covered. Detect first loop with st->nents and > put instructions for that case in their own block for easier > reading. > > Signed-off-by: Krzysztof Karas <[email protected]> > --- > v3: > * Split refactoring and put it after the fix in shmem folio > counting suggested by Andi. > > drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 29 +++++++++++------------ > 1 file changed, 14 insertions(+), 15 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c > b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c > index 7c8de8fe0a22..66d0f8f6ffcc 100644 > --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c > +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c > @@ -135,11 +135,11 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915, > struct sg_table *st, > unsigned int page_count; /* restricted by sg_alloc_table */ > unsigned long next_pfn = 0; /* suppress gcc warning */ > unsigned long folio_start = 0; > + unsigned long pages_done = 0; > unsigned long folio_end = 0; > struct folio *folio = NULL; > struct scatterlist *sg; > gfp_t noreclaim; > - unsigned long i; > int ret; > > page_count = size / PAGE_SIZE; > @@ -163,15 +163,15 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915, > struct sg_table *st, > > sg = st->sgl; > st->nents = 0; > - for (i = 0; i < page_count; i++) { > + while (pages_done < page_count) { > unsigned long folio_page_index = 0; > unsigned long nr_pages; > gfp_t gfp = noreclaim; > > /* Grab the next folio if we exhausted the current one. */ > - if (!i || i > folio_end) { > - folio = shmem_shrink_get_folio(mapping, i, gfp, > - page_count, i915); > + if (!pages_done || pages_done > folio_end) { > + folio = shmem_shrink_get_folio(mapping, pages_done, gfp, > + page_count - pages_done, > i915); > if (IS_ERR(folio)) { > ret = PTR_ERR(folio); > goto err_sg; > @@ -181,7 +181,7 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915, > struct sg_table *st, > folio_end = folio_start + folio_nr_pages(folio) - 1; > } > > - folio_page_index = i - folio_start; > + folio_page_index = pages_done - folio_start; > if (WARN_ON_ONCE(folio_page_index >= folio_nr_pages(folio))) { > ret = -EINVAL; > folio_put(folio); > @@ -190,16 +190,15 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915, > struct sg_table *st, > > nr_pages = min_array(((unsigned long[]) { > folio_nr_pages(folio) - > folio_page_index, > - page_count - i, > + page_count - pages_done, > max_t(unsigned int, 1, max_segment / > PAGE_SIZE), > }), 3); > - > - if (!i || > - sg->length >= max_segment || > - folio_pfn(folio) + folio_page_index != next_pfn) { > - if (i) > - sg = sg_next(sg); > - > + if (!st->nents) { > + st->nents++; > + sg_set_page(sg, folio_page(folio, 0), nr_pages * > PAGE_SIZE, 0); > + } else if (sg->length >= max_segment || > + folio_pfn(folio) + folio_page_index != next_pfn) { > + sg = sg_next(sg);
Repeating two or three lines of code to avoid calling another one conditionally doesn't look optimal to me. Maybe you could invent a simple replacement of that 'if (i)' conditional expression. Thanks, Janusz > st->nents++; > sg_set_page(sg, folio_page(folio, folio_page_index), > nr_pages * PAGE_SIZE, 0); > @@ -212,7 +211,7 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915, > struct sg_table *st, > } > > next_pfn = folio_pfn(folio) + folio_page_index + nr_pages; > - i += nr_pages - 1; > + pages_done += nr_pages; > > /* Check that the i965g/gm workaround works. */ > GEM_BUG_ON(gfp & __GFP_DMA32 && next_pfn >= 0x00100000UL);
