[Intel-gfx] [PATCH] drm/i915: Replace 4096 with PAGE_SIZE or I915_GTT_PAGE_SIZE

[Chris Wilson]

On Tue, Jan 10, 2017 at 03:55:35PM +0200, Joonas Lahtinen wrote:
> 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 at 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.

Yes, MIN_ALIGN makes more sense here.
 
> > +++ 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?

We only allocated a physical page for the render state, not a virtual page.

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

Probably best left as 4096 as I have no idea what the hw is actually
doing.

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

We allocate 1 physical page for scratch.
 
> 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.

Right, it the object allocation ones that are the most contentious - are
we only allocating a physical page (such as for scratch / hws) or do we
want to occupy a full GTT_PAGE. A bit like asking if we want to allocate
a 2+GiB physical page where only 4096 bytes would do. :)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre