Re: [PATCH 06/23] drm/i915: introduce page_size members

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

On Mon, 2017-08-21 at 19:34 +0100, Matthew Auld wrote:
> In preparation for supporting huge gtt pages for the ppgtt, we introduce
> page size members for gem objects.  We fill in the page sizes by
> scanning the sg table.
> 
> v2: pass the sg_mask to set_pages
> 
> v3: calculate the sg_mask inline with populating the sg_table where
> possible, and pass to set_pages along with the pages.
> 
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel@ffwll.ch>

<SNIP>

> @@ -2477,6 +2490,18 @@ void __i915_gem_object_set_pages(struct drm_i915_gem_object *obj,
>  		__i915_gem_object_pin_pages(obj);
>  		obj->mm.quirked = true;
>  	}
> +
> +	GEM_BUG_ON(!sg_mask);

You can drop this extra newline.

> +
> +	obj->mm.page_sizes.phys = sg_mask;
> +
> +	obj->mm.page_sizes.sg = 0;

Maybe worthy a comment here that any higher multiple of supported page
sizes can be filled with current page size.

> +	for_each_set_bit(bit, &supported_page_sizes, BITS_PER_LONG) {
> +		if (obj->mm.page_sizes.phys & ~0u << bit)
> +			obj->mm.page_sizes.sg |= BIT(bit);

'i' might make this less confusing to read as BIT(i).

> +	}
> +
> +	GEM_BUG_ON(!HAS_PAGE_SIZE(i915, obj->mm.page_sizes.sg));

This usage makes me think we should call the macro HAS_PAGE_SIZES()?

> @@ -259,13 +259,19 @@ struct dma_buf *i915_gem_prime_export(struct drm_device *dev,
>  static int i915_gem_object_get_pages_dmabuf(struct drm_i915_gem_object *obj)
>  {
>  	struct sg_table *pages;
> +	struct scatterlist *sg;
> +	unsigned int sg_mask = 0;

As we are alternating between "unsigned long" and "unsigned int" for
the page size masks, make sure sparse is not complaining.

> +	int n;
>  
>  	pages = dma_buf_map_attachment(obj->base.import_attach,
>  				       DMA_BIDIRECTIONAL);
>  	if (IS_ERR(pages))
>  		return PTR_ERR(pages);
>  
> -	__i915_gem_object_set_pages(obj, pages);

Chris will like you if you do it here;

	sg_mask = 0;
> +	for_each_sg(pages->sgl, sg, pages->nents, n)
> +		sg_mask |= sg->length;
> +
> +	__i915_gem_object_set_pages(obj, pages, sg_mask);
>  
>  	return 0;
>  }

<SNIP>

> @@ -75,6 +76,7 @@ static int i915_gem_object_get_pages_internal(struct drm_i915_gem_object *obj)
>  	}
>  
>  create_st:
> +	sg_mask = 0;

Maybe move this just before the loop, too.

>  	st = kmalloc(sizeof(*st), GFP_KERNEL);
>  	if (!st)
>  		return -ENOMEM;
> @@ -104,6 +106,7 @@ static int i915_gem_object_get_pages_internal(struct drm_i915_gem_object *obj)
>  		} while (1);
>  
>  		sg_set_page(sg, page, PAGE_SIZE << order, 0);
> +		sg_mask |= PAGE_SIZE << order;
>  		st->nents++;
>  
>  		npages -= 1 << order;

<SNIP>

> +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
> @@ -422,12 +423,17 @@ st_set_pages(struct sg_table **st, struct page **pvec, int num_pages)
>  
>  		for_each_sg((*st)->sgl, sg, num_pages, n)
>  			sg_set_page(sg, pvec[n], PAGE_SIZE, 0);
> +
> +		*sg_mask = PAGE_SIZE;

This strictly assigns.

>  	} else {
>  		ret = sg_alloc_table_from_pages(*st, pvec, num_pages,
>  						0, num_pages << PAGE_SHIFT,
>  						GFP_KERNEL);
>  		if (ret)
>  			goto err;
> +
> +		for_each_sg((*st)->sgl, sg, num_pages, n)
> +			*sg_mask |= sg->length;

This appends and assumes input is zero. Maybe zero before the loop?

With above fixed, this is:

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx