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@xxxxxxxxxxxxxxxxxx>
>
> <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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
