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

On the other hand, the 4096 here is FENCE_PAGE units. So perhaps better
as


diff --git a/drivers/gpu/drm/i915/i915_gem_fence_reg.c b/drivers/gpu/drm/i915/i915_gem_fence_reg.c
index cd93468c6db2..d3bcacea63de 100644
--- a/drivers/gpu/drm/i915/i915_gem_fence_reg.c
+++ b/drivers/gpu/drm/i915/i915_gem_fence_reg.c
@@ -60,10 +60,14 @@
 static void i965_write_fence_reg(struct drm_i915_fence_reg *fence,
                                 struct i915_vma *vma)
 {
+#define I965_FENCE_PAGE 4096UL
        i915_reg_t fence_reg_lo, fence_reg_hi;
        int fence_pitch_shift;
        u64 val;
 
+       /* Make sure that we are consistent with i915_gem_fence_alignment() */
+       BUILD_BUG_ON(!IS_ALIGNED(I915_GTT_MIN_ALIGNMENT, I965_FENCE_PAGE));
+
        if (INTEL_INFO(fence->i915)->gen >= 6) {
                fence_reg_lo = FENCE_REG_GEN6_LO(fence->id);
                fence_reg_hi = FENCE_REG_GEN6_HI(fence->id);
@@ -80,11 +84,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(!IS_ALIGNED(vma->node.start, I915_GTT_PAGE_SIZE));
-               GEM_BUG_ON(!IS_ALIGNED(vma->fence_size, I915_GTT_PAGE_SIZE));
+               GEM_BUG_ON(!IS_ALIGNED(vma->node.start, I965_FENCE_PAGE));
+               GEM_BUG_ON(!IS_ALIGNED(vma->fence_size, I965_FENCE_PAGE));
                GEM_BUG_ON(!IS_ALIGNED(stride, 128));
 
-               val = (vma->node.start + vma->fence_size - I915_GTT_PAGE_SIZE) << 32;
+               val = (vma->node.start + vma->fence_size - I965_FENCE_PAGE) << 32;
                val |= vma->node.start;
                val |= (u64)((stride / 128) - 1) << fence_pitch_shift;
                if (i915_gem_object_get_tiling(vma->obj) == I915_TILING_Y)
@@ -111,6 +115,7 @@ static void i965_write_fence_reg(struct drm_i915_fence_reg *fence,
                I915_WRITE(fence_reg_lo, lower_32_bits(val));
                POSTING_READ(fence_reg_lo);
        }
+#undef I965_FENCE_PAGE
 }


-- 
Chris Wilson, Intel Open Source Technology Centre