Re: [PATCH] drm/i915: Replace 4096 with PAGE_SIZE or I915_GTT_PAGE_SIZE

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

On ti, 2017-01-10 at 12:15 +0000, Chris Wilson wrote:
> Start converting over from the byte count to its semantic macro, either
> we want to allocate the size of a physical page in main memory or we
> want the size of a virtual page in the GTT. 4096 could mean either, but
> PAGE_SIZE and I915_GTT_PAGE_SIZE are explicit and should help improve
> code comprehension and future changes. In the future, we may want to use
> variable GTT page sizes and so have the challenge of knowing which
> hardcoded values were used to represent a physical page vs the virtual
> page.
> 
> v2: Look for a few more 4096s to convert, discover IS_ALIGNED().
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

<SNIP>

> 
> @@ -80,11 +80,11 @@ static void i965_write_fence_reg(struct drm_i915_fence_reg *fence,
> >  		unsigned int stride = i915_gem_object_get_stride(vma->obj);
>  
>  		GEM_BUG_ON(!i915_vma_is_map_and_fenceable(vma));
> -		GEM_BUG_ON(vma->node.start & 4095);
> -		GEM_BUG_ON(vma->fence_size & 4095);
> -		GEM_BUG_ON(stride & 127);
> +		GEM_BUG_ON(!IS_ALIGNED(vma->node.start, I915_GTT_PAGE_SIZE));
> +		GEM_BUG_ON(!IS_ALIGNED(vma->fence_size, I915_GTT_PAGE_SIZE));

GTT_MIN_ALIGN would make more sense here? I don't think this test
should change if page size increases.

> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -40,6 +40,9 @@
>  #include "i915_gem_timeline.h"
>  #include "i915_gem_request.h"
>  
> +#define I915_GTT_PAGE_SIZE 4096

Could be 4096UL (to mimic PAGE_SIZE) as discussed in IRC.

> +++ b/drivers/gpu/drm/i915/i915_gem_render_state.c
> @@ -187,14 +187,14 @@ int i915_gem_render_state_init(struct intel_engine_cs *engine)
>  	if (!rodata)
>  		return 0;
>  
> -	if (rodata->batch_items * 4 > 4096)
> +	if (rodata->batch_items * 4 > PAGE_SIZE)
>  		return -EINVAL;

Umm, how is not render state tied to GT page size?

> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -485,7 +485,7 @@ int i915_gem_init_stolen(struct drm_i915_private *dev_priv)
>  	stolen_usable_start = 0;
>  	/* WaSkipStolenMemoryFirstPage:bdw+ */
>  	if (INTEL_GEN(dev_priv) >= 8)
> -		stolen_usable_start = 4096;
> +		stolen_usable_start = PAGE_SIZE;

This is again borderline. Maybe we should have I915_GTT_MIN_PAGE_SIZE
and use it for MIN_SIZE and MIN_ALIGNMENT? It would also be self-
documenting not to necessarily be the actual page size for the object
in question?

> @@ -82,7 +82,7 @@ u32 i915_gem_fence_size(struct drm_i915_private *i915,
>  
>  	if (INTEL_GEN(i915) >= 4) {
>  		stride *= i915_gem_tile_height(tiling);
> -		GEM_BUG_ON(stride & 4095);
> +		GEM_BUG_ON(!IS_ALIGNED(stride, I915_GTT_PAGE_SIZE));

MIN_ALIGN would read better here.

> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1927,7 +1927,7 @@ int logical_render_ring_init(struct intel_engine_cs *engine)
>  	engine->emit_breadcrumb = gen8_emit_breadcrumb_render;
>  	engine->emit_breadcrumb_sz = gen8_emit_breadcrumb_render_sz;
>  
> -	ret = intel_engine_create_scratch(engine, 4096);
> +	ret = intel_engine_create_scratch(engine, PAGE_SIZE);

GTT_PAGE?

The ones after this point seem to follow the logic of getting accessed
from CPU and are thus PAGE_SIZE. So maybe max(PAGE_SIZE,
GTT_MIN_PAGE_SIZE) as unified_page_size :P Others could comment here,
too.

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