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

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