From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Widawsky Subject: [PATCH 5/8] drm/i915/context: context validation for execbuffer2 Date: Wed, 2 Feb 2011 15:00:17 -0800 Message-ID: <1296687620-27019-6-git-send-email-bwidawsk@gmail.com> References: <1296687620-27019-1-git-send-email-bwidawsk@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-yx0-f177.google.com (mail-yx0-f177.google.com [209.85.213.177]) by gabe.freedesktop.org (Postfix) with ESMTP id 8415A9E9BE for ; Wed, 2 Feb 2011 15:00:50 -0800 (PST) Received: by mail-yx0-f177.google.com with SMTP id 30so242900yxd.36 for ; Wed, 02 Feb 2011 15:00:50 -0800 (PST) In-Reply-To: <1296687620-27019-1-git-send-email-bwidawsk@gmail.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org Adds the support for actually doing something with buffer validation for the context when submitted via execbuffer2. When a set of context flags are submitted in the execbuffer call, the code is able to handle the commands. It will also go through the existing buffers associated with the context and make sure they are still present. The big thing missing is if the client wants to change the domains of buffers which have already been associated. In this case, the client must disassociate and then re-associate with the proper domains. Fixed up some small issues which hooks the main gem code the context code to do buffer association and validation. As a result of this, the unit test posted on the mailing list should behave as expected. --- drivers/gpu/drm/i915/i915_context.c | 111 +++++++++++++++++++++++++++- drivers/gpu/drm/i915/i915_drv.h | 13 ++- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 12 ++- include/drm/i915_drm.h | 5 +- 4 files changed, 130 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_context.c b/drivers/gpu/drm/i915/i915_context.c index dac09be..4795caf 100644 --- a/drivers/gpu/drm/i915/i915_context.c +++ b/drivers/gpu/drm/i915/i915_context.c @@ -29,6 +29,13 @@ #include "i915_drm.h" #include "intel_drv.h" +#define PPGTT_NEEDS_RELOC (1ULL << 63) +#define PPGTT_USER_DEFINED (1ULL << 62) +#define PPGTT_RELOCATED (1ULL << 61) +#define PPGTT_ADDR(x) (x & 0xFFFFFFFFFFULL) + +#define PPGTT_PINNED(x) ((uint64_t)x & (PPGTT_USER_DEFINED | PPGTT_RELOCATED)) + static int i915_context_gen_id(struct drm_i915_private *dev_priv, struct drm_i915_gem_context *ctx) @@ -70,8 +77,110 @@ int i915_context_validate(struct drm_device *dev, struct drm_file *file, uint32_t ctx_id, struct drm_i915_context_flag *ctx_flag, int count) { - return 0; + struct drm_gem_object *obj; + struct drm_i915_gem_object *obj_priv; + struct drm_i915_gem_context *ctx; + int i, ret = 0; + + ctx = i915_context_lookup_id(dev, ctx_id); + if (ctx == NULL) { + DRM_ERROR("Couldn't find context\n"); + return -ENXIO; + } + + if ((!count || !ctx_flag)) + goto out; + + for (i = 0; i < count; i++) { + struct drm_i915_context_flag *flag = &ctx_flag[i]; + if (flag->slot >= ctx->slot_count) { + DRM_ERROR("Context command for invalid slot\n"); + continue; + } + if (flag->command & (flag->command - 1)) { + DRM_ERROR("More than one command per context flag is not allowed\n"); + continue; + } + + switch (flag->command) { + case I915_CTX_ASSOC_BUF: + mutex_lock(&ctx->slot_mtx); + if (ctx->bufs[flag->slot] != NULL) { + DRM_DEBUG("Overwriting buffer slot without " + "disassociating\n"); + } + obj = drm_gem_object_lookup(dev, file, flag->handle); + if (obj == NULL) { + DRM_ERROR("Couldn't find object\n"); + mutex_unlock(&ctx->slot_mtx); + continue; + } + obj_priv = to_intel_bo(obj); + if (flag->offset && HAS_PPGTT(dev)) { + /* + * No need to check for overlaps because this is + * in their local GTT so they can only screw up + * themselves. But do check serious violations + */ + if (flag->offset + obj->size >= 1ULL << 40) { + mutex_unlock(&ctx->slot_mtx); + continue; + } + obj_priv->ppgtt_offset = flag->offset | PPGTT_USER_DEFINED; + } else + obj_priv->ppgtt_offset = PPGTT_NEEDS_RELOC; + + ctx->bufs[flag->slot] = obj; + mutex_unlock(&ctx->slot_mtx); + break; + case I915_CTX_DISASSOC_BUF: + mutex_lock(&ctx->slot_mtx); + ctx->bufs[flag->slot] = NULL; + mutex_unlock(&ctx->slot_mtx); + break; + case I915_CTX_DOMAIN_BUF: + DRM_ERROR("Changing domains is not yet supported\n"); + break; + default: + DRM_ERROR("Unknown slot command\n"); + } + } + +out: + mutex_lock(&ctx->slot_mtx); + /* Go through all slots to make sure everything is sane. */ + for (i = 0; i < ctx->slot_count; i++) { + uint64_t ppgtt_offset; + if (ctx->bufs[i] == NULL) + continue; + obj_priv = to_intel_bo(ctx->bufs[i]); + if (obj_priv->ppgtt_offset == PPGTT_NEEDS_RELOC) + continue; + + ppgtt_offset = PPGTT_ADDR(obj_priv->ppgtt_offset); + if (PPGTT_PINNED(obj_priv->ppgtt_offset) && + ppgtt_offset != obj_priv->gtt_offset) { + DRM_DEBUG_DRIVER("Context associated buffer has moved" + " %p->%p\n", + ppgtt_offset, obj_priv->gtt_offset); + ret = -EIO; + break; + } + } + + mutex_unlock(&ctx->slot_mtx); + return ret; } + +void i915_context_bind_ppgtt(struct drm_i915_gem_object *obj) +{ + if (obj->ppgtt_offset == PPGTT_NEEDS_RELOC) { + obj->ppgtt_offset = obj->gtt_offset; + obj->ppgtt_offset |= PPGTT_RELOCATED; + } +} + + /** * i915_context_alloc_backing_obj - Allocate and pin space in the global GTT for * use by the HW to save, and restore context information. diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index d21b125..dc7b132 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -851,6 +851,8 @@ struct drm_i915_gem_object { * reaches 0, dev_priv->pending_flip_queue will be woken up. */ atomic_t pending_flip; + + uint64_t ppgtt_offset; }; #define to_intel_bo(x) container_of(x, struct drm_i915_gem_object, base) @@ -1321,10 +1323,13 @@ extern void i915_context_init(struct drm_device *dev); extern void i915_context_fini(struct drm_device *dev); extern void i915_context_close(struct drm_device *dev, struct drm_file *file); struct drm_i915_context_flag; -extern int i915_context_validate(struct drm_device *dev, - struct drm_file *file, uint32_t ctx_id, - struct drm_i915_context_flag *ctx_flag, - int count); +extern struct drm_i915_gem_context * +i915_context_validate(struct drm_device *dev, + struct drm_file *file, uint32_t ctx_id, + struct drm_i915_context_flag *ctx_flag, + int count); +extern void i915_context_bind_ppgtt(struct drm_i915_gem_object *obj); + #define LP_RING(d) (&((struct drm_i915_private *)(d))->ring[RCS]) diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index a60996d..356388a 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -589,6 +589,7 @@ i915_gem_execbuffer_reserve(struct intel_ring_buffer *ring, } entry->offset = obj->gtt_offset; + i915_context_bind_ppgtt(obj); } /* Decrement pin count for bound objects */ @@ -991,6 +992,7 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, struct intel_ring_buffer *ring; u32 exec_start, exec_len; u32 seqno; + u32 ctx_id; int ret, mode, i; if (!i915_gem_check_execbuffer(args)) { @@ -1001,11 +1003,13 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, ret = validate_exec_list(exec, args->buffer_count); if (ret) return ret; - if (ctx_flags) { - ret = i915_context_validate(dev, file, EXECBUFFER2_CTX_ID(args), + + ctx_id = EXECBUFFER2_CTX_ID(args); + if (ctx_id) { + ret = i915_context_validate(dev, file, ctx_id, ctx_flags, flag_count); if (ret) { - if (ret == -EAGAIN) + if (ret == -EIO) DRM_DEBUG_DRIVER("Context resubmission required\n"); return ret; } @@ -1370,7 +1374,7 @@ i915_gem_execbuffer2(struct drm_device *dev, void *data, return -EFAULT; } if (EXECBUFFER2_FLAGS_PTR(args) && EXECBUFFER2_FLAGS_COUNT(argc)) { - flag_count = EXECBUFFER2_FLAGS_COUNT(argc); + flag_count = EXECBUFFER2_FLAGS_COUNT(args); flags = drm_malloc_ab(sizeof(*flags), flag_count); if (flags == NULL) { DRM_ERROR("allocation of flags failed\n"); diff --git a/include/drm/i915_drm.h b/include/drm/i915_drm.h index 76b37f6..5605e97 100644 --- a/include/drm/i915_drm.h +++ b/include/drm/i915_drm.h @@ -667,8 +667,9 @@ struct drm_i915_context_flag { __u64 offset; __u32 read_domain; __u32 write_domain; -#define I915_CTX_ASSOC_BUF (1 << 0) -#define I915_CTX_DISASSOC_BUF (1 << 1) +#define I915_CTX_ASSOC_BUF (1 << 0) +#define I915_CTX_DISASSOC_BUF (1 << 1) +#define I915_CTX_DOMAIN_BUF (1 << 2) __u32 command; }; -- 1.7.3.4