Re: [PATCH v3 4/5] drm/i915/gem: Read and shrink memory in a separate function

Hi Janusz,

On 2026-07-15 at 17:31:56 +0200, Janusz Krzysztofik wrote:
> On Mon, 2026-07-13 at 09:58 +0000, Krzysztof Karas wrote:
> > Continue unloading shmem_sg_alloc_table by placing reading
> > folios and shrink call into a new helper.
> > Make the loop a bit more reader-friendly by removing iteration
> > over a structure and replacing it with a do-while loop.
> > 
> > Signed-off-by: Krzysztof Karas <[email protected]>
> > ---
> > v3:
> >  * Split refactoring and put it after the fix in shmem folio
> >   counting suggested by Andi.
> >  * Use do-while loop suggested by Robin. 
> > 
> >  drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 102 ++++++++++++----------
> >  1 file changed, 55 insertions(+), 47 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> > index 4a61b012fb6f..7c8de8fe0a22 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> > @@ -78,6 +78,55 @@ static int validate_size(size_t size, unsigned int page_count,
> >  	return 0;
> >  }
> >  
> > +static struct folio *shmem_shrink_get_folio(struct address_space *mapping,
> > +					    unsigned long folio_index,
> > +					    gfp_t gfp, unsigned int page_count,
> > +					    struct drm_i915_private *i915)
> > +{
> > +	struct folio *folio = NULL;
> > +	unsigned int retries = 2;
> > +
> > +	do {
> > +		cond_resched();
> > +		folio = shmem_read_folio_gfp(mapping, folio_index, gfp);
> > +		if (IS_ERR(folio)) {
> 
> Going again through then unused shrinking and modification of gfp doesn't 
> make sense, I believe.  Could be avoided based on retries value as an 
> additional condition.
If calling i915_gem_shrink again doesn't give us anything, then
looping doesn not really benefit us here. We could do something
like this instead:

folio = shmem_read_folio_gfp(...);
if (IS_ERR(folio)) {
	i915_gem_shrink(...);
	gfp = mapping_gfp_mask(mapping);
	gfp |= __GFP_RETRY_MAYFAIL | __GFP_NOWARN;
	/* again */
	folio = shmem_read_folio_gfp(...);
}

return folio;

That way we'd be explicit about shrinking once and retrying
folio reading only once. Reduced indentation would be added
bonus.

What do you think?

-- 
Best Regards,
Krzysztof