Re: [PATCH v5 1/6] drm/i915/gem: Count mapped pages in a folio

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.


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.

>  	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.

>  	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
>