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.
> +++ 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
