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@xxxxxxxxx>
> Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Daniel Vetter <daniel@xxxxxxxx>
<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@xxxxxxxxxxxxxxx>
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
