On Wed, Aug 10, 2016 at 11:03:39AM +0300, Joonas Lahtinen wrote: > 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?
You mean 0x08. > > - 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. It's a redundant warn that should have been thrown out before. It doesn't even deserve mentioning. > > @@ -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. Bspec vs alignment request, with warns that the allocation meets the requst. > > @@ -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? I had written a custom routine to do it, and then removed it to keep this patch concise. In the next patch it is obsolete. > > 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? Because the code would otherwise dereference a NULL pointer. It gets removed again in the next patches when we pass vma to error object capture. > > 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? No, it still contains the logical GPU state as opposed to the ring which also has its own vma, and so potentially confusing. -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx