Hi Janusz, On 2026-07-15 at 19:04:42 +0200, Janusz Krzysztofik wrote: > 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. Perhaps it is not optimal. I do not feel comfortable having two conditions that contradict each other in the same block, which is why I wanted to take out the first iteration setup.
It is more about aesthetics here, so I do not have a strong argument here besides readability. If that is not enough, then I'll revert to the previous code. -- Best Regards, Krzysztof
