On Mon, 2026-07-20 at 08:25 +0000, Krzysztof Karas wrote: > 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.
I think that also depends on how you address my comment to your patch 1/5 on that if condition, so we'll see if this comment will be still applicable. Thanks, Janusz
