Re: [PATCH 18/33] drm/i915: Use VMA as the primary object for context state

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

On su, 2016-08-07 at 15:45 +0100, Chris Wilson wrote:
> -	if (!i915_gem_obj_ggtt_bound(ctx_obj))
> -		seq_puts(m, "\tNot bound in GGTT\n");
> -	else
> -		ggtt_offset = i915_gem_obj_ggtt_offset(ctx_obj);
> +	if (vma->flags & I915_VMA_GLOBAL_BIND)
> +		seq_printf(m, "\tBound in GGTT at 0x%x\n",

0x%04x?

> +			   lower_32_bits(vma->node.start));
>  
> -	if (i915_gem_object_get_pages(ctx_obj)) {
> -		seq_puts(m, "\tFailed to get pages for context object\n");
> +	if (i915_gem_object_get_pages(vma->obj)) {
> +		seq_puts(m, "\tFailed to get pages for context object\n\n");
>  		return;
>  	}
>  
> -	page = i915_gem_object_get_page(ctx_obj, LRC_STATE_PN);
> -	if (!WARN_ON(page == NULL)) {
> -		reg_state = kmap_atomic(page);
> +	page = i915_gem_object_get_page(vma->obj, LRC_STATE_PN);
> +	if (page) {

Dropped the WARN_ON? No mention in the commit message.

> @@ -620,9 +631,7 @@ mi_set_context(struct drm_i915_gem_request *req, u32 hw_flags)
>  
>  	intel_ring_emit(ring, MI_NOOP);
>  	intel_ring_emit(ring, MI_SET_CONTEXT);
> -	intel_ring_emit(ring,
> -			i915_gem_obj_ggtt_offset(req->ctx->engine[RCS].state) |
> -			flags);
> +	intel_ring_emit(ring, req->ctx->engine[RCS].state->node.start | flags);

Do we somewhere make sure flags do not collide with address? Not
related to this patch, though.

> @@ -778,16 +788,12 @@ static int do_rcs_switch(struct drm_i915_gem_request *req)
>  	from = engine->last_context;
>  
>  	/*
> -	 * Clear this page out of any CPU caches for coherent swap-in/out. Note
> -	 * that thanks to write = false in this call and us not setting any gpu
> -	 * write domains when putting a context object onto the active list
> -	 * (when switching away from it), this won't block.
> -	 *
> -	 * XXX: We need a real interface to do this instead of trickery.

What is changed to make this comment obsolete or should have it been
removed earlier?
 
>  
>  	return 0;
>  
> -unpin_out:
> -	i915_gem_object_ggtt_unpin(to->engine[RCS].state);
> +unpin_vma:

sole error path; "err"

> +	i915_vma_unpin(to->engine[RCS].state);
>  	return ret;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> index c621fa23cd28..21a4d0220c17 100644
> --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> @@ -1077,9 +1077,10 @@ static void i915_gem_record_rings(struct drm_i915_private *dev_priv,
>  					i915_error_ggtt_object_create(dev_priv,
>  								      engine->scratch.obj);
>  
> -			ee->ctx =
> -				i915_error_ggtt_object_create(dev_priv,
> -							      request->ctx->engine[i].state);
> +			if (request->ctx->engine[i].state) {
> +				ee->ctx = i915_error_ggtt_object_create(dev_priv,
> +									request->ctx->engine[i].state->obj);
> +			}

Why conditional now?

>  
>  	i915_gem_context_get(ctx);
>  	return 0;
>  
>  unpin_map:
> -	i915_gem_object_unpin_map(ce->state);
> -unpin_ctx_obj:
> -	i915_gem_object_ggtt_unpin(ce->state);
> +	i915_gem_object_unpin_map(ce->state->obj);
> +unpin_vma:
> +	__i915_vma_unpin(ce->state);

err_vma while at it?

> @@ -2161,7 +2162,7 @@ static int execlists_context_deferred_alloc(struct i915_gem_context *ctx,
>  	}
>  
>  	ce->ring = ring;
> -	ce->state = ctx_obj;
> +	ce->state = vma;

Maybe the member name could be just ce->vma too?

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx