Re: [PATCH v3 5/5] drm/i915/gem: Remove iterator and use while loop

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