| Message ID | 1415631411-11726-4-git-send-email-robert@sixbynine.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Nov 10, 2014 at 02:56:50PM +0000, Robert Bragg wrote: > This adds i915_gem_context_pin/unpin_state functions so that code > outside i915_gem_context.c can pin/unpin a context without duplicating > knowledge about the alignment constraints. > > Signed-off-by: Robert Bragg <robert@sixbynine.org> At first I've thought this is ok to get going, but then I pondered a bit more. We unpin ctx objects from the shrinker, and this here torpedos that a bit. And I think the proper implementation isn't more fuzz that this here: When pinning the context we call an OA function to update OA state. When OA notices that this is the buffer it cares about (for filtered mode) and that it has moved, it updates OA hw/sw state. I don't think this can race since a ctx can only be moved when it's not actively used by the gpu, so I think we should be able to do this. Also unbinding ctxs is a fairly last-resort kind of thing, so even when this requires a stall (maybe OA needs to be stopped/restarted) it should still be ok. Or do I miss something and this is too much fuzz? -Daniel > --- > drivers/gpu/drm/i915/i915_drv.h | 4 ++++ > drivers/gpu/drm/i915/i915_gem_context.c | 30 +++++++++++++++++++++--------- > 2 files changed, 25 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 3212d62..acaf76c 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -2675,6 +2675,10 @@ int i915_gem_context_create_ioctl(struct drm_device *dev, void *data, > int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data, > struct drm_file *file); > > +int i915_gem_context_pin_state(struct drm_device *dev, > + struct intel_context *ctx); > +void i915_gem_context_unpin_state(struct intel_context *ctx); > + > /* i915_gem_evict.c */ > int __must_check i915_gem_evict_something(struct drm_device *dev, > struct i915_address_space *vm, > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c > index a5221d8..feb1a23 100644 > --- a/drivers/gpu/drm/i915/i915_gem_context.c > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > @@ -132,6 +132,20 @@ static int get_context_size(struct drm_device *dev) > return ret; > } > > +int i915_gem_context_pin_state(struct drm_device *dev, > + struct intel_context *ctx) > +{ > + BUG_ON(!mutex_is_locked(&dev->struct_mutex)); > + > + return i915_gem_obj_ggtt_pin(ctx->legacy_hw_ctx.rcs_state, > + get_context_alignment(dev), 0); > +} > + > +void i915_gem_context_unpin_state(struct intel_context *ctx) > +{ > + i915_gem_object_ggtt_unpin(ctx->legacy_hw_ctx.rcs_state); > +} > + > void i915_gem_context_free(struct kref *ctx_ref) > { > struct intel_context *ctx = container_of(ctx_ref, > @@ -253,8 +267,7 @@ i915_gem_create_context(struct drm_device *dev, > * be available. To avoid this we always pin the default > * context. > */ > - ret = i915_gem_obj_ggtt_pin(ctx->legacy_hw_ctx.rcs_state, > - get_context_alignment(dev), 0); > + ret = i915_gem_context_pin_state(dev, ctx); > if (ret) { > DRM_DEBUG_DRIVER("Couldn't pin %d\n", ret); > goto err_destroy; > @@ -278,7 +291,7 @@ i915_gem_create_context(struct drm_device *dev, > > err_unpin: > if (is_global_default_ctx && ctx->legacy_hw_ctx.rcs_state) > - i915_gem_object_ggtt_unpin(ctx->legacy_hw_ctx.rcs_state); > + i915_gem_context_unpin_state(ctx); > err_destroy: > i915_gem_context_unreference(ctx); > return ERR_PTR(ret); > @@ -375,12 +388,12 @@ void i915_gem_context_fini(struct drm_device *dev) > if (dev_priv->ring[RCS].last_context == dctx) { > /* Fake switch to NULL context */ > WARN_ON(dctx->legacy_hw_ctx.rcs_state->active); > - i915_gem_object_ggtt_unpin(dctx->legacy_hw_ctx.rcs_state); > + i915_gem_context_unpin_state(dctx); > i915_gem_context_unreference(dctx); > dev_priv->ring[RCS].last_context = NULL; > } > > - i915_gem_object_ggtt_unpin(dctx->legacy_hw_ctx.rcs_state); > + i915_gem_context_unpin_state(dctx); > } > > for (i = 0; i < I915_NUM_RINGS; i++) { > @@ -534,8 +547,7 @@ static int do_switch(struct intel_engine_cs *ring, > > /* Trying to pin first makes error handling easier. */ > if (ring == &dev_priv->ring[RCS]) { > - ret = i915_gem_obj_ggtt_pin(to->legacy_hw_ctx.rcs_state, > - get_context_alignment(ring->dev), 0); > + ret = i915_gem_context_pin_state(ring->dev, to); > if (ret) > return ret; > } > @@ -616,7 +628,7 @@ static int do_switch(struct intel_engine_cs *ring, > BUG_ON(from->legacy_hw_ctx.rcs_state->ring != ring); > > /* obj is kept alive until the next request by its active ref */ > - i915_gem_object_ggtt_unpin(from->legacy_hw_ctx.rcs_state); > + i915_gem_context_unpin_state(from); > i915_gem_context_unreference(from); > } > > @@ -643,7 +655,7 @@ done: > > unpin_out: > if (ring->id == RCS) > - i915_gem_object_ggtt_unpin(to->legacy_hw_ctx.rcs_state); > + i915_gem_context_unpin_state(to); > return ret; > } > > -- > 2.1.3 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Nov 12, 2014 at 9:37 AM, Daniel Vetter <daniel@ffwll.ch> wrote: > On Mon, Nov 10, 2014 at 02:56:50PM +0000, Robert Bragg wrote: > > This adds i915_gem_context_pin/unpin_state functions so that code > > outside i915_gem_context.c can pin/unpin a context without duplicating > > knowledge about the alignment constraints. > > > > Signed-off-by: Robert Bragg <robert@sixbynine.org> > > At first I've thought this is ok to get going, but then I pondered a bit > more. We unpin ctx objects from the shrinker, and this here torpedos that > a bit. And I think the proper implementation isn't more fuzz that this > here: > > When pinning the context we call an OA function to update OA state. When > OA notices that this is the buffer it cares about (for filtered mode) and > that it has moved, it updates OA hw/sw state. I don't think this can race > since a ctx can only be moved when it's not actively used by the gpu, so I > think we should be able to do this. Also unbinding ctxs is a fairly > last-resort kind of thing, so even when this requires a stall (maybe OA > needs to be stopped/restarted) it should still be ok. > > Or do I miss something and this is too much fuzz? > Yeah, this seems workable too; and seems similar to what I had in mind with the previous version I sent out to the lkml: /* XXX: Not sure that this is really acceptable... * * i915_gem_context.c currently owns pinning/unpinning legacy * context buffers and although that code has a * get_context_alignment() func to handle a different * constraint for gen6 we are assuming it's fixed for gen7 * here. Another option besides pinning here would be to * instead hook into context switching and update the * OACONTROL configuration on the fly. */ if (dev_priv->oa_pmu.specific_ctx) { struct intel_context *ctx = dev_priv->oa_pmu.specific_ctx; int ret; ret = i915_gem_obj_ggtt_pin(ctx->legacy_hw_ctx.rcs_state, 4096, 0); if (ret) { DRM_DEBUG_DRIVER("Couldn't pin %d\n", ret); ret = -EBUSY; goto err; } } Incidentally; considering some initial patches that Peng Li sent me recently adding experimental Broadwell support, that also requires us to hook into the context switching to be able to insert some LRIs after the MI_SET_CONTEXT so adding some form of pre set-context hook for this too seems similar. Thanks, - Robert > -Daniel > > > --- > > drivers/gpu/drm/i915/i915_drv.h | 4 ++++ > > drivers/gpu/drm/i915/i915_gem_context.c | 30 > +++++++++++++++++++++--------- > > 2 files changed, 25 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > b/drivers/gpu/drm/i915/i915_drv.h > > index 3212d62..acaf76c 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -2675,6 +2675,10 @@ int i915_gem_context_create_ioctl(struct > drm_device *dev, void *data, > > int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data, > > struct drm_file *file); > > > > +int i915_gem_context_pin_state(struct drm_device *dev, > > + struct intel_context *ctx); > > +void i915_gem_context_unpin_state(struct intel_context *ctx); > > + > > /* i915_gem_evict.c */ > > int __must_check i915_gem_evict_something(struct drm_device *dev, > > struct i915_address_space *vm, > > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c > b/drivers/gpu/drm/i915/i915_gem_context.c > > index a5221d8..feb1a23 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_context.c > > +++ b/drivers/gpu/drm/i915/i915_gem_context.c > > @@ -132,6 +132,20 @@ static int get_context_size(struct drm_device *dev) > > return ret; > > } > > > > +int i915_gem_context_pin_state(struct drm_device *dev, > > + struct intel_context *ctx) > > +{ > > + BUG_ON(!mutex_is_locked(&dev->struct_mutex)); > > + > > + return i915_gem_obj_ggtt_pin(ctx->legacy_hw_ctx.rcs_state, > > + get_context_alignment(dev), 0); > > +} > > + > > +void i915_gem_context_unpin_state(struct intel_context *ctx) > > +{ > > + i915_gem_object_ggtt_unpin(ctx->legacy_hw_ctx.rcs_state); > > +} > > + > > void i915_gem_context_free(struct kref *ctx_ref) > > { > > struct intel_context *ctx = container_of(ctx_ref, > > @@ -253,8 +267,7 @@ i915_gem_create_context(struct drm_device *dev, > > * be available. To avoid this we always pin the default > > * context. > > */ > > - ret = i915_gem_obj_ggtt_pin(ctx->legacy_hw_ctx.rcs_state, > > - get_context_alignment(dev), 0); > > + ret = i915_gem_context_pin_state(dev, ctx); > > if (ret) { > > DRM_DEBUG_DRIVER("Couldn't pin %d\n", ret); > > goto err_destroy; > > @@ -278,7 +291,7 @@ i915_gem_create_context(struct drm_device *dev, > > > > err_unpin: > > if (is_global_default_ctx && ctx->legacy_hw_ctx.rcs_state) > > - i915_gem_object_ggtt_unpin(ctx->legacy_hw_ctx.rcs_state); > > + i915_gem_context_unpin_state(ctx); > > err_destroy: > > i915_gem_context_unreference(ctx); > > return ERR_PTR(ret); > > @@ -375,12 +388,12 @@ void i915_gem_context_fini(struct drm_device *dev) > > if (dev_priv->ring[RCS].last_context == dctx) { > > /* Fake switch to NULL context */ > > WARN_ON(dctx->legacy_hw_ctx.rcs_state->active); > > - > i915_gem_object_ggtt_unpin(dctx->legacy_hw_ctx.rcs_state); > > + i915_gem_context_unpin_state(dctx); > > i915_gem_context_unreference(dctx); > > dev_priv->ring[RCS].last_context = NULL; > > } > > > > - i915_gem_object_ggtt_unpin(dctx->legacy_hw_ctx.rcs_state); > > + i915_gem_context_unpin_state(dctx); > > } > > > > for (i = 0; i < I915_NUM_RINGS; i++) { > > @@ -534,8 +547,7 @@ static int do_switch(struct intel_engine_cs *ring, > > > > /* Trying to pin first makes error handling easier. */ > > if (ring == &dev_priv->ring[RCS]) { > > - ret = i915_gem_obj_ggtt_pin(to->legacy_hw_ctx.rcs_state, > > - > get_context_alignment(ring->dev), 0); > > + ret = i915_gem_context_pin_state(ring->dev, to); > > if (ret) > > return ret; > > } > > @@ -616,7 +628,7 @@ static int do_switch(struct intel_engine_cs *ring, > > BUG_ON(from->legacy_hw_ctx.rcs_state->ring != ring); > > > > /* obj is kept alive until the next request by its active > ref */ > > - i915_gem_object_ggtt_unpin(from->legacy_hw_ctx.rcs_state); > > + i915_gem_context_unpin_state(from); > > i915_gem_context_unreference(from); > > } > > > > @@ -643,7 +655,7 @@ done: > > > > unpin_out: > > if (ring->id == RCS) > > - i915_gem_object_ggtt_unpin(to->legacy_hw_ctx.rcs_state); > > + i915_gem_context_unpin_state(to); > > return ret; > > } > > > > -- > > 2.1.3 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx >
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 3212d62..acaf76c 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2675,6 +2675,10 @@ int i915_gem_context_create_ioctl(struct drm_device *dev, void *data, int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data, struct drm_file *file); +int i915_gem_context_pin_state(struct drm_device *dev, + struct intel_context *ctx); +void i915_gem_context_unpin_state(struct intel_context *ctx); + /* i915_gem_evict.c */ int __must_check i915_gem_evict_something(struct drm_device *dev, struct i915_address_space *vm, diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index a5221d8..feb1a23 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -132,6 +132,20 @@ static int get_context_size(struct drm_device *dev) return ret; } +int i915_gem_context_pin_state(struct drm_device *dev, + struct intel_context *ctx) +{ + BUG_ON(!mutex_is_locked(&dev->struct_mutex)); + + return i915_gem_obj_ggtt_pin(ctx->legacy_hw_ctx.rcs_state, + get_context_alignment(dev), 0); +} + +void i915_gem_context_unpin_state(struct intel_context *ctx) +{ + i915_gem_object_ggtt_unpin(ctx->legacy_hw_ctx.rcs_state); +} + void i915_gem_context_free(struct kref *ctx_ref) { struct intel_context *ctx = container_of(ctx_ref, @@ -253,8 +267,7 @@ i915_gem_create_context(struct drm_device *dev, * be available. To avoid this we always pin the default * context. */ - ret = i915_gem_obj_ggtt_pin(ctx->legacy_hw_ctx.rcs_state, - get_context_alignment(dev), 0); + ret = i915_gem_context_pin_state(dev, ctx); if (ret) { DRM_DEBUG_DRIVER("Couldn't pin %d\n", ret); goto err_destroy; @@ -278,7 +291,7 @@ i915_gem_create_context(struct drm_device *dev, err_unpin: if (is_global_default_ctx && ctx->legacy_hw_ctx.rcs_state) - i915_gem_object_ggtt_unpin(ctx->legacy_hw_ctx.rcs_state); + i915_gem_context_unpin_state(ctx); err_destroy: i915_gem_context_unreference(ctx); return ERR_PTR(ret); @@ -375,12 +388,12 @@ void i915_gem_context_fini(struct drm_device *dev) if (dev_priv->ring[RCS].last_context == dctx) { /* Fake switch to NULL context */ WARN_ON(dctx->legacy_hw_ctx.rcs_state->active); - i915_gem_object_ggtt_unpin(dctx->legacy_hw_ctx.rcs_state); + i915_gem_context_unpin_state(dctx); i915_gem_context_unreference(dctx); dev_priv->ring[RCS].last_context = NULL; } - i915_gem_object_ggtt_unpin(dctx->legacy_hw_ctx.rcs_state); + i915_gem_context_unpin_state(dctx); } for (i = 0; i < I915_NUM_RINGS; i++) { @@ -534,8 +547,7 @@ static int do_switch(struct intel_engine_cs *ring, /* Trying to pin first makes error handling easier. */ if (ring == &dev_priv->ring[RCS]) { - ret = i915_gem_obj_ggtt_pin(to->legacy_hw_ctx.rcs_state, - get_context_alignment(ring->dev), 0); + ret = i915_gem_context_pin_state(ring->dev, to); if (ret) return ret; } @@ -616,7 +628,7 @@ static int do_switch(struct intel_engine_cs *ring, BUG_ON(from->legacy_hw_ctx.rcs_state->ring != ring); /* obj is kept alive until the next request by its active ref */ - i915_gem_object_ggtt_unpin(from->legacy_hw_ctx.rcs_state); + i915_gem_context_unpin_state(from); i915_gem_context_unreference(from); } @@ -643,7 +655,7 @@ done: unpin_out: if (ring->id == RCS) - i915_gem_object_ggtt_unpin(to->legacy_hw_ctx.rcs_state); + i915_gem_context_unpin_state(to); return ret; }
This adds i915_gem_context_pin/unpin_state functions so that code outside i915_gem_context.c can pin/unpin a context without duplicating knowledge about the alignment constraints. Signed-off-by: Robert Bragg <robert@sixbynine.org> --- drivers/gpu/drm/i915/i915_drv.h | 4 ++++ drivers/gpu/drm/i915/i915_gem_context.c | 30 +++++++++++++++++++++--------- 2 files changed, 25 insertions(+), 9 deletions(-)