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

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

On Fri, 2017-10-06 at 15:50 +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.
> 
> v4: bunch of improvements from Joonas
> 
> v5: fix num_pages blunder
>     introduce i915_sg_page_sizes helper
> 
> v6: prefer GEM_BUG_ON(sizes == 0)
> 
> 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>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

<SNIP>

> @@ -3101,6 +3116,10 @@ intel_info(const struct drm_i915_private *dev_priv)
>  #define USES_PPGTT(dev_priv)		(i915_modparams.enable_ppgtt)
>  #define USES_FULL_PPGTT(dev_priv)	(i915_modparams.enable_ppgtt >= 2)
>  #define USES_FULL_48BIT_PPGTT(dev_priv)	(i915_modparams.enable_ppgtt == 3)
> +#define HAS_PAGE_SIZES(dev_priv, sizes) ({ \
> +	GEM_BUG_ON((sizes) == 0); \
> +	((sizes) & ~(dev_priv)->info.page_sizes) == 0; \
> +})

Maybe,

#define HAS_PAGE_SIZES(dev_priv, sizes) (\
	BUILD_BUG_ON_ZERO((sizes) == 0) +
	...
)

To avoid the compound statement. Unlikely to be used in static const
expressions, but no reason not to have it flxible from the beginning.

> @@ -2266,6 +2266,8 @@ void __i915_gem_object_put_pages(struct drm_i915_gem_object *obj,
>  	if (!IS_ERR(pages))
>  		obj->ops->put_pages(obj, pages);
>  
> +	obj->mm.page_sizes.phys = obj->mm.page_sizes.sg = 0;

Could be "obj->mm.page_sizes = { .phys = 0, .sg = 0 };" Or at least
split this to two lines so checkpatch won't complain.

> @@ -2308,6 +2310,7 @@ static int i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
>  	struct page *page;
>  	unsigned long last_pfn = 0;	/* suppress gcc warning */
>  	unsigned int max_segment = i915_sg_segment_size();
> +	unsigned int sg_mask;

Was 'sg_page_sizes' considered?

> @@ -2460,8 +2468,13 @@ static int i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
>  }
>  
>  void __i915_gem_object_set_pages(struct drm_i915_gem_object *obj,
> -				 struct sg_table *pages)
> +				 struct sg_table *pages,
> +				 unsigned int sg_mask)
>  {
> +	struct drm_i915_private *i915 = to_i915(obj->base.dev);
> +	unsigned long supported = INTEL_INFO(i915)->page_sizes;

'supported_sizes' might be more descriptive if this ever grows bigger.

Only thing I really feel strongly about is the renaming of
s/sg_mask/sg_page_sizes/. That fixed;

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