Quoting Tvrtko Ursulin (2019-03-05 17:01:09)
>
> On 01/03/2019 14:03, Chris Wilson wrote:
> > @@ -249,12 +249,8 @@ static void i915_gem_context_free(struct i915_gem_context *ctx)
> >
> > kfree(ctx->engines);
> >
> > - for (n = 0; n < ARRAY_SIZE(ctx->__engine); n++) {
> > - struct intel_context *ce = &ctx->__engine[n];
> > -
> > - if (ce->ops)
> > - ce->ops->destroy(ce);
> > - }
>
> /*
> * No need to take lock since no one can access the context at this
> * point.
> */
>
> ?
I hope that's self-evident from being the free function. Hmm, perhaps
not if we move to a HW context centric viewpoint. Maybe worth revisiting
in the future when GEM context withers away.
> > @@ -1839,13 +1817,15 @@ static int get_sseu(struct i915_gem_context *ctx,
> > if (!engine)
> > return -EINVAL;
> >
> > + ce = intel_context_instance(ctx, engine);
> > + if (IS_ERR(ce))
> > + return PTR_ERR(ce);
>
> Interesting! Nevermind..
Wait for the next patch, that one is just for you :)
> > +void __intel_context_remove(struct intel_context *ce)
> > +{
> > + struct i915_gem_context *ctx = ce->gem_context;
> > +
> > + spin_lock(&ctx->hw_contexts_lock);
> > + rb_erase(&ce->node, &ctx->hw_contexts);
> > + spin_unlock(&ctx->hw_contexts_lock);
> > +}
>
> Gets used in a later patch?
For the embedded context within virtual_engine.
> > +struct intel_context *
> > +intel_context_instance(struct i915_gem_context *ctx,
> > + struct intel_engine_cs *engine)
> > +{
> > + struct intel_context *ce, *pos;
> > +
> > + ce = intel_context_lookup(ctx, engine);
> > + if (likely(ce))
> > + return ce;
> > +
> > + ce = kzalloc(sizeof(*ce), GFP_KERNEL);
>
> At some point you'll move this to global slab?
Can do so now, I didn't think these would be frequent enough to justify
a slab. We expect a few hundred on a system; whereas we expect
thousands, tens of thousands of requests and objects.
Still slabs have secondary benefits of debugging allocation patterns.
> > diff --git a/drivers/gpu/drm/i915/intel_guc_ads.c b/drivers/gpu/drm/i915/intel_guc_ads.c
> > index f0db62887f50..da220561ac41 100644
> > --- a/drivers/gpu/drm/i915/intel_guc_ads.c
> > +++ b/drivers/gpu/drm/i915/intel_guc_ads.c
> > @@ -121,8 +121,8 @@ int intel_guc_ads_create(struct intel_guc *guc)
> > * to find it. Note that we have to skip our header (1 page),
> > * because our GuC shared data is there.
> > */
> > - kernel_ctx_vma = to_intel_context(dev_priv->kernel_context,
> > - dev_priv->engine[RCS])->state;
> > + kernel_ctx_vma = intel_context_lookup(dev_priv->kernel_context,
> > + dev_priv->engine[RCS])->state;
> > blob->ads.golden_context_lrca =
> > intel_guc_ggtt_offset(guc, kernel_ctx_vma) + skipped_offset;
>
> Scary moment but kernel context is perma pinned, ok.
Scary moment, but its guc.
> > diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c
> > index 119d3326bb5e..4935e0555135 100644
> > --- a/drivers/gpu/drm/i915/intel_guc_submission.c
> > +++ b/drivers/gpu/drm/i915/intel_guc_submission.c
> > @@ -382,7 +382,7 @@ static void guc_stage_desc_init(struct intel_guc_client *client)
> > desc->db_id = client->doorbell_id;
> >
> > for_each_engine_masked(engine, dev_priv, client->engines, tmp) {
> > - struct intel_context *ce = to_intel_context(ctx, engine);
> > + struct intel_context *ce = intel_context_lookup(ctx, engine);
>
> This one seems to be handling !ce->state a bit below. So it feels you
> have to add the !ce test as well.
I'm not allowed to explode here, shame.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
