From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>, intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 25/26] drm/i915: Replace 4096 with PAGE_SIZE or I915_GTT_PAGE_SIZE
Date: Mon, 9 Jan 2017 16:19:03 +0000 [thread overview]
Message-ID: <a1dd1254-82cc-37a4-24de-68f3a2be0c49@linux.intel.com> (raw)
In-Reply-To: <20161231120702.21204-26-chris@chris-wilson.co.uk>
On 31/12/2016 12:07, 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
> coe comprehension and future changes.
code
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
> drivers/gpu/drm/i915/i915_gem.c | 4 ++--
> drivers/gpu/drm/i915/i915_gem_context.c | 5 +++--
> drivers/gpu/drm/i915/i915_gem_evict.c | 4 ++--
> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +-
> drivers/gpu/drm/i915/i915_gem_fence_reg.c | 6 +++---
> drivers/gpu/drm/i915/i915_gem_gtt.c | 6 +++---
> drivers/gpu/drm/i915/i915_gem_gtt.h | 5 ++++-
> drivers/gpu/drm/i915/i915_gem_render_state.c | 4 ++--
> drivers/gpu/drm/i915/i915_gem_stolen.c | 7 ++++---
> drivers/gpu/drm/i915/i915_vma.c | 4 ++--
> drivers/gpu/drm/i915/intel_lrc.c | 5 +++--
> drivers/gpu/drm/i915/intel_ringbuffer.c | 10 +++++-----
> 12 files changed, 34 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 0424cee5c767..bad57279e817 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2092,7 +2092,7 @@ u32 i915_gem_get_ggtt_alignment(struct drm_i915_private *dev_priv, u32 size,
> * if a fence register is needed for the object.
> */
> if (INTEL_GEN(dev_priv) >= 4 || tiling_mode == I915_TILING_NONE)
> - return 4096;
> + return I915_GTT_MIN_ALIGNMENT;
>
> /*
> * Previous chips need to be aligned to the size of the smallest
> @@ -3560,7 +3560,7 @@ i915_gem_object_unpin_from_display_plane(struct i915_vma *vma)
> return;
>
> if (--vma->obj->pin_display == 0)
> - vma->display_alignment = 4096;
> + vma->display_alignment = I915_GTT_MIN_ALIGNMENT;
>
> /* Bump the LRU to try and avoid premature eviction whilst flipping */
> if (!i915_vma_is_active(vma))
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index a8f6b8843bdf..d4780cdfb8a0 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -341,7 +341,7 @@ __create_hw_context(struct drm_i915_private *dev_priv,
> if (HAS_GUC(dev_priv) && i915.enable_guc_loading)
> ctx->ggtt_offset_bias = GUC_WOPCM_TOP;
> else
> - ctx->ggtt_offset_bias = 4096;
> + ctx->ggtt_offset_bias = I915_GTT_PAGE_SIZE;
>
> return ctx;
>
> @@ -456,7 +456,8 @@ int i915_gem_context_init(struct drm_i915_private *dev_priv)
> dev_priv->hw_context_size = 0;
> } else if (HAS_HW_CONTEXTS(dev_priv)) {
> dev_priv->hw_context_size =
> - round_up(get_context_size(dev_priv), 4096);
> + round_up(get_context_size(dev_priv),
> + I915_GTT_PAGE_SIZE);
> if (dev_priv->hw_context_size > (1<<20)) {
> DRM_DEBUG_DRIVER("Disabling HW Contexts; invalid size %d\n",
> dev_priv->hw_context_size);
> diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
> index 50129ec1caab..70ba92f91148 100644
> --- a/drivers/gpu/drm/i915/i915_gem_evict.c
> +++ b/drivers/gpu/drm/i915/i915_gem_evict.c
> @@ -263,9 +263,9 @@ int i915_gem_evict_for_vma(struct i915_vma *target, unsigned int flags)
> if (check_color) {
> /* Expand search to cover neighbouring guard pages (or lack!) */
> if (start > target->vm->start)
> - start -= 4096;
> + start -= I915_GTT_PAGE_SIZE;
> if (end < target->vm->start + target->vm->total)
> - end += 4096;
> + end += I915_GTT_PAGE_SIZE;
> }
>
> drm_mm_for_each_node_in_range(node, &target->vm->mm, start, end) {
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index a5fe299da1d3..c6922a5f0514 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -438,7 +438,7 @@ static void *reloc_iomap(struct drm_i915_gem_object *obj,
> memset(&cache->node, 0, sizeof(cache->node));
> ret = drm_mm_insert_node_in_range_generic
> (&ggtt->base.mm, &cache->node,
> - 4096, 0, I915_COLOR_UNEVICTABLE,
> + PAGE_SIZE, 0, I915_COLOR_UNEVICTABLE,
This one is both really, hardcodes that they have to be equal. Hm, yeah
don't know. Still there is value in doing this patch so leave the
problem for later.
> 0, ggtt->mappable_end,
> DRM_MM_SEARCH_DEFAULT,
> DRM_MM_CREATE_DEFAULT);
> diff --git a/drivers/gpu/drm/i915/i915_gem_fence_reg.c b/drivers/gpu/drm/i915/i915_gem_fence_reg.c
> index 6de527df6cdc..67835d7b39c7 100644
> --- a/drivers/gpu/drm/i915/i915_gem_fence_reg.c
> +++ b/drivers/gpu/drm/i915/i915_gem_fence_reg.c
> @@ -81,11 +81,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(vma->node.start & (I915_GTT_PAGE_SIZE - 1));
> + GEM_BUG_ON(vma->fence_size & (I915_GTT_PAGE_SIZE - 1));
> GEM_BUG_ON(stride & 127);
>
> - val = (vma->node.start + vma->fence_size - 4096) << 32;
> + val = (vma->node.start + vma->fence_size - I915_GTT_PAGE_SIZE) << 32;
> val |= vma->node.start;
> val |= (u64)((stride / 128) - 1) << fence_pitch_shift;
> if (tiling == I915_TILING_Y)
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 2c579946447c..116e43bffdb1 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2716,11 +2716,11 @@ static void i915_gtt_color_adjust(const struct drm_mm_node *node,
> u64 *end)
> {
> if (node->color != color)
> - *start += 4096;
> + *start += I915_GTT_PAGE_SIZE;
>
> node = list_next_entry(node, node_list);
> if (node->allocated && node->color != color)
> - *end -= 4096;
> + *end -= I915_GTT_PAGE_SIZE;
> }
>
> int i915_gem_init_ggtt(struct drm_i915_private *dev_priv)
> @@ -2747,7 +2747,7 @@ int i915_gem_init_ggtt(struct drm_i915_private *dev_priv)
> /* Reserve a mappable slot for our lockless error capture */
> ret = drm_mm_insert_node_in_range_generic(&ggtt->base.mm,
> &ggtt->error_capture,
> - 4096, 0,
> + PAGE_SIZE, 0,
> I915_COLOR_UNEVICTABLE,
> 0, ggtt->mappable_end,
> 0, 0);
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index a4070c27d69b..e217b14e14fd 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ 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
> +#define I915_GTT_MIN_ALIGNMENT I915_GTT_PAGE_SIZE
> +
> #define I915_FENCE_REG_NONE -1
> #define I915_MAX_NUM_FENCES 32
> /* 32 fences + sign bit for FENCE_REG_NONE */
> @@ -563,6 +566,6 @@ void i915_gem_gtt_finish_pages(struct drm_i915_gem_object *obj,
> #define PIN_HIGH BIT(9)
> #define PIN_OFFSET_BIAS BIT(10)
> #define PIN_OFFSET_FIXED BIT(11)
> -#define PIN_OFFSET_MASK (~4095)
> +#define PIN_OFFSET_MASK (-I915_GTT_PAGE_SIZE)
>
> #endif
> diff --git a/drivers/gpu/drm/i915/i915_gem_render_state.c b/drivers/gpu/drm/i915/i915_gem_render_state.c
> index 5af19b0bf713..63ae7e813335 100644
> --- a/drivers/gpu/drm/i915/i915_gem_render_state.c
> +++ 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;
>
> so = kmalloc(sizeof(*so), GFP_KERNEL);
> if (!so)
> return -ENOMEM;
>
> - obj = i915_gem_object_create_internal(engine->i915, 4096);
> + obj = i915_gem_object_create_internal(engine->i915, PAGE_SIZE);
> if (IS_ERR(obj)) {
> ret = PTR_ERR(obj);
> goto err_free;
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index 5fd851336a99..be611f1367b4 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -482,7 +482,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;
>
> ggtt->stolen_usable_size =
> ggtt->stolen_size - reserved_total - stolen_usable_start;
> @@ -644,8 +644,9 @@ i915_gem_object_create_stolen_for_preallocated(struct drm_i915_private *dev_priv
> stolen_offset, gtt_offset, size);
>
> /* KISS and expect everything to be page-aligned */
> - if (WARN_ON(size == 0) || WARN_ON(size & 4095) ||
> - WARN_ON(stolen_offset & 4095))
> + if (WARN_ON(size == 0) ||
> + WARN_ON(size & (I915_GTT_PAGE_SIZE - 1)) ||
> + WARN_ON(stolen_offset & (I915_GTT_MIN_ALIGNMENT - 1)))
> return NULL;
>
> stolen = kzalloc(sizeof(*stolen), GFP_KERNEL);
> diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
> index ae00d9f4e520..6596d0963da1 100644
> --- a/drivers/gpu/drm/i915/i915_vma.c
> +++ b/drivers/gpu/drm/i915/i915_vma.c
> @@ -91,7 +91,7 @@ __i915_vma_create(struct drm_i915_gem_object *obj,
> vma->vm = vm;
> vma->obj = obj;
> vma->size = obj->base.size;
> - vma->display_alignment = 4096;
> + vma->display_alignment = I915_GTT_MIN_ALIGNMENT;
>
> if (view) {
> vma->ggtt_view = *view;
> @@ -435,7 +435,7 @@ i915_vma_insert(struct i915_vma *vma, u64 size, u64 alignment, u64 flags)
> * with zero alignment, so where possible use the optimal
> * path.
> */
> - if (alignment <= 4096)
> + if (alignment <= I915_GTT_MIN_ALIGNMENT)
> alignment = 0;
Luckily for the comment, or this would look a bit strange. :)
>
> search_free:
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 600518ee89db..c809eda65f61 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1937,7 +1937,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);
> if (ret)
> return ret;
>
> @@ -2219,7 +2219,8 @@ static int execlists_context_deferred_alloc(struct i915_gem_context *ctx,
>
> WARN_ON(ce->state);
>
> - context_size = round_up(intel_lr_context_size(engine), 4096);
> + context_size = round_up(intel_lr_context_size(engine),
> + I915_GTT_PAGE_SIZE);
>
> /* One extra page as the sharing data between driver and GuC */
> context_size += PAGE_SIZE * LRC_PPHWSP_PN;
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 0971ac396b60..ab83fc22d207 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1736,7 +1736,7 @@ static int init_status_page(struct intel_engine_cs *engine)
> void *vaddr;
> int ret;
>
> - obj = i915_gem_object_create_internal(engine->i915, 4096);
> + obj = i915_gem_object_create_internal(engine->i915, PAGE_SIZE);
> if (IS_ERR(obj)) {
> DRM_ERROR("Failed to allocate status page\n");
> return PTR_ERR(obj);
> @@ -1777,7 +1777,7 @@ static int init_status_page(struct intel_engine_cs *engine)
>
> engine->status_page.vma = vma;
> engine->status_page.ggtt_offset = i915_ggtt_offset(vma);
> - engine->status_page.page_addr = memset(vaddr, 0, 4096);
> + engine->status_page.page_addr = memset(vaddr, 0, PAGE_SIZE);
>
> DRM_DEBUG_DRIVER("%s hws offset: 0x%08x\n",
> engine->name, i915_ggtt_offset(vma));
> @@ -2049,7 +2049,7 @@ static int intel_init_ring_buffer(struct intel_engine_cs *engine)
> }
>
> /* Ring wraparound at offset 0 sometimes hangs. No idea why. */
> - ret = intel_ring_pin(ring, 4096);
> + ret = intel_ring_pin(ring, I915_GTT_PAGE_SIZE);
> if (ret) {
> intel_ring_free(ring);
> goto error;
> @@ -2466,7 +2466,7 @@ static void intel_ring_init_semaphores(struct drm_i915_private *dev_priv,
> if (INTEL_GEN(dev_priv) >= 8 && !dev_priv->semaphore) {
> struct i915_vma *vma;
>
> - obj = i915_gem_object_create(dev_priv, 4096);
> + obj = i915_gem_object_create(dev_priv, PAGE_SIZE);
> if (IS_ERR(obj))
> goto err;
>
> @@ -2683,7 +2683,7 @@ int intel_init_render_ring_buffer(struct intel_engine_cs *engine)
> return ret;
>
> if (INTEL_GEN(dev_priv) >= 6) {
> - ret = intel_engine_create_scratch(engine, 4096);
> + ret = intel_engine_create_scratch(engine, PAGE_SIZE);
> if (ret)
> return ret;
> } else if (HAS_BROKEN_CS_TLB(dev_priv)) {
>
Replacing of 4096-es with names is welcomed, I am just not sure we need
two. Since it will be fixed to eternity perhaps we could just have
PAGE_SIZE for brevity. Not sure. Second opinions please! :)
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2017-01-09 16:19 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-12-31 12:06 Small GEM fixes Chris Wilson
2016-12-31 12:06 ` [PATCH 01/26] drm/i915: Assert that we do create the deferred context Chris Wilson
2016-12-31 12:06 ` [PATCH 02/26] drm/i915: Move a few utility macros into a separate header Chris Wilson
2016-12-31 12:06 ` [PATCH 03/26] drm/i915: Use fixed-sized types for stolen Chris Wilson
2016-12-31 12:06 ` [PATCH 04/26] drm/i915: Use range_overflows() Chris Wilson
2016-12-31 12:06 ` [PATCH 05/26] drm/i915/guc: Exclude the upper end of the Global GTT for the GuC Chris Wilson
2017-01-02 15:25 ` Arkadiusz Hiler
2016-12-31 12:06 ` [PATCH 06/26] drm/i915: Purge loose pages if we run out of DMA remap space Chris Wilson
2016-12-31 12:06 ` [PATCH 07/26] drm/i915: Fix phys pwrite for struct_mutex-less operation Chris Wilson
2016-12-31 12:06 ` [PATCH 08/26] drm/i915: Drain freed objects for mmap space exhaustion Chris Wilson
2016-12-31 12:06 ` [PATCH 09/26] drm/i915: Simplify testing for am-I-the-kernel-context? Chris Wilson
2016-12-31 12:06 ` [PATCH 10/26] drm/i915: Name the anonymous structs inside i915_ggtt_view Chris Wilson
2016-12-31 12:06 ` [PATCH 11/26] drm/i915: Pack the partial view size and offset into a single u64 Chris Wilson
2016-12-31 12:06 ` [PATCH 12/26] drm/i915: Convert i915_ggtt_view to use an anonymous union Chris Wilson
2016-12-31 12:06 ` [PATCH 13/26] drm/i915: Eliminate superfluous i915_ggtt_view_rotated Chris Wilson
2016-12-31 12:06 ` [PATCH 14/26] drm/i915: Eliminate superfluous i915_ggtt_view_normal Chris Wilson
2016-12-31 12:06 ` [PATCH 15/26] drm/i915: Extact compute_partial_view() Chris Wilson
2016-12-31 12:06 ` [PATCH 16/26] drm/i915: Clip the partial view against the object not vma Chris Wilson
2016-12-31 12:06 ` [PATCH 17/26] drm/i915: Align GGTT sizes to a fence tile row Chris Wilson
2016-12-31 12:06 ` [PATCH 18/26] drm/i915: Replace WARNs in fence register writes with extensive asserts Chris Wilson
2016-12-31 12:06 ` [PATCH 19/26] drm/i915: Store required fence size/alignment for GGTT vma Chris Wilson
2016-12-31 12:06 ` [PATCH 20/26] drm/i915: Remove the rounding down of the gen4+ fence region Chris Wilson
2016-12-31 12:06 ` [PATCH 21/26] drm/i915: Include ioctl in set-tiling and get-tiling function names Chris Wilson
2017-01-09 15:12 ` Tvrtko Ursulin
2016-12-31 12:06 ` [PATCH 22/26] drm/i915: Split out i915_gem_object_set_tiling() Chris Wilson
2017-01-09 15:43 ` Tvrtko Ursulin
2017-01-09 15:58 ` Chris Wilson
2017-01-09 16:10 ` Tvrtko Ursulin
2016-12-31 12:06 ` [PATCH 23/26] drm/i915: Construct a request even if the GPU is currently hung Chris Wilson
2016-12-31 12:07 ` [PATCH 24/26] drm/i915: Skip switch to kernel context if already done Chris Wilson
2016-12-31 12:07 ` [PATCH 25/26] drm/i915: Replace 4096 with PAGE_SIZE or I915_GTT_PAGE_SIZE Chris Wilson
2017-01-09 16:19 ` Tvrtko Ursulin [this message]
2017-01-09 16:28 ` Chris Wilson
2016-12-31 12:07 ` [PATCH 26/26] drm/i915: Use the MRU stack search after evicting Chris Wilson
2016-12-31 13:23 ` ✗ Fi.CI.BAT: failure for series starting with [01/26] drm/i915: Assert that we do create the deferred context Patchwork
2016-12-31 13:29 ` Chris Wilson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a1dd1254-82cc-37a4-24de-68f3a2be0c49@linux.intel.com \
--to=tvrtko.ursulin@linux.intel.com \
--cc=chris@chris-wilson.co.uk \
--cc=intel-gfx@lists.freedesktop.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.