[Intel-gfx] [RFC PATCH 3/4] drm/i915: add api to pin/unpin context state
Bragg, Robert
robert.bragg at intel.com
Wed Nov 12 18:22:22 CET 2014
On Wed, Nov 12, 2014 at 9:37 AM, Daniel Vetter <daniel at 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 at 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 at 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20141112/dd66732b/attachment.html>
More information about the Intel-gfx
mailing list