[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.
> 
> > +++ 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.

Now using I965_FENCE_PAGE (a new 4096ul from i915_gem_fence_reg.h).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre