[Intel-gfx] [PATCH 10/38] drm/i915: Remove highly confusing i915_gem_obj_ggtt_pin()
Daniel Vetter
daniel at ffwll.ch
Wed Jun 8 09:43:57 UTC 2016
On Fri, Jun 03, 2016 at 05:55:25PM +0100, Chris Wilson wrote:
> Since i915_gem_obj_ggtt_pin() is an idiom breaking curry function for
> i915_gem_object_ggtt_pin(), spare us the confustion and remove it.
> Removing it now simplifies later patches to change the i915_vma_pin()
> (and friends) interface.
>
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
Diff looks like accidentally squashed in speed-up to help gcc along with
bitfields in vma. Needs to be unsquashed.
-Daniel
> ---
> drivers/gpu/drm/i915/i915_drv.h | 35 ++++++++-------------
> drivers/gpu/drm/i915/i915_gem.c | 46 +++++++++++++--------------
> drivers/gpu/drm/i915/i915_gem_context.c | 5 ++-
> drivers/gpu/drm/i915/i915_gem_execbuffer.c | 10 +++---
> drivers/gpu/drm/i915/i915_gem_gtt.h | 47 +++++++++++++++-------------
> drivers/gpu/drm/i915/i915_gem_render_state.c | 2 +-
> drivers/gpu/drm/i915/i915_guc_submission.c | 4 +--
> drivers/gpu/drm/i915/intel_guc_loader.c | 2 +-
> drivers/gpu/drm/i915/intel_lrc.c | 8 +++--
> drivers/gpu/drm/i915/intel_overlay.c | 3 +-
> drivers/gpu/drm/i915/intel_ringbuffer.c | 16 +++++-----
> 11 files changed, 89 insertions(+), 89 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index f537d8fc5e0f..861d132b2fe4 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2934,32 +2934,32 @@ void i915_gem_free_object(struct drm_gem_object *obj);
> int __must_check
> i915_vma_pin(struct i915_vma *vma, u64 size, u64 alignment, u64 flags);
> /* Flags used by pin/bind&friends. */
> -#define PIN_MAPPABLE (1<<0)
> -#define PIN_NONBLOCK (1<<1)
> -#define PIN_GLOBAL (1<<2)
> -#define PIN_OFFSET_BIAS (1<<3)
> -#define PIN_USER (1<<4)
> -#define PIN_UPDATE (1<<5)
> -#define PIN_ZONE_4G (1<<6)
> -#define PIN_HIGH (1<<7)
> -#define PIN_OFFSET_FIXED (1<<8)
> +#define PIN_GLOBAL (1<<0)
> +#define PIN_USER (1<<1)
> +#define PIN_UPDATE (1<<2)
> +#define PIN_MAPPABLE (1<<3)
> +#define PIN_ZONE_4G (1<<4)
> +#define PIN_NONBLOCK (1<<5)
> +#define PIN_HIGH (1<<6)
> +#define PIN_OFFSET_BIAS (1<<7)
> +#define PIN_OFFSET_FIXED (1<<8)
> #define PIN_OFFSET_MASK (~4095)
>
> static inline void __i915_vma_pin(struct i915_vma *vma)
> {
> GEM_BUG_ON(vma->pin_count == DRM_I915_GEM_OBJECT_MAX_PIN_COUNT);
> - vma->pin_count++;
> + vma->flags++;
> }
>
> static inline bool i915_vma_is_pinned(struct i915_vma *vma)
> {
> - return vma->pin_count;
> + return vma->flags & DRM_I915_GEM_OBJECT_MAX_PIN_COUNT;
> }
>
> static inline void __i915_vma_unpin(struct i915_vma *vma)
> {
> GEM_BUG_ON(!i915_vma_is_pinned(vma));
> - vma->pin_count--;
> + vma->flags--;
> }
>
> static inline void i915_vma_unpin(struct i915_vma *vma)
> @@ -2972,7 +2972,7 @@ int __must_check
> i915_gem_object_ggtt_pin(struct drm_i915_gem_object *obj,
> const struct i915_ggtt_view *view,
> uint64_t size,
> - uint32_t alignment,
> + uint64_t alignment,
> uint64_t flags);
>
> int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
> @@ -3223,15 +3223,6 @@ static inline bool i915_gem_obj_ggtt_bound(struct drm_i915_gem_object *obj)
> unsigned long
> i915_gem_obj_ggtt_size(struct drm_i915_gem_object *obj);
>
> -static inline int __must_check
> -i915_gem_obj_ggtt_pin(struct drm_i915_gem_object *obj,
> - uint32_t alignment,
> - unsigned flags)
> -{
> - return i915_gem_object_ggtt_pin(obj, &i915_ggtt_view_normal,
> - 0, alignment, flags);
> -}
> -
> void i915_gem_object_ggtt_unpin_view(struct drm_i915_gem_object *obj,
> const struct i915_ggtt_view *view);
> static inline void
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 71a32a9f9858..53776a071ce7 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -772,7 +772,9 @@ i915_gem_gtt_pwrite_fast(struct drm_device *dev,
> char __user *user_data;
> int page_offset, page_length, ret;
>
> - ret = i915_gem_obj_ggtt_pin(obj, 0, PIN_MAPPABLE | PIN_NONBLOCK);
> + ret = i915_gem_object_ggtt_pin(obj, NULL,
> + 0, 0,
> + PIN_MAPPABLE | PIN_NONBLOCK);
> if (ret)
> goto out;
>
> @@ -3408,32 +3410,35 @@ void __i915_vma_set_map_and_fenceable(struct i915_vma *vma)
> int
> i915_vma_pin(struct i915_vma *vma, u64 size, u64 alignment, u64 flags)
> {
> - unsigned bound = vma->bound;
> + unsigned bound;
> int ret;
>
> GEM_BUG_ON((flags & (PIN_GLOBAL | PIN_USER)) == 0);
> GEM_BUG_ON((flags & PIN_GLOBAL) && !vma->is_ggtt);
>
> - if (WARN_ON(vma->pin_count == DRM_I915_GEM_OBJECT_MAX_PIN_COUNT))
> - return -EBUSY;
> -
> /* Pin early to prevent the shrinker/eviction logic from destroying
> * our vma as we insert and bind.
> */
> - __i915_vma_pin(vma);
> + bound = vma->flags++;
> + if (WARN_ON((bound & 0xf) == (DRM_I915_GEM_OBJECT_MAX_PIN_COUNT-1))) {
> + ret = -EBUSY;
> + goto err;
> + }
>
> - if (!bound) {
> + if ((bound & 0xff) == 0) {
> ret = i915_vma_insert(vma, size, alignment, flags);
> if (ret)
> goto err;
> }
>
> - ret = i915_vma_bind(vma, vma->obj->cache_level, flags);
> - if (ret)
> - goto err;
> + if (~(bound >> 4) & (flags & (GLOBAL_BIND | LOCAL_BIND))) {
> + ret = i915_vma_bind(vma, vma->obj->cache_level, flags);
> + if (ret)
> + goto err;
>
> - if ((bound ^ vma->bound) & GLOBAL_BIND)
> - __i915_vma_set_map_and_fenceable(vma);
> + if ((bound ^ vma->flags) & (GLOBAL_BIND << 4))
> + __i915_vma_set_map_and_fenceable(vma);
> + }
>
> GEM_BUG_ON(i915_vma_misplaced(vma, size, alignment, flags));
> return 0;
> @@ -3447,13 +3452,14 @@ int
> i915_gem_object_ggtt_pin(struct drm_i915_gem_object *obj,
> const struct i915_ggtt_view *view,
> uint64_t size,
> - uint32_t alignment,
> + uint64_t alignment,
> uint64_t flags)
> {
> struct i915_vma *vma;
> int ret;
>
> - BUG_ON(!view);
> + if (view == NULL)
> + view = &i915_ggtt_view_normal;
>
> vma = i915_gem_obj_lookup_or_create_ggtt_vma(obj, view);
> if (IS_ERR(vma))
> @@ -3465,11 +3471,11 @@ i915_gem_object_ggtt_pin(struct drm_i915_gem_object *obj,
>
> WARN(vma->pin_count,
> "bo is already pinned in ggtt with incorrect alignment:"
> - " offset=%08x %08x, req.alignment=%x, req.map_and_fenceable=%d,"
> + " offset=%08x %08x, req.alignment=%llx, req.map_and_fenceable=%d,"
> " obj->map_and_fenceable=%d\n",
> upper_32_bits(vma->node.start),
> lower_32_bits(vma->node.start),
> - alignment,
> + (long long)alignment,
> !!(flags & PIN_MAPPABLE),
> obj->map_and_fenceable);
> ret = i915_vma_unbind(vma);
> @@ -3484,13 +3490,7 @@ void
> i915_gem_object_ggtt_unpin_view(struct drm_i915_gem_object *obj,
> const struct i915_ggtt_view *view)
> {
> - struct i915_vma *vma = i915_gem_obj_to_ggtt_view(obj, view);
> -
> - GEM_BUG_ON(!vma);
> - WARN_ON(i915_vma_is_pinned(vma));
> - WARN_ON(!i915_gem_obj_ggtt_bound_view(obj, view));
> -
> - __i915_vma_unpin(vma);
> + i915_vma_unpin(i915_gem_obj_to_ggtt_view(obj, view));
> }
>
> int
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 5ed91406d4e9..c9b8c2c62828 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -722,9 +722,8 @@ static int do_rcs_switch(struct drm_i915_gem_request *req)
> return 0;
>
> /* Trying to pin first makes error handling easier. */
> - ret = i915_gem_obj_ggtt_pin(to->engine[RCS].state,
> - to->ggtt_alignment,
> - 0);
> + ret = i915_gem_object_ggtt_pin(to->engine[RCS].state, NULL, 0,
> + to->ggtt_alignment, 0);
> if (ret)
> return ret;
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index cc9c0e4073ff..69bf73b51df9 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -34,10 +34,10 @@
> #include <linux/dma_remapping.h>
> #include <linux/uaccess.h>
>
> -#define __EXEC_OBJECT_HAS_PIN (1<<31)
> -#define __EXEC_OBJECT_HAS_FENCE (1<<30)
> -#define __EXEC_OBJECT_NEEDS_MAP (1<<29)
> -#define __EXEC_OBJECT_NEEDS_BIAS (1<<28)
> +#define __EXEC_OBJECT_HAS_PIN (1U<<31)
> +#define __EXEC_OBJECT_HAS_FENCE (1U<<30)
> +#define __EXEC_OBJECT_NEEDS_MAP (1U<<29)
> +#define __EXEC_OBJECT_NEEDS_BIAS (1U<<28)
>
> #define BATCH_OFFSET_BIAS (256*1024)
>
> @@ -1263,7 +1263,7 @@ i915_gem_execbuffer_parse(struct intel_engine_cs *engine,
> if (ret)
> goto err;
>
> - ret = i915_gem_obj_ggtt_pin(shadow_batch_obj, 0, 0);
> + ret = i915_gem_object_ggtt_pin(shadow_batch_obj, NULL, 0, 0, 0);
> if (ret)
> goto err;
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index 2bd8ec7e1948..5655358a60e1 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -184,13 +184,30 @@ struct i915_vma {
>
> struct i915_gem_active last_read[I915_NUM_ENGINES];
>
> - /** Flags and address space this VMA is bound to */
> + union {
> + struct {
> + /**
> + * How many users have pinned this object in GTT space. The following
> + * users can each hold at most one reference: pwrite/pread, execbuffer
> + * (objects are not allowed multiple times for the same batchbuffer),
> + * and the framebuffer code. When switching/pageflipping, the
> + * framebuffer code has at most two buffers pinned per crtc.
> + *
> + * In the worst case this is 1 + 1 + 1 + 2*2 = 7. That would fit into 3
> + * bits with absolutely no headroom. So use 4 bits. */
> + unsigned int pin_count : 4;
> +#define DRM_I915_GEM_OBJECT_MAX_PIN_COUNT 0xf
> +
> + /** Flags and address space this VMA is bound to */
> #define GLOBAL_BIND (1<<0)
> #define LOCAL_BIND (1<<1)
> - unsigned int bound : 4;
> - unsigned int active : I915_NUM_ENGINES;
> - bool is_ggtt : 1;
> - bool closed : 1;
> + unsigned int bound : 4;
> + unsigned int active : I915_NUM_ENGINES;
> + bool is_ggtt : 1;
> + bool closed : 1;
> + };
> + unsigned int flags;
> + };
>
> /**
> * Support different GGTT views into the same object.
> @@ -215,39 +232,27 @@ struct i915_vma {
> struct hlist_node exec_node;
> unsigned long exec_handle;
> struct drm_i915_gem_exec_object2 *exec_entry;
> -
> - /**
> - * How many users have pinned this object in GTT space. The following
> - * users can each hold at most one reference: pwrite/pread, execbuffer
> - * (objects are not allowed multiple times for the same batchbuffer),
> - * and the framebuffer code. When switching/pageflipping, the
> - * framebuffer code has at most two buffers pinned per crtc.
> - *
> - * In the worst case this is 1 + 1 + 1 + 2*2 = 7. That would fit into 3
> - * bits with absolutely no headroom. So use 4 bits. */
> - unsigned int pin_count:4;
> -#define DRM_I915_GEM_OBJECT_MAX_PIN_COUNT 0xf
> };
>
> static inline bool i915_vma_is_active(const struct i915_vma *vma)
> {
> - return vma->active;
> + return vma->flags & (((1 << I915_NUM_ENGINES) - 1) << 8);
> }
>
> static inline void i915_vma_set_active(struct i915_vma *vma, unsigned engine)
> {
> - vma->active |= 1 << engine;
> + vma->flags |= 0x100 << engine;
> }
>
> static inline void i915_vma_unset_active(struct i915_vma *vma, unsigned engine)
> {
> - vma->active &= ~(1 << engine);
> + vma->flags &= ~(0x100 << engine);
> }
>
> static inline bool i915_vma_has_active_engine(const struct i915_vma *vma,
> unsigned engine)
> {
> - return vma->active & (1 << engine);
> + return vma->flags & (0x100 << engine);
> }
>
> struct i915_page_dma {
> diff --git a/drivers/gpu/drm/i915/i915_gem_render_state.c b/drivers/gpu/drm/i915/i915_gem_render_state.c
> index c0abe9a2210f..4cf82697b3db 100644
> --- a/drivers/gpu/drm/i915/i915_gem_render_state.c
> +++ b/drivers/gpu/drm/i915/i915_gem_render_state.c
> @@ -72,7 +72,7 @@ static int render_state_init(struct render_state *so,
> if (IS_ERR(so->obj))
> return PTR_ERR(so->obj);
>
> - ret = i915_gem_obj_ggtt_pin(so->obj, 4096, 0);
> + ret = i915_gem_object_ggtt_pin(so->obj, NULL, 0, 0, 0);
> if (ret)
> goto free_gem;
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index cc4792df249d..63ef34c78494 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -613,8 +613,8 @@ static struct drm_i915_gem_object *gem_allocate_guc_obj(struct drm_device *dev,
> return NULL;
> }
>
> - if (i915_gem_obj_ggtt_pin(obj, PAGE_SIZE,
> - PIN_OFFSET_BIAS | GUC_WOPCM_TOP)) {
> + if (i915_gem_object_ggtt_pin(obj, NULL, 0, PAGE_SIZE,
> + PIN_OFFSET_BIAS | GUC_WOPCM_TOP)) {
> i915_gem_object_put(obj);
> return NULL;
> }
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index 74a5f11a5689..be93b458968a 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -321,7 +321,7 @@ static int guc_ucode_xfer(struct drm_i915_private *dev_priv)
> return ret;
> }
>
> - ret = i915_gem_obj_ggtt_pin(guc_fw->guc_fw_obj, 0, 0);
> + ret = i915_gem_object_ggtt_pin(guc_fw->guc_fw_obj, NULL, 0, 0, 0);
> if (ret) {
> DRM_DEBUG_DRIVER("pin failed %d\n", ret);
> return ret;
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index 964108cbb9c0..6cdc421fdc37 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -774,8 +774,9 @@ static int intel_lr_context_pin(struct i915_gem_context *ctx,
> if (ce->pin_count++)
> return 0;
>
> - ret = i915_gem_obj_ggtt_pin(ce->state, GEN8_LR_CONTEXT_ALIGN,
> - PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
> + ret = i915_gem_object_ggtt_pin(ce->state, NULL,
> + 0, GEN8_LR_CONTEXT_ALIGN,
> + PIN_OFFSET_BIAS | GUC_WOPCM_TOP);
> if (ret)
> goto err;
>
> @@ -1154,7 +1155,8 @@ static int lrc_setup_wa_ctx_obj(struct intel_engine_cs *engine, u32 size)
> return ret;
> }
>
> - ret = i915_gem_obj_ggtt_pin(engine->wa_ctx.obj, PAGE_SIZE, 0);
> + ret = i915_gem_object_ggtt_pin(engine->wa_ctx.obj, NULL,
> + 0, PAGE_SIZE, 0);
> if (ret) {
> DRM_DEBUG_DRIVER("pin LRC WA ctx backing obj failed: %d\n",
> ret);
> diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
> index 5f645ad2babd..9b0fb7e23cbb 100644
> --- a/drivers/gpu/drm/i915/intel_overlay.c
> +++ b/drivers/gpu/drm/i915/intel_overlay.c
> @@ -1412,7 +1412,8 @@ void intel_setup_overlay(struct drm_i915_private *dev_priv)
> }
> overlay->flip_addr = reg_bo->phys_handle->busaddr;
> } else {
> - ret = i915_gem_obj_ggtt_pin(reg_bo, PAGE_SIZE, PIN_MAPPABLE);
> + ret = i915_gem_object_ggtt_pin(reg_bo, NULL,
> + 0, PAGE_SIZE, PIN_MAPPABLE);
> if (ret) {
> DRM_ERROR("failed to pin overlay register bo\n");
> goto out_free_bo;
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index d63e4fdc60de..f86039455c5a 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -648,7 +648,7 @@ int intel_init_pipe_control(struct intel_engine_cs *engine, int size)
> goto err;
> }
>
> - ret = i915_gem_obj_ggtt_pin(obj, 4096, PIN_HIGH);
> + ret = i915_gem_object_ggtt_pin(obj, NULL, 0, 4096, PIN_HIGH);
> if (ret)
> goto err_unref;
>
> @@ -1816,7 +1816,7 @@ static int init_status_page(struct intel_engine_cs *engine)
> * actualy map it).
> */
> flags |= PIN_MAPPABLE;
> - ret = i915_gem_obj_ggtt_pin(obj, 4096, flags);
> + ret = i915_gem_object_ggtt_pin(obj, NULL, 0, 4096, flags);
> if (ret) {
> err_unref:
> i915_gem_object_put(obj);
> @@ -1863,7 +1863,7 @@ int intel_ring_pin(struct intel_ring *ring)
> int ret;
>
> if (HAS_LLC(dev_priv) && !obj->stolen) {
> - ret = i915_gem_obj_ggtt_pin(obj, PAGE_SIZE, flags);
> + ret = i915_gem_object_ggtt_pin(obj, NULL, 0, PAGE_SIZE, flags);
> if (ret)
> return ret;
>
> @@ -1877,8 +1877,8 @@ int intel_ring_pin(struct intel_ring *ring)
> goto err_unpin;
> }
> } else {
> - ret = i915_gem_obj_ggtt_pin(obj, PAGE_SIZE,
> - flags | PIN_MAPPABLE);
> + ret = i915_gem_object_ggtt_pin(obj, NULL, 0, PAGE_SIZE,
> + flags | PIN_MAPPABLE);
> if (ret)
> return ret;
>
> @@ -2007,7 +2007,8 @@ static int intel_ring_context_pin(struct i915_gem_context *ctx,
> return 0;
>
> if (ce->state) {
> - ret = i915_gem_obj_ggtt_pin(ce->state, ctx->ggtt_alignment, 0);
> + ret = i915_gem_object_ggtt_pin(ce->state, NULL, 0,
> + ctx->ggtt_alignment, 0);
> if (ret)
> goto error;
> }
> @@ -2574,7 +2575,8 @@ int intel_init_render_ring_buffer(struct drm_device *dev)
> i915.semaphores = 0;
> } else {
> i915_gem_object_set_cache_level(obj, I915_CACHE_LLC);
> - ret = i915_gem_obj_ggtt_pin(obj, 0, PIN_NONBLOCK);
> + ret = i915_gem_object_ggtt_pin(obj, NULL,
> + 0, 0, 0);
> if (ret != 0) {
> i915_gem_object_put(obj);
> DRM_ERROR("Failed to pin semaphore bo. Disabling semaphores\n");
> --
> 2.8.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the Intel-gfx
mailing list