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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
