Hi Andi,
On 2026-08-19 at 13:56:13 +0200, Andi Shyti wrote:
> Hi Krzysztof,
>
> your patch looks good, but it needs to be polished a bit.
>
> ...
>
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> > b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> > index 06543ae60706..f338dc39fad1 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> > @@ -68,10 +68,13 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915,
> > struct sg_table *st,
> > unsigned int max_segment)
> > {
> > unsigned int page_count; /* restricted by sg_alloc_table */
> > - unsigned long i;
> > + unsigned long next_pfn = 0; /* suppress gcc warning */
> > + unsigned long folio_start = 0;
> > + unsigned long folio_end = 0;
>
> Can these go inside the for loop? Keep variables in the innermost
> section needed.
They need to keep their value between iterations, as folio is
acquired once, not every iteration.
>
>
> Besides
>
> > + struct folio *folio = NULL;
> > struct scatterlist *sg;
> > - unsigned long next_pfn = 0; /* suppress gcc warning */
> > gfp_t noreclaim;
> > + unsigned long i;
>
> Please, don't reorder variables in this patch, it's out of the
> scope.
Very well.
>
> > int ret;
> >
> > if (overflows_type(size / PAGE_SIZE, page_count))
> > @@ -85,6 +88,9 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915,
> > struct sg_table *st,
> > if (size > resource_size(&mr->region))
> > return -ENOMEM;
> >
> > + if (max_segment < PAGE_SIZE)
> > + return -EINVAL;
> > +
>
> the max_segment validation is out of the scope.
Understood, I'll move that to a separate patch to keep Janusz's
suggestion in.
>
> > if (sg_alloc_table(st, page_count, GFP_KERNEL | __GFP_NOWARN))
> > return -ENOMEM;
> >
> > @@ -101,7 +107,7 @@ 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++) {
> > - struct folio *folio;
>
> Why are you moving this out of the for loop?
>
> > + unsigned long folio_page_index = 0;
>
> This initialization is not necessary.
>
> > unsigned long nr_pages;
> > const unsigned int shrink[] = {
> > I915_SHRINK_BOUND | I915_SHRINK_UNBOUND,
>
> ...
>
> > + }
> > + } while (1);
> > +
> > + folio_start = folio_pgoff(folio);
> > + folio_end = folio_start + folio_nr_pages(folio) - 1;
> > + }
> > +
> > + folio_page_index = i - folio_start;
> > + if (WARN_ON_ONCE(folio_page_index >= folio_nr_pages(folio))) {
> > + ret = -EINVAL;
> > + folio_put(folio);
>
> I think shmem_sg_free_table() drops the folio reference already,
> right?
>
> > + goto err_sg;
> > + }
>
> Is this WARN_ON_ONCE() check really necessary?
>
> >
> > nr_pages = min_array(((unsigned long[]) {
> > - folio_nr_pages(folio),
> > + folio_nr_pages(folio) -
> > folio_page_index,
> > page_count - i,
> > max_segment / PAGE_SIZE,
> > }), 3);
>
> ...
>
> > @@ -186,6 +216,7 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915,
> > struct sg_table *st,
> > i915_sg_trim(st);
> >
> > return 0;
> > +
>
> Drop this change
>
> What I want from this patch is a minimal change, without any
> churn not belonging to the sole scope of the patch.
>
> Thanks, Krzysztof,
> Andi
>
> > err_sg:
> > sg_mark_end(sg);
> > if (sg != st->sgl) {
> > --
> > 2.34.1
> >
I'll apply the suggestions in the next version.
Thanks for looking at this!
--
Best Regards,
Krzysztof
