Hi Andi,
Thanks for review.
On Wednesday, 25 February 2026 15:41:13 CET Andi Shyti wrote:
> Hi Janusz,
>
> ...
>
> > @@ -153,8 +153,12 @@ int shmem_sg_alloc_table(struct drm_i915_private
> > *i915, struct sg_table *st,
> > }
> > } while (1);
> >
>
> Perhaps we could add here:
>
> max_pages = max_segment >> PAGE_SHIFT;
> /* Just to be paranoic, but not necessary */
> if (!max_pages)
GEM_BUG_ON(!max_pages), I would rather say. The max_segment comes from
drivers/gpu/drm/i915/i915_scatterlist.h:i915_sg_segment_size(struct device
*dev),
let's assume that can't be that much broken.
> max_pages = 1;
>
>
> > - nr_pages = min_t(unsigned long,
> > - folio_nr_pages(folio), page_count - i);
> > + nr_pages = min_array(((unsigned long[]) {
> > + folio_nr_pages(folio),
> > + page_count - i,
> > + max_segment / PAGE_SIZE,
>
> max_segment >> PAGE_SHIFT ?
Yeah, shift seems more optimal than division here, however, I've just followed
the patter used a few lines below. where we can see a multiplication, not a
right shift, and since PAGE_SIZE is a constant, I hope the compiler will
optimize that.
>
> For clarity this can be written as
>
> nr_pages = min_t(unsigned long,
> folio_nr_pages(folio), page_count - i);
> nr_pages = min_t(unsigned long, nr_pages, max_pages);
Do you think the min_array() is less clear? Let's see what others say.
>
> But these are nitpicks, it's then up to you to choose the style.
>
> Reviewed-by: Andi Shyti <[email protected]>
Thank you,
Janusz
>
> Thanks,
> Andi
>
> > + }), 3);
> > +
>
